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 0e4af84

Browse filesBrowse files
addaleaxaduh95
addaleax
authored and
aduh95
committed
src: convert context_frame field in AsyncWrap to internal field
Using an internal field instead of a `v8::Global<>` removes an unnecessary memory leak footgun. This includes a test that demonstrates the issue, albeit using internal APIs. It's worth noting that if this PR is not accepted, we'd still be missing memory tracking for the `context_frame_` field, and we'd need to add it through our memory tracking API. PR-URL: #62103 Backport-PR-URL: #62357 Reviewed-By: Anna Henningsen <anna@addaleax.net>
1 parent f1b2861 commit 0e4af84
Copy full SHA for 0e4af84

5 files changed

+80-19Lines changed: 80 additions & 19 deletions

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎src/async_wrap-inl.h‎

Copy file name to clipboardExpand all lines: src/async_wrap-inl.h
+19-1Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,25 @@ inline double AsyncWrap::get_trigger_async_id() const {
5050
}
5151

5252
inline v8::Local<v8::Value> AsyncWrap::context_frame() const {
53-
return context_frame_.Get(env()->isolate());
53+
auto as_data = object()->GetInternalField(kAsyncContextFrame);
54+
DCHECK_IMPLIES(!as_data.IsEmpty(),
55+
as_data->IsValue() || as_data->IsPrivate());
56+
if (as_data->IsPrivate()) {
57+
DCHECK(as_data.As<v8::Private>()->Name()->SameValue(
58+
env()->empty_context_frame_sentinel_symbol()->Name()));
59+
return {};
60+
}
61+
return as_data.As<v8::Value>();
62+
}
63+
64+
inline void AsyncWrap::set_context_frame(v8::Local<v8::Value> value) {
65+
if (value.IsEmpty()) {
66+
// Empty values are not allowed in internal fields
67+
object()->SetInternalField(kAsyncContextFrame,
68+
env()->empty_context_frame_sentinel_symbol());
69+
} else {
70+
object()->SetInternalField(kAsyncContextFrame, value);
71+
}
5472
}
5573

5674
inline v8::MaybeLocal<v8::Value> AsyncWrap::MakeCallback(
Collapse file

‎src/async_wrap.cc‎

Copy file name to clipboardExpand all lines: src/async_wrap.cc
+8-15Lines changed: 8 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -348,7 +348,7 @@ void AsyncWrap::EmitDestroy(bool from_gc) {
348348
HandleScope handle_scope(env()->isolate());
349349
USE(object()->Set(env()->context(), env()->resource_symbol(), object()));
350350
}
351-
context_frame_.Reset();
351+
set_context_frame({});
352352
}
353353
}
354354

@@ -530,9 +530,9 @@ AsyncWrap::AsyncWrap(Environment* env,
530530
}
531531

532532
AsyncWrap::AsyncWrap(Environment* env, Local<Object> object)
533-
: BaseObject(env, object),
534-
context_frame_(env->isolate(),
535-
async_context_frame::current(env->isolate())) {}
533+
: BaseObject(env, object) {
534+
set_context_frame(async_context_frame::current(env->isolate()));
535+
}
536536

