From 7986a9bee48627c983179fa9a0963769a305f056 Mon Sep 17 00:00:00 2001 From: Bryan English Date: Tue, 25 May 2021 23:52:45 -0400 Subject: [PATCH 1/8] src: set PromiseHooks by Environment 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 --- src/async_wrap.cc | 4 +- src/env-inl.h | 47 +++++++++++++++++++ src/env.h | 13 +++++ src/node_contextify.cc | 2 + .../test-async-local-storage-contexts.js | 35 ++++++++++++++ 5 files changed, 99 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..9942172271f50e 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::Strong(*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,37 @@ 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); +} + +inline void AsyncHooks::RemoveContext(v8::Local ctx) { + for (auto it = contexts_.begin(); it != contexts_.end(); it++) { + v8::Local saved_context = PersistentToLocal::Strong(*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 +349,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.h b/src/env.h index 6b8444f0bc578c..b9490f109efd50 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,10 @@ 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 +779,10 @@ class AsyncHooks : public MemoryRetainer { // Non-empty during deserialization const SerializeInfo* info_ = nullptr; + + std::vector> contexts_; + + v8::Global js_promise_hooks_[4]; }; class ImmediateInfo : public MemoryRetainer { diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 984569ea6b5be2..a208683be811bf 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -127,6 +127,8 @@ ContextifyContext::ContextifyContext( ContextifyContext::~ContextifyContext() { env()->RemoveCleanupHook(CleanupHook, this); + + env()->async_hooks()->RemoveContext(PersistentToLocal::Strong(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(); From bb559761d3690be97e01eba1c50575f812743bf1 Mon Sep 17 00:00:00 2001 From: Bryan English Date: Wed, 26 May 2021 21:28:23 -0400 Subject: [PATCH 2/8] fixup! switch to weak globals --- src/env-inl.h | 3 ++- src/node_contextify.cc | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index 9942172271f50e..ba874a95805143 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -104,7 +104,7 @@ inline void AsyncHooks::SetJSPromiseHooks(v8::Local init, 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::Strong(*it) + PersistentToLocal::Weak(env()->isolate(), *it) ->SetPromiseHooks(init, before, after, resolve); } } @@ -249,6 +249,7 @@ inline void AsyncHooks::AddContext(v8::Local ctx) { 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) { diff --git a/src/node_contextify.cc b/src/node_contextify.cc index a208683be811bf..57b6aa5742d549 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -128,7 +128,8 @@ ContextifyContext::ContextifyContext( ContextifyContext::~ContextifyContext() { env()->RemoveCleanupHook(CleanupHook, this); - env()->async_hooks()->RemoveContext(PersistentToLocal::Strong(context_)); + env()->async_hooks() + ->RemoveContext(PersistentToLocal::Weak(env()->isolate(), context_)); } From 0f4342b87e8dc2938698f716b08e39c86d53dacd Mon Sep 17 00:00:00 2001 From: Bryan English Date: Thu, 27 May 2021 07:15:16 -0400 Subject: [PATCH 3/8] fixup! switch to weak globals for hook functions --- src/env-inl.h | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index ba874a95805143..fc25e4987250d0 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -100,9 +100,13 @@ inline void AsyncHooks::SetJSPromiseHooks(v8::Local init, v8::Local after, v8::Local resolve) { js_promise_hooks_[0].Reset(env()->isolate(), init); + if (!init.IsEmpty()) js_promise_hooks_[0].SetWeak(); js_promise_hooks_[1].Reset(env()->isolate(), before); + if (!before.IsEmpty()) js_promise_hooks_[1].SetWeak(); js_promise_hooks_[2].Reset(env()->isolate(), after); + if (!after.IsEmpty()) js_promise_hooks_[2].SetWeak(); js_promise_hooks_[3].Reset(env()->isolate(), resolve); + if (!resolve.IsEmpty()) js_promise_hooks_[3].SetWeak(); for (auto it = contexts_.begin(); it != contexts_.end(); it++) { PersistentToLocal::Weak(env()->isolate(), *it) ->SetPromiseHooks(init, before, after, resolve); @@ -232,19 +236,20 @@ void AsyncHooks::clear_async_id_stack() { } inline void AsyncHooks::AddContext(v8::Local ctx) { + v8::Isolate * isolate = ctx->GetIsolate(); ctx->SetPromiseHooks( js_promise_hooks_[0].IsEmpty() ? v8::Local() : - PersistentToLocal::Strong(js_promise_hooks_[0]), + PersistentToLocal::Weak(isolate, js_promise_hooks_[0]), js_promise_hooks_[1].IsEmpty() ? v8::Local() : - PersistentToLocal::Strong(js_promise_hooks_[1]), + PersistentToLocal::Weak(isolate, js_promise_hooks_[1]), js_promise_hooks_[2].IsEmpty() ? v8::Local() : - PersistentToLocal::Strong(js_promise_hooks_[2]), + PersistentToLocal::Weak(isolate, js_promise_hooks_[2]), js_promise_hooks_[3].IsEmpty() ? v8::Local() : - PersistentToLocal::Strong(js_promise_hooks_[3])); + PersistentToLocal::Weak(isolate, js_promise_hooks_[3])); size_t id = contexts_.size(); contexts_.resize(id + 1); From d1667ada04a75b2af2d01ddb3571df05012ae811 Mon Sep 17 00:00:00 2001 From: Bryan English Date: Fri, 28 May 2021 14:43:41 -0400 Subject: [PATCH 4/8] fixup! Revert "fixup! switch to weak globals for hook functions" This reverts commit 0f4342b87e8dc2938698f716b08e39c86d53dacd. --- src/env-inl.h | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index fc25e4987250d0..ba874a95805143 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -100,13 +100,9 @@ inline void AsyncHooks::SetJSPromiseHooks(v8::Local init, v8::Local after, v8::Local resolve) { js_promise_hooks_[0].Reset(env()->isolate(), init); - if (!init.IsEmpty()) js_promise_hooks_[0].SetWeak(); js_promise_hooks_[1].Reset(env()->isolate(), before); - if (!before.IsEmpty()) js_promise_hooks_[1].SetWeak(); js_promise_hooks_[2].Reset(env()->isolate(), after); - if (!after.IsEmpty()) js_promise_hooks_[2].SetWeak(); js_promise_hooks_[3].Reset(env()->isolate(), resolve); - if (!resolve.IsEmpty()) js_promise_hooks_[3].SetWeak(); for (auto it = contexts_.begin(); it != contexts_.end(); it++) { PersistentToLocal::Weak(env()->isolate(), *it) ->SetPromiseHooks(init, before, after, resolve); @@ -236,20 +232,19 @@ void AsyncHooks::clear_async_id_stack() { } inline void AsyncHooks::AddContext(v8::Local ctx) { - v8::Isolate * isolate = ctx->GetIsolate(); ctx->SetPromiseHooks( js_promise_hooks_[0].IsEmpty() ? v8::Local() : - PersistentToLocal::Weak(isolate, js_promise_hooks_[0]), + PersistentToLocal::Strong(js_promise_hooks_[0]), js_promise_hooks_[1].IsEmpty() ? v8::Local() : - PersistentToLocal::Weak(isolate, js_promise_hooks_[1]), + PersistentToLocal::Strong(js_promise_hooks_[1]), js_promise_hooks_[2].IsEmpty() ? v8::Local() : - PersistentToLocal::Weak(isolate, js_promise_hooks_[2]), + PersistentToLocal::Strong(js_promise_hooks_[2]), js_promise_hooks_[3].IsEmpty() ? v8::Local() : - PersistentToLocal::Weak(isolate, js_promise_hooks_[3])); + PersistentToLocal::Strong(js_promise_hooks_[3])); size_t id = contexts_.size(); contexts_.resize(id + 1); From d3ec54a0872c5a3960233d669a11e38c29e5e9d8 Mon Sep 17 00:00:00 2001 From: Bryan English Date: Fri, 28 May 2021 17:15:10 -0400 Subject: [PATCH 5/8] fixup! switch accidental strong ref, and use handle scopes --- src/env-inl.h | 5 ++++- src/node_contextify.cc | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index ba874a95805143..e0345b0daf22e4 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -253,8 +253,11 @@ inline void AsyncHooks::AddContext(v8::Local ctx) { } 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::Strong(*it); + v8::Local saved_context = + PersistentToLocal::Weak(env()->isolate(), *it); if (saved_context == ctx) { it->Reset(); contexts_.erase(it); diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 57b6aa5742d549..704020dbec3f23 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -30,6 +30,7 @@ #include "node_internals.h" #include "node_watchdog.h" #include "util-inl.h" +#include "v8.h" namespace node { namespace contextify { @@ -127,9 +128,11 @@ ContextifyContext::ContextifyContext( ContextifyContext::~ContextifyContext() { env()->RemoveCleanupHook(CleanupHook, this); + Isolate* isolate = env()->isolate(); + HandleScope scope(isolate); env()->async_hooks() - ->RemoveContext(PersistentToLocal::Weak(env()->isolate(), context_)); + ->RemoveContext(PersistentToLocal::Weak(isolate, context_)); } From add236021404028855cf86efb3659d96886d0e1d Mon Sep 17 00:00:00 2001 From: Bryan English Date: Mon, 31 May 2021 15:55:29 -0400 Subject: [PATCH 6/8] fixup! review nits and AsyncHooks::MemoryInfo --- src/env.cc | 4 ++++ src/env.h | 1 - src/node_contextify.cc | 1 - 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/env.cc b/src/env.cc index 0952d863b7a3e7..5ee489fafdf4f5 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1164,6 +1164,10 @@ 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->TrackFieldWithSize( + "contexts", contexts_.size() * sizeof(v8::Global)); + tracker->TrackFieldWithSize( + "js_promise_hooks", 4 * sizeof(v8::Global)); } void AsyncHooks::grow_async_ids_stack() { diff --git a/src/env.h b/src/env.h index b9490f109efd50..08d7b4d59ef649 100644 --- a/src/env.h +++ b/src/env.h @@ -719,7 +719,6 @@ class AsyncHooks : public MemoryRetainer { inline void AddContext(v8::Local ctx); inline void RemoveContext(v8::Local ctx); - AsyncHooks(const AsyncHooks&) = delete; AsyncHooks& operator=(const AsyncHooks&) = delete; AsyncHooks(AsyncHooks&&) = delete; diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 704020dbec3f23..ae5fc2fbf1a862 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -30,7 +30,6 @@ #include "node_internals.h" #include "node_watchdog.h" #include "util-inl.h" -#include "v8.h" namespace node { namespace contextify { From a968969d7bbf66ca4319c00da8fbc27100ad9bd9 Mon Sep 17 00:00:00 2001 From: Bryan English Date: Mon, 31 May 2021 16:38:48 -0400 Subject: [PATCH 7/8] fixup! correct MemoryInfo --- src/env.cc | 5 +---- src/env.h | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/src/env.cc b/src/env.cc index 5ee489fafdf4f5..042b7243ac1087 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1164,10 +1164,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->TrackFieldWithSize( - "contexts", contexts_.size() * sizeof(v8::Global)); - tracker->TrackFieldWithSize( - "js_promise_hooks", 4 * sizeof(v8::Global)); + 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 08d7b4d59ef649..ba7cde0d447f05 100644 --- a/src/env.h +++ b/src/env.h @@ -781,7 +781,7 @@ class AsyncHooks : public MemoryRetainer { std::vector> contexts_; - v8::Global js_promise_hooks_[4]; + std::array, 4> js_promise_hooks_; }; class ImmediateInfo : public MemoryRetainer { From 5ea90f7b2c226174fd5302053d0481bf17b8c86b Mon Sep 17 00:00:00 2001 From: Bryan English Date: Tue, 1 Jun 2021 11:38:58 -0400 Subject: [PATCH 8/8] fixup! add checks for snapshot --- src/env.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/env.cc b/src/env.cc index 042b7243ac1087..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; }