FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: convert context_frame field in AsyncWrap to internal field · nodejs/node@0e4af84 · GitHub

/ node Public

Commit 0e4af84

Browse files
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

5 files changed

Lines changed: 80 additions & 19 deletions

File tree

‎src/async_wrap-inl.h‎

Lines 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(

‎src/async_wrap.cc‎

Lines 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.

‎src/async_wrap.h‎

Lines 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

‎src/env_properties.h‎

Lines 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") \

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

Lines 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)
Back | FazBrowse Home | New Git URL