This repository has been archived by the owner on Aug 31, 2018. It is now read-only.
Apply some async hooks changes from upstream #87
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
addaleax
commented
Sep 30, 2017
| @@ -150,38 +150,39 @@ static void DestroyIdsCb(uv_timer_t* handle) { | |||
| TryCatch try_catch(env->isolate()); | |||
|
|
|||
| do { | |||
| std::vector<double> destroy_ids_list; | |||
| destroy_ids_list.swap(*env->destroy_ids_list()); | |||
| std::vector<double> destroy_async_id_list; | |||
| if (!env->can_call_into_js()) return; | |||
There was a problem hiding this comment.
Is the swapping of can_call_into_js check and the list swap intentional?
There was a problem hiding this comment.
@TimothyGu Not really intentional, but I don’t think it matters – that there is a similar check in PushBackDestroyAsyncId() means we’re okay with dropping destroy callbacks if can_call_into_js() is false.
If we were not okay with that, I think this change would make it More Correct™, because right now we would silently drop the destroy id queue?
There was a problem hiding this comment.
Okay, just making sure this wasn't overlooked.
There was a problem hiding this comment.
Yeah, thanks for pointing to it :)
| } | ||
|
|
||
|
|
||
| static void PushBackDestroyId(Environment* env, double id) { | ||
| static void PushBackDestroyAsyncId(Environment* env, double id) { | ||
| if (env->async_hooks()->fields()[AsyncHooks::kDestroy] == 0) | ||
| return; | ||
|
|
||
| if (!env->can_call_into_js()) | ||
| return; |
TimothyGu
approved these changes
Sep 30, 2017
PR-URL: ayojs#87 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Original-PR-URL: nodejs/node#15569 Original-Reviewed-By: Refael Ackermann <refack@gmail.com> Original-Reviewed-By: Anna Henningsen <anna@addaleax.net> Original-Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Original-Reviewed-By: James M Snell <jasnell@gmail.com>
Allow the user to pass in an execution_async_id instead of always generating one. This way the JS API can be used to pre-allocate the execution_async_id when the JS object is instantiated, before the native resource is created. Also allow the new execution_async_id to be passed via asyncReset(). PR-URL: nodejs/node#14208 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
If bootstrap throws and if ids are added to the async id stack and if
the exception wasn't handled by the fatal exception handler then the
AsyncCallbackScope destructor will cause the AsyncHooks::pop_ids() stack
check to fail. Causing the application to crash. So clear the async id
stack manually.
This is only possible if the user: 1) manually calls MakeCallback() or
2) uses async await in the top level. Which will cause _tickCallback()
to fire before bootstrap finishes executing.
The following example shows how the application can fail due to
exceeding the maximum call stack while using async await:
async function fn() {
fn();
throw new Error();
}
(async function() { await fn(); })();
If this occurs during bootstrap then the application will pring the
following warning a number of times then exit with a status of 0:
(node:*) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: *): Error
Here's the same recursive call done after enabling a new AsyncHook() the
following will print instead of the above warning and exit with a
non-zero code (currently it's 7 because of how node::FatalException
assigns error codes based on where the failure happened):
script.js:25
async function fn() {
^
RangeError: Maximum call stack size exceeded
at <anonymous>
at fn (script.js:25:18)
at fn (script.js:26:3)
....
This has to do with how Promises lazily enable PromiseHook if an
AsyncHook() is enabled. Whether these need to be made uniform is outside
the scope of this commit
Fixes: nodejs/node#15448
PR-URL: nodejs/node#15553
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax
force-pushed
the
upstream-async-hooks-changes
branch
from
9a16a7b to
e82107f
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
All of the conflicts were in the first commit, and only with
env->can_call_into_js()calls, so this should not be a big deal.