Backporting Async Wrap changes by AndreasMadsen · Pull Request #7048 · nodejs/node

@nodejs-github-bot added the c++

Issues and PRs that require attention from people who are familiar with C++.

label

May 29, 2016
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>

MylesBorins pushed a commit that referenced this pull request

Jun 24, 2016
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>

MylesBorins pushed a commit that referenced this pull request

Jun 24, 2016
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>

MylesBorins pushed a commit that referenced this pull request

Jun 24, 2016
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>

MylesBorins pushed a commit that referenced this pull request

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

MylesBorins pushed a commit that referenced this pull request

Jun 24, 2016
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>

MylesBorins pushed a commit that referenced this pull request

Jun 24, 2016
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>

MylesBorins pushed a commit that referenced this pull request

Jun 24, 2016
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>

MylesBorins pushed a commit that referenced this pull request

Jun 24, 2016
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>

MylesBorins pushed a commit that referenced this pull request

Jun 24, 2016
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>

MylesBorins pushed a commit that referenced this pull request

Jun 24, 2016
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>

MylesBorins pushed a commit that referenced this pull request

Jun 24, 2016
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>

MylesBorins pushed a commit that referenced this pull request

Jun 24, 2016
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>

MylesBorins pushed a commit that referenced this pull request

Jun 24, 2016
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>

MylesBorins pushed a commit that referenced this pull request

Jun 24, 2016
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>

MylesBorins pushed a commit that referenced this pull request

Jun 24, 2016
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>

MylesBorins pushed a commit that referenced this pull request

Jun 24, 2016
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>

MylesBorins pushed a commit that referenced this pull request

Jun 24, 2016
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>

MylesBorins pushed a commit that referenced this pull request

Jun 24, 2016
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>

MylesBorins pushed a commit that referenced this pull request

Jun 28, 2016
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>

MylesBorins pushed a commit that referenced this pull request

Jun 28, 2016
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>

MylesBorins pushed a commit that referenced this pull request

Jun 28, 2016
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>

MylesBorins pushed a commit that referenced this pull request

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

MylesBorins pushed a commit that referenced this pull request

Jun 28, 2016
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>

MylesBorins pushed a commit that referenced this pull request

Jun 28, 2016
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>

MylesBorins pushed a commit that referenced this pull request

Jun 28, 2016
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>