From 7ba305552f2f2a4ad3a5b33fed1f1d8393b783a4 Mon Sep 17 00:00:00 2001 From: Bryan English Date: Tue, 25 May 2021 23:52:45 -0400 Subject: [PATCH] src: set PromiseHooks by Environment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The new JS PromiseHooks introduced in the referenced PR are per v8::Context. This meant that code depending on them, such as AsyncLocalStorage, wouldn't behave correctly across vm.Context instances. PromiseHooks are now synchronized across the main Context and any Context created via vm.Context. Refs: https://github.com/nodejs/node/pull/36394 Fixes: https://github.com/nodejs/node/issues/38781 Signed-off-by: Bryan English PR-URL: https://github.com/nodejs/node/pull/38821 Reviewed-By: Stephen Belanger Reviewed-By: Vladimir de Turckheim Reviewed-By: Anna Henningsen Reviewed-By: Gerhard Stöbich Reviewed-By: Joyee Cheung --- src/async_wrap.cc | 4 +- src/env-inl.h | 51 +++++++++++++++++++ src/env.cc | 7 +++ src/env.h | 12 +++++ src/node_contextify.cc | 5 ++ .../test-async-local-storage-contexts.js | 35 +++++++++++++ 6 files changed, 112 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-async-local-storage-contexts.js diff --git a/src/async_wrap.cc b/src/async_wrap.cc index a1c76b94138762..1bca27d6a0fd25 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -454,8 +454,8 @@ static void EnablePromiseHook(const FunctionCallbackInfo& args) { static void SetPromiseHooks(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - Local ctx = env->context(); - ctx->SetPromiseHooks( + + env->async_hooks()->SetJSPromiseHooks( args[0]->IsFunction() ? args[0].As() : Local(), args[1]->IsFunction() ? args[1].As() : Local(), args[2]->IsFunction() ? args[2].As() : Local(), diff --git a/src/env-inl.h b/src/env-inl.h index f88e7648155186..e0345b0daf22e4 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -95,6 +95,20 @@ v8::Local AsyncHooks::native_execution_async_resource(size_t i) { return PersistentToLocal::Strong(native_execution_async_resources_[i]); } +inline void AsyncHooks::SetJSPromiseHooks(v8::Local init, + v8::Local before, + v8::Local after, + v8::Local resolve) { + js_promise_hooks_[0].Reset(env()->isolate(), init); + js_promise_hooks_[1].Reset(env()->isolate(), before); + js_promise_hooks_[2].Reset(env()->isolate(), after); + js_promise_hooks_[3].Reset(env()->isolate(), resolve); + for (auto it = contexts_.begin(); it != contexts_.end(); it++) { + PersistentToLocal::Weak(env()->isolate(), *it) + ->SetPromiseHooks(init, before, after, resolve); + } +} + inline v8::Local AsyncHooks::provider_string(int idx) { return env()->isolate_data()->async_wrap_provider(idx); } @@ -217,6 +231,41 @@ void AsyncHooks::clear_async_id_stack() { fields_[kStackLength] = 0; } +inline void AsyncHooks::AddContext(v8::Local ctx) { + ctx->SetPromiseHooks( + js_promise_hooks_[0].IsEmpty() ? + v8::Local() : + PersistentToLocal::Strong(js_promise_hooks_[0]), + js_promise_hooks_[1].IsEmpty() ? + v8::Local() : + PersistentToLocal::Strong(js_promise_hooks_[1]), + js_promise_hooks_[2].IsEmpty() ? + v8::Local() : + PersistentToLocal::Strong(js_promise_hooks_[2]), + js_promise_hooks_[3].IsEmpty() ? + v8::Local() : + PersistentToLocal::Strong(js_promise_hooks_[3])); + + size_t id = contexts_.size(); + contexts_.resize(id + 1); + contexts_[id].Reset(env()->isolate(), ctx); + contexts_[id].SetWeak(); +} + +inline void AsyncHooks::RemoveContext(v8::Local ctx) { + v8::Isolate* isolate = env()->isolate(); + v8::HandleScope handle_scope(isolate); + for (auto it = contexts_.begin(); it != contexts_.end(); it++) { + v8::Local saved_context = + PersistentToLocal::Weak(env()->isolate(), *it); + if (saved_context == ctx) { + it->Reset(); + contexts_.erase(it); + break; + } + } +} + // The DefaultTriggerAsyncIdScope(AsyncWrap*) constructor is defined in // async_wrap-inl.h to avoid a circular dependency. @@ -304,6 +353,8 @@ inline void Environment::AssignToContext(v8::Local context, #if HAVE_INSPECTOR inspector_agent()->ContextCreated(context, info); #endif // HAVE_INSPECTOR + + this->async_hooks()->AddContext(context); } inline Environment* Environment::GetCurrent(v8::Isolate* isolate) { diff --git a/src/env.cc b/src/env.cc index 0952d863b7a3e7..9f6172de82bd92 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1156,6 +1156,12 @@ AsyncHooks::SerializeInfo AsyncHooks::Serialize(Local context, context, native_execution_async_resources_[i].Get(context->GetIsolate())); } + CHECK_EQ(contexts_.size(), 1); + CHECK_EQ(contexts_[0], env()->context()); + CHECK(js_promise_hooks_[0].IsEmpty()); + CHECK(js_promise_hooks_[1].IsEmpty()); + CHECK(js_promise_hooks_[2].IsEmpty()); + CHECK(js_promise_hooks_[3].IsEmpty()); return info; } @@ -1164,6 +1170,7 @@ void AsyncHooks::MemoryInfo(MemoryTracker* tracker) const { tracker->TrackField("async_ids_stack", async_ids_stack_); tracker->TrackField("fields", fields_); tracker->TrackField("async_id_fields", async_id_fields_); + tracker->TrackField("js_promise_hooks", js_promise_hooks_); } void AsyncHooks::grow_async_ids_stack() { diff --git a/src/env.h b/src/env.h index ce88d74355c732..4a50227ee8dc2d 100644 --- a/src/env.h +++ b/src/env.h @@ -701,6 +701,11 @@ class AsyncHooks : public MemoryRetainer { // The `js_execution_async_resources` array contains the value in that case. inline v8::Local native_execution_async_resource(size_t index); + inline void SetJSPromiseHooks(v8::Local init, + v8::Local before, + v8::Local after, + v8::Local resolve); + inline v8::Local provider_string(int idx); inline void no_force_checks(); @@ -711,6 +716,9 @@ class AsyncHooks : public MemoryRetainer { inline bool pop_async_context(double async_id); inline void clear_async_id_stack(); // Used in fatal exceptions. + inline void AddContext(v8::Local ctx); + inline void RemoveContext(v8::Local ctx); + AsyncHooks(const AsyncHooks&) = delete; AsyncHooks& operator=(const AsyncHooks&) = delete; AsyncHooks(AsyncHooks&&) = delete; @@ -770,6 +778,10 @@ class AsyncHooks : public MemoryRetainer { // Non-empty during deserialization const SerializeInfo* info_ = nullptr; + + std::vector> contexts_; + + std::array, 4> js_promise_hooks_; }; class ImmediateInfo : public MemoryRetainer { diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 984569ea6b5be2..ae5fc2fbf1a862 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -127,6 +127,11 @@ ContextifyContext::ContextifyContext( ContextifyContext::~ContextifyContext() { env()->RemoveCleanupHook(CleanupHook, this); + Isolate* isolate = env()->isolate(); + HandleScope scope(isolate); + + env()->async_hooks() + ->RemoveContext(PersistentToLocal::Weak(isolate, context_)); } diff --git a/test/parallel/test-async-local-storage-contexts.js b/test/parallel/test-async-local-storage-contexts.js new file mode 100644 index 00000000000000..9a6327133794f6 --- /dev/null +++ b/test/parallel/test-async-local-storage-contexts.js @@ -0,0 +1,35 @@ +'use strict'; + +require('../common'); +const assert = require('assert'); +const vm = require('vm'); +const { AsyncLocalStorage } = require('async_hooks'); + +// Regression test for https://github.com/nodejs/node/issues/38781 + +const context = vm.createContext({ + AsyncLocalStorage, + assert +}); + +vm.runInContext(` + const storage = new AsyncLocalStorage() + async function test() { + return storage.run({ test: 'vm' }, async () => { + assert.strictEqual(storage.getStore().test, 'vm'); + await 42; + assert.strictEqual(storage.getStore().test, 'vm'); + }); + } + test() +`, context); + +const storage = new AsyncLocalStorage(); +async function test() { + return storage.run({ test: 'main context' }, async () => { + assert.strictEqual(storage.getStore().test, 'main context'); + await 42; + assert.strictEqual(storage.getStore().test, 'main context'); + }); +} +test();