Fix HTTP/2 RST_STREAM behaviour, add auto-drain, deprecate 'aborted', fix related compat API issues#63249
Conversation
Signed-off-by: Tim Perry <pimterry@gmail.com>
Previously, stream errors were completely swallowed and never exposed. With this change, they're exposed only if there is a listener for them - this is the exact same pattern used in HTTP/1 itself, so hopefully a good fit for the compat API! This also changes the compat API to only report 'finish' when the writable has actually finished - previously all closes were reporting with finish, even when the writable was aborted part way through.
|
Review requested:
|
nodejs-github-bot
added
c++
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #63249 +/- ##
========================================
Coverage 90.02% 90.03%
========================================
Files 713 714 +1
Lines 224950 225330 +380
Branches 42530 42615 +85
========================================
+ Hits 202513 202875 +362
- Misses 14220 14236 +16
- Partials 8217 8219 +2
🚀 New features to boost your workflow:
|
mcollina
added
the
semver-major
|
Pushed updates to the deprecation docs, tightened the tests a little, and covered an extra case: it turns out |
|
It looks like this is ready to land. |
|
In theory yes, but it needs re-review (ping @nodejs/http2 & @nodejs/http) and 2x @nodejs/tsc approval for the breaking change |
pimterry
added
request-ci
github-actions
Bot
removed
the
request-ci
This comment was marked as outdated.
This comment was marked as outdated.
pimterry
added
commit-queue
nodejs-github-bot
added
commit-queue-failed
Commit Queue failed- Loading data for nodejs/node/pull/63249 ✔ Done loading data for nodejs/node/pull/63249 ----------------------------------- PR info ------------------------------------ Title Fix HTTP/2 RST_STREAM behaviour, add auto-drain, deprecate 'aborted', fix related compat API issues (#63249) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch pimterry:fix-h2-abort -> nodejs:main Labels c++, semver-major, lib / src, needs-ci Commits 7 - http2: error for incomplete reads on RST, auto-drain, deprecate aborted - http2: fix stream error reporting & 'finish' in the compat API - Fix cpp formatting - Make respondWithFD match fixed H2 RST behaviour too - Add example to deprecations & update PR ids - Tighten up the test cases a little - Make sure the respondWithFD test isn't flaky Committers 1 - Tim Perry <pimterry@gmail.com> PR-URL: https://github.com/nodejs/node/pull/63249 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/63249 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> -------------------------------------------------------------------------------- ℹ This PR was created on Mon, 11 May 2026 14:48:22 GMT ✔ Approvals: 3 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/63249#pullrequestreview-4269262560 ✔ - Stephen Belanger (@Qard): https://github.com/nodejs/node/pull/63249#pullrequestreview-4334215202 ✔ - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/63249#pullrequestreview-4334262768 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2026-06-01T05:00:41Z: https://ci.nodejs.org/job/node-test-pull-request/73843/ - Querying data for job/node-test-pull-request/73843/ ✔ Build data downloaded ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 63249 From https://github.com/nodejs/node * branch refs/pull/63249/merge -> FETCH_HEAD ✔ Fetched commits as 681308005866..fc2bb1a36f63 -------------------------------------------------------------------------------- Auto-merging doc/api/deprecations.md Auto-merging doc/api/errors.md Auto-merging doc/api/http2.md Auto-merging lib/internal/errors.js Auto-merging lib/internal/http2/core.js [main bb0a8d60c8] http2: error for incomplete reads on RST, auto-drain, deprecate aborted Author: Tim Perry <pimterry@gmail.com> Date: Thu May 7 19:01:04 2026 +0200 30 files changed, 639 insertions(+), 125 deletions(-) create mode 100644 test/parallel/test-http2-client-auto-discard-no-response-listener.js create mode 100644 test/parallel/test-http2-reset-aborts-readable-not-drained.js create mode 100644 test/parallel/test-http2-reset-happy-close.js create mode 100644 test/parallel/test-http2-server-respond-without-reading-body.js create mode 100644 test/parallel/test-http2-server-stream-destroy-after-reset.js Auto-merging lib/internal/http2/compat.js [main 3cb1044a7e] http2: fix stream error reporting & 'finish' in the compat API Author: Tim Perry <pimterry@gmail.com> Date: Mon May 11 16:13:56 2026 +0200 7 files changed, 26 insertions(+), 15 deletions(-) [main 55b9f921a4] Fix cpp formatting Author: Tim Perry <pimterry@gmail.com> Date: Tue May 12 12:37:01 2026 +0200 2 files changed, 4 insertions(+), 8 deletions(-) Auto-merging lib/internal/http2/core.js [main d88cae8223] Make respondWithFD match fixed H2 RST behaviour too Author: Tim Perry <pimterry@gmail.com> Date: Tue May 12 13:15:56 2026 +0200 2 files changed, 122 insertions(+), 4 deletions(-) create mode 100644 test/parallel/test-http2-respond-with-fd-finish.js Auto-merging doc/api/deprecations.md Auto-merging doc/api/http2.md [main dc85de5ff7] Add example to deprecations & update PR ids Author: Tim Perry <pimterry@gmail.com> Date: Tue May 12 14:08:57 2026 +0200 2 files changed, 46 insertions(+), 6 deletions(-) [main b4f33eccf8] Tighten up the test cases a little Author: Tim Perry <pimterry@gmail.com> Date: Tue May 12 14:15:31 2026 +0200 4 files changed, 113 insertions(+), 66 deletions(-) [main 325e2575bd] Make sure the respondWithFD test isn't flaky Author: Tim Perry <pimterry@gmail.com> Date: Tue May 12 15:59:59 2026 +0200 1 file changed, 14 insertions(+), 17 deletions(-) ✔ Patches applied There are 7 commits in the PR. Attempting autorebase. (node:363) [DEP0190] DeprecationWarning: Passing args to a child process with shell option true can lead to security vulnerabilities, as the arguments are not escaped, only concatenated. (Use `node --trace-deprecation ...` to show where the warning was created) Rebasing (2/14) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- http2: error for incomplete reads on RST, auto-drain, deprecate abortedhttps://github.com/nodejs/node/actions/runs/26745240301 |
pimterry
added
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 5f96180 |
7 participants
This PR replaces #62923, which aims to fix #56627. The goal here is to make hard-shutdown (RST_STREAM messages) for HTTP/2 streams behave predictably and remove a bunch of footguns. This became a bit of a rabbit hole, but I think I have a good understanding of the problem and a much better behaviour that should be workable, although is definitely requires some breaking changes (though ~all affected code is broken in some ways today anyway).
To implement this, this PR does a few related things:
'error'when we receiveRST_STREAM(hard-stopping the stream in both directions) in specific cases only:'response'handler before the response arrives.'data'/pipe/etc).abortedevent & field, while preserving their current behaviour for now. These are confusing to the point of being effectively broken, they're already deprecated in HTTP/1, and they're not necessary.This fixes a few major existing issues. Most notably, without this PR, if you have a stream where you're finished writing but you're only part way through reading (e.g. normal client request flow) and you get a non-error RST_STREAM (a remote stream cancellation) then we close the readable cleanly, with no errors. In effect, we currently truncate cancelled responses with no warning whatsoever. The existing
'aborted'event is only emitted for incomplete writes, not reads.This resolves that issue for reads. For writes, it stays close to existing behaviour but relaxes one key case: if you've read everything, and receive a RST_STREAM cancellation while writing (most commonly: a client aborts a request while a server is responding) then the writable closes immediately but doesn't emit an 'error' event. Effectively, we treat this as a quiet shutdown. This roughly matches HTTP/1 behaviour, and avoid spurious errors server-side. Even though there's no
'error'event, it's still easy to detect this: if you get'close' without'finish'(withoutwritableFinishedbeing true) while writing then the client has cancelled the request.We could make
abortedmatch the error behaviour here, but I've left it as is to avoid churn there, since I think we should deprecate it regardless so there's no point making unnecessary breaking changes in the meantime.In the compat API, there's two other related existing problems this fixes:
'finish'when the stream closes - even if the write was cancelled (and so never actually finished) by either peer. We now emit finish only if the writable finished.streamErrorevent, to deal with people not handling'error', and then in http2: refactor close/destroy for Http2Stream and Http2Session #17406 we removedstreamErrorentirely. The HTTP/1'aborted'event does fire in most (all?) of these cases, but that's deprecated already anyway.As a happy side bonus, as part of this work I hunted down the underlying issue for #58252: buggy cleanup of reset streams. This includes a reliable repro for the underlying issue (
test/parallel/test-http2-server-stream-destroy-after-reset.js) and fixes that test and the flaky one as well, through with better cleanup behaviour in onStreamClose.I suspect there'll be some debate around this, and it is definitely a non-trivial breaking change, but on the flip side this does show some quite substantial major issues in the current APIs, including silent read & write data loss everywhere, so I think it's worthwhile.