Skip to content

fix(h2): ignore late data frames after request completion#4845

Merged
metcoder95 merged 1 commit intomainfrom
fix/h2-ignore-late-data-after-complete
Mar 4, 2026
Merged

fix(h2): ignore late data frames after request completion#4845
metcoder95 merged 1 commit intomainfrom
fix/h2-ignore-late-data-after-complete

Conversation

@mcollina
Copy link
Member

Description

Fixes a crash in the HTTP/2 dispatcher when a late data event arrives after the request has already completed.

In client-h2, stream.on('data') called request.onData() unconditionally. If completion happened first (for example via trailers), request.onData() could hit the internal assert(!this.completed) and crash the process.

This change:

  • Adds an early return in the H2 data handler when request.aborted || request.completed
  • Removes data listeners before invoking request.onComplete(trailers) in the trailers path
  • Adds a regression test that simulates response -> trailers -> late data

Fixes: #4843

Testing

  • npx borp -p "test/http2-late-data.js"
  • npx borp -p "test/http2-trailers.js"
  • npx borp -p "test/http2-dispatcher.js"
  • npm run lint

Guard the HTTP/2 data handler when a request is already completed or aborted and remove data listeners before completing from trailers.

Adds a regression test covering trailers followed by late data without crashing.

Fixes: #4843
hxinhan

This comment was marked as duplicate.

// the request remains in-flight and headers hasn't been received yet
// for those scenarios, best effort is to destroy the stream immediately
// as there's no value to keep it open.
if (request.aborted) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also check if request is complete here?

Copy link
Contributor

@hxinhan hxinhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this bug report. I left a comment about potential request completion check in additional call path

@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.13%. Comparing base (0a23610) to head (e9b08c5).
⚠️ Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
lib/dispatcher/client-h2.js 60.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4845      +/-   ##
==========================================
- Coverage   93.19%   93.13%   -0.06%     
==========================================
  Files         109      109              
  Lines       34222    34244      +22     
==========================================
+ Hits        31893    31894       +1     
- Misses       2329     2350      +21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 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.

@mcollina mcollina requested a review from metcoder95 March 2, 2026 08:22
@hxinhan
Copy link
Contributor

hxinhan commented Mar 4, 2026

Should we merge the PR now?

@metcoder95 metcoder95 merged commit 13d5fef into main Mar 4, 2026
84 of 93 checks passed
@metcoder95 metcoder95 deleted the fix/h2-ignore-late-data-after-complete branch March 4, 2026 16:37
@github-actions github-actions bot mentioned this pull request Mar 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HTTP/2: assert(!this.completed) crash in Request.onData

4 participants