http2: fix onread assert when destroying session from stream handler by sankalpsthakur · Pull Request #65116 · nodejs/node

sankalpsthakur · 2026-08-07T18:42:47Z

When session.destroy() is called from a 'stream' handler, MakeCallback drains nextTick while nghttp2 is still inside mem_recv. Session close is deferred for that window (from #64166), so later HEADERS in the same receive buffer created C++ streams without a JS wrapper / onread. Subsequent DATA delivery hit:

Assertion failed: onread->IsFunction()
  <- StreamBase::CallJSOnreadMethod
  <- Http2StreamListener::OnStreamRead
  <- Http2Session::OnDataChunkReceived

Changes

Do not create streams while the session is closing (OnBeginHeadersCallback) — RST with NGHTTP2_CANCEL instead.
If on_headers runs after JS destroy, destroy the C++ handle (mirrors the existing session.closed path).
Defensive: drop DATA when onread is not a function rather than aborting.
Regression test exercising concurrent POSTs with session.destroy() from the stream handler.

Test plan

New test/parallel/test-http2-session-destroy-stream-handler.js (40 rounds × 8 concurrent POSTs)

Fixes #64850

AI/LLM disclosure

AI coding tools (including Grok and/or Codex agent-assisted editing) were used to help draft or modify code and this PR description.
I reviewed the complete change, understand the reasoning, and added the regression test listed above.
This submission is original work of authorship under the project CLA / contributor terms; AI output was not pasted unreviewed.

When session.destroy() runs from a 'stream' handler, MakeCallback drains nextTick while nghttp2 is still inside mem_recv. Close is deferred for that window (see nodejs#64166), so later HEADERS in the same buffer created C++ streams without a JS wrapper or onread, and DATA delivery aborted with Assertion failed: onread->IsFunction(). - Reject new streams while the session is closing - Destroy the C++ handle if on_headers runs after JS destroy - Drop DATA when onread is not installed (defensive) Fixes: nodejs#64850 Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com>

nodejs-github-bot · 2026-08-07T18:42:52Z

Review requested:

@nodejs/http2
@nodejs/net

mcollina

lgtm

nodejs-github-bot · 2026-08-31T09:45:59Z

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

codecov · 2026-08-31T11:23:32Z

Codecov Report

❌ Patch coverage is 63.63636% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.05%. Comparing base (e2d7b34) to head (ae6f128).
⚠️ Report is 422 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/http2/core.js	50.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #65116      +/-   ##
==========================================
- Coverage   90.31%   90.05%   -0.27%     
==========================================
  Files         759      754       -5     
  Lines      248290   256406    +8116     
  Branches    46859    48500    +1641     
==========================================
+ Hits       224241   230895    +6654     
- Misses      15472    16620    +1148     
- Partials     8577     8891     +314

Files with missing lines	Coverage Δ
src/node_http2.cc	`81.96% <100.00%> (+0.21%)`	⬆️
lib/internal/http2/core.js	`94.99% <50.00%> (-0.09%)`	⬇️

... and 269 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.

pimterry

Thanks @sankalpsthakur!

Mostly looks good. The 3 fixes seem to all separately solve the same issue redundantly, is that right? I think for the first two that's OK and they're independently valuable (with minor tweaks) but we should drop the 3rd chunk to avoid missing major issues in future.

There's also a failing lint here, you can fix this locally with make format-cpp.

pimterry · 2026-08-31T10:23:37Z

+          session->session(),
+          NGHTTP2_FLAG_NONE,
+          id,
+          NGHTTP2_CANCEL);


I think this should be NGHTTP2_REFUSED_STREAM (which tells the client that we didn't even start processing, and so it can safely retry) rather than CANCEL which just says we didn't finish.

pimterry · 2026-08-31T10:43:56Z

+          ->GetInternalField(StreamBase::kOnReadFunctionField)
+          .As<Value>();
+  if (!onread->IsFunction())
+    return;


The other 2 fixes handle a specific valid shutdown scenario and cleanly make sure we don't start new work.

This fix though covers a generic broken state by simply ignoring it and dropping data.

We shouldn't do this. With this other fixes this is now unreachable, but if there's any other future scenario where we end up with a broken read callback state here somehow, we want to know about it rather than losing data silently.

pimterry · 2026-08-31T12:38:36Z

+  // still walking the receive buffer). Tear down the C++ stream so subsequent
+  // DATA frames do not call CallJSOnreadMethod with a missing onread.
+  if (session.destroyed) {
+    handle.destroy();


Nit: This should send REFUSED_STREAM too I think, matching the session.closed below that it's mirroring.

Nit because really this code is just defense in depth, and the 2nd chunk here makes it unreachable, but if we ever did reach it somehow, we would want to tell the other peer we haven't processed their stream so they can retry it, rather than silently disappearing.

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 Aug 7, 2026

mcollina approved these changes Aug 31, 2026

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 31, 2026

mcollina requested a review from pimterry August 31, 2026 09:44

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 31, 2026

pimterry requested changes Aug 31, 2026

View reviewed changes

sankalpsthakur added 2 commits September 1, 2026 01:42

http2: use refused stream on deferred close

5ef12f1

http2: address review feedback

ae6f128

Navigation Menu

Uh oh!

Uh oh!

http2: fix onread assert when destroying session from stream handler - #65116

http2: fix onread assert when destroying session from stream handler#65116
sankalpsthakur wants to merge 3 commits into
nodejs:mainfrom
sankalpsthakur:fix/64850-http2-onread-assert

sankalpsthakur commented Aug 7, 2026 •

edited

Loading

Uh oh!

nodejs-github-bot commented Aug 7, 2026

Uh oh!

mcollina left a comment

Uh oh!

nodejs-github-bot commented Aug 31, 2026

Uh oh!

codecov Bot commented Aug 31, 2026 •

edited

Loading

Uh oh!

pimterry left a comment

Uh oh!

pimterry Aug 31, 2026

Uh oh!

pimterry Aug 31, 2026

Uh oh!

pimterry Aug 31, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Uh oh!

Conversation

sankalpsthakur commented Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Test plan

AI/LLM disclosure

Uh oh!

nodejs-github-bot commented Aug 7, 2026

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Aug 31, 2026

Uh oh!

codecov Bot commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pimterry left a comment

Choose a reason for hiding this comment

Uh oh!

pimterry Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

pimterry Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

pimterry Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sankalpsthakur commented Aug 7, 2026 •

edited

Loading

codecov Bot commented Aug 31, 2026 •

edited

Loading

pimterry Aug 31, 2026 •

edited

Loading