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: invoke connect callback immediately if session was connected #19842

Closed
wants to merge 3 commits into from

Conversation

pietermees
Copy link
Contributor

@pietermees pietermees commented Apr 5, 2018

Resolves #19839

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

@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x http2 Issues or PRs related to the http2 subsystem. labels Apr 5, 2018
@pietermees pietermees changed the title Invoke http2.connect callback immediately if the Http2Session was connected synchronously http2: invoke connect callback immediately if session was connected Apr 5, 2018
@lpinca
Copy link
Member

lpinca commented Apr 6, 2018

Is it possible to add a test?

@BridgeAR
Copy link
Member

BridgeAR commented Apr 6, 2018

@pietermees as @lpinca pointed out, it would be great if you could add a test. There is a documentation for how to write tests in doc/guides/writing-tests.md. If you need any further help, please let me know and I am happy to help you out. In general: tests here in Node.js are pretty much written as if you would write a own application besides having some Node.js specific helper functions available.

@BridgeAR
Copy link
Member

BridgeAR commented Apr 6, 2018

@pietermees and thanks a lot for contributing! Great that you went ahead to fix the issue right away 🎉.

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

There are some changes that need to be made before this lands. A test should definitely be added as it would help the suggested changes below, plus any other potential issues.

if (session.connecting)
session.once('connect', listener);
else
listener.call(this, socket);
Copy link
Member

Choose a reason for hiding this comment

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

The context passed to the listener should be session rather than this from the current scope. Also, please use Reflect.apply(listener, session, [socket]); for user-provided callbacks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 1fee57b30601f351c5b8eb60d2797917c0604a6b

@pietermees
Copy link
Contributor Author

In d575ac7a49ca38f84fcd20f4ef0a01786e3e0f23 I added documentation for the connecting property. Is this something we want to do, and if so, did I do it correctly?

@pietermees
Copy link
Contributor Author

And I'll add a test :)

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

Just the comment about the version.

doc/api/http2.md Outdated
@@ -320,6 +320,17 @@ added: v9.4.0
Will be `true` if this `Http2Session` instance has been closed, otherwise
`false`.

#### http2session.connecting
<!-- YAML
added: v9.12.0
Copy link
Member

Choose a reason for hiding this comment

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

This should not be a fixed value. Instead of v9.12.0 just put in: REPLACEME. This is going to be automatically be replaced during publishing a new release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 8316d4f36b769fdf3224b65fdd32f4194a441ada

doc/api/http2.md Outdated
added: v9.12.0
-->

* Value: {boolean}
Copy link
Member

Choose a reason for hiding this comment

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

Just a hint: the Value will probably be removed soon. See #19869. If that lands before this PR, this should get a update and vice versa.

Copy link
Member

Choose a reason for hiding this comment

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

Update: it just landed and now the Value should be removed ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this is a new section, so no conflicts for now, but the * Value: {boolean} should b replaced with just * {boolean}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 8316d4f36b769fdf3224b65fdd32f4194a441ada

if (session.connecting)
session.once('connect', listener);
else
Reflect.apply(listener, session, [socket]);
Copy link
Member

@apapirovski apapirovski Apr 9, 2018

Choose a reason for hiding this comment

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

@jasnell @mcollina Thinking about this a bit more, should we use process.nextTick and emit a connect event regardless rather than directly calling the listener? I know we don't recommend this code pattern anywhere but something like the following would still not work after this change:

const session = http2.connect(SERVER_URL, { createConnection: () => socket });
session.once('connect', listener);

Copy link
Member

Choose a reason for hiding this comment

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

@apapirovski I think we should copy-paste what we are doing for http, https, net and tls. So, we should adhere to the prevailing pattern.

Copy link
Member

Choose a reason for hiding this comment

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

I'm good with emitting on nextTick

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We actually do emit connect though on the following lines:

So I guess we're saying that we'll register the listener as a connect listener and change the two emits to run on the next tick?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See f21670f5d51d451cd2f2203f14d0f804ba107905

