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

http2: multiple cleanups and s/streamClosed/close by jasnell · Pull Request #17328 · nodejs/node · GitHub

/ node Public

http2: multiple cleanups and s/streamClosed/close - #17328

Closed
jasnell wants to merge 7 commits into
nodejs:masterfrom
jasnell:http2-refactor4
ConversationCommits (7)ChecksFiles changed
Closed

http2: multiple cleanups and s/streamClosed/close#17328
jasnell wants to merge 7 commits into
nodejs:masterfrom
jasnell:http2-refactor4

Conversation

jasnell commented Nov 26, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Includes multiple cleanups in core.js along with:

  • Use correct connect event for TLSSocket
  • Use 'close' event instead of streamClosed

/cc @nodejs/http2

Fixes: #15303

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

http2

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Nov 26, 2017

jasnell commented Nov 26, 2017

Copy link
Copy Markdown
Member Author

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

jasnell added the http2 Issues or PRs related to the http2 subsystem. label Nov 26, 2017
addaleax reviewed Nov 26, 2017
View reviewed changes
Comment thread lib/internal/http2/core.js Outdated

addaleax Nov 26, 2017

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

Is it possible to add a test for this?

jasnell Nov 26, 2017

Copy link
Copy Markdown
Member Author

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

There are already tests for it that have been flaky, this should fix those flaky tests. One of the existing ones could be modified to check that a secureConnect handler has been registered.

addaleax Nov 26, 2017

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

@jasnell Can you point me to one? I volunteer to try and make it fail/pass consistently :)

jasnell Nov 26, 2017

Copy link
Copy Markdown
Member Author

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

http2-create-client-secure-session is one, which should be fixed by this PR.
I've updated it to include the check for secureConnect

apapirovski approved these changes Nov 26, 2017
View reviewed changes

apapirovski left a comment

Copy link
Copy Markdown
Contributor

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 👍

Comment thread lib/internal/http2/compat.js Outdated

apapirovski Nov 26, 2017

Copy link
Copy Markdown
Contributor

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

Can you remove this and the associated function? They should not be necessary.

Comment thread lib/internal/http2/compat.js Outdated

apapirovski Nov 26, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

Same as above.

Can you remove this and the associated function? They should not be necessary.

Comment thread lib/internal/http2/core.js Outdated

apapirovski Nov 26, 2017

Copy link
Copy Markdown
Contributor

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

Could this be a bit more descriptively named? (Similar to setupFn elsewhere in this code.)

jasnell commented Nov 27, 2017

Copy link
Copy Markdown
Member Author

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

sebdeckers approved these changes Nov 27, 2017
View reviewed changes

sebdeckers left a comment

Copy link
Copy Markdown
Contributor

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 👍 This event confused me many times. Thanks for clearing it up.

jasnell commented Nov 28, 2017

Copy link
Copy Markdown
Member Author

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

jasnell added a commit that referenced this pull request Nov 28, 2017
http2: use correct connect event for TLS Socket
f3686f2 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>
jasnell added a commit that referenced this pull request Nov 28, 2017
http2: general cleanups
d4111d0 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>
jasnell added a commit that referenced this pull request Nov 28, 2017
http2: reduce code duplication in settings
df33d8d 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>
jasnell added a commit that referenced this pull request Nov 28, 2017
http2: use 'close' event instead of 'streamClosed'
1b99542 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>
jasnell added a commit that referenced this pull request Nov 28, 2017
test: fixup test-http2-create-client-secure-session
5fe111a 
General improvements to test and verify that a secureConnect
handler is present

PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>
jasnell added a commit that referenced this pull request Nov 28, 2017
http2: remove unnecessary event handlers
973b8e0 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>
jasnell added a commit that referenced this pull request Nov 28, 2017
http2: use more descriptive names
093a870 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>

jasnell commented Nov 28, 2017

Copy link
Copy Markdown
Member Author

Landed in 0fb1e07...093a870

jasnell closed this Nov 28, 2017
MylesBorins pushed a commit that referenced this pull request Dec 7, 2017
http2: use correct connect event for TLS Socket
d607718 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 7, 2017
http2: use correct connect event for TLS Socket
c81e968 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>
evanlucas pushed a commit that referenced this pull request Dec 7, 2017
http2: use correct connect event for TLS Socket
71cf21a 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>
evanlucas mentioned this pull request Dec 7, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
http2: general cleanups
e5f92cd 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
http2: reduce code duplication in settings
67abc1e 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
test: fixup test-http2-create-client-secure-session
047bac2 
General improvements to test and verify that a secureConnect
handler is present

PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
http2: remove unnecessary event handlers
4994d57 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
http2: use more descriptive names
658338e 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>
MylesBorins mentioned this pull request Dec 12, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Dec 19, 2017
http2: general cleanups
bdcbe81 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 19, 2017
http2: reduce code duplication in settings
31cdbd6 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 19, 2017
test: fixup test-http2-create-client-secure-session
927a281 
General improvements to test and verify that a secureConnect
handler is present

PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 19, 2017
http2: remove unnecessary event handlers
9fd0b32 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 19, 2017
http2: use more descriptive names
673e263 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Sebastiaan Deckers <sebdeckers83@gmail.com>
This was referenced Dec 20, 2017
Closed
Merged
kjin mentioned this pull request Dec 21, 2017
Merged
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

addaleax addaleax left review comments
+2 more reviewers
apapirovski apapirovski approved these changes
sebdeckers sebdeckers 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. lib / src Issues and PRs related to general changes in the lib or src directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

http2 - clarify difference between 'streamClosed' and 'close'

6 participants

Back | FazBrowse Home | New Git URL