From 18ca52d78efcf146df693da56f3005c7d180cac7 Mon Sep 17 00:00:00 2001 From: Andrey Pechkurov Date: Tue, 28 Jul 2020 22:36:40 +0300 Subject: [PATCH] async_hooks: fix id assignment in fast-path promise hook MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: https://github.com/nodejs/node/pull/34512 PR-URL: https://github.com/nodejs/node/pull/34548 Reviewed-By: Anna Henningsen Reviewed-By: Benjamin Gruenbaum Reviewed-By: Gerhard Stöbich --- src/async_wrap.cc | 82 ++++++++++--------- ...ync-hooks-enable-before-promise-resolve.js | 25 ++++++ 2 files changed, 69 insertions(+), 38 deletions(-) create mode 100644 test/parallel/test-async-hooks-enable-before-promise-resolve.js diff --git a/src/async_wrap.cc b/src/async_wrap.cc index 7d8cc4d7393a16..d6511a1dd8b3c3 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -38,9 +38,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; @@ -188,6 +191,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 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()) + : Just(AsyncWrap::kInvalidAsyncId); +} + class PromiseWrap : public AsyncWrap { public: PromiseWrap(Environment* env, Local object, bool silent) @@ -230,18 +248,17 @@ PromiseWrap* PromiseWrap::New(Environment* env, // Skip for init events if (silent) { - Local maybe_async_id = promise - ->Get(context, env->async_id_symbol()) - .ToLocalChecked(); - - Local 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()->Value(); - double trigger_async_id = maybe_trigger_async_id.As()->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); } } @@ -320,46 +337,35 @@ static void FastPromiseHook(PromiseHookType type, Local promise, if (type == PromiseHookType::kBefore && env->async_hooks()->fields()[AsyncHooks::kBefore] == 0) { - Local maybe_async_id; - if (!promise->Get(context, env->async_id_symbol()) - .ToLocal(&maybe_async_id)) { - return; - } - - Local 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()->Value(); - double trigger_async_id = maybe_trigger_async_id.As()->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 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()->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 && diff --git a/test/parallel/test-async-hooks-enable-before-promise-resolve.js b/test/parallel/test-async-hooks-enable-before-promise-resolve.js new file mode 100644 index 00000000000000..c96c9e5dd96655 --- /dev/null +++ b/test/parallel/test-async-hooks-enable-before-promise-resolve.js @@ -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); +}));