537537
// This method is necessary to work around one specific problem:
538538
// Before the init() hook runs, if there is one, the BaseObject() constructor
@@ -635,7 +635,7 @@ void AsyncWrap::AsyncReset(Local<Object> resource, double execution_async_id) {
635635

636636
EmitTraceAsyncStart();
637637

638-
context_frame_.Reset(isolate, async_context_frame::current(isolate));
638+
set_context_frame(async_context_frame::current(isolate));
639639

640640
EmitAsyncInit(env(), resource,
641641
env()->async_hooks()->provider_string(provider_type()),
@@ -678,7 +678,7 @@ MaybeLocal<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
678678
EmitTraceEventBefore();
679679

680680
#ifdef DEBUG
681-
if (context_frame_.IsEmpty()) {
681+
if (context_frame().IsEmpty()) {
682682
ProcessEmitWarning(env(),
683683
"MakeCallback() called without context_frame, "
684684
"likely use after destroy of AsyncWrap.");
@@ -687,15 +687,8 @@ MaybeLocal<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
687687

688688
ProviderType provider = provider_type();
689689
async_context context { get_async_id(), get_trigger_async_id() };
690-
MaybeLocal<Value> ret =
691-
InternalMakeCallback(env(),
692-
object(),
693-
object(),
694-
cb,
695-
argc,
696-
argv,
697-
context,
698-
context_frame_.Get(env()->isolate()));
690+
MaybeLocal<Value> ret = InternalMakeCallback(
691+
env(), object(), object(), cb, argc, argv, context, context_frame());
699692

700693
// This is a static call with cached values because the `this` object may
701694
// no longer be alive at this point.
Collapse file

‎src/async_wrap.h‎

Copy file name to clipboardExpand all lines: src/async_wrap.h
+3-3Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,8 @@ class ExternalReferenceRegistry;
119119
class AsyncWrap : public BaseObject {
120120
public:
121121
enum InternalFields {
122-
kInternalFieldCount = BaseObject::kInternalFieldCount,
122+
kAsyncContextFrame = BaseObject::kInternalFieldCount,
123+
kInternalFieldCount,
123124
};
124125

125126
enum ProviderType {
@@ -201,6 +202,7 @@ class AsyncWrap : public BaseObject {
201202
inline double get_trigger_async_id() const;
202203

203204
inline v8::Local<v8::Value> context_frame() const;
205+
inline void set_context_frame(v8::Local<v8::Value> value);
204206

205207
void AsyncReset(v8::Local<v8::Object> resource,
206208
double execution_async_id = kInvalidAsyncId);
@@ -244,8 +246,6 @@ class AsyncWrap : public BaseObject {
244246
// Because the values may be Reset(), cannot be made const.
245247
double async_id_ = kInvalidAsyncId;
246248
double trigger_async_id_ = kInvalidAsyncId;
247-
248-
v8::Global<v8::Value> context_frame_;
249249
};
250250

251251
} // namespace node
Collapse file

‎src/env_properties.h‎

Copy file name to clipboardExpand all lines: src/env_properties.h
+1Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
V(arrow_message_private_symbol, "node:arrowMessage") \
2222
V(contextify_context_private_symbol, "node:contextify:context") \
2323
V(decorated_private_symbol, "node:decorated") \
24+
V(empty_context_frame_sentinel_symbol, "node:empty_context_frame_sentinel") \
2425
V(transfer_mode_private_symbol, "node:transfer_mode") \
2526
V(host_defined_option_symbol, "node:host_defined_option_symbol") \
2627
V(js_transferable_wrapper_private_symbol, "node:js_transferable_wrapper") \
Collapse file

‎test/parallel/test-async-local-storage-weak-asyncwrap-leak.js‎

Copy file name to clipboard
+49Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
// Flags: --expose-gc
2+
'use strict';
3+
const common = require('../common');
4+
const assert = require('node:assert');
5+
const zlib = require('node:zlib');
6+
const v8 = require('node:v8');
7+
const { AsyncLocalStorage } = require('node:async_hooks');
8+
9+
// This test verifies that referencing an AsyncLocalStorage store from
10+
// a weak AsyncWrap does not prevent the store from being garbage collected.
11+
// We use zlib streams as examples of weak AsyncWraps here, but the
12+
// issue is not specific to zlib.
13+
14+
class Store {}
15+
16+
let zlibDone = false;
17+
18+
// Use immediates to ensure no accidental async context propagation occurs
19+
setImmediate(common.mustCall(() => {
20+
const asyncLocalStorage = new AsyncLocalStorage();
21+
const store = new Store();
22+
asyncLocalStorage.run(store, common.mustCall(() => {
23+
(async () => {
24+
const zlibStream = zlib.createGzip();
25+
// (Make sure this test does not break if _handle is renamed
26+
// to something else)
27+
assert.strictEqual(typeof zlibStream._handle, 'object');
28+
// Create backreference from AsyncWrap to store
29+
store.zlibStream = zlibStream._handle;
30+
// Let the zlib stream finish (not strictly necessary)
31+
zlibStream.end('hello world');
32+
await zlibStream.toArray();
33+
zlibDone = true;
34+
})().then(common.mustCall());
35+
}));
36+
}));
37+
38+
const finish = common.mustCall(() => {
39+
global.gc();
40+
// Make sure the ALS instance has been garbage-collected
41+
assert.strictEqual(v8.queryObjects(Store), 0);
42+
});
43+
44+
const interval = setInterval(() => {
45+
if (zlibDone) {
46+
clearInterval(interval);
47+
finish();
48+
}
49+
}, 5);

0 commit comments

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