Skip to content

[v22.x backport] http2: fix allowHttp1+Upgrade, broken by shouldUpgradeCallback - #60341

Closed
pimterry wants to merge 1 commit into
nodejs:v22.x-stagingfrom
pimterry:backport-59924-to-22
Closed

[v22.x backport] http2: fix allowHttp1+Upgrade, broken by shouldUpgradeCallback#60341
pimterry wants to merge 1 commit into
nodejs:v22.x-stagingfrom
pimterry:backport-59924-to-22

Conversation

@pimterry

Copy link
Copy Markdown
Member

Backport for #59924.

This also backports the corresponding full code for the websocket test server into the tests. That was introduced in #59404 (which is marked dont-land-on-v22) but it's required for the tests for this change.

This is required to use HTTP/1 websockets on an HTTP/2 server, which is
fairly common as websockets over HTTP/2 is much less widely supported.
This was broken by the recent shouldUpgradeCallback HTTP/1 addition,
which wasn't correctly added to the corresponding allowHttp1 part of
the HTTP/2 implementation.
PR-URL: nodejs#59924
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net

@nodejs-github-botnodejs-github-bot added http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch. labels Oct 21, 2025
@marco-ippolitomarco-ippolito added v20.x and removed v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch. labels Oct 22, 2025
@marco-ippolito

marco-ippolito commented Oct 22, 2025

Copy link
Copy Markdown
Member

Actually this points to v22, perhaps the title is wrong?

@marco-ippolitomarco-ippolito added v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch. and removed v20.x labels Oct 22, 2025
@pimterrypimterry changed the title [v20.x backport] http2: fix allowHttp1+Upgrade, broken by shouldUpgradeCallback[v22.x backport] http2: fix allowHttp1+Upgrade, broken by shouldUpgradeCallbackOct 22, 2025
@pimterry

Copy link
Copy Markdown
MemberAuthor

@marco-ippolito yes, you're totally right, now fixed - this is indeed for v22.

@pimterrypimterry added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 22, 2025
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 22, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

aduh95 pushed a commit that referenced this pull request Oct 22, 2025
This is required to use HTTP/1 websockets on an HTTP/2 server, which is
fairly common as websockets over HTTP/2 is much less widely supported.
This was broken by the recent shouldUpgradeCallback HTTP/1 addition,
which wasn't correctly added to the corresponding allowHttp1 part of
the HTTP/2 implementation.
PR-URL: #59924
Backport-PR-URL: #60341
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@aduh95

Copy link
Copy Markdown
Contributor

Landed in 23468fd

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

http2Issues or PRs related to the http2 subsystem.needs-ciPRs that need a full CI run.v22.xIssues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@pimterry@nodejs-github-bot@marco-ippolito@aduh95@richardlau@trivikr