Backporting Async Wrap changes #7048

AndreasMadsen · 2016-05-29T11:54:20Z

Checklist

tests and code linting passes
a test and/or benchmark is included
the commit message follows commit guidelines

Affected core subsystem(s)

src, http, http_parser, test, async_wrap

Description of change

Async Wrap in 4.x is very far behind and it creates a lot of issues, see the issue references in nodejs/Release#86. I recall in some discussion (I can't find it) that this shouldn't be straight forward, however it was actually really easy and all the tests passes. Am I missing something?

This backports the following changes:

src: fix MakeCallback error handling #4507 - src: fix MakeCallback error handling
b72dbdb from misc cleanup #5233 - src: remove unnecessary check
async_wrap: add uid argument to all asyncWrap hooks #4600 - async_wrap: add uid argument to all asyncWrap hooks
http_parser: use MakeCallback #5419 - http_parser: use MakeCallback
src,http: fix uncaughtException miss in http #5591 - src,http: fix uncaughtException miss in http
Asyncwrap more #5756 - Asyncwrap more

/cc @thealphanerd @trevnorris

AndreasMadsen · 2016-05-30T23:25:30Z

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

MylesBorins · 2016-05-31T04:37:14Z

citgm against v4.x: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/287/

trevnorris · 2016-05-31T23:32:46Z

@AndreasMadsen You are amazing. Will give this a full review tomorrow morning, but if CI is good then don't wait for my sign off.

MylesBorins · 2016-05-31T23:49:38Z

@AndreasMadsen awesome work! Please hold off on landing this yourself, I'll likely try to bring it in while putting together an rc after sign off 😄

trevnorris · 2016-06-01T18:38:30Z

This reminded me of a bug that got lost on my list of todo's. #7096

It's a minor fix.

trevnorris · 2016-06-01T21:42:38Z

@AndreasMadsen Thanks for the review. The fix landed in f81f0c3.

AndreasMadsen · 2016-06-01T22:10:39Z

Should I add f81f0c3 to this pull request?

trevnorris · 2016-06-01T23:05:30Z

@AndreasMadsen Yes, please.

AndreasMadsen · 2016-06-02T08:04:19Z

cherry-picked it in.

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

trevnorris · 2016-06-02T15:42:36Z

Awesome stuff. Thanks again for doing this. LGTM.

trevnorris · 2016-06-03T18:15:17Z

@thealphanerd I'm not involved w/ a lot of backporting. What's the criterion for landing this?

MylesBorins · 2016-06-03T18:34:49Z

@trevnorris as long as all of the commits have lived in at least one release cycle without a regression I would say we are most likely ready to go.

We likely want to have @nodejs/lts sign off on this as well

Citgm for posterity: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/291/

trevnorris · 2016-06-03T22:39:19Z

@thealphanerd Everything's there I can see. Only failure on Citgm is for OSX

  not ok 19 Connect Timeout Error should set
  'connect' property to true

Not sure if that's related to this or not. So just waiting for another LTS member to sign off.

MylesBorins · 2016-06-03T22:44:02Z

rerunning citgm against just fs-extra (single failure on osx) --> https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/293/

MylesBorins · 2016-06-06T22:33:56Z

so citgm looks good.

@nodejs/lts are we ok landing this in v4.5.0?

jasnell · 2016-06-07T00:00:22Z

As long as ci and citgm are good, lgtm

Implementations of error handling between node::MakeCallback() and AsyncWrap::MakeCallback() do not return at the same point. Make both executions work the same by moving the early return if there's a caught exception just after the AsyncWrap post callback. Since the domain's call stack is cleared on a caught exception there is no reason to call its exit() callback. Remove the SetVerbose() statement in the AsyncWrap pre/post callback calls since it does not affect the callback call. PR-URL: #4507 Reviewed-By: Fedor Indutny <fedor@indutny.com>

Add a scope that will allow MakeCallback to know whether or not it's currently running. This will prevent nextTickQueue and the MicrotaskQueue from being processed recursively. It is also required to wrap the bootloading stage since it doesn't run within a MakeCallback. Ref: nodejs/node-v0.x-archive#9245 PR-URL: #4507 Reviewed-By: Fedor Indutny <fedor@indutny.com>

Environment::TickInfo::last_threw() is no longer in use. Also pass Isolate to few methods and fix whitespace alignment. PR-URL: #4507 Reviewed-By: Fedor Indutny <fedor@indutny.com>

PR-URL: #4507 Reviewed-By: Fedor Indutny <fedor@indutny.com>

After attempting to use ReThrow() and Reset() there were cases where firing the domain's error handlers was not happening. Or in some cases reentering MakeCallback would still cause the domain enter callback to abort (because the error had not been Reset yet). In order for the script to properly stop execution when a subsequent call to MakeCallback throws it must not be located within a TryCatch. PR-URL: #4507 Reviewed-By: Fedor Indutny <fedor@indutny.com>

Make sure that calling MakeCallback multiple times within the same stack does not allow the nextTickQueue or MicrotaskQueue to be processed in any more than the first MakeCallback call. Check that domains enter/exit poperly with multiple MakeCallback calls and that errors are handled as expected PR-URL: #4507 Reviewed-By: Fedor Indutny <fedor@indutny.com>

The value's type is unsigned so it will always be >= 0. PR-URL: #5233 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

AndreasMadsen · 2016-06-07T16:09:03Z

There was a conflict with c63d48f because it changed ARRAY_SIZE to arraysize. I have updated the PR accordingly.

This one was actually quite tricky as git somehow didn't catch all the conflicts by itself.

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

