src: track async resources via pointers to stack-allocated handles

src/api/callback.cc

@@ -47,17 +47,26 @@ InternalCallbackScope::InternalCallbackScope(AsyncWrap* async_wrap, int flags)
  flags,
  async_wrap->context_frame()){}
 
-InternalCallbackScope::InternalCallbackScope(Environment* env,
- Local<Object> object,
- const async_context& asyncContext,
- int flags,
- Local<Value> context_frame)
+InternalCallbackScope::InternalCallbackScope(
+ Environment* env,
+ std::variant<Local<Object>, Local<Object>*> object,
+ const async_context& asyncContext,
+ int flags,
+ Local<Value> context_frame)
  : env_(env),
  async_context_(asyncContext),
- object_(object),
  skip_hooks_(flags & kSkipAsyncHooks),
  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_;
+ } else{
+ object_ = std::get<Local<Object>*>(object);
+ CHECK_NOT_NULL(object_);
+ }
+
  env->PushAsyncCallbackScope();
 
  if (!env->can_call_into_js()){
@@ -84,7 +93,7 @@ InternalCallbackScope::InternalCallbackScope(Environment* env,
  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;
 

src/async_wrap.cc

@@ -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, {});
+ env->async_hooks()->push_async_context(async_id, trigger_async_id, nullptr);
 }
 
 

src/env-inl.h

@@ -106,8 +106,10 @@ 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()) return{};
- return native_execution_async_resources_[i];
+ if (i >= native_execution_async_resources_.size() ||
+ native_execution_async_resources_[i] == nullptr)
+ return{};
+ return *native_execution_async_resources_[i];
 }
 
 inline v8::Local<v8::String> AsyncHooks::provider_string(int idx){

src/env.cc

@@ -122,7 +122,8 @@ 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){
+ Local<Object>* resource){
+ CHECK_IMPLIES(resource != nullptr, !resource->IsEmpty());
  // Since async_hooks is experimental, do only perform the check
  // when async_hooks is enabled.
  if (fields_[kCheck] > 0){
@@ -140,14 +141,14 @@ void AsyncHooks::push_async_context(double async_id,
 
 #ifdef DEBUG
  for (uint32_t i = offset; i < native_execution_async_resources_.size(); i++)
-CHECK(native_execution_async_resources_[i].IsEmpty());
+CHECK_NULL(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.IsEmpty()){
+ if (resource != nullptr){
  native_execution_async_resources_.resize(offset + 1);
- // Caveat: This is a v8::Local<> assignment, we do not keep a v8::Global<>!
+ // Caveat: This is a v8::Local<>* assignment, we do not keep a v8::Global<>!
  native_execution_async_resources_[offset] = resource;
  }
 }
@@ -172,11 +173,11 @@ bool AsyncHooks::pop_async_context(double async_id){
  fields_[kStackLength] = offset;
 
  if (offset < native_execution_async_resources_.size() &&
-!native_execution_async_resources_[offset].IsEmpty()) [[likely]]{
+ native_execution_async_resources_[offset] != nullptr) [[likely]]{
 #ifdef DEBUG
  for (uint32_t i = offset + 1; i < native_execution_async_resources_.size();
  i++){
-CHECK(native_execution_async_resources_[i].IsEmpty());
+CHECK_NULL(native_execution_async_resources_[i]);
  }
 #endif
  native_execution_async_resources_.resize(offset);
@@ -1721,7 +1722,6 @@ AsyncHooks::AsyncHooks(Isolate* isolate, const SerializeInfo* info)
  fields_(isolate, kFieldsCount, MAYBE_FIELD_PTR(info, fields)),
  async_id_fields_(
  isolate, kUidFieldsCount, MAYBE_FIELD_PTR(info, async_id_fields)),
- native_execution_async_resources_(isolate),
  info_(info){
  HandleScope handle_scope(isolate);
  if (info == nullptr){
@@ -1810,10 +1810,9 @@ AsyncHooks::SerializeInfo AsyncHooks::Serialize(Local<Context> context,
  native_execution_async_resources_.size());
  for (size_t i = 0; i < native_execution_async_resources_.size(); i++){
  info.native_execution_async_resources[i] =
- native_execution_async_resources_[i].IsEmpty() ? SIZE_MAX :
- creator->AddData(
- context,
- native_execution_async_resources_[i]);
+ native_execution_async_resources_[i] == nullptr
+ ? SIZE_MAX
+ : creator->AddData(context, *native_execution_async_resources_[i]);
  }
 
  // At the moment, promise hooks are not supported in the startup snapshot.

src/env.h

@@ -59,6 +59,7 @@
 #include <array>
 #include <atomic>
 #include <cstdint>
+#include <deque>
 #include <functional>
 #include <list>
 #include <memory>
@@ -324,7 +325,7 @@ class AsyncHooks : public MemoryRetainer{
  // `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);
+ v8::Local<v8::Object>* execution_async_resource);
  bool pop_async_context(double async_id);
  void clear_async_id_stack(); // Used in fatal exceptions.
 
@@ -386,15 +387,9 @@ class AsyncHooks : public MemoryRetainer{
 
  v8::Global<v8::Array> js_execution_async_resources_;
 
- // TODO(@jasnell): Note that this is technically illegal use of
- // v8::Locals which should be kept on the stack. Here, the entries
- // in this object grows and shrinks with the C stack, and entries
- // will be in the right handle scopes, but v8::Locals are supposed
- // to remain on the stack and not the heap. For general purposes
- // this *should* be ok but may need to be looked at further should
- // v8 become stricter in the future about v8::Locals being held in
- // the stack.
- v8::LocalVector<v8::Object> native_execution_async_resources_;
+ // 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_;
 
  // Non-empty during deserialization
  const SerializeInfo* info_ = nullptr;

src/node_internals.h

@@ -37,6 +37,7 @@
 #include <cstdlib>
 
 #include <string>
+#include <variant>
 #include <vector>
 
 struct sockaddr;
@@ -245,9 +246,14 @@ class InternalCallbackScope{
  // compatibility issues, but it shouldn't.)
  kSkipTaskQueues = 2
  };
+ // You need to either guarantee that this `InternalCallbackScope` is
+ // 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).
  InternalCallbackScope(
  Environment* env,
- v8::Local<v8::Object> object,
+std::variant<v8::Local<v8::Object>, v8::Local<v8::Object>*> object,
  const async_context& asyncContext,
  int flags = kNoFlags,
  v8::Local<v8::Value> context_frame = v8::Local<v8::Value>());
@@ -263,7 +269,8 @@ class InternalCallbackScope{
  private:
  Environment* env_;
  async_context async_context_;
- v8::Local<v8::Object> object_;
+ v8::Local<v8::Object> object_storage_;
+ v8::Local<v8::Object>* object_;
  bool skip_hooks_;
  bool skip_task_queues_;
  bool failed_ = false;

src/node_task_queue.cc

@@ -100,10 +100,11 @@ void PromiseRejectCallback(PromiseRejectMessage message){
 if (!GetAssignedPromiseAsyncId(env, promise, env->trigger_async_id_symbol())
  .To(&trigger_async_id)) return;
 
+ Local<Object> promise_as_obj = promise;
 if (async_id != AsyncWrap::kInvalidAsyncId &&
  trigger_async_id != AsyncWrap::kInvalidAsyncId){
  env->async_hooks()->push_async_context(
- async_id, trigger_async_id, promise);
+ async_id, trigger_async_id, &promise_as_obj);
  }
 
 USE(callback->Call(