http2: invoke pending write callback on stream destroy - #61821
Closed
mcollina wants to merge 1 commit into
Closed
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
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
Copy link
Copy Markdown
Collaborator
|
Review requested:
|
Sorry, something went wrong.
Copy link
Copy Markdown
Member
|
Is it possible to add a dedicated test? This allows us to clean up test-http2-close-while-writing. |
Sorry, something went wrong.
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 |
Sorry, something went wrong.
Copy link
Copy Markdown
Codecov Report✅ All modified and coverable lines are covered by tests. @@ 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
... and 73 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
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. if (!waitingForWriteCallback && !waitingForEndCheck) {
const callback = cb;
cb = nop;
callback(err);
} |
Sorry, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
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