Skip to content

fix(http2): reject upgrade streams closed before response headers#5069

Merged
mcollina merged 4 commits intonodejs:mainfrom
trivikr:h2-websocket-client-upgrade
Apr 21, 2026
Merged

fix(http2): reject upgrade streams closed before response headers#5069
mcollina merged 4 commits intonodejs:mainfrom
trivikr:h2-websocket-client-upgrade

Conversation

@trivikr
Copy link
Copy Markdown
Member

@trivikr trivikr commented Apr 19, 2026

This relates to...

Fixes: #5064

Rationale

The H2 upgrade paths only advanced the request once a response event arrived.

If the peer closed the stream before sending headers, the request was never settled and the queue slot was not released, which could leave the operation hanging and interfere with later cleanup.

Changes

  • Added shared pre-response handling for H2 upgrade streams in lib/dispatcher/client-h2.js
  • Count upgrade/connect streams as open immediately after session.request(...)
  • Reject upgrade/connect requests on pre-header error, end, or close
  • Advance the client queue when those failures happen so the request does not remain stuck

Features

N/A

Bug Fixes

  • Fixed an H2 websocket upgrade hang when the server closes the stream before sending response headers
  • Fixed the same pre-header hang in the H2 CONNECT path
  • Fixed request queue cleanup for those failure cases so client.close() does not wait on a stuck upgrade/connect request

Breaking Changes and Deprecations

N/A

Status

@mcollina
Copy link
Copy Markdown
Member

CI is very red

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 19, 2026

Codecov Report

❌ Patch coverage is 98.24561% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 93.11%. Comparing base (79b7ebd) to head (6bc1a56).

Files with missing lines Patch % Lines
lib/dispatcher/client-h2.js 98.24% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5069      +/-   ##
==========================================
+ Coverage   93.09%   93.11%   +0.01%     
==========================================
  Files         110      110              
  Lines       35801    35818      +17     
==========================================
+ Hits        33330    33351      +21     
+ Misses       2471     2467       -4     

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

@trivikr trivikr force-pushed the h2-websocket-client-upgrade branch from 97e7827 to 0e144eb Compare April 19, 2026 16:18
@trivikr
Copy link
Copy Markdown
Member Author

trivikr commented Apr 20, 2026

The CI failures caused by timeouts in ‎test/websocket/opening-handshake.js were fixed by tracking server-side HTTP/2 sessions and closing them explicitly during teardown

Copy link
Copy Markdown
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina requested a review from metcoder95 April 20, 2026 05:38
@trivikr trivikr force-pushed the h2-websocket-client-upgrade branch from 5ee2fce to 7338e9b Compare April 20, 2026 15:48
@trivikr trivikr force-pushed the h2-websocket-client-upgrade branch from 7338e9b to 2eced88 Compare April 21, 2026 06:03
Copy link
Copy Markdown
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit 7b86e7b into nodejs:main Apr 21, 2026
35 checks passed
@trivikr trivikr deleted the h2-websocket-client-upgrade branch April 21, 2026 15:19
@github-actions github-actions Bot mentioned this pull request May 1, 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 websocket client.upgrade() hangs if the stream closes before response headers

3 participants