Skip to content

Commit

Permalink
src: set PromiseHooks by Environment
Browse files Browse the repository at this point in the history
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: #36394
Fixes: #38781
Signed-off-by: Bryan English <bryan@bryanenglish.com>

PR-URL: #38821
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
  • Loading branch information
bengl authored and danielleadams committed Jun 2, 2021
1 parent f162896 commit 7ba3055
Show file tree
Hide file tree
Showing 6 changed files with 112 additions and 2 deletions.
4 changes: 2 additions & 2 deletions src/async_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -454,8 +454,8 @@ static void EnablePromiseHook(const FunctionCallbackInfo<Value>& args) {

static void SetPromiseHooks(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
Local<Context> ctx = env->context();
ctx->SetPromiseHooks(

env->async_hooks()->SetJSPromiseHooks(
args[0]->IsFunction() ? args[0].As<Function>() : Local<Function>(),
args[1]->IsFunction() ? args[1].As<Function>() : Local<Function>(),
args[2]->IsFunction() ? args[2].As<Function>() : Local<Function>(),
Expand Down
51 changes: 51 additions & 0 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,20 @@ v8::Local<v8::Object> AsyncHooks::native_execution_async_resource(size_t i) {
return PersistentToLocal::Strong(native_execution_async_resources_[i]);
}

inline void AsyncHooks::SetJSPromiseHooks(v8::Local<v8::Function> init,
v8::Local<v8::Function> before,
v8::Local<v8::Function> after,
v8::Local<v8::Function> 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<v8::String> AsyncHooks::provider_string(int idx) {
return env()->isolate_data()->async_wrap_provider(idx);
}
Expand Down Expand Up @@ -217,6 +231,41 @@ void AsyncHooks::clear_async_id_stack() {
fields_[kStackLength] = 0;
}

inline void AsyncHooks::AddContext(v8::Local<v8::Context> ctx) {
ctx->SetPromiseHooks(
js_promise_hooks_[0].IsEmpty() ?
v8::Local<v8::Function>() :
PersistentToLocal::Strong(js_promise_hooks_[0]),
js_promise_hooks_[1].IsEmpty() ?
v8::Local<v8::Function>() :
PersistentToLocal::Strong(js_promise_hooks_[1]),
js_promise_hooks_[2].IsEmpty() ?
v8::Local<v8::Function>() :
PersistentToLocal::Strong(js_promise_hooks_[2]),
js_promise_hooks_[3].IsEmpty() ?
v8::Local<v8::Function>() :
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<v8::Context> ctx) {
v8::Isolate* isolate = env()->isolate();
v8::HandleScope handle_scope(isolate);
for (auto it = contexts_.begin(); it != contexts_.end(); it++) {
v8::Local<v8::Context> 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.

Expand Down Expand Up @@ -304,6 +353,8 @@ inline void Environment::AssignToContext(v8::Local<v8::Context> context,
#if HAVE_INSPECTOR
inspector_agent()->ContextCreated(context, info);
#endif // HAVE_INSPECTOR

this->async_hooks()->AddContext(context);
}

inline Environment* Environment::GetCurrent(v8::Isolate* isolate) {
Expand Down
7 changes: 7 additions & 0 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1156,6 +1156,12 @@ AsyncHooks::SerializeInfo AsyncHooks::Serialize(Local<Context> 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;
}
Expand All @@ -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() {
Expand Down
12 changes: 12 additions & 0 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -701,6 +701,11 @@ class AsyncHooks : public MemoryRetainer {
// The `js_execution_async_resources` array contains the value in that case.
inline v8::Local<v8::Object> native_execution_async_resource(size_t index);

inline void SetJSPromiseHooks(v8::Local<v8::Function> init,
v8::Local<v8::Function> before,
v8::Local<v8::Function> after,
v8::Local<v8::Function> resolve);

inline v8::Local<v8::String> provider_string(int idx);

inline void no_force_checks();
Expand All @@ -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<v8::Context> ctx);
inline void RemoveContext(v8::Local<v8::Context> ctx);

AsyncHooks(const AsyncHooks&) = delete;
AsyncHooks& operator=(const AsyncHooks&) = delete;
AsyncHooks(AsyncHooks&&) = delete;
Expand Down Expand Up @@ -770,6 +778,10 @@ class AsyncHooks : public MemoryRetainer {

// Non-empty during deserialization
const SerializeInfo* info_ = nullptr;

std::vector<v8::Global<v8::Context>> contexts_;

std::array<v8::Global<v8::Function>, 4> js_promise_hooks_;
};

class ImmediateInfo : public MemoryRetainer {
Expand Down
5 changes: 5 additions & 0 deletions src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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_));
}


Expand Down
35 changes: 35 additions & 0 deletions test/parallel/test-async-local-storage-contexts.js
Original file line number Diff line number Diff line change
@@ -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();

0 comments on commit 7ba3055

Please sign in to comment.