Skip to content

http2: send deferred RST_STREAM on stream destroy - #67372

Open
inoway46 wants to merge 1 commit into
nodejs:mainfrom
inoway46:http2-rst-on-destroy-after-close
Open

http2: send deferred RST_STREAM on stream destroy#67372
inoway46 wants to merge 1 commit into
nodejs:mainfrom
inoway46:http2-rst-on-destroy-after-close

Conversation

@inoway46

@inoway46 inoway46 commented Nov 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

close() with NO_ERROR waits for 'finish' before sending RST_STREAM. If the body is stuck behind flow control and the stream is destroyed first, RST_STREAM is never sent and nghttp2 keeps the stream open. On the next WINDOW_UPDATE, OnRead finds no stream and returns 0 without EOF, so nghttp2 sends empty DATA frames in a loop until the process runs out of memory.

The issue reports this for client requests, but server responses are affected in the same way.

Send the deferred RST_STREAM in _destroy(). As a backstop, return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE from OnRead when the stream is gone, which nghttp2 documents as the way to reset a stream on error. Add regression tests for client and server streams.

Both tests run out of memory on current main, v22.23.4, v24.22.2 and v26.13.0, and pass with this change.

Fixes: #67363

@nodejs-github-bot

nodejs-github-bot commented Nov 10, 2026

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues and PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. labels Nov 10, 2026
@inoway46
http2: send deferred RST_STREAM on stream destroy
7288a16 
close() with NO_ERROR waits for 'finish' before sending RST_STREAM.
If the body is stuck behind flow control and the stream is destroyed
first, RST_STREAM is never sent and nghttp2 keeps the stream open.
On the next WINDOW_UPDATE, OnRead finds no stream and returns 0
without EOF, so nghttp2 sends empty DATA frames until OOM. Both
client and server streams are affected.

Send the deferred RST_STREAM in _destroy(). As a backstop, return
NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE from OnRead when the stream is
gone, which nghttp2 documents as the way to reset a stream on error.

Fixes: nodejs#67363
Signed-off-by: inoway46 <[email protected]>
@inoway46
inoway46 force-pushed the http2-rst-on-destroy-after-close branch from 5708532 to 7288a16 Compare November 10, 2026 06:14
@inoway46
inoway46 marked this pull request as ready for review November 10, 2026 06:20
@inoway46 inoway46 mentioned this pull request Nov 10, 2026
Open
@codecov

codecov Bot commented Nov 10, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.71%. Comparing base (2929073) to head (7288a16).
⚠️ Report is 58 commits behind head on main.

Files with missing lines Patch % Lines
src/node_http2.cc 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #67372      +/-   ##
==========================================
- Coverage   92.62%   90.71%   -1.91%     
==========================================
  Files         430      798     +368     
  Lines      196333   280032   +83699     
  Branches    29897    53860   +23963     
==========================================
+ Hits       181849   254038   +72189     
- Misses      14159    16283    +2124     
- Partials      325     9711    +9386
Files with missing lines Coverage Δ
lib/internal/http2/core.js 94.94% <100.00%> (+0.14%) ⬆️
src/node_http2.cc 82.02% <0.00%> (ø)

... and 508 files with indirect coverage changes

🚀 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 gabedemesa mentioned this pull request Nov 12, 2026
Open
7 tasks done
@gabedemesa

gabedemesa commented Nov 12, 2026 •
edited
Loading

Copy link
Copy Markdown

I tested this against the production case in nodejs/undici#6181: a Node 26.12.1 service using fetch over HTTP/2 that was OOM-killed in Http2Session::CopyDataIntoOutgoing.

With this commit cherry-picked onto main at 75d55af (macOS arm64), none of the following run away. Without it, each one exceeds 1.5 GB of RSS within about a second on v26.12.1:

  • Two or more concurrent fetch() POSTs with 32 KB bodies, aborted mid-upload.
  • A server that advertises a zero initial window, waits for the client's AbortSignal.timeout() to fire, then raises the window.
  • Plain node:http2: close() followed by destroy() while the body is still queued.
  • 5,000 aborted uploads with random body sizes against a slow reader.

The parallel/test-http2-* and sequential/test-http2-* suites pass with the change.

We are hitting this in production: a Node 26.12.1 service using fetch over HTTP/2 is being OOM-killed (details in nodejs/undici#6181). Could this be considered for review, and for backporting to v26.x and v24.x once it lands? Thank you.

@gabedemesa gabedemesa mentioned this pull request Nov 12, 2026
Open
@pimterry pimterry added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 12, 2026
@pimterry

pimterry commented Nov 12, 2026

Copy link
Copy Markdown
Member

Looks like we'll need manual backports for this

@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 12, 2026
@nodejs-github-bot

nodejs-github-bot commented Nov 12, 2026

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/79006/

This was referenced Nov 12, 2026
Draft
Draft
@gabedemesa

gabedemesa commented Nov 12, 2026

Copy link
Copy Markdown

Looks like we'll need manual backports for this

raised backports, will rebase them once this is merged.

@pimterry pimterry added the resume-ci Add this label to resume the latest eligible Jenkins CI run on a PR with an approving review. label Nov 12, 2026
@github-actions github-actions Bot removed the resume-ci Add this label to resume the latest eligible Jenkins CI run on a PR with an approving review. label Nov 12, 2026
@nodejs-github-bot

nodejs-github-bot commented Nov 12, 2026

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/79016/

@inoway46 inoway46 added the resume-ci Add this label to resume the latest eligible Jenkins CI run on a PR with an approving review. label Nov 13, 2026
@github-actions github-actions Bot removed the resume-ci Add this label to resume the latest eligible Jenkins CI run on a PR with an approving review. label Nov 13, 2026
@nodejs-github-bot

nodejs-github-bot commented Nov 13, 2026

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/79022/

jazelly
jazelly approved these changes Nov 13, 2026
View reviewed changes

@jazelly jazelly left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@inoway46 inoway46 added the resume-ci Add this label to resume the latest eligible Jenkins CI run on a PR with an approving review. label Nov 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pimterry pimterry pimterry approved these changes
@jazelly jazelly jazelly approved these changes
+1 more reviewer
@metcoder95 metcoder95 metcoder95 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues and PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. resume-ci Add this label to resume the latest eligible Jenkins CI run on a PR with an approving review.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

http2: closing and then destroying a client stream whose body is still queued sends no RST_STREAM and loops on empty DATA frames until OOM

6 participants

@inoway46 @nodejs-github-bot @gabedemesa @pimterry @metcoder95 @jazelly