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

Revert "net: remove unnecessary process.nextTick()" #12854

Merged
merged 2 commits into from May 23, 2017

Conversation

@evanlucas
Copy link
Member

commented May 5, 2017

This reverts commit 571882c.

Removing the process.nextTick() call can prevent the consumer
from being able to catch error events.

Looks like we ran into this before in af249fa

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)

/cc @bnoordhuis

@cjihrig
cjihrig approved these changes May 5, 2017
@evanlucas evanlucas referenced this pull request May 5, 2017
@benjamingr

This comment has been minimized.

Copy link
Member

commented May 5, 2017

Commit description should probably note that there is no behavior change here since inernalConnect already defers callbacks.

@mscdex

This comment has been minimized.

Copy link
Contributor

commented May 5, 2017

I wonder if it would be better to instead just use process.nextTick() if there was an immediate error while connecting to an IP address? That way we can still avoid calling process.nextTick() for the successful connection case.

@jasnell
jasnell approved these changes May 7, 2017
Copy link
Member

left a comment

This is the second regression from this PR we've seen. We definitely may be able to take another stab at refactoring this, but a simple revert is best, I think, and we can do more later.

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented May 7, 2017

A regression test should be added. #2054 could (and arguably should) have done that but better late than never.

@evanlucas

This comment has been minimized.

Copy link
Member Author

commented May 10, 2017

A regression test should be added. #2054 could (and arguably should) have done that but better late than never.

Yep, will get that updated tonight or tomorrow

@evanlucas

This comment has been minimized.

Copy link
Member Author

commented May 11, 2017

Ok, so it looks like 503b089 added a regression test for the original issue, but only tested against net and not an http request. In this case, the http request calls this.agent.addRequest(this, options) which will then call req.onSocket(socket) which then calls onSocketNT(req, socket) which calls tickOnSocket(req, socket).

The error listener is added to the socket here which is called in process.nextTick() in onSocket(). So that is why the test added before did not catch it. I'll add an additional test for http as well.

@evanlucas evanlucas force-pushed the evanlucas:revertnet branch May 11, 2017

@evanlucas

This comment has been minimized.

Copy link
Member Author

commented May 11, 2017

@evanlucas

This comment has been minimized.

Copy link
Member Author

commented May 11, 2017

Ok, so looks like this now breaks an async-hooks test

➜ ./node test/async-hooks/test-tcpwrap.js
assert.js:92
  throw new AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 1 TCPCONNECTWRAP present when client is connecting
    at Object.<anonymous> (/Volumes/code/forks/node/test/async-hooks/test-tcpwrap.js:56:10)
    at Module._compile (module.js:569:30)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:503:32)
    at tryModuleLoad (module.js:466:12)
    at Function.Module._load (module.js:458:3)
    at Function.Module.runMain (module.js:605:10)
    at startup (bootstrap_node.js:144:16)
    at bootstrap_node.js:561:3

Going to look more into this for the best solution

@evanlucas

This comment has been minimized.

Copy link
Member Author

commented May 16, 2017

/cc @AndreasMadsen (since it seems to be async-hooks related) any suggestions on the error I'm now getting above with this change?

@evanlucas evanlucas force-pushed the evanlucas:revertnet branch May 16, 2017

@AndreasMadsen

This comment has been minimized.

Copy link
Member

commented May 16, 2017

I think it should be nextTick(self[async_id_symbol], function() { instead of process.nextTick and then some of the test counters might have to changed since it is no longer an immediate initialization.

@evanlucas

This comment has been minimized.

Copy link
Member Author

commented May 17, 2017

ok, updated to account for async-hooks. PTAL

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

@refack

This comment has been minimized.

Copy link
Member

commented May 18, 2017

I killed the CI by mistake, sorry.
New CI: https://ci.nodejs.org/job/node-test-pull-request/8166/

@evanlucas evanlucas force-pushed the evanlucas:revertnet branch May 22, 2017

@evanlucas

This comment has been minimized.

Copy link
Member Author

commented May 22, 2017

@cjihrig @jasnell @benjamingr mind taking another quick look. I added a regression test since yall approved it previously. Thanks!

CI one more time: https://ci.nodejs.org/job/node-test-pull-request/8226/

@cjihrig
Copy link
Contributor

left a comment

LGTM. A comment near the top of the test and some validation of the error object might be useful.

evanlucas added 2 commits May 5, 2017
Revert "net: remove unnecessary process.nextTick()"
This reverts commit 571882c.

Removing the process.nextTick() call can prevent the consumer
from being able to catch error events.

PR-URL: #12854
Fixes: #12841
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
test: add regression test for immediate socket errors
This test ensures that a http client request with the default agent
that has a socket that is immediately destroyed can still be caught by
adding an error event listener to the request object.

PR-URL: #12854
Fixes: #12841
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

@evanlucas evanlucas force-pushed the evanlucas:revertnet branch to 2cf4882 May 23, 2017

@evanlucas evanlucas closed this May 23, 2017

@evanlucas evanlucas deleted the evanlucas:revertnet branch May 23, 2017

@evanlucas

This comment has been minimized.

Copy link
Member Author

commented May 23, 2017

Landed in ba0dbaa and 2cf4882. Thanks!

@evanlucas evanlucas merged commit 2cf4882 into nodejs:master May 23, 2017

jasnell added a commit that referenced this pull request May 23, 2017
Revert "net: remove unnecessary process.nextTick()"
This reverts commit 571882c.

Removing the process.nextTick() call can prevent the consumer
from being able to catch error events.

PR-URL: #12854
Fixes: #12841
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
jasnell added a commit that referenced this pull request May 23, 2017
test: add regression test for immediate socket errors
This test ensures that a http client request with the default agent
that has a socket that is immediately destroyed can still be caught by
adding an error event listener to the request object.

PR-URL: #12854
Fixes: #12841
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
jasnell added a commit that referenced this pull request May 28, 2017
Revert "net: remove unnecessary process.nextTick()"
This reverts commit 571882c.

Removing the process.nextTick() call can prevent the consumer
from being able to catch error events.

PR-URL: #12854
Fixes: #12841
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
jasnell added a commit that referenced this pull request May 28, 2017
test: add regression test for immediate socket errors
This test ensures that a http client request with the default agent
that has a socket that is immediately destroyed can still be caught by
adding an error event listener to the request object.

PR-URL: #12854
Fixes: #12841
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@jasnell jasnell referenced this pull request May 28, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.