Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: set PromiseHooks by Environment #38821

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
48 changes: 48 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,38 @@ 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]));
addaleax marked this conversation as resolved.
Show resolved Hide resolved

size_t id = contexts_.size();
contexts_.resize(id + 1);
contexts_[id].Reset(env()->isolate(), ctx);
contexts_[id].SetWeak();
addaleax marked this conversation as resolved.
Show resolved Hide resolved
}

inline void AsyncHooks::RemoveContext(v8::Local<v8::Context> ctx) {
for (auto it = contexts_.begin(); it != contexts_.end(); it++) {
v8::Local<v8::Context> 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.

Expand Down Expand Up @@ -304,6 +350,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
13 changes: 13 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,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<v8::Context> ctx);
inline void RemoveContext(v8::Local<v8::Context> ctx);


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just one newline?

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

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

std::vector<v8::Global<v8::Context>> contexts_;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we should implement snapshot support for these, but since they aren't used by default in the bootstrap, I think we could just CHECK that these are empty in AsyncHooks::Serialize to indicate that these are not yet supported (I am not sure if the support of promise hooks in snapshots is implemented on the V8 side either though).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joyeecheung There will be one (weak ref'd) context in the vector at snapshot time (the main context). Do you mean the promise hook functions?

e.g.

diff --git a/src/env.cc b/src/env.cc
index 042b7243ac..f4d789710e 100644
--- a/src/env.cc
+++ b/src/env.cc
@@ -1156,6 +1156,11 @@ AsyncHooks::SerializeInfo AsyncHooks::Serialize(Local<Context> context,
         context,
         native_execution_async_resources_[i].Get(context->GetIsolate()));
   }
+  CHECK_EQ(contexts_.size(), 1);
+  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;
 }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, that looks about right (maybe for additional safety, you could check that one context is indeed the main context)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, done in latest fixup commit 👍


v8::Global<v8::Function> js_promise_hooks_[4];
};

class ImmediateInfo : public MemoryRetainer {
Expand Down
3 changes: 3 additions & 0 deletions src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,9 @@ ContextifyContext::ContextifyContext(

ContextifyContext::~ContextifyContext() {
env()->RemoveCleanupHook(CleanupHook, this);

env()->async_hooks()
->RemoveContext(PersistentToLocal::Weak(env()->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();