Skip to content

http2: move events to the JSStreamSocket#35772

Merged
Trott merged 2 commits intonodejs:masterfrom
mmomtchev:http2-tls-destroy
Nov 8, 2020
Merged

http2: move events to the JSStreamSocket#35772
Trott merged 2 commits intonodejs:masterfrom
mmomtchev:http2-tls-destroy

Conversation

@mmomtchev
Copy link
Contributor

@mmomtchev mmomtchev commented Oct 23, 2020

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

nodejs-github-bot commented Oct 23, 2020

Review requested:

  • @nodejs/http2
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added the http2 Issues or PRs related to the http2 subsystem. label Oct 23, 2020
mcollina
mcollina approved these changes Oct 23, 2020
View reviewed changes
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

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 23, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 23, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 23, 2020

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

@codecov-io
Copy link

codecov-io commented Oct 23, 2020

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

mmomtchev commented Oct 23, 2020

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

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

This comment has been minimized.

Sign in to view
@rickyes rickyes added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 27, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 27, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 27, 2020

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

@mmomtchev
Copy link
Contributor Author

mmomtchev commented Nov 3, 2020

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

Trott commented Nov 8, 2020 •
edited
Loading

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

nodejs-github-bot commented Nov 8, 2020

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

@mmomtchev @Trott
http2: move events to the JSStreamSocket
6abb7e5 
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 <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
@mmomtchev @Trott
http2: centralise socket event binding in Http2Session
adae822 
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 <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
@Trott Trott force-pushed the http2-tls-destroy branch from f05ccd6 to adae822 Compare November 8, 2020 14:29
@Trott
Copy link
Member

Trott commented Nov 8, 2020

Landed in 133fdd4...adae822

@Trott Trott merged commit adae822 into nodejs:master Nov 8, 2020
danielleadams pushed a commit that referenced this pull request Nov 9, 2020
@mmomtchev @danielleadams
http2: move events to the JSStreamSocket
429113e 
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 <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
danielleadams pushed a commit that referenced this pull request Nov 9, 2020
@mmomtchev @danielleadams
http2: centralise socket event binding in Http2Session
28ed7d0 
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 <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
@danielleadams danielleadams mentioned this pull request Nov 9, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Dec 9, 2020
@mmomtchev @BethGriggs
http2: move events to the JSStreamSocket
dc2935d 
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 <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 9, 2020
@mmomtchev @BethGriggs
http2: centralise socket event binding in Http2Session
c6900dd 
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 <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
@mmomtchev @BethGriggs
http2: move events to the JSStreamSocket
7cd5589 
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 <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
@mmomtchev @BethGriggs
http2: centralise socket event binding in Http2Session
9eea2a6 
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 <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
@mmomtchev @BethGriggs
http2: move events to the JSStreamSocket
dce01fd 
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 <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
@mmomtchev @BethGriggs
http2: centralise socket event binding in Http2Session
c455b84 
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 <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@addaleax addaleax addaleax approved these changes

+1 more reviewer

@rickyes rickyes rickyes approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

http2 Issues or PRs related to the http2 subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Http2: Cannot read property 'finishWrite' of null

8 participants

@mmomtchev @nodejs-github-bot @codecov-io @Trott @mcollina @jasnell @addaleax @rickyes