trevnorris · 2016-06-07T19:18:01Z

CI is all green. Land it!

The value's type is unsigned so it will always be >= 0. Ref: #7048 PR-URL: #5233 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

By doing this users can use a Map object for storing information instead of modifying the handle object. Ref: #7048 PR-URL: #4600 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

All other hooks have uid as the first argument, this makes it concistent for all hooks. Ref: #7048 PR-URL: #4600 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

When the parent uid is required it is not necessary to store the uid in the parent handle object. Ref: #7048 PR-URL: #4600 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

Make `HTTPParser` an instance of `AsyncWrap` and make it use `MakeCallback`. This means that async wrap hooks will be called on consumed TCP sockets as well as on non-consumed ones. Additional uses of `AsyncCallbackScope` are necessary to prevent improper state from progressing that triggers failure in the test-http-pipeline-flood.js test. Optimally this wouldn't be necessary, but for the time being it's the most sure way to allow operations to proceed as they have. Ref: #7048 Fix: #4416 PR-URL: #5419 Reviewed-By: Fedor Indutny <fedor@indutny.com>

In AsyncWrap::MakeCallback always return empty handle if there is an error. In the future this should change to return a v8::MaybeLocal, but that major change will have to wait for v6.x, and these changes are meant to be backported to v4.x. The HTTParser call to AsyncWrap::MakeCallback failed because it expected a thrown call to return an empty handle. In node::MakeCallback return an empty handle if the call is in_makecallback(), otherwise return v8::Undefined() as usual to preserve backwards compatibility. Ref: #7048 Fixes: #5555 PR-URL: #5591 Reviewed-By: Julien Gilli <jgilli@nodejs.org>

Now that HTTPParser uses MakeCallback it is unnecessary to manually process the nextTickQueue. The KickNextTick function is now no longer needed so code has moved back to node::MakeCallback to simplify implementation. Include minor cleanup moving Environment::tick_info() call below the early return to save an operation. Ref: #7048 PR-URL: #5756 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Andreas Madsen <amwebdk@gmail.com>

Make comment clear that Undefined() is returned for legacy compatibility. This will change in the future as a semver-major change, but to be able to port this to previous releases it needs to stay as is. Ref: #7048 PR-URL: #5756 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Andreas Madsen <amwebdk@gmail.com>

The number of callbacks accepted to setupHooks was getting unwieldy. Instead change the implementation to accept an object with all callbacks Ref: #7048 PR-URL: #5756 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Andreas Madsen <amwebdk@gmail.com>

The second argument of the post callback is a boolean indicating whether the callback threw and was intercepted by uncaughtException or a domain. Currently node::MakeCallback has no way of retrieving a uid for the object. This is coming in a future patch. Ref: #7048 PR-URL: #5756 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Andreas Madsen <amwebdk@gmail.com>

Rather than abort if the init/pre/post/final/destroy callbacks throw, force the exception to propagate and not be made catchable. This way the application is still not allowed to proceed but also allowed the location of the failure to print before exiting. Though the stack itself may not be of much use since all callbacks except init are called from the bottom of the call stack. /tmp/async-test.js:14 throw new Error('pre'); ^ Error: pre at InternalFieldObject.pre (/tmp/async-test.js:14:9) Ref: #7048 PR-URL: #5756 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Andreas Madsen <amwebdk@gmail.com>

Passing the uid via v8::Integer::New() converts it to a uint32_t. Which will trim the value early. Instead use v8::Number::New() to convert the int64_t to a double so that JS can see the full 2^53 range of uid's. Ref: #7048 PR-URL: #7096 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com>

Implementations of error handling between node::MakeCallback() and AsyncWrap::MakeCallback() do not return at the same point. Make both executions work the same by moving the early return if there's a caught exception just after the AsyncWrap post callback. Since the domain's call stack is cleared on a caught exception there is no reason to call its exit() callback. Remove the SetVerbose() statement in the AsyncWrap pre/post callback calls since it does not affect the callback call. Ref: #7048 PR-URL: #4507 Reviewed-By: Fedor Indutny <fedor@indutny.com>

Add a scope that will allow MakeCallback to know whether or not it's currently running. This will prevent nextTickQueue and the MicrotaskQueue from being processed recursively. It is also required to wrap the bootloading stage since it doesn't run within a MakeCallback. Ref: nodejs/node-v0.x-archive#9245 Ref: #7048 PR-URL: #4507 Reviewed-By: Fedor Indutny <fedor@indutny.com>

Environment::TickInfo::last_threw() is no longer in use. Also pass Isolate to few methods and fix whitespace alignment. Ref: #7048 PR-URL: #4507 Reviewed-By: Fedor Indutny <fedor@indutny.com>

Ref: #7048 PR-URL: #4507 Reviewed-By: Fedor Indutny <fedor@indutny.com>

After attempting to use ReThrow() and Reset() there were cases where firing the domain's error handlers was not happening. Or in some cases reentering MakeCallback would still cause the domain enter callback to abort (because the error had not been Reset yet). In order for the script to properly stop execution when a subsequent call to MakeCallback throws it must not be located within a TryCatch. Ref: #7048 PR-URL: #4507 Reviewed-By: Fedor Indutny <fedor@indutny.com>

Make sure that calling MakeCallback multiple times within the same stack does not allow the nextTickQueue or MicrotaskQueue to be processed in any more than the first MakeCallback call. Check that domains enter/exit poperly with multiple MakeCallback calls and that errors are handled as expected Ref: #7048 PR-URL: #4507 Reviewed-By: Fedor Indutny <fedor@indutny.com>