http2: invoke pending write callback on stream destroy#61821
Open
mcollina wants to merge 1 commit intonodejs:mainfrom
Open
http2: invoke pending write callback on stream destroy#61821mcollina wants to merge 1 commit intonodejs:mainfrom
mcollina wants to merge 1 commit intonodejs:mainfrom
Conversation
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
Collaborator
|
Review requested:
|
Member
|
Is it possible to add a dedicated test? This allows up to clean up |
Member
|
It does not fix the issue: |
Codecov Report✅ All modified and coverable lines are covered by tests. 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
🚀 New features to boost your workflow:
|
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
3 participants
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
Http2StreamviakPendingWriteCband invoke it in_destroy()before callinghandle.destroy(), ensuring the Writable stream can clean up properly when a stream is destroyed mid-writeWhen 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'skWritingflag set permanently, preventingerrorBufferfrom cleaning up remaining buffered writes and keeping references alive that block event loop exit.Fixes: #58252
Refs: #58253
Test plan
test-http2-close-while-writingpasses consistentlytest-http2-*) passes with no regressions