Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 7ba3055

Browse filesBrowse files
bengldanielleadams
bengl
authored and
danielleadams
committed
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: #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>
1 parent f162896 commit 7ba3055
Copy full SHA for 7ba3055

File tree

Expand file treeCollapse file tree

6 files changed

+112
-2
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

6 files changed

+112
-2
lines changed
Open diff view settings
Collapse file

‎src/async_wrap.cc‎

Copy file name to clipboardExpand all lines: src/async_wrap.cc
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -454,8 +454,8 @@ static void EnablePromiseHook(const FunctionCallbackInfo<Value>& args) {
454454

455455
static void SetPromiseHooks(const FunctionCallbackInfo<Value>& args) {
456456
Environment* env = Environment::GetCurrent(args);
457-
Local<Context> ctx = env->context();
458-
ctx->SetPromiseHooks(
457+
458+
env->async_hooks()->SetJSPromiseHooks(
459459
args[0]->IsFunction() ? args[0].As<Function>() : Local<Function>(),
460460
args[1]->IsFunction() ? args[1].As<Function>() : Local<Function>(),
461461
args[2]->IsFunction() ? args[2].As<Function>() : Local<Function>(),
Collapse file

‎src/env-inl.h‎

Copy file name to clipboardExpand all lines: src/env-inl.h
+51Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,20 @@ v8::Local<v8::Object> AsyncHooks::native_execution_async_resource(size_t i) {
9595
return PersistentToLocal::Strong(native_execution_async_resources_[i]);
9696
}
9797

98+
inline void AsyncHooks::SetJSPromiseHooks(v8::Local<v8::Function> init,
99+
v8::Local<v8::Function> before,
100+
v8::Local<v8::Function> after,
101+
v8::Local<v8::Function> resolve) {
102+
js_promise_hooks_[0].Reset(env()->isolate(), init);
103+
js_promise_hooks_[1].Reset(env()->isolate(), before);
104+
js_promise_hooks_[2].Reset(env()->isolate(), after);
105+
js_promise_hooks_[3].Reset(env()->isolate(), resolve);
106+
for (auto it = contexts_.begin(); it != contexts_.end(); it++) {
107+
PersistentToLocal::Weak(env()->isolate(), *it)
108+
->SetPromiseHooks(init, before, after, resolve);
109+
}
110+
}
111+
98112
inline v8::Local<v8::String> AsyncHooks::provider_string(int idx) {
99113
return env()->isolate_data()->async_wrap_provider(idx);
100114
}
@@ -217,6 +231,41 @@ void AsyncHooks::clear_async_id_stack() {
217231
fields_[kStackLength] = 0;
218232
}
219233

234+
inline void AsyncHooks::AddContext(v8::Local<v8::Context> ctx) {
235+
ctx->SetPromiseHooks(
236+
js_promise_hooks_[0].IsEmpty() ?
237+
v8::Local<v8::Function>() :
238+
PersistentToLocal::Strong(js_promise_hooks_[0]),
239+
js_promise_hooks_[1].IsEmpty() ?
240+
v8::Local<v8::Function>() :
241+
PersistentToLocal::Strong(js_promise_hooks_[1]),
242+
js_promise_hooks_[2].IsEmpty() ?
243+
v8::Local<v8::Function>() :
244+
PersistentToLocal::Strong(js_promise_hooks_[2]),
245+
js_promise_hooks_[3].IsEmpty() ?
246+
v8::Local<v8::Function>() :
247+
PersistentToLocal::Strong(js_promise_hooks_[3]));
248+
249+
size_t id = contexts_.size();
250+
contexts_.resize(id + 1);
251+
contexts_[id].Reset(env()->isolate(), ctx);
252+
contexts_[id].SetWeak();
253+
}
254+
255+
inline void AsyncHooks::RemoveContext(v8::Local<v8::Context> ctx) {
256+
v8::Isolate* isolate = env()->isolate();
257+
v8::HandleScope handle_scope(isolate);
258+
for (auto it = contexts_.begin(); it != contexts_.end(); it++) {
259+
v8::Local<v8::Context> saved_context =
260+
PersistentToLocal::Weak(env()->isolate(), *it);
261+
if (saved_context == ctx) {
262+
it->Reset();
263+
contexts_.erase(it);
264+
break;
265+
}
266+
}
267+
}
268+
220269
// The DefaultTriggerAsyncIdScope(AsyncWrap*) constructor is defined in
221270
// async_wrap-inl.h to avoid a circular dependency.
222271

@@ -304,6 +353,8 @@ inline void Environment::AssignToContext(v8::Local<v8::Context> context,
304353
#if HAVE_INSPECTOR
305354
inspector_agent()->ContextCreated(context, info);
306355
#endif // HAVE_INSPECTOR
356+
357+
this->async_hooks()->AddContext(context);
307358
}
308359

309360
inline Environment* Environment::GetCurrent(v8::Isolate* isolate) {
Collapse file

‎src/env.cc‎

Copy file name to clipboardExpand all lines: src/env.cc
+7Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1156,6 +1156,12 @@ AsyncHooks::SerializeInfo AsyncHooks::Serialize(Local<Context> context,
11561156
context,
11571157
native_execution_async_resources_[i].Get(context->GetIsolate()));
11581158
}
1159+
CHECK_EQ(contexts_.size(), 1);
1160+
CHECK_EQ(contexts_[0], env()->context());
1161+
CHECK(js_promise_hooks_[0].IsEmpty());
1162+
CHECK(js_promise_hooks_[1].IsEmpty());
1163+
CHECK(js_promise_hooks_[2].IsEmpty());
1164+
CHECK(js_promise_hooks_[3].IsEmpty());
11591165

11601166
return info;
11611167
}
@@ -1164,6 +1170,7 @@ void AsyncHooks::MemoryInfo(MemoryTracker* tracker) const {
11641170
tracker->TrackField("async_ids_stack", async_ids_stack_);
11651171
tracker->TrackField("fields", fields_);
11661172
tracker->TrackField("async_id_fields", async_id_fields_);
1173+
tracker->TrackField("js_promise_hooks", js_promise_hooks_);
11671174
}
11681175

11691176
void AsyncHooks::grow_async_ids_stack() {
Collapse file

‎src/env.h‎

Copy file name to clipboardExpand all lines: src/env.h
+12Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -701,6 +701,11 @@ class AsyncHooks : public MemoryRetainer {
701701
// The `js_execution_async_resources` array contains the value in that case.
702702
inline v8::Local<v8::Object> native_execution_async_resource(size_t index);
703703

704+
inline void SetJSPromiseHooks(v8::Local<v8::Function> init,
705+
v8::Local<v8::Function> before,
706+
v8::Local<v8::Function> after,
707+
v8::Local<v8::Function> resolve);
708+
704709
inline v8::Local<v8::String> provider_string(int idx);
705710

706711
inline void no_force_checks();
@@ -711,6 +716,9 @@ class AsyncHooks : public MemoryRetainer {
711716
inline bool pop_async_context(double async_id);
712717
inline void clear_async_id_stack(); // Used in fatal exceptions.
713718

719+
inline void AddContext(v8::Local<v8::Context> ctx);
720+
inline void RemoveContext(v8::Local<v8::Context> ctx);
721+
714722
AsyncHooks(const AsyncHooks&) = delete;
715723
AsyncHooks& operator=(const AsyncHooks&) = delete;
716724
AsyncHooks(AsyncHooks&&) = delete;
@@ -770,6 +778,10 @@ class AsyncHooks : public MemoryRetainer {
770778

771779
// Non-empty during deserialization
772780
const SerializeInfo* info_ = nullptr;
781+
782+
std::vector<v8::Global<v8::Context>> contexts_;
783+
784+
std::array<v8::Global<v8::Function>, 4> js_promise_hooks_;
773785
};
774786

775787
class ImmediateInfo : public MemoryRetainer {
Collapse file

‎src/node_contextify.cc‎

Copy file name to clipboardExpand all lines: src/node_contextify.cc
+5Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,11 @@ ContextifyContext::ContextifyContext(
127127

128128
ContextifyContext::~ContextifyContext() {
129129
env()->RemoveCleanupHook(CleanupHook, this);
130+
Isolate* isolate = env()->isolate();
131+
HandleScope scope(isolate);
132+
133+
env()->async_hooks()
134+
->RemoveContext(PersistentToLocal::Weak(isolate, context_));
130135
}
131136

132137

Collapse file

‎test/parallel/test-async-local-storage-contexts.js‎

Copy file name to clipboard
+35Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
'use strict';
2+
3+
require('../common');
4+
const assert = require('assert');
5+
const vm = require('vm');
6+
const { AsyncLocalStorage } = require('async_hooks');
7+
8+
// Regression test for https://github.com/nodejs/node/issues/38781
9+
10+
const context = vm.createContext({
11+
AsyncLocalStorage,
12+
assert
13+
});
14+
15+
vm.runInContext(`
16+
const storage = new AsyncLocalStorage()
17+
async function test() {
18+
return storage.run({ test: 'vm' }, async () => {
19+
assert.strictEqual(storage.getStore().test, 'vm');
20+
await 42;
21+
assert.strictEqual(storage.getStore().test, 'vm');
22+
});
23+
}
24+
test()
25+
`, context);
26+
27+
const storage = new AsyncLocalStorage();
28+
async function test() {
29+
return storage.run({ test: 'main context' }, async () => {
30+
assert.strictEqual(storage.getStore().test, 'main context');
31+
await 42;
32+
assert.strictEqual(storage.getStore().test, 'main context');
33+
});
34+
}
35+
test();

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.