Skip to content

Commit

Permalink
async_hooks: fix id assignment in fast-path promise hook
Browse files Browse the repository at this point in the history
Native side of fast-path promise hook was not calling JS
fastPromiseHook function when there were no async ids
previously assigned to the promise. Because of that already
created promises could not get id assigned in situations
when an async hook without a before listener function is
enabled after their creation. As the result executionAsyncId
could return wrong id when called within promise's .then().

Refs: #34512

PR-URL: #34548
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
  • Loading branch information
puzpuzpuz committed Aug 3, 2020
1 parent 2ba93e1 commit b7a2329
Show file tree
Hide file tree
Showing 2 changed files with 69 additions and 38 deletions.
82 changes: 44 additions & 38 deletions src/async_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,12 @@ using v8::Global;
using v8::HandleScope;
using v8::Integer;
using v8::Isolate;
using v8::Just;
using v8::Local;
using v8::Maybe;
using v8::MaybeLocal;
using v8::Name;
using v8::Nothing;
using v8::Number;
using v8::Object;
using v8::ObjectTemplate;
Expand Down Expand Up @@ -189,6 +192,21 @@ void AsyncWrap::EmitAfter(Environment* env, double async_id) {
env->async_hooks_after_function());
}

// TODO(addaleax): Remove once we're on C++17.
constexpr double AsyncWrap::kInvalidAsyncId;

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 Nothing<double>();
}
return maybe_async_id->IsNumber()
? maybe_async_id->NumberValue(env->context())
: Just(AsyncWrap::kInvalidAsyncId);
}

class PromiseWrap : public AsyncWrap {
public:
PromiseWrap(Environment* env, Local<Object> object, bool silent)
Expand Down Expand Up @@ -231,18 +249,17 @@ PromiseWrap* PromiseWrap::New(Environment* env,

// Skip for init events
if (silent) {
Local<Value> maybe_async_id = promise
->Get(context, env->async_id_symbol())
.ToLocalChecked();

Local<Value> maybe_trigger_async_id = promise
->Get(context, env->trigger_async_id_symbol())
.ToLocalChecked();

if (maybe_async_id->IsNumber() && maybe_trigger_async_id->IsNumber()) {
double async_id = maybe_async_id.As<Number>()->Value();
double trigger_async_id = maybe_trigger_async_id.As<Number>()->Value();
return new PromiseWrap(env, obj, async_id, trigger_async_id);
double async_id;
double trigger_async_id;
if (!GetAssignedPromiseAsyncId(env, promise, env->async_id_symbol())
.To(&async_id)) return nullptr;
if (!GetAssignedPromiseAsyncId(env, promise, env->trigger_async_id_symbol())
.To(&trigger_async_id)) return nullptr;

if (async_id != AsyncWrap::kInvalidAsyncId &&
trigger_async_id != AsyncWrap::kInvalidAsyncId) {
return new PromiseWrap(
env, obj, async_id, trigger_async_id);
}
}

Expand Down Expand Up @@ -321,46 +338,35 @@ static void FastPromiseHook(PromiseHookType type, Local<Promise> promise,

if (type == PromiseHookType::kBefore &&
env->async_hooks()->fields()[AsyncHooks::kBefore] == 0) {
Local<Value> maybe_async_id;
if (!promise->Get(context, env->async_id_symbol())
.ToLocal(&maybe_async_id)) {
return;
}

Local<Value> maybe_trigger_async_id;
if (!promise->Get(context, env->trigger_async_id_symbol())
.ToLocal(&maybe_trigger_async_id)) {
return;
}

if (maybe_async_id->IsNumber() && maybe_trigger_async_id->IsNumber()) {
double async_id = maybe_async_id.As<Number>()->Value();
double trigger_async_id = maybe_trigger_async_id.As<Number>()->Value();
double async_id;
double trigger_async_id;
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) {
env->async_hooks()->push_async_context(
async_id, trigger_async_id, promise);
return;
}

return;
}

if (type == PromiseHookType::kAfter &&
env->async_hooks()->fields()[AsyncHooks::kAfter] == 0) {
Local<Value> maybe_async_id;
if (!promise->Get(context, env->async_id_symbol())
.ToLocal(&maybe_async_id)) {
return;
}
double async_id;
if (!GetAssignedPromiseAsyncId(env, promise, env->async_id_symbol())
.To(&async_id)) return;

if (maybe_async_id->IsNumber()) {
double async_id = maybe_async_id.As<Number>()->Value();
if (async_id != AsyncWrap::kInvalidAsyncId) {
if (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);
}
return;
}

return;
}

if (type == PromiseHookType::kResolve &&
Expand Down
25 changes: 25 additions & 0 deletions test/parallel/test-async-hooks-enable-before-promise-resolve.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const async_hooks = require('async_hooks');

// This test ensures that fast-path PromiseHook assigns async ids
// to already created promises when the native hook function is
// triggered on before event.

let initialAsyncId;
const promise = new Promise((resolve) => {
setTimeout(() => {
initialAsyncId = async_hooks.executionAsyncId();
async_hooks.createHook({
after: common.mustCall(() => {}, 2)
}).enable();
resolve();
}, 0);
});

promise.then(common.mustCall(() => {
const id = async_hooks.executionAsyncId();
assert.notStrictEqual(id, initialAsyncId);
assert.ok(id > 0);
}));

0 comments on commit b7a2329

Please sign in to comment.