http2: reinject data received before http2 is attached by mmomtchev · Pull Request #35678 · nodejs/node

mmomtchev · 2020-10-16T11:44:25Z

[x ] make -j4 test (UNIX), or vcbuild test (Windows) passes
[x ] tests and/or benchmarks are included
[x ] commit message follows commit guidelines

Reinject the data already received from the TLS socket when the HTTP2 client is attached with a delay Fixes: nodejs#35475

nodejs-github-bot · 2020-10-16T11:44:30Z

Review requested:

@nodejs/http2
@nodejs/net

addaleax

Can you also do this for TLS, so that #34958 is fully covered? :)

mmomtchev · 2020-10-16T11:51:42Z

Can you also do this for TLS, so that #34958 is fully covered? :)

Not very sure I understand, in fact I shamelessly copied that from the TLS code?
#34958 seems to be an alternative implementation of the same problem?

mcollina

lgtm

addaleax · 2020-10-16T11:53:27Z

Ah, right :) Cool. I’ll close my PR. :)

mmomtchev · 2020-10-16T12:12:22Z

That PR has one drawback: it doesn't allow for the user code to listen for remote_settings - when the data is already waiting in the socket, remote_settings will be sent before the user callback has been installed

nodejs-github-bot · 2020-10-16T20:32:53Z

CI: https://ci.nodejs.org/job/node-test-pull-request/33679/

github-actions · 2020-10-17T09:01:02Z

Commit Queue failed

- Loading data for nodejs/node/pull/35678
✔  Done loading data for nodejs/node/pull/35678
----------------------------------- PR info ------------------------------------
Title      http2: reinject data received before http2 is attached (#35678)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     mmomtchev:http2-tls-reinject -> nodejs:master
Labels     C++, lib / src
Commits    1
 - http2: reinject data received before http2 is attached
Committers 1
 - Momtchil Momtchev 
PR-URL: https://github.com/nodejs/node/pull/35678
Reviewed-By: Anna Henningsen 
Reviewed-By: Matteo Collina 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/35678
Reviewed-By: Anna Henningsen 
Reviewed-By: Matteo Collina 
--------------------------------------------------------------------------------
   ✖  Last GitHub CI failed
   ℹ  Last Full PR CI on 2020-10-16T20:32:53Z: https://ci.nodejs.org/job/node-test-pull-request/33679/
- Querying data for job/node-test-pull-request/33679/
✔  Build data downloaded
- Querying failures of job/node-test-commit/41433/
✔  Data downloaded
   ✖  3 failure(s) on the last Jenkins CI run
   ℹ  This PR was created on Fri, 16 Oct 2020 11:44:25 GMT
   ✔  Approvals: 2
   ✔  - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/35678#pullrequestreview-510395321
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/35678#pullrequestreview-510398648
   ✖  This PR needs to wait 26 more hours to land
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

Commit Queue action: https://github.com/nodejs/node/actions/runs/312171239

mildsunrise

Thanks for the patch :)

lib/internal/http2/core.js

mildsunrise · 2020-10-18T11:55:37Z

I still think I prefer @addaleax approach (using JSStreamSocket in these cases) because it's simple and doesn't require us to add 'inject' logic to every native handle, doesn't have the drawback mentioned here, and can be reverted easily once/if we fix this properly in StreamBase. But no big deal, let's fix this for now and we'll see :)

@mildsunrise

We reinject when the sockets has already waiting data, remarked by @mildsunrise Co-authored-by: Alba Mendez <me@alba.sh>

nodejs-github-bot · 2020-10-20T02:28:19Z

CI: https://ci.nodejs.org/job/node-test-pull-request/33739/

rickyes

lgtm, thx

nodejs-github-bot · 2020-10-21T12:55:12Z

CI: https://ci.nodejs.org/job/node-test-pull-request/33778/ ✅

github-actions · 2020-10-25T10:29:50Z

Landed in 629e1ab...1f703e1

Reinject the data already received from the TLS socket when the HTTP2 client is attached with a delay Fixes: #35475 PR-URL: #35678 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Alba Mendez <me@alba.sh> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com>

@mildsunrise

We reinject when the sockets has already waiting data, remarked by @mildsunrise Co-authored-by: Alba Mendez <me@alba.sh> PR-URL: #35678 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Alba Mendez <me@alba.sh> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com>

mildsunrise · 2020-10-25T11:45:54Z

hmm, this landed as two separate commits instead of squashing them into one... what do we do in these cases?

mildsunrise · 2020-10-25T11:50:46Z