@pietermees
Copy link
Contributor Author

@BridgeAR @apapirovski @jasnell updated to emit connect on next tick and added a test

@pietermees
Copy link
Contributor Author

streamlined the commits

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

@pietermees
Copy link
Contributor Author

@mcollina the CI error seems to be that test-tls-buffersize.js was still running when tests finished on debian8-x86. Doesn't sound related?

@mcollina
Copy link
Member

That's probably flaky. We can land this.

For the flaky test, open an issue with the output of the failure if you can :).

@cjihrig
Copy link
Contributor

cjihrig commented Apr 10, 2018

No need to open a new issue. See #19903

@pietermees
Copy link
Contributor Author

@cjihrig sure that's the same issue? The tests seem to succeed, but the process keeps running:

08:24:44 # Clean up any leftover processes, error if found.
08:24:44 ps awwx | grep Release/node | grep -v grep | cat
08:24:44  3155 pts/0    Rl+    0:00 out/Release/node /home/iojs/debug-18998/test/parallel/test-tls-buffersize.js

See https://ci.nodejs.org/job/node-test-commit-linux/17820/nodes=debian8-x86/console

@mcollina
Copy link
Member

@apapirovski are you ok for this to land?

@cjihrig
Copy link
Contributor

cjihrig commented Apr 10, 2018

/home/iojs/debug-18998

I'm guessing the two are related. #18998 was closed in favor of #19903. That may be left over from @addaleax's debugging session?

@addaleax
Copy link
Member

@cjihrig Yup, exactly. I’m sorry for the inconvenience, I think this was worth it though…

@apapirovski apapirovski added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 10, 2018
@apapirovski
Copy link
Member

Thanks for making the changes @pietermees — sorry for the back-and-forth. I think this should be ready to land.

@mcollina
Copy link
Member

Landed in 4ac7753...2852521

@mcollina mcollina closed this Apr 11, 2018
mcollina pushed a commit that referenced this pull request Apr 11, 2018
PR-URL: #19842
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
mcollina pushed a commit that referenced this pull request Apr 11, 2018
PR-URL: #19842
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
@pietermees pietermees deleted the fix/http2-connect-cb branch April 11, 2018 21:32
targos pushed a commit that referenced this pull request Apr 12, 2018
PR-URL: #19842
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
targos pushed a commit that referenced this pull request Apr 12, 2018
PR-URL: #19842
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
jasnell pushed a commit that referenced this pull request Apr 16, 2018
PR-URL: #19842
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
jasnell pushed a commit that referenced this pull request Apr 16, 2018
PR-URL: #19842
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018
PR-URL: nodejs#19842
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018
PR-URL: nodejs#19842
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
Backport-PR-URL: #20456
PR-URL: #19842
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
Backport-PR-URL: #20456
PR-URL: #19842
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
MylesBorins pushed a commit that referenced this pull request May 15, 2018
Backport-PR-URL: #20456
PR-URL: #19842
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 15, 2018
Backport-PR-URL: #20456
PR-URL: #19842
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 15, 2018
Backport-PR-URL: #20456
PR-URL: #19842
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 15, 2018
Backport-PR-URL: #20456
PR-URL: #19842
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
@grantila
Copy link

This seems to till be an issue in Node 12. I have it working on 10 and 14, but it doesn't work for me in 12.

I'm digging through Node.js code to see what could potentially cause 12 to really break when 10 and 14 works, and I find weird things, undocumented. What is this (on master):

node/lib/_tls_wrap.js

Lines 1570 to 1571 in 2e6c3e2

if (!options.keepAlive)
options.singleUse = true;

These two options, keepAlive and singleUse aren't documented, meaning, if I happen to have them in my options object for some reason (I don't, just saying), what will that cause? Not sure it's related though.

@grantila
Copy link

Never mind, this is fixed by #33343

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

http2.connect callback not called if createConnection returns a socket that's already connected