fix: prevent race condition between onEnd and onTrailers in HTTP/2 client (#5216)#5343
Draft
mcollina wants to merge 2 commits into
Draft
fix: prevent race condition between onEnd and onTrailers in HTTP/2 client (#5216)#5343mcollina wants to merge 2 commits into
mcollina wants to merge 2 commits into
Conversation
…ient
When the 'end' event fires before 'trailers' (observed on Windows),
onEnd would call onResponseEnd({}) with empty trailers and remove
the 'trailers' listener, causing the flaky test in issue #5216.
Fix: in onEnd, check if the 'trailers' listener is still registered.
If so, defer completion to onTrailers via a process.nextTick fallback.
This ensures onTrailers gets a chance to pass the actual trailers,
regardless of event ordering.
Adapts the test 'Should remove request-owned http2 stream listeners
after completion' to check listener counts after the async fallback.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5343 +/- ##
==========================================
- Coverage 93.23% 93.23% -0.01%
==========================================
Files 110 110
Lines 36631 36688 +57
==========================================
+ Hits 34154 34206 +52
- Misses 2477 2482 +5 ☔ View full report in Codecov by Sentry. 🚀 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
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.
Description
Fixes a flaky test
test/http2-trailers.js(issue #5216) caused by a race condition between theonEndandonTrailersevent handlers in the HTTP/2 client (lib/dispatcher/client-h2.js).Root Cause
On Windows, Node.js can emit
'end'before'trailers'(reversing the expected event order). WhenonEndfires first, it callsrequest.onResponseEnd({})with empty trailers and removes the'trailers'listener viareleaseRequestStream(stream). By the timeonTrailersfires, it seesrequest.completed === trueand returns without passing the actual trailers. The test assertiont.strictEqual(trailers['x-trailer'], 'hello')then fails.Fix
In
onEnd, checkstream.listenerCount('trailers')before handling completion:'trailers'listener is still registered, defer completion toonTrailersvia aprocess.nextTickfallback instead of finalizing immediately.onTrailersnever fires (no-trailers case), the fallback handles completion.onTrailersfires first (normal event order), it removes the'end'listener andonEndnever runs.Test Adaptation
The test 'Should remove request-owned http2 stream listeners after completion' now checks listener counts after the
process.nextTickfallback has run, since listeners are now removed asynchronously in the no-trailers case.Testing
All 295 HTTP/2 and client tests pass, including
test/http2-trailers.js.