Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http2: handle existing socket data when creating HTTP/2 server sessions #41185

Merged
merged 1 commit into from
Dec 23, 2021

Conversation

pimterry
Copy link
Member

Fixes #34532 (maybe #29902 too, TBC)

When emitting a 'connection' event to manually inject connections into a server, it's common for the provided stream to already contain readable data, e.g. after sniffing a connection to detect HTTP/2 from the initial bytes.

Previously this was supported only for outgoing HTTP/2 sessions created with http2.connect(). This change ensures that HTTP/2 over existing streams is supported on both outgoing and incoming sessions.

There was an attempt to fix this previously in #34958, which works! Unfortunately that was closed and replaced by #35678, which fixes the same issue but only for outgoing connections (by fixing http2.connect directly), not all HTTP/2 sessions.

This PR takes the HTTP/2 test from #34958 (which covers the server case) and moves the fix from #35678 into the session so it applies to all HTTP/2 sessions.

When emitting a 'connection' event to manually inject connections into a
server, it's common for the provided stream to already contain readable
data, e.g. after sniffing a connection to detect HTTP/2 from the initial
bytes.

Previously this was supported only for outgoing HTTP/2 sessions created
with http2.connect(). This change ensures that HTTP/2 over existing
streams is supported on both outgoing and incoming sessions.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. labels Dec 15, 2021
@pimterry
Copy link
Member Author

The coverage-windows failure here seems to be an unrelated npm connection error installing coverage reporting tools:

Run npx c8 report
  npx c8 report
  shell: C:\Program Files\PowerShell\7\pwsh.EXE -command ". '{0}'"
  env:
    PYTHON_VERSION: 3.10
    FLAKY_TESTS: dontcare
    pythonLocation: C:\hostedtoolcache\windows\Python\3.10.0\x64
    NODE_OPTIONS: --max-old-space-size=8192
npm WARN exec The following package was not found and will be installed: c8
npm ERR! code FETCH_ERROR
npm ERR! errno FETCH_ERROR
npm ERR! invalid json response body at registry.npmjs.org/shebang-command reason: Unexpected end of JSON input

@nodejs-github-bot
Copy link
Collaborator

Copy link
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

@richardlau
Copy link
Member

The coverage-windows failure here seems to be an unrelated npm connection error installing coverage reporting tools:

Run npx c8 report
  npx c8 report
  shell: C:\Program Files\PowerShell\7\pwsh.EXE -command ". '{0}'"
  env:
    PYTHON_VERSION: 3.10
    FLAKY_TESTS: dontcare
    pythonLocation: C:\hostedtoolcache\windows\Python\3.10.0\x64
    NODE_OPTIONS: --max-old-space-size=8192
npm WARN exec The following package was not found and will be installed: c8
npm ERR! code FETCH_ERROR
npm ERR! errno FETCH_ERROR
npm ERR! invalid json response body at registry.npmjs.org/shebang-command reason: Unexpected end of JSON input

FYI npm had an outage earlier today: https://status.npmjs.org/incidents/7klw65jd5f5r

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@mcollina mcollina added commit-queue Add this label to land a pull request using GitHub Actions. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Dec 23, 2021
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 23, 2021
@nodejs-github-bot nodejs-github-bot merged commit cadeabc into nodejs:master Dec 23, 2021
@nodejs-github-bot
Copy link
Collaborator

Landed in cadeabc

@pimterry pimterry deleted the fix-http2-stream-unshift branch December 23, 2021 19:10
targos pushed a commit that referenced this pull request Jan 14, 2022
When emitting a 'connection' event to manually inject connections into a
server, it's common for the provided stream to already contain readable
data, e.g. after sniffing a connection to detect HTTP/2 from the initial
bytes.

Previously this was supported only for outgoing HTTP/2 sessions created
with http2.connect(). This change ensures that HTTP/2 over existing
streams is supported on both outgoing and incoming sessions.

PR-URL: #41185
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
When emitting a 'connection' event to manually inject connections into a
server, it's common for the provided stream to already contain readable
data, e.g. after sniffing a connection to detect HTTP/2 from the initial
bytes.

Previously this was supported only for outgoing HTTP/2 sessions created
with http2.connect(). This change ensures that HTTP/2 over existing
streams is supported on both outgoing and incoming sessions.

PR-URL: #41185
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
When emitting a 'connection' event to manually inject connections into a
server, it's common for the provided stream to already contain readable
data, e.g. after sniffing a connection to detect HTTP/2 from the initial
bytes.

Previously this was supported only for outgoing HTTP/2 sessions created
with http2.connect(). This change ensures that HTTP/2 over existing
streams is supported on both outgoing and incoming sessions.

PR-URL: nodejs#41185
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
When emitting a 'connection' event to manually inject connections into a
server, it's common for the provided stream to already contain readable
data, e.g. after sniffing a connection to detect HTTP/2 from the initial
bytes.

Previously this was supported only for outgoing HTTP/2 sessions created
with http2.connect(). This change ensures that HTTP/2 over existing
streams is supported on both outgoing and incoming sessions.

PR-URL: #41185
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP/2 server does not accept injected 'connection' events for sockets with peeked data
5 participants