FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

http2: move events to the JSStreamSocket by mmomtchev · Pull Request #35772 · nodejs/node · GitHub

/ node Public

http2: move events to the JSStreamSocket - #35772

Merged
Trott merged 2 commits into
nodejs:masterfrom
mmomtchev:http2-tls-destroy
Nov 8, 2020
ConversationCommits (2)ChecksFiles changed
Merged

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

Conversation

mmomtchev commented Oct 23, 2020

Copy link
Copy Markdown
Contributor

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 commented Oct 23, 2020

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net

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 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

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

nodejs-github-bot commented Oct 23, 2020

Copy link
Copy Markdown
Collaborator

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

codecov-io commented Oct 23, 2020

Copy link
Copy Markdown

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 commented Oct 23, 2020

Copy link
Copy Markdown
Contributor Author

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

Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 25, 2020
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
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

nodejs-github-bot commented Oct 27, 2020

Copy link
Copy Markdown
Collaborator

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

mmomtchev commented Nov 3, 2020

Copy link
Copy Markdown
Contributor Author

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 commented Nov 8, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

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 commented Nov 8, 2020

Copy link
Copy Markdown
Collaborator

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

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>
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 force-pushed the http2-tls-destroy branch from f05ccd6 to adae822 Compare November 8, 2020 14:29

Trott commented Nov 8, 2020

Copy link
Copy Markdown
Member

Landed in 133fdd4...adae822

Trott merged commit adae822 into nodejs:master Nov 8, 2020
danielleadams pushed a commit that referenced this pull request Nov 9, 2020
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
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 mentioned this pull request Nov 9, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Dec 9, 2020
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
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
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
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 mentioned this pull request Dec 10, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
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
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
jasnell jasnell approved these changes
Trott Trott approved these changes
addaleax addaleax approved these changes
+1 more reviewer
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

Back | FazBrowse Home | New Git URL