Undici.Request and AbortController doesn't work well

Bug Description

Sometimes Aborting Undici.Request causes whole program to stop, even though everything is wrapped around try catch.

Reproducible By

~~I don't have minimal reproducible example, as my code is larger in size.~~ But Undici.Fetch doesn't have this issue.

Here is minimal reproducible example:

const undici = require('undici')

async function testUndici() {
    const controller = new AbortController()
    const {statusCode,headers,trailers,body} = await undici.request("https://github.com/fawazahmed0/tiger/releases/download/v2/pipetocsv.7z", { signal: controller.signal }).catch(console.error)
    setTimeout(()=>controller.abort(), 500)
}
testUndici()

Expected Behavior

Undici.Request and AbortController should work

Logs & Screenshots

node:events:498
      throw er; // Unhandled 'error' event
      ^
DOMException [AbortError]: This operation was aborted
    at new DOMException (node:internal/per_context/domexception:53:5)
    at AbortController.abort (node:internal/abort_controller:395:18)
    at Timeout._onTimeout (T:\xx\other scripts\undici-debug.js:7:31)
    at listOnTimeout (node:internal/timers:573:17)
    at process.processTimers (node:internal/timers:514:7)
Emitted 'error' event on BodyReadable instance at:
    at emitErrorNT (node:internal/streams/destroy:170:8)
    at emitErrorCloseNT (node:internal/streams/destroy:129:3)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)

Node.js v22.3.0

Environment

Ubuntu 24.04 & Windows 11
Node v22.3.0
Undici 6.19.1

I am having the same problem. The bug was introduced in 6.16.0. I think the problem is in this #3209. In case of onComplete we don't remove the listener from the signal

Adding this in api-request.js fixes the issue

@Gigioliva

@Gigioliva Would you like to send a Pull Request to address this issue? Remember to add unit tests.

👍🏼 I will do

@mcollina

@mcollina #3354

The problem is that you are not cleaning up / destroying / consuming the body of returned response object which keep the signal handler referenced.

    while (!response?.ok) {
        await response?.body.dump()
        response = await undici.request(link,{maxRedirections: 30, signal: controller?.signal})
    }

@ronag

@ronag In my case I was consuming the response 🤔 await result.body.text()

Not between iterations of the while loop

Yes. within the loop. My code is something like:

class MyClass {
  readonly #pool: Agent;

  constructor() {
    this.#pool = new Agent({ connections: MAX_CONCURRENCY });
  }

  async runFetch(urls: string[]): Promise<string[]> {
    return await Promise.all(urls.map((url) => this.doGet(url)));
  }

  private async doGet(url: string) {
    const result = await this.#pool.request({
      origin: url,
      path: "/foo",
      method: "GET",
      signal: AbortSignal.timeout(1_000),
    });
    const text = await result.body.text();
    console.log(text);
    return text;
  }
}

Do you have a repro then? Because your other repro was incorrect. Without a repro we cannot help.

Yup. I can reproduce it in my tool tests. I will try to extract the test and share with you

Here is a repro:

const undici = require('undici')

async function testUndici(link, controller) {
    let undiciResponse;
    while (!(undiciResponse?.statusCode >= 200 && undiciResponse?.statusCode < 300))
        undiciResponse = await undici.request(link, { maxRedirections: 30, signal: controller?.signal })

    sleep(500).then(() => controller.abort())

}
async function begin() {

    const controller = new AbortController()
    while (true) 
        await testUndici("https://github.com/fawazahmed0/tiger/releases/download/v2/pipetocsv.7z", controller).catch(console.error)
    
}
begin()

function sleep(ms) {
    return new Promise(resolve => setTimeout(resolve, ms));
}

Update: Removed unnecessary fetch from repro

fix: attempt to fix uncaught exception per <nodejs/undici#3353>

@titanism

@titanism I'm still seeing this issue on nodejs 23.1.0

the code in question is https://github.com/webtorrent/bittorrent-tracker/blob/master/lib/client/http-tracker.js#L147

I'm getting the DOMException from the line where the AbortController is created, that is outside the try/catch

Any idea?

Thanks

