Skip to content

fix(h2): reset aborted streams with CANCEL - #6201

Open
gabedemesa wants to merge 2 commits into
nodejs:mainfrom
gabedemesa:h2-abort-cancel
Open

gabedemesa wants to merge 2 commits into
nodejs:mainfrom
gabedemesa:h2-abort-cancel

Conversation

@gabedemesa

@gabedemesa gabedemesa commented Nov 12, 2026 •

Copy link
Copy Markdown

This relates to...

Refs #6181, nodejs/node#67363

Rationale

abort() in lib/dispatcher/client-h2.js closes the stream with the default NO_ERROR code and then destroys it synchronously. When the request body is still queued, Node defers a NO_ERROR RST_STREAM until the body has been written, and the synchronous destroy means it is never sent. On affected Node versions this leaves the stream open in nghttp2, which can then loop on empty DATA frames until the process runs out of memory (nodejs/node#67363, fixed in Node by nodejs/node#67372).

NGHTTP2_CANCEL is the code for a stream that is no longer needed, and Node submits it immediately, so aborts no longer depend on the Node fix.

Changes

Features

N/A

Bug Fixes

  • Close aborted HTTP/2 streams with NGHTTP2_CANCEL, and correct the comment that said close() had already queued the RST_STREAM.
  • Add test/http2-abort-rst-cancel.js: aborting a request whose body is held by flow control must reset the stream with CANCEL. Without the change the peer sees code 0.
  • Document in docs/docs/api/Client.md that an aborted HTTP/2 request resets its stream with CANCEL.

Breaking Changes and Deprecations

N/A

Testing

On Node v26.12.1, the reproduction from #6181 (concurrent fetch() uploads aborted mid-body) runs away with main and stays bounded with this change. ESLint is clean. Locally, test/h2-disconnect-guard.js, test/http2-max-concurrent-streams-zero-unref.js, test/http2-dispatcher.js and test/http2-connection.js fail or time out identically on main, so I could not use them to check this change; the other HTTP/2 tests I ran pass.

Benchmarks

The change only runs when a request is aborted.

Aborted uploads (32 KB fetch() POSTs over HTTP/2, aborted on the next tick, 20,000 requests, 32 concurrent, three runs each). This used Node main with nodejs/node#67372 applied, because main of undici runs away on released Node versions in this scenario:

Run 1 Run 2 Run 3 RSS
main 11,669 req/s 12,131 req/s 11,406 req/s 267 MB
This PR 12,281 req/s 11,816 req/s 11,629 req/s 267 MB

benchmarks/benchmark-http2.js (SAMPLES=30 CONNECTIONS=50, Node v26.12.1), which does not abort requests:

Test main This PR
native - http2 6,627.83 req/s ± 4.30% 6,737.49 req/s ± 3.47%
undici - fetch 8,615.87 req/s ± 5.24% 8,840.41 req/s ± 1.80%
undici - pipeline 10,952.08 req/s ± 2.75% 11,245.60 req/s ± 2.80%
undici - dispatch 13,022.13 req/s ± 2.52% 13,398.57 req/s ± 2.64%
undici - stream 13,579.04 req/s ± 2.91% 14,894.53 req/s ± 2.96%
undici - request 13,630.19 req/s ± 0.97% 10,844.58 req/s ± 7.13%

The differences go in both directions and are run-to-run variation; the changed code is not reached in this benchmark. Both on macOS arm64.

Status

🤖 Generated with Claude Code

abort() closed the stream with the default NO_ERROR code and then
destroyed it synchronously. When the request body is still queued,
Node defers a NO_ERROR RST_STREAM until the body has been written, and
the synchronous destroy means that never happens, so the peer is never
told the stream is gone.

Close with NGHTTP2_CANCEL, the code for a stream that is no longer
needed, which Node submits immediately.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: gdemesa <gdemesa@opengov.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: gdemesa <gdemesa@opengov.com>
@codecov-commenter

codecov-commenter commented Nov 13, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.67%. Comparing base (691eb16) to head (3808fd1).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6201      +/-   ##
==========================================
- Coverage   91.68%   91.67%   -0.01%     
==========================================
  Files         113      113              
  Lines       44449    44442       -7     
==========================================
- Hits        40751    40744       -7     
  Misses       3698     3698              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@gabedemesa

Copy link
Copy Markdown
Author

Thanks for the review! This is approved and CI is green. Could a collaborator land it when convenient?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants