Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
Next Next commit
src: use Global for storing resource in Node-API callback scope
This is a follow-up to 234c26c. The Node-API interface does
not allow us to enforce that values are stored in a specific location,
e.g. on the stack or not; however, V8 requires `Local<>` handles
to be stored on the stack.

To circumvent this restriction, we add the ability to the async handle
stack to store either `Local<>*` pointers or `Global<>*` pointers, with
Node-API making use of the latter.
  • Loading branch information
addaleax committed Sep 17, 2025
commit 66ef1041c3876fc94025fb231021b384c1ec0b70
41 changes: 31 additions & 10 deletions src/api/callback.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ namespace node {

using v8::Context;
using v8::Function;
using v8::Global;
using v8::HandleScope;
using v8::Isolate;
using v8::Local;
Expand All @@ -26,13 +27,30 @@ CallbackScope::CallbackScope(Isolate* isolate,
CallbackScope::CallbackScope(Environment* env,
Local<Object> object,
async_context asyncContext)
: resource_storage_(object),
private_(
new InternalCallbackScope(env, &resource_storage_, asyncContext)),
: resource_storage_({.local = object}),
private_(new InternalCallbackScope(
env, &resource_storage_.local, asyncContext)),
try_catch_(env->isolate()) {
try_catch_.SetVerbose(true);
}

CallbackScope::CallbackScope(Environment* env,
Global<Object>* object,
async_context asyncContext)
: resource_storage_({.global_ptr = object}),
private_(new InternalCallbackScope(
env, resource_storage_.global_ptr, asyncContext)),
try_catch_(env->isolate()) {
try_catch_.SetVerbose(true);
// These checks can be removed in a future major version -- they ensure
// ABI compatibility with previous Node.js versions.
static_assert(sizeof(resource_storage_) == sizeof(Global<Object>*));
static_assert(sizeof(resource_storage_) == sizeof(Local<Object>));
static_assert(alignof(decltype(resource_storage_)) ==
alignof(Global<Object>*));
static_assert(alignof(decltype(resource_storage_)) == alignof(Local<Object>));
}

CallbackScope::~CallbackScope() {
if (try_catch_.HasCaught())
private_->MarkAsFailed();
Expand All @@ -49,7 +67,7 @@ InternalCallbackScope::InternalCallbackScope(AsyncWrap* async_wrap, int flags)

InternalCallbackScope::InternalCallbackScope(
Environment* env,
std::variant<Local<Object>, Local<Object>*> object,
std::variant<Local<Object>, Local<Object>*, Global<Object>*> object_arg,
const async_context& asyncContext,
int flags,
Local<Value> context_frame)
Expand All @@ -59,13 +77,16 @@ InternalCallbackScope::InternalCallbackScope(
skip_task_queues_(flags & kSkipTaskQueues) {
CHECK_NOT_NULL(env);

if (std::holds_alternative<Local<Object>>(object)) {
object_storage_ = std::get<Local<Object>>(object);
object_ = &object_storage_;
std::variant<v8::Local<v8::Object>*, v8::Global<v8::Object>*> object;
if (std::holds_alternative<Local<Object>>(object_arg)) {
object_storage_ = std::get<Local<Object>>(object_arg);
object = &object_storage_;
} else if (std::holds_alternative<Local<Object>*>(object_arg)) {
object = std::get<Local<Object>*>(object_arg);
} else {
object_ = std::get<Local<Object>*>(object);
CHECK_NOT_NULL(object_);
object = std::get<Global<Object>*>(object_arg);
}
std::visit([](auto* ptr) { CHECK_NOT_NULL(ptr); }, object);

env->PushAsyncCallbackScope();

Expand Down Expand Up @@ -93,7 +114,7 @@ InternalCallbackScope::InternalCallbackScope(
isolate, async_context_frame::exchange(isolate, context_frame));

env->async_hooks()->push_async_context(
async_context_.async_id, async_context_.trigger_async_id, object_);
async_context_.async_id, async_context_.trigger_async_id, object);

pushed_ids_ = true;

Expand Down
2 changes: 1 addition & 1 deletion src/async_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ void AsyncWrap::PushAsyncContext(const FunctionCallbackInfo<Value>& args) {
// then the checks in push_async_ids() and pop_async_id() will.
double async_id = args[0]->NumberValue(env->context()).FromJust();
double trigger_async_id = args[1]->NumberValue(env->context()).FromJust();
env->async_hooks()->push_async_context(async_id, trigger_async_id, nullptr);
env->async_hooks()->push_async_context(async_id, trigger_async_id, {});
}


Expand Down
17 changes: 13 additions & 4 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -106,10 +106,19 @@ v8::Local<v8::Array> AsyncHooks::js_execution_async_resources() {
}

v8::Local<v8::Object> AsyncHooks::native_execution_async_resource(size_t i) {
if (i >= native_execution_async_resources_.size() ||
native_execution_async_resources_[i] == nullptr)
return {};
return *native_execution_async_resources_[i];
if (i >= native_execution_async_resources_.size()) return {};
auto resource = native_execution_async_resources_[i];
if (std::holds_alternative<v8::Global<v8::Object>*>(resource)) [[unlikely]] {
auto* global = std::get<v8::Global<v8::Object>*>(resource);
if (global == nullptr) [[unlikely]]
return {};
return global->Get(env()->isolate());
} else {
auto* local = std::get<v8::Local<v8::Object>*>(resource);
if (local == nullptr) [[unlikely]]
return {};
return *local;
}
}

inline v8::Local<v8::String> AsyncHooks::provider_string(int idx) {
Expand Down
27 changes: 16 additions & 11 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ using v8::EmbedderGraph;
using v8::EscapableHandleScope;
using v8::ExternalMemoryAccounter;
using v8::Function;
using v8::Global;
using v8::HandleScope;
using v8::HeapProfiler;
using v8::HeapSpaceStatistics;
Expand Down Expand Up @@ -120,10 +121,12 @@ void Environment::ResetPromiseHooks(Local<Function> init,
}

// Remember to keep this code aligned with pushAsyncContext() in JS.
void AsyncHooks::push_async_context(double async_id,
double trigger_async_id,
Local<Object>* resource) {
CHECK_IMPLIES(resource != nullptr, !resource->IsEmpty());
void AsyncHooks::push_async_context(
double async_id,
double trigger_async_id,
std::variant<Local<Object>*, Global<Object>*> resource) {
std::visit([](auto* ptr) { CHECK_IMPLIES(ptr != nullptr, !ptr->IsEmpty()); },
resource);
// Since async_hooks is experimental, do only perform the check
// when async_hooks is enabled.
if (fields_[kCheck] > 0) {
Expand All @@ -141,12 +144,13 @@ void AsyncHooks::push_async_context(double async_id,

#ifdef DEBUG
for (uint32_t i = offset; i < native_execution_async_resources_.size(); i++)
CHECK_NULL(native_execution_async_resources_[i]);
std::visit([](auto* ptr) { CHECK_NULL(ptr); },
native_execution_async_resources_[i]);
#endif

// When this call comes from JS (as a way of increasing the stack size),
// `resource` will be empty, because JS caches these values anyway.
if (resource != nullptr) {
if (std::visit([](auto* ptr) { return ptr != nullptr; }, resource)) {
native_execution_async_resources_.resize(offset + 1);
// Caveat: This is a v8::Local<>* assignment, we do not keep a v8::Global<>!
native_execution_async_resources_[offset] = resource;
Expand All @@ -173,11 +177,13 @@ bool AsyncHooks::pop_async_context(double async_id) {
fields_[kStackLength] = offset;

if (offset < native_execution_async_resources_.size() &&
native_execution_async_resources_[offset] != nullptr) [[likely]] {
std::visit([](auto* ptr) { return ptr != nullptr; },
native_execution_async_resources_[offset])) [[likely]] {
#ifdef DEBUG
for (uint32_t i = offset + 1; i < native_execution_async_resources_.size();
i++) {
CHECK_NULL(native_execution_async_resources_[i]);
std::visit([](auto* ptr) { CHECK_NULL(ptr); },
native_execution_async_resources_[i]);
}
#endif
native_execution_async_resources_.resize(offset);
Expand Down Expand Up @@ -1821,10 +1827,9 @@ AsyncHooks::SerializeInfo AsyncHooks::Serialize(Local<Context> context,
info.native_execution_async_resources.resize(
native_execution_async_resources_.size());
for (size_t i = 0; i < native_execution_async_resources_.size(); i++) {
auto resource = native_execution_async_resource(i);
info.native_execution_async_resources[i] =
native_execution_async_resources_[i] == nullptr
? SIZE_MAX
: creator->AddData(context, *native_execution_async_resources_[i]);
resource.IsEmpty() ? SIZE_MAX : creator->AddData(context, resource);
}

// At the moment, promise hooks are not supported in the startup snapshot.
Expand Down
16 changes: 12 additions & 4 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@
#include <string>
#include <unordered_map>
#include <unordered_set>
#include <variant>
#include <vector>

namespace node {
Expand Down Expand Up @@ -323,9 +324,11 @@ class AsyncHooks : public MemoryRetainer {
// NB: This call does not take (co-)ownership of `execution_async_resource`.
// The lifetime of the `v8::Local<>` pointee must last until
// `pop_async_context()` or `clear_async_id_stack()` are called.
void push_async_context(double async_id,
double trigger_async_id,
v8::Local<v8::Object>* execution_async_resource);
void push_async_context(
double async_id,
double trigger_async_id,
std::variant<v8::Local<v8::Object>*, v8::Global<v8::Object>*>
execution_async_resource);
bool pop_async_context(double async_id);
void clear_async_id_stack(); // Used in fatal exceptions.

Expand Down Expand Up @@ -389,7 +392,12 @@ class AsyncHooks : public MemoryRetainer {

// We avoid storing the handles directly here, because they are already
// properly allocated on the stack, we just need access to them here.
std::deque<v8::Local<v8::Object>*> native_execution_async_resources_;
// The `v8::Global<>` variant is here because the Node-API API design
// does not allow us to make sure that we exclusively store this value
// on the stack, so we accept the small perf hit that comes with
// global handles in that case.
std::deque<std::variant<v8::Local<v8::Object>*, v8::Global<v8::Object>*>>
native_execution_async_resources_;

// Non-empty during deserialization
const SerializeInfo* info_ = nullptr;
Expand Down
13 changes: 11 additions & 2 deletions src/node.h
Original file line number Diff line number Diff line change
Expand Up @@ -1444,6 +1444,12 @@ class NODE_EXTERN CallbackScope {
CallbackScope(Environment* env,
v8::Local<v8::Object> resource,
async_context asyncContext);
// `resource` needs to outlive the scope in this case.
// This is for the rare situation in which `CallbackScope` cannot be
// stack-allocated. `resource` needs to outlive this scope.
CallbackScope(Environment* env,
v8::Global<v8::Object>* resource,
async_context asyncContext);
~CallbackScope();

void operator=(const CallbackScope&) = delete;
Expand All @@ -1452,8 +1458,11 @@ class NODE_EXTERN CallbackScope {
CallbackScope(CallbackScope&&) = delete;

private:
void* reserved_;
v8::Local<v8::Object> resource_storage_;
void* resource_storage_global_;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is this field for? I'm getting a -Wunused-private-field warning with Clang 21.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I opened #60802 in case it's safe to remove.

union {
v8::Local<v8::Object> local;
v8::Global<v8::Object>* global_ptr;
} resource_storage_;
InternalCallbackScope* private_;
v8::TryCatch try_catch_;
};
Expand Down
33 changes: 22 additions & 11 deletions src/node_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -584,10 +584,9 @@ class AsyncContext {

inline napi_callback_scope OpenCallbackScope() {
EnsureReference();
napi_callback_scope it =
reinterpret_cast<napi_callback_scope>(new CallbackScope(this));
auto scope = new HeapAllocatedCallbackScope(this);
env_->open_callback_scopes++;
return it;
return scope->to_opaque();
}

inline void EnsureReference() {
Expand All @@ -609,8 +608,7 @@ class AsyncContext {

static inline void CloseCallbackScope(node_napi_env env,
napi_callback_scope s) {
CallbackScope* callback_scope = reinterpret_cast<CallbackScope*>(s);
delete callback_scope;
delete HeapAllocatedCallbackScope::FromOpaque(s);
env->open_callback_scopes--;
}

Expand All @@ -621,13 +619,26 @@ class AsyncContext {
}

private:
class CallbackScope : public node::CallbackScope {
class HeapAllocatedCallbackScope final {
public:
explicit CallbackScope(AsyncContext* async_context)
: node::CallbackScope(async_context->node_env(),
async_context->resource_.Get(
async_context->node_env()->isolate()),
async_context->async_context()) {}
napi_callback_scope to_opaque() {
return reinterpret_cast<napi_callback_scope>(this);
}
static HeapAllocatedCallbackScope* FromOpaque(napi_callback_scope s) {
return reinterpret_cast<HeapAllocatedCallbackScope*>(s);
}

explicit HeapAllocatedCallbackScope(AsyncContext* async_context)
: resource_storage_(async_context->node_env()->isolate(),
async_context->resource_.Get(
async_context->node_env()->isolate())),
cs_(async_context->node_env(),
&resource_storage_,
async_context->async_context()) {}

private:
v8::Global<v8::Object> resource_storage_;
node::CallbackScope cs_;
};

node_napi_env env_;
Expand Down
7 changes: 4 additions & 3 deletions src/node_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -250,10 +250,12 @@ class InternalCallbackScope {
// stack-allocated itself, OR that `object` is a pointer to a stack-allocated
// `v8::Local<v8::Object>` which outlives this scope (e.g. for the
// public `CallbackScope` which indirectly allocates an instance of
// this class for ABI stability purposes).
// this class for ABI stability purposes), OR pass a `Global<>`.
InternalCallbackScope(
Environment* env,
std::variant<v8::Local<v8::Object>, v8::Local<v8::Object>*> object,
std::variant<v8::Local<v8::Object>,
v8::Local<v8::Object>*,
v8::Global<v8::Object>*> object,
const async_context& asyncContext,
int flags = kNoFlags,
v8::Local<v8::Value> context_frame = v8::Local<v8::Value>());
Expand All @@ -270,7 +272,6 @@ class InternalCallbackScope {
Environment* env_;
async_context async_context_;
v8::Local<v8::Object> object_storage_;
v8::Local<v8::Object>* object_;
bool skip_hooks_;
bool skip_task_queues_;
bool failed_ = false;
Expand Down