From 8699a07ff7ec0b13034c3aca2316bd7ae886b30e Mon Sep 17 00:00:00 2001 From: Sajal Khandelwal Date: Sun, 7 Feb 2021 11:33:33 -0500 Subject: [PATCH] async_hooks: execute unhandledRejection cb in Promise execution context This commit now executes `process.on('unhandledRejection')` in the async execution context of the concerned `Promise`. --- lib/internal/async_hooks.js | 2 + lib/internal/process/promises.js | 34 ++++++--- src/node_task_queue.cc | 71 ++++++++++++++++++- .../test-unhandled-rejection-context.js | 36 ++++++++++ 4 files changed, 132 insertions(+), 11 deletions(-) create mode 100644 test/async-hooks/test-unhandled-rejection-context.js diff --git a/lib/internal/async_hooks.js b/lib/internal/async_hooks.js index f3014318071d9f..43ba749cd69946 100644 --- a/lib/internal/async_hooks.js +++ b/lib/internal/async_hooks.js @@ -573,6 +573,8 @@ module.exports = { emitBefore: emitBeforeScript, emitAfter: emitAfterScript, emitDestroy: emitDestroyScript, + pushAsyncContext, + popAsyncContext, registerDestroyHook, useDomainTrampoline, nativeHooks: { diff --git a/lib/internal/process/promises.js b/lib/internal/process/promises.js index 023f7df0360d02..c4eb9c53e242e7 100644 --- a/lib/internal/process/promises.js +++ b/lib/internal/process/promises.js @@ -24,6 +24,12 @@ const { triggerUncaughtException } = internalBinding('errors'); +const { + pushAsyncContext, + popAsyncContext, +} = require('internal/async_hooks'); +const async_hooks = require('async_hooks'); + // *Must* match Environment::TickInfo::Fields in src/env.h. const kHasRejectionToWarn = 1; @@ -116,11 +122,28 @@ function resolveError(type, promise, reason) { } function unhandledRejection(promise, reason) { + const asyncId = async_hooks.executionAsyncId(); + const triggerAsyncId = async_hooks.triggerAsyncId(); + const resource = promise; + + const emit = (reason, promise, promiseInfo) => { + try { + pushAsyncContext(asyncId, triggerAsyncId, resource); + if (promiseInfo.domain) { + return promiseInfo.domain.emit('error', reason); + } + return process.emit('unhandledRejection', reason, promise); + } finally { + popAsyncContext(asyncId); + } + }; + maybeUnhandledPromises.set(promise, { reason, uid: ++lastPromiseId, warned: false, - domain: process.domain + domain: process.domain, + emit }); // This causes the promise to be referenced at least for one tick. ArrayPrototypePush(pendingUnhandledRejections, promise); @@ -194,13 +217,8 @@ function processPromiseRejections() { continue; } promiseInfo.warned = true; - const { reason, uid } = promiseInfo; - function emit(reason, promise, promiseInfo) { - if (promiseInfo.domain) { - return promiseInfo.domain.emit('error', reason); - } - return process.emit('unhandledRejection', reason, promise); - } + const { reason, uid, emit } = promiseInfo; + switch (unhandledRejectionsMode) { case kStrictUnhandledRejections: { const err = reason instanceof Error ? diff --git a/src/node_task_queue.cc b/src/node_task_queue.cc index 0aa021fc5b27df..7bd36ae698034a 100644 --- a/src/node_task_queue.cc +++ b/src/node_task_queue.cc @@ -1,3 +1,4 @@ +#include "async_wrap.h" #include "env-inl.h" #include "node.h" #include "node_errors.h" @@ -16,18 +17,55 @@ using v8::Context; using v8::Function; using v8::FunctionCallbackInfo; using v8::Isolate; +using v8::Just; using v8::kPromiseHandlerAddedAfterReject; using v8::kPromiseRejectAfterResolved; using v8::kPromiseRejectWithNoHandler; using v8::kPromiseResolveAfterResolved; using v8::Local; +using v8::Maybe; using v8::Number; +using v8::Nothing; using v8::Object; using v8::Promise; using v8::PromiseRejectEvent; using v8::PromiseRejectMessage; using v8::Value; +static Maybe GetAssignedPromiseAsyncId(Environment* env, + Local promise, + Local id_symbol) { + Local maybe_async_id; + if (!promise->Get(env->context(), id_symbol).ToLocal(&maybe_async_id)) { + return Nothing(); + } + return maybe_async_id->IsNumber() + ? maybe_async_id->NumberValue(env->context()) + : v8::Just(AsyncWrap::kInvalidAsyncId); +} + +static Maybe GetAssignedPromiseWrapAsyncId(Environment* env, + Local promise, + Local id_symbol) { + // This check is imperfect. If the internal field is set, it should + // be an object. If it's not, we just ignore it. Ideally v8 would + // have had GetInternalField returning a MaybeLocal but this works + // for now. + Local promiseWrap = promise->GetInternalField(0); + if (promiseWrap->IsObject()) { + Local maybe_async_id; + if (!promiseWrap.As()->Get(env->context(), id_symbol).ToLocal(&maybe_async_id)) { + return Nothing(); + } + return maybe_async_id->IsNumber() + ? maybe_async_id->NumberValue(env->context()) + : v8::Just(AsyncWrap::kInvalidAsyncId); + } + else { + return Nothing(); + } +} + void PromiseRejectCallback(PromiseRejectMessage message) { static std::atomic unhandledRejections{0}; static std::atomic rejectionsHandledAfter{0}; @@ -76,12 +114,39 @@ void PromiseRejectCallback(PromiseRejectMessage message) { Local args[] = { type, promise, value }; - // V8 does not expect this callback to have a scheduled exceptions once it - // returns, so we print them out in a best effort to do something about it - // without failing silently and without crashing the process. + double async_id = AsyncWrap::kInvalidAsyncId; + double trigger_async_id = AsyncWrap::kInvalidAsyncId; + GetAssignedPromiseAsyncId(env, promise, env->async_id_symbol()) + .To(&async_id); + GetAssignedPromiseAsyncId(env, promise, env->trigger_async_id_symbol()) + .To(&trigger_async_id); + + GetAssignedPromiseWrapAsyncId(env, promise, env->async_id_symbol()) + .To(&async_id); + GetAssignedPromiseWrapAsyncId(env, promise, env->trigger_async_id_symbol()) + .To(&trigger_async_id); + + if (async_id != AsyncWrap::kInvalidAsyncId && + trigger_async_id != AsyncWrap::kInvalidAsyncId) { + env->async_hooks()->push_async_context( + async_id, trigger_async_id, promise); + } + TryCatchScope try_catch(env); USE(callback->Call( env->context(), Undefined(isolate), arraysize(args), args)); + + if (async_id != AsyncWrap::kInvalidAsyncId && + trigger_async_id != AsyncWrap::kInvalidAsyncId && + env->execution_async_id() == async_id) { + // This condition might not be true if async_hooks was enabled during + // the promise callback execution. + env->async_hooks()->pop_async_context(async_id); + } + + // V8 does not expect this callback to have a scheduled exceptions once it + // returns, so we print them out in a best effort to do something about it + // without failing silently and without crashing the process. if (try_catch.HasCaught() && !try_catch.HasTerminated()) { fprintf(stderr, "Exception in PromiseRejectCallback:\n"); PrintCaughtException(isolate, env->context(), try_catch); diff --git a/test/async-hooks/test-unhandled-rejection-context.js b/test/async-hooks/test-unhandled-rejection-context.js new file mode 100644 index 00000000000000..55fe246937a878 --- /dev/null +++ b/test/async-hooks/test-unhandled-rejection-context.js @@ -0,0 +1,36 @@ +'use strict'; + +const common = require('../common'); + +const assert = require('assert'); +const initHooks = require('./init-hooks'); +const async_hooks = require('async_hooks'); + +if (!common.isMainThread) + common.skip('Worker bootstrapping works differently -> different async IDs'); + +const promiseAsyncIds = []; +const hooks = initHooks({ + oninit(asyncId, type) { + if (type === 'PROMISE') { + promiseAsyncIds.push(asyncId); + } + } +}); + +hooks.enable(); + +// Promise.resolve() +// .then(() => { +// throw new Error('Throwing on purpose'); +// }); + +Promise.reject(); + +process.on('unhandledRejection', common.mustCall(() => { + // assert.strictEqual(promiseAsyncIds.length, 2); + // assert.strictEqual(async_hooks.executionAsyncId(), promiseAsyncIds[1]); + + assert.strictEqual(promiseAsyncIds.length, 1); + assert.strictEqual(async_hooks.executionAsyncId(), promiseAsyncIds[0]); +}));