Skip to content

http2: invoke pending write callback on stream destroy - #61821

Closed
mcollina wants to merge 1 commit into
nodejs:mainfrom
mcollina:http2-flaky-mac
Closed

http2: invoke pending write callback on stream destroy#61821
mcollina wants to merge 1 commit into
nodejs:mainfrom
mcollina:http2-flaky-mac

Conversation

@mcollina

Copy link
Copy Markdown
Member

Summary

  • Track the pending write callback on Http2Stream via kPendingWriteCb and invoke it in _destroy() before calling handle.destroy(), ensuring the Writable stream can clean up properly when a stream is destroyed mid-write
  • Make the write callback idempotent so duplicate invocations from the C++ side are harmless no-ops

When an HTTP/2 stream is destroyed while a write is in progress, the pending write callback may never be called if the write data has already been consumed by nghttp2 and moved to the session's outgoing_buffers_. This leaves the Writable stream's kWriting flag set permanently, preventing errorBuffer from cleaning up remaining buffered writes and keeping references alive that block event loop exit.

Fixes: #58252
Refs: #58253

Test plan

  • test-http2-close-while-writing passes consistently
  • Stress tested with 500 repetitions — zero failures
  • Full HTTP/2 test suite (test-http2-*) passes with no regressions

When an HTTP/2 stream is destroyed while a write is in progress, the
pending write callback may never be called if the write data has already
been consumed by nghttp2 and moved to the session's outgoing_buffers_.
In this case, the C++ side's SetImmediate cleanup finds nothing in the
stream's write queue, and the callback depends on session socket write
completion — which may never happen during shutdown.
This leaves the Writable stream's internal state stuck, preventing
cleanup of buffered writes and keeping references alive that block
event loop exit.
Track the pending write callback on the stream and invoke it in
_destroy() before calling handle.destroy(), ensuring the Writable
stream can clean up properly. The callback is made idempotent so
duplicate invocations from the C++ side are harmless no-ops.
Fixes: nodejs#58252
Refs: nodejs#58253
@mcollina
mcollina requested a review from lpincaFebruary 14, 2026 17:37
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net

@nodejs-github-botnodejs-github-bot added http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. labels Feb 14, 2026
@lpinca

lpinca commented Feb 14, 2026

Copy link
Copy Markdown
Member

Is it possible to add a dedicated test? This allows us to clean up test-http2-close-while-writing.

@lpinca

Copy link
Copy Markdown
Member

It does not fix the issue:

$ python3 tools/test.py --repeat=10000 parallel/test-http2-close-while-writing
=== release test-http2-close-while-writing === Path: parallel/test-http2-close-while-writing
Command: out/Release/node /Users/luigi/code/node/test/parallel/test-http2-close-while-writing.js
--- TIMEOUT ---
=== release test-http2-close-while-writing === Path: parallel/test-http2-close-while-writing
Command: out/Release/node /Users/luigi/code/node/test/parallel/test-http2-close-while-writing.js
--- TIMEOUT ---
=== release test-http2-close-while-writing === Path: parallel/test-http2-close-while-writing
Command: out/Release/node /Users/luigi/code/node/test/parallel/test-http2-close-while-writing.js
--- TIMEOUT ---
=== release test-http2-close-while-writing === Path: parallel/test-http2-close-while-writing
Command: out/Release/node /Users/luigi/code/node/test/parallel/test-http2-close-while-writing.js
--- TIMEOUT ---
=== release test-http2-close-while-writing === Path: parallel/test-http2-close-while-writing
Command: out/Release/node /Users/luigi/code/node/test/parallel/test-http2-close-while-writing.js
--- TIMEOUT ---
[05:03|% 100|+ 9995|- 5]: Done Failed tests:
out/Release/node /Users/luigi/code/node/test/parallel/test-http2-close-while-writing.js
out/Release/node /Users/luigi/code/node/test/parallel/test-http2-close-while-writing.js
out/Release/node /Users/luigi/code/node/test/parallel/test-http2-close-while-writing.js
out/Release/node /Users/luigi/code/node/test/parallel/test-http2-close-while-writing.js
out/Release/node /Users/luigi/code/node/test/parallel/test-http2-close-while-writing.js

@codecov

codecovBot commented Feb 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.72%. Comparing base (ae2ffce) to head (c16f526).
⚠️ Report is 406 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #61821 +/- ##
==========================================
- Coverage 89.75% 89.72% -0.03% 
==========================================
Files 674 675 +1 Lines 204416 204807 +391 Branches 39285 39351 +66 ==========================================
+ Hits 183472 183766 +294 - Misses 13227 13331 +104 + Partials 7717 7710 -7 
Files with missing linesCoverage Δ
lib/internal/http2/core.js95.20% <100.00%> (-0.03%)⬇️

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

@amyssnippet

Copy link
Copy Markdown
Contributor

i think this partially addresses the issue, but there are a few tests which are still flaking as @lpinca mentioned.
i suspect the done() barrier in lib/internal/http2/core.js is the remaining culprit. Even with pendingWriteCb being invoked, done() still gates on both waitingForWriteCallback and waitingForEndCheck like

if(!waitingForWriteCallback&&!waitingForEndCheck){constcallback=cb;cb=nop;callback(err);}

@mcollinamcollina closed this Apr 5, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

http2Issues or PRs related to the http2 subsystem.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

parallel/test-http2-close-while-writing is flaky

4 participants

@mcollina@nodejs-github-bot@lpinca@amyssnippet