(original issue: webtorrent/webtorrent#2874)

Hi team, we got bitten by this issue in production like others in this thread.

I think that while Readable and undici are each working as intended, the combination of the two leads to an API that is unnecessarily hostile for a user.

In our case, the issue manifested because we're holding onto a reference to clientRes to pipe into a subsequent bit of work. However, after obtaining a reference to clientRes and before calling pipeline(clientRes.body, nextConsumer, { signal }), we call signal.throwIfAborted().

This leads to a whole-program crash despite the whole sequence above being orchestrated in an async function, attempting idiomatic use of AbortSignal.

The fix, in our case is the following, which I believe is indicative of the dangerous nature of this interaction:

async function criticalPath() {
  clientRes = await performUndiciRequest(signal);
  
  // This is the fix.
  clientRes.body.on('error', () => {
    // Ignore the error so that we don't get uncaught exceptions and crash.
  });
  
  // ... do some async work
  
  signal.throwIfAborted();
  
  // ... consume the body
  
  await pipeline([clientRes.body, targetStream], { signal });
}

@ggoodman

@ggoodman how do you safely do cleanup and destroy the stream after? otherwise you will encounter 100% cpu usage, e.g. https://www.reddit.com/r/node/comments/vcxx4n/comment/ideo7s4/

@kyrylkov can you elaborate on how you solved this via code snippet?

So we use undici fetch that returns WebStream readable. We then convert that readable into Node.js readable. Then we async iterate over it. Turns out that after we consume the whole readable, the stream wouldn't close for some reason. We've started explicitly destroying stream after consuming them and the problem went away. Still not sure why streams were not destroyed implicitly, because our understanding is that they should have.

@titanism

how do you safely do cleanup and destroy the stream after? otherwise you will encounter 100% cpu usage

@titanism my understanding is that the problematic behaviour with CPU usage is limited to fetch and Web streams. In our case, we're using undici as a lower-overhead http/1.1 client for some high-volume, low-latency reverse proxying.

Is there evidence that import('node:stream').Readable also suffers from this footgun?

I don't see a problem here. You are mostly mis-using the api... The signal.throwIfAborted() is not necessary.

@ronag

I suppose we should use node-fetch or an alternative instead, e.g. XHR due to the CPU issues.

@ronag How do you properly destroy a fetch? See that Reddit link above; we have the exact same issue that @kyrylkov had, which is using PM2 and after ~20-30 mins time, undici fetch calls seem to cause event loop to build-up and queue, due to Readable. We used v8-profiler-next and cpupro, but it's very limited in insight:

Anonymous function #994 is this:

      queueMicrotask(() => {
        stream[kConsume] = {
          type,
          stream,
          resolve,
          reject,
          length: 0,
          body: []
        }

        stream
          .on('error', function (err) {
            consumeFinish(this[kConsume], err)
          })
          .on('close', function () {
            if (this[kConsume].body !== null) {
              consumeFinish(this[kConsume], new RequestAbortedError())
            }
          })

        consumeStart(stream[kConsume])
      })

@titanism

@titanism That does not seem relateed to this issue, please create a new one

Usage here https://github.com/forwardemail/nodejs-dns-over-https-tangerine/blob/4403cbabcb8b19ab2b118c00a461c6c7e64407dc/index.js#L1119-L1159

@ronag

@ronag I think I see where you're coming from now. This appears to be the precedent for all Node.js Streams produced with a supplied signal (https://github.com/nodejs/node/blob/f17b9a4c7838f37022255f4dcee09a083686018e/lib/internal/streams/add-abort-signal.js#L41-L60). At least, that's the precedent for synchronous APIs returning streams.

I think the surprising thing here is that this is a hybrid promise / stream API. Both the initial promise for getting the Response object AND the stream are governed by the signal. I suppose that I was only expecting the initial Promise for the headers to be controlled by the signal. I was already controlling the consumption of the body stream using the signal via pipeline(). I expected any possible failure modes to result in promise rejections instead of this subtle interaction with node's emitter-based legacy.

@mcollina

I think the surprising thing here is that this is a hybrid promise / stream API. Both the initial promise for getting the Response object AND the stream are governed by the signal. I suppose that I was only expecting the initial Promise for the headers to be controlled by the signal. I was already controlling the consumption of the body stream using the signal via pipeline(). I expected any possible failure modes to result in promise rejections instead of this subtle interaction with node's emitter-based legacy.

@mcollina wdyt? I think it's a reasonable opinion but not sure what to do with it. Would break everyone to change it now.

Bugfix - when using AbortController, errors on resulting stream must …

fix: attempt to fix uncaught exception per <nodejs/undici#3353>

Navigation Menu

Repository navigation

Undici.Request and AbortController doesn't work well #3353

Bug Description

Reproducible By

Expected Behavior

Logs & Screenshots

Environment

21 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Undici.Request and AbortController doesn't work well #3353

Description

Bug Description

Reproducible By

Expected Behavior

Logs & Screenshots

Environment

Activity

Gigioliva commented on Jun 21, 2024

Gigioliva commented on Jun 21, 2024

mcollina commented on Jun 21, 2024

Gigioliva commented on Jun 21, 2024

Gigioliva commented on Jun 21, 2024

ronag commented on Jun 22, 2024

Gigioliva commented on Jun 22, 2024

ronag commented on Jun 22, 2024

Gigioliva commented on Jun 22, 2024

ronag commented on Jun 22, 2024

Gigioliva commented on Jun 22, 2024

fawazahmed0 commented on Jun 22, 2024

21 remaining items

uriva commented on Nov 3, 2024

ggoodman commented on Nov 12, 2024

titanism commented on Nov 12, 2024

titanism commented on Nov 12, 2024

ggoodman commented on Nov 12, 2024

ronag commented on Nov 12, 2024

titanism commented on Nov 12, 2024

titanism commented on Nov 12, 2024

ronag commented on Nov 12, 2024

titanism commented on Nov 12, 2024

ggoodman commented on Nov 12, 2024

ronag commented on Nov 13, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions