Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http2: wait for secureConnect before initializing #32958

Closed
wants to merge 1 commit into from

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Apr 21, 2020

As @murgatroid99 points out in #32922, a connection may have its connecting property set to true, while TLS is still connecting. This results in setupFn() being fired before the connection is actually ready.

This PR introduces the property secureConnecting on TLSSocket, which will not be set to true until the secureConnection event is emitted.

Fixes: #32922

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

@nodejs-github-bot nodejs-github-bot added http2 Issues or PRs related to the http2 subsystem. tls Issues and PRs related to the tls subsystem. labels Apr 21, 2020
@bcoe bcoe requested review from jasnell and addaleax and removed request for jasnell April 21, 2020 00:38
@nodejs-github-bot

This comment has been minimized.

@bcoe bcoe added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 21, 2020
lib/internal/fs/promises.js Outdated Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

bcoe added a commit that referenced this pull request Apr 23, 2020
PR-URL: #32958
Fixes: #32922
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@bcoe
Copy link
Contributor Author

bcoe commented Apr 23, 2020

Landed in 6a07eca

@bcoe bcoe closed this Apr 23, 2020
@bcoe bcoe deleted the http2-connect-fix branch April 23, 2020 01:24
BethGriggs pushed a commit that referenced this pull request Apr 27, 2020
PR-URL: #32958
Fixes: #32922
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Apr 27, 2020
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
PR-URL: #32958
Fixes: #32922
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Apr 28, 2020
targos pushed a commit that referenced this pull request Apr 30, 2020
PR-URL: #32958
Fixes: #32922
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@@ -464,6 +464,7 @@ function TLSSocket(socket, opts) {
this._securePending = false;
this._newSessionPending = false;
this._controlReleased = false;
this.secureConnecting = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is never set to false on client sockets.
So if Http2Session constructor (in lib/internal/http2/core.js) is called after secureConnect is emitted, then secureConnecting is still true but it'll end up never calling setupHandle and connect will never be emitted.

targos pushed a commit that referenced this pull request May 13, 2020
PR-URL: #32958
Fixes: #32922
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http2 Issues or PRs related to the http2 subsystem. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

http2.connect mishandles the socket option when connecting to an HTTPS server
5 participants