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

async_hooks: execute unhandledRejection cb in Promise execution context #37281

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/internal/async_hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,8 @@ module.exports = {
emitBefore: emitBeforeScript,
emitAfter: emitAfterScript,
emitDestroy: emitDestroyScript,
pushAsyncContext,
popAsyncContext,
registerDestroyHook,
useDomainTrampoline,
nativeHooks: {
Expand Down
34 changes: 26 additions & 8 deletions lib/internal/process/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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 ?
Expand Down
77 changes: 74 additions & 3 deletions src/node_task_queue.cc
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#include "async_wrap.h"
#include "env-inl.h"
#include "node.h"
#include "node_errors.h"
Expand All @@ -16,18 +17,54 @@ 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::Object;
using v8::Promise;
using v8::PromiseRejectEvent;
using v8::PromiseRejectMessage;
using v8::Value;

static Maybe<double> GetAssignedPromiseAsyncId(Environment* env,
Local<Promise> promise,
Local<Value> id_symbol) {
Local<Value> maybe_async_id;
if (!promise->Get(env->context(), id_symbol).ToLocal(&maybe_async_id)) {
return v8::Just(AsyncWrap::kInvalidAsyncId);
}
return maybe_async_id->IsNumber()
? maybe_async_id->NumberValue(env->context())
: v8::Just(AsyncWrap::kInvalidAsyncId);
}

static Maybe<double> GetAssignedPromiseWrapAsyncId(Environment* env,
Local<Promise> promise,
Local<Value> 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<Value> promiseWrap = promise->GetInternalField(0);
if (promiseWrap->IsObject()) {
Local<Value> maybe_async_id;
if (!promiseWrap.As<Object>()->Get(env->context(), id_symbol)
.ToLocal(&maybe_async_id)) {
return v8::Just(AsyncWrap::kInvalidAsyncId);
}
return maybe_async_id->IsNumber()
? maybe_async_id->NumberValue(env->context())
: v8::Just(AsyncWrap::kInvalidAsyncId);
} else {
return v8::Just(AsyncWrap::kInvalidAsyncId);
}
}

void PromiseRejectCallback(PromiseRejectMessage message) {
static std::atomic<uint64_t> unhandledRejections{0};
static std::atomic<uint64_t> rejectionsHandledAfter{0};
Expand Down Expand Up @@ -76,12 +113,46 @@ void PromiseRejectCallback(PromiseRejectMessage message) {

Local<Value> 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;
TryCatchScope try_catch(env);

if (!GetAssignedPromiseAsyncId(env, promise, env->async_id_symbol())
.To(&async_id)) return;
if (!GetAssignedPromiseAsyncId(env, promise, env->trigger_async_id_symbol())
.To(&trigger_async_id)) return;

if (async_id == AsyncWrap::kInvalidAsyncId &&
trigger_async_id == AsyncWrap::kInvalidAsyncId) {
// That means that promise might be a PromiseWrap, so we'll
// check there as well.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add tests for both cases here?

Copy link
Contributor Author

@sajal50 sajal50 Feb 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually manually ran the other case by commenting this line out while running the test case added in this PR. It passed.

However, I guess we can change ActivityCollector to not use a noop function when ondestroy is not provided. What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why the test would need to use ActivityCollector, or be in test/async-hooks -- a lot of async_hooks tests that don't make use of the special features that that subdirectory has are just in test/parallel.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. What are these "special features" out of curiosity?

Also, if I just assumed initHooks had convenient methods when dealing with async hooks. What is otherwise the purpose?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are these "special features" out of curiosity?

Things like ActivityCollector, and the rest that’s in async-hooks/init-hooks.js and async-hooks/hook-checks.js.

Also, if I just assumed initHooks had convenient methods when dealing with async hooks. What is otherwise the purpose?

They do, but it’s not like you have to use them. At least for the test here, the difference to using async hooks directly seems to be relatively small.

if (!GetAssignedPromiseWrapAsyncId(env, promise, env->async_id_symbol())
.To(&async_id)) return;
if (!GetAssignedPromiseWrapAsyncId(
env, promise, env->trigger_async_id_symbol())
.To(&trigger_async_id)) return;
}

if (async_id != AsyncWrap::kInvalidAsyncId &&
trigger_async_id != AsyncWrap::kInvalidAsyncId) {
env->async_hooks()->push_async_context(
async_id, trigger_async_id, promise);
}

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);
Expand Down
27 changes: 27 additions & 0 deletions test/async-hooks/test-unhandled-rejection-context.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
'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.reject();
Copy link
Contributor Author

@sajal50 sajal50 Feb 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benjamingr I noticed an interesting case. If you just do Promise.reject the async_hooks.executionAsyncId() is incorrect in process.on('unhandledRejection')'s callback. But it is correct if you do,

Promise.resolve()
.then(() => {
   throw new Error('Throwing on purpose');
 });

Therefore, I tried out setting the context in node_task_queue, and then it seems to be correct.

Let me know your thoughts.


process.on('unhandledRejection', common.mustCall(() => {
assert.strictEqual(promiseAsyncIds.length, 1);
assert.strictEqual(async_hooks.executionAsyncId(), promiseAsyncIds[0]);
}));
1 change: 1 addition & 0 deletions test/parallel/test-bootstrap-modules.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ const expectedModules = new Set([
'NativeModule internal/worker/io',
'NativeModule internal/worker/js_transferable',
'NativeModule internal/blob',
'NativeModule async_hooks',
'NativeModule path',
'NativeModule stream',
'NativeModule timers',
Expand Down