diff --git a/src/async_wrap-inl.h b/src/async_wrap-inl.h index 432b22e6c8d185..5dd2b72036ce77 100644 --- a/src/async_wrap-inl.h +++ b/src/async_wrap-inl.h @@ -50,7 +50,25 @@ inline double AsyncWrap::get_trigger_async_id() const { } inline v8::Local AsyncWrap::context_frame() const { - return context_frame_.Get(env()->isolate()); + auto as_data = object()->GetInternalField(kAsyncContextFrame); + DCHECK_IMPLIES(!as_data.IsEmpty(), + as_data->IsValue() || as_data->IsPrivate()); + if (as_data->IsPrivate()) { + DCHECK(as_data.As()->Name()->SameValue( + env()->empty_context_frame_sentinel_symbol()->Name())); + return {}; + } + return as_data.As(); +} + +inline void AsyncWrap::set_context_frame(v8::Local value) { + if (value.IsEmpty()) { + // Empty values are not allowed in internal fields + object()->SetInternalField(kAsyncContextFrame, + env()->empty_context_frame_sentinel_symbol()); + } else { + object()->SetInternalField(kAsyncContextFrame, value); + } } inline v8::MaybeLocal AsyncWrap::MakeCallback( diff --git a/src/async_wrap.cc b/src/async_wrap.cc index e8c8db0425704c..5007fd34414abf 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -348,7 +348,7 @@ void AsyncWrap::EmitDestroy(bool from_gc) { HandleScope handle_scope(env()->isolate()); USE(object()->Set(env()->context(), env()->resource_symbol(), object())); } - context_frame_.Reset(); + set_context_frame({}); } } @@ -530,9 +530,9 @@ AsyncWrap::AsyncWrap(Environment* env, } AsyncWrap::AsyncWrap(Environment* env, Local object) - : BaseObject(env, object), - context_frame_(env->isolate(), - async_context_frame::current(env->isolate())) {} + : BaseObject(env, object) { + set_context_frame(async_context_frame::current(env->isolate())); +} // This method is necessary to work around one specific problem: // Before the init() hook runs, if there is one, the BaseObject() constructor @@ -635,7 +635,7 @@ void AsyncWrap::AsyncReset(Local resource, double execution_async_id) { EmitTraceAsyncStart(); - context_frame_.Reset(isolate, async_context_frame::current(isolate)); + set_context_frame(async_context_frame::current(isolate)); EmitAsyncInit(env(), resource, env()->async_hooks()->provider_string(provider_type()), @@ -678,7 +678,7 @@ MaybeLocal AsyncWrap::MakeCallback(const Local cb, EmitTraceEventBefore(); #ifdef DEBUG - if (context_frame_.IsEmpty()) { + if (context_frame().IsEmpty()) { ProcessEmitWarning(env(), "MakeCallback() called without context_frame, " "likely use after destroy of AsyncWrap."); @@ -687,15 +687,8 @@ MaybeLocal AsyncWrap::MakeCallback(const Local cb, ProviderType provider = provider_type(); async_context context { get_async_id(), get_trigger_async_id() }; - MaybeLocal ret = - InternalMakeCallback(env(), - object(), - object(), - cb, - argc, - argv, - context, - context_frame_.Get(env()->isolate())); + MaybeLocal ret = InternalMakeCallback( + env(), object(), object(), cb, argc, argv, context, context_frame()); // This is a static call with cached values because the `this` object may // no longer be alive at this point. diff --git a/src/async_wrap.h b/src/async_wrap.h index 8ca74820da5674..e4884cb88301d4 100644 --- a/src/async_wrap.h +++ b/src/async_wrap.h @@ -119,7 +119,8 @@ class ExternalReferenceRegistry; class AsyncWrap : public BaseObject { public: enum InternalFields { - kInternalFieldCount = BaseObject::kInternalFieldCount, + kAsyncContextFrame = BaseObject::kInternalFieldCount, + kInternalFieldCount, }; enum ProviderType { @@ -201,6 +202,7 @@ class AsyncWrap : public BaseObject { inline double get_trigger_async_id() const; inline v8::Local context_frame() const; + inline void set_context_frame(v8::Local value); void AsyncReset(v8::Local resource, double execution_async_id = kInvalidAsyncId); @@ -244,8 +246,6 @@ class AsyncWrap : public BaseObject { // Because the values may be Reset(), cannot be made const. double async_id_ = kInvalidAsyncId; double trigger_async_id_ = kInvalidAsyncId; - - v8::Global context_frame_; }; } // namespace node diff --git a/src/env_properties.h b/src/env_properties.h index 91e2e06c3c2703..7f0abdac9ca452 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -21,6 +21,7 @@ V(arrow_message_private_symbol, "node:arrowMessage") \ V(contextify_context_private_symbol, "node:contextify:context") \ V(decorated_private_symbol, "node:decorated") \ + V(empty_context_frame_sentinel_symbol, "node:empty_context_frame_sentinel") \ V(transfer_mode_private_symbol, "node:transfer_mode") \ V(host_defined_option_symbol, "node:host_defined_option_symbol") \ V(js_transferable_wrapper_private_symbol, "node:js_transferable_wrapper") \ diff --git a/src/node_snapshotable.cc b/src/node_snapshotable.cc index c3e995adf43741..859ef370645fc7 100644 --- a/src/node_snapshotable.cc +++ b/src/node_snapshotable.cc @@ -1504,6 +1504,11 @@ StartupData SerializeNodeContextInternalFields(Local holder, return StartupData{data, size}; } + // Do not serialize fields unknown to BaseObject. + if (index >= BaseObject::kInternalFieldCount) { + return StartupData{nullptr, 0}; + } + // To serialize the slot field, invoke Serialize() method on the object. DCHECK_IS_SNAPSHOT_SLOT(index); diff --git a/test/parallel/test-async-local-storage-weak-asyncwrap-leak.js b/test/parallel/test-async-local-storage-weak-asyncwrap-leak.js new file mode 100644 index 00000000000000..baa13f610ed9aa --- /dev/null +++ b/test/parallel/test-async-local-storage-weak-asyncwrap-leak.js @@ -0,0 +1,49 @@ +// Flags: --expose-gc +'use strict'; +const common = require('../common'); +const assert = require('node:assert'); +const zlib = require('node:zlib'); +const v8 = require('node:v8'); +const { AsyncLocalStorage } = require('node:async_hooks'); + +// This test verifies that referencing an AsyncLocalStorage store from +// a weak AsyncWrap does not prevent the store from being garbage collected. +// We use zlib streams as examples of weak AsyncWraps here, but the +// issue is not specific to zlib. + +class Store {} + +let zlibDone = false; + +// Use immediates to ensure no accidental async context propagation occurs +setImmediate(common.mustCall(() => { + const asyncLocalStorage = new AsyncLocalStorage(); + const store = new Store(); + asyncLocalStorage.run(store, common.mustCall(() => { + (async () => { + const zlibStream = zlib.createGzip(); + // (Make sure this test does not break if _handle is renamed + // to something else) + assert.strictEqual(typeof zlibStream._handle, 'object'); + // Create backreference from AsyncWrap to store + store.zlibStream = zlibStream._handle; + // Let the zlib stream finish (not strictly necessary) + zlibStream.end('hello world'); + await zlibStream.toArray(); + zlibDone = true; + })().then(common.mustCall()); + })); +})); + +const finish = common.mustCall(() => { + global.gc(); + // Make sure the ALS instance has been garbage-collected + assert.strictEqual(v8.queryObjects(Store), 0); +}); + +const interval = setInterval(() => { + if (zlibDone) { + clearInterval(interval); + finish(); + } +}, 5);