Aborting HTTP2 to streaming endpoints throws an assertion error

[jackschu]

Bug Description

Aborting streaming endpoints immediately the after response is ready results in an assertion failure
This might be related to #2364 but the assert is appearing in a different spot
And the repro doesnt require multiple requests to the same domain
But if these are deemed the same, happy for yall to consolidate to that one issue

Reproducible By

https://replit.com/@JS7/undici-h2-abort-failure

const undici = require('undici');

undici.setGlobalDispatcher(new undici.Agent({
  allowH2: true,
}));

(async () => {
  const controller = new AbortController();
  const response = await undici.fetch('https://mirror-cdn.xtom.com/ubuntu/dists/xenial/Release.gpg', { signal: controller.signal });
  controller.abort()
  setTimeout(()=>console.log("done"), 100);
})();

Expected Behavior

At some point I see the console.log printing "done". Nothing is thrown.

Actual Behavior

I see this and no other output

node:assert:399
    throw err;
    ^

AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:

  assert(!this.aborted)

    at Request.onData (/home/runner/undici-h2-abort-failure/node_modules/undici/lib/core/request.js:246:5)
    at ClientHttp2Stream.<anonymous> (/home/runner/undici-h2-abort-failure/node_modules/undici/lib/client.js:1814:17)
    at ClientHttp2Stream.emit (node:events:513:28)
    at addChunk (node:internal/streams/readable:324:12)
    at readableAddChunk (node:internal/streams/readable:297:9)
    at Readable.push (node:internal/streams/readable:234:10)
    at Http2Stream.onStreamRead (node:internal/stream_base_commons:190:23) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: false,
  expected: true,
  operator: '=='
}

Environment

Node.js v18.16.1
undici@5.26.4

[metcoder95]

The behavior is correct. The body is not being consumed when you abort the controller

[ronag]

assertions should never hit in correct behavior?

[metcoder95]

The issue is rooted in the way the signal is handled by node:http2.
It's hitting the Request.onData, so this means it's trying to read the data from the h2 stream which seems to not be aborted (maybe the data is already buffered and not read?).

Based on the example, I'm assuming the ongoing request has been already solved, and for instance, the abort signal is not going to take effect (haven't check this at Node.js core), as it is possible the data is already buffered and ready to be read, as fetch still consuming such data in the background but the request has been aborted, the Request.onData hits the wall with the assertion !this.aborted.

Here, it seems, that unless we change and manually handle the AbortSignal behaviour, although weird this seems to be expected.

Or maybe I overlooked and didn’t connect it with the aborted flow?

[metcoder95]

If you read the body, this does not happen:

const undici = require('.')

undici.setGlobalDispatcher(
  new undici.Agent({
    allowH2: true
  })
)

;(async () => {
  const controller = new AbortController()
  const response = await undici.fetch(
    'https://mirror-cdn.xtom.com/ubuntu/dists/xenial/Release.gpg',
    { signal: controller.signal }
  )
  await response.text()
  controller.abort()
  setTimeout(() => console.log('done'), 500)
})()

If otherwise you abort and late try to consume the body, you hit fetch wall instead:

const undici = require('.')

undici.setGlobalDispatcher(
  new undici.Agent({
    allowH2: true
  })
)

;(async () => {
  const controller = new AbortController()
  const response = await undici.fetch(
    'https://mirror-cdn.xtom.com/ubuntu/dists/xenial/Release.gpg',
    { signal: controller.signal }
  )
  controller.abort()
  await response.text()
  setTimeout(() => console.log('done'), 500)
})()

Output:

(node:28788) [UNDICI-H2] Warning: H2 support is experimental, expect them to change at any time.
(Use `node --trace-warnings ...` to show where the warning was created)

node:internal/per_context/domexception:53
    ErrorCaptureStackTrace(this);
    ^
DOMException [AbortError]: The operation was aborted.
    at new DOMException (node:internal/per_context/domexception:53:5)
    at throwIfAborted (/Users/metcoder/Documents/Workspace/MetCoder/Undici_Core/lib/fetch/body.js:323:11)
    at specConsumeBody (/Users/metcoder/Documents/Workspace/MetCoder/Undici_Core/lib/fetch/body.js:502:3)
    at Response.text (/Users/metcoder/Documents/Workspace/MetCoder/Undici_Core/lib/fetch/body.js:363:14)
    at /Users/metcoder/Documents/Workspace/MetCoder/Undici_Core/playground.js:147:18
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

Node.js v18.14.2

[jackschu]

Yeah I agree that if I'm awaiting an abortable promise, then I should expect it to settle as a rejection if abort is signaled.

But here I'm not awaiting an abortable promise, so I dont think this is expected behavior. And defending against the throw is pretty much impossible? Especially given that the exception is an assertion error, catching it seems unreasonable.

---

Its interesting that this doesnt happen if I put the abort on the task queue. Which I definitely wouldnt just want to get in the habit of in my production code.

  const response = await undici.fetch('http://raw.githubusercontent.com/nodejs/undici/main/README.md', { signal: controller.signal });
  setTimeout(() => controller.abort(), 0)
  setTimeout(()=>console.log("done"), 100);

[metcoder95]

And defending against the throw is pretty much impossible?

At the current state, yeah; I didn't expect node:http2 to have that behaviour, to solve it we'll pretty much need to change the way the data is read when the request is aborted, so no more data is read and we just dump it instead.

I'll do more research on this side. But I'm pretty sure that this will solve #2364 as well

Its interesting that this doesnt happen if I put the abort on the task queue. Which I definitely wouldnt just want to get in the habit of in my production code.

It might be that that loop tick allows the buffer to be fully loaded by fetch by then. Then the data even is not triggered anymore from http2 as the body was fully read already.