http2: move events to the JSStreamSocket #35772

mmomtchev · 2020-10-23T11:17:53Z

When using a JSStreamSocket, the
HTTP2Session constructor will replace
the socket object
http2 events should be attached to the
JSStreamSocket object because the http2
session handle lives there

Fixes: #35695

Checklist

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

nodejs-github-bot · 2020-10-23T11:17:57Z

Review requested:

@nodejs/http2
@nodejs/net

mcollina

lgtm

nodejs-github-bot · 2020-10-23T11:36:34Z

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

codecov-io · 2020-10-23T12:24:21Z

Codecov Report

Merging #35772 into master will decrease coverage by 8.50%.
The diff coverage is 75.55%.

@@            Coverage Diff             @@
##           master   #35772      +/-   ##
==========================================
- Coverage   96.40%   87.90%   -8.51%     
==========================================
  Files         223      477     +254     
  Lines       73685   113092   +39407     
  Branches        0    24629   +24629     
==========================================
+ Hits        71038    99414   +28376     
- Misses       2647     7967    +5320     
- Partials        0     5711    +5711

Impacted Files	Coverage Δ
src/inspector_profiler.cc	`76.17% <69.44%> (ø)`
lib/internal/http2/core.js	`95.06% <100.00%> (-1.88%)`	⬇️
lib/v8.js	`99.65% <100.00%> (-0.35%)`	⬇️
src/inspector_profiler.h	`86.95% <100.00%> (ø)`
lib/internal/idna.js	`55.55% <0.00%> (-11.12%)`	⬇️
lib/internal/blocklist.js	`88.70% <0.00%> (-10.49%)`	⬇️
lib/internal/crypto/mac.js	`73.45% <0.00%> (-9.48%)`	⬇️
lib/internal/modules/esm/get_format.js	`84.72% <0.00%> (-8.34%)`	⬇️
lib/internal/crypto/aes.js	`84.21% <0.00%> (-6.44%)`	⬇️
lib/internal/crypto/dsa.js	`85.28% <0.00%> (-6.04%)`	⬇️
... and 393 more

mmomtchev · 2020-10-23T13:58:21Z

That test passes on my Macbook but I do not have QUIC enabled, I will look into it

nodejs-github-bot · 2020-10-27T13:28:54Z

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

mmomtchev · 2020-11-03T11:30:08Z

The quic unit test failure is an unrelated problem for which I have a PR here: #35820
I don't understand the node-test-commit-linuxone » rhel7-s390 failure

Trott · 2020-11-08T14:15:06Z

I don't understand the node-test-commit-linuxone » rhel7-s390 failure

That test is known to be unreliable (and has a likely simple solution to that): #35881

nodejs-github-bot · 2020-11-08T14:15:55Z

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

When using a JSStreamSocket, the HTTP2Session constructor will replace the socket object http2 events should be attached to the JSStreamSocket object because the http2 session handle lives there Fixes: nodejs#35695 PR-URL: nodejs#35772 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ricky Zhou <[email protected]>

Move the socket event binding to the HTTP2Session constructor so that an error event could be delivered should the constructor fail Ref: nodejs#35772 PR-URL: nodejs#35772 Fixes: nodejs#35695 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ricky Zhou <[email protected]>

Trott · 2020-11-08T14:29:30Z

Landed in 133fdd4...adae822

When using a JSStreamSocket, the HTTP2Session constructor will replace the socket object http2 events should be attached to the JSStreamSocket object because the http2 session handle lives there Fixes: #35695 PR-URL: #35772 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ricky Zhou <[email protected]>

Move the socket event binding to the HTTP2Session constructor so that an error event could be delivered should the constructor fail Ref: #35772 PR-URL: #35772 Fixes: #35695 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ricky Zhou <[email protected]>

When using a JSStreamSocket, the HTTP2Session constructor will replace the socket object http2 events should be attached to the JSStreamSocket object because the http2 session handle lives there Fixes: #35695 PR-URL: #35772 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ricky Zhou <[email protected]>

Move the socket event binding to the HTTP2Session constructor so that an error event could be delivered should the constructor fail Ref: #35772 PR-URL: #35772 Fixes: #35695 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ricky Zhou <[email protected]>

When using a JSStreamSocket, the HTTP2Session constructor will replace the socket object http2 events should be attached to the JSStreamSocket object because the http2 session handle lives there Fixes: #35695 PR-URL: #35772 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ricky Zhou <[email protected]>

Move the socket event binding to the HTTP2Session constructor so that an error event could be delivered should the constructor fail Ref: #35772 PR-URL: #35772 Fixes: #35695 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ricky Zhou <[email protected]>

When using a JSStreamSocket, the HTTP2Session constructor will replace the socket object http2 events should be attached to the JSStreamSocket object because the http2 session handle lives there Fixes: #35695 PR-URL: #35772 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ricky Zhou <[email protected]>

Move the socket event binding to the HTTP2Session constructor so that an error event could be delivered should the constructor fail Ref: #35772 PR-URL: #35772 Fixes: #35695 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ricky Zhou <[email protected]>

nodejs-github-bot added the http2 Issues or PRs related to the http2 subsystem. label Oct 23, 2020

mcollina approved these changes Oct 23, 2020

View reviewed changes

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

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

addaleax approved these changes Oct 23, 2020

View reviewed changes

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

Trott approved these changes Oct 25, 2020

View reviewed changes

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

This comment has been minimized.

Sign in to view

jasnell approved these changes Oct 26, 2020

View reviewed changes

rickyes approved these changes Oct 27, 2020

View reviewed changes

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

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

This comment has been minimized.

Sign in to view

mmomtchev added 2 commits November 8, 2020 06:28

Trott force-pushed the http2-tls-destroy branch from f05ccd6 to adae822 Compare November 8, 2020 14:29

Trott merged commit adae822 into nodejs:master Nov 8, 2020

danielleadams mentioned this pull request Nov 9, 2020

v15.2.0 proposal #36055

Merged

BethGriggs mentioned this pull request Dec 10, 2020

v14.15.2 proposal #36476

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http2: move events to the JSStreamSocket #35772

http2: move events to the JSStreamSocket #35772

mmomtchev commented Oct 23, 2020

nodejs-github-bot commented Oct 23, 2020

mcollina left a comment

nodejs-github-bot commented Oct 23, 2020

codecov-io commented Oct 23, 2020

mmomtchev commented Oct 23, 2020

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Oct 27, 2020

mmomtchev commented Nov 3, 2020

Trott commented Nov 8, 2020 •

edited

Loading

nodejs-github-bot commented Nov 8, 2020

Trott commented Nov 8, 2020

http2: move events to the JSStreamSocket #35772

http2: move events to the JSStreamSocket #35772

Conversation

mmomtchev commented Oct 23, 2020

Checklist

nodejs-github-bot commented Oct 23, 2020

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Oct 23, 2020

codecov-io commented Oct 23, 2020

Codecov Report

mmomtchev commented Oct 23, 2020

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Oct 27, 2020

mmomtchev commented Nov 3, 2020

Trott commented Nov 8, 2020 • edited Loading

nodejs-github-bot commented Nov 8, 2020

Trott commented Nov 8, 2020

Trott commented Nov 8, 2020 •

edited

Loading