it's still (almost) at the tip of the branch, I volunteer to do a force push to undo and land properly if given permission... if it's too risky we can leave it like that

mutza97 · 2020-10-25T13:46:23Z

Reply

Reinject the data already received from the TLS socket when the HTTP2 client is attached with a delay Fixes: #35475 PR-URL: #35678 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Alba Mendez <me@alba.sh> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com>

@mildsunrise

We reinject when the sockets has already waiting data, remarked by @mildsunrise Co-authored-by: Alba Mendez <me@alba.sh> PR-URL: #35678 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Alba Mendez <me@alba.sh> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com>

Reinject the data already received from the TLS socket when the HTTP2 client is attached with a delay Fixes: #35475 PR-URL: #35678 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Alba Mendez <me@alba.sh> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com>

@mildsunrise

We reinject when the sockets has already waiting data, remarked by @mildsunrise Co-authored-by: Alba Mendez <me@alba.sh> PR-URL: #35678 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Alba Mendez <me@alba.sh> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com>

Reinject the data already received from the TLS socket when the HTTP2 client is attached with a delay Fixes: #35475 PR-URL: #35678 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Alba Mendez <me@alba.sh> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com>

@mildsunrise

We reinject when the sockets has already waiting data, remarked by @mildsunrise Co-authored-by: Alba Mendez <me@alba.sh> PR-URL: #35678 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Alba Mendez <me@alba.sh> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com>

Reinject the data already received from the TLS socket when the HTTP2 client is attached with a delay Fixes: #35475 PR-URL: #35678 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Alba Mendez <me@alba.sh> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com>

@mildsunrise

We reinject when the sockets has already waiting data, remarked by @mildsunrise Co-authored-by: Alba Mendez <me@alba.sh> PR-URL: #35678 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Alba Mendez <me@alba.sh> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com>

http2: reinject data received before http2 is attached

e2374d7

Reinject the data already received from the TLS socket when the HTTP2 client is attached with a delay Fixes: nodejs#35475

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 16, 2020

addaleax approved these changes Oct 16, 2020

View reviewed changes

mcollina approved these changes Oct 16, 2020

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 16, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 16, 2020

mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 17, 2020

github-actions bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 17, 2020

Trott approved these changes Oct 17, 2020

View reviewed changes

mildsunrise reviewed Oct 18, 2020

View reviewed changes

lib/internal/http2/core.js Outdated Show resolved Hide resolved

mildsunrise added http2 Issues or PRs related to the http2 subsystem. stream Issues and PRs related to the stream subsystem. labels Oct 18, 2020

http2: fix reinjection check

e36b31c

We reinject when the sockets has already waiting data, remarked by @mildsunrise Co-authored-by: Alba Mendez <me@alba.sh>

mildsunrise approved these changes Oct 18, 2020

View reviewed changes

fhinkel approved these changes Oct 19, 2020

View reviewed changes

rickyes added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Oct 20, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 20, 2020

rickyes approved these changes Oct 20, 2020

View reviewed changes

rickyes added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 22, 2020

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 25, 2020

github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 25, 2020

github-actions bot closed this Oct 25, 2020

targos mentioned this pull request Nov 3, 2020

v15.1.0 release proposal #35948

Merged

szmarczak mentioned this pull request Nov 5, 2020

remoteSettings event is not emitted on a HTTP/2 session when given a socket with some data #35981

Closed

BethGriggs mentioned this pull request Dec 10, 2020

v14.15.2 proposal #36476

Merged

vinsonchuong mentioned this pull request Apr 3, 2021

Undeprecate _stream_wrap #34296

Closed

pimterry mentioned this pull request Dec 15, 2021

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

Merged

Uh oh!

Conversation

mmomtchev commented Oct 16, 2020

Uh oh!

nodejs-github-bot commented Oct 16, 2020

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

mmomtchev commented Oct 16, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Oct 16, 2020

Uh oh!

mmomtchev commented Oct 16, 2020

Uh oh!

nodejs-github-bot commented Oct 16, 2020

Uh oh!

github-actions bot commented Oct 17, 2020

Uh oh!

mildsunrise left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mildsunrise commented Oct 18, 2020

Uh oh!

nodejs-github-bot commented Oct 20, 2020

Uh oh!

rickyes left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Oct 21, 2020 • edited by rickyes Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 25, 2020

Uh oh!

mildsunrise commented Oct 25, 2020

Uh oh!

mildsunrise commented Oct 25, 2020

Uh oh!

mutza97 commented Oct 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

nodejs-github-bot commented Oct 21, 2020 •

edited by rickyes

Loading