src: allow generic C++ callables in SetImmediate()

Modify the native `SetImmediate()` functions to take generic C++
callables as arguments. This makes passing arguments to the callback
easier, and in particular, it allows passing `std::unique_ptr`s
directly, which in turn makes sure that the data they point to is
deleted if the `Environment` is torn down before the callback can run.

Author: addaleax

src/async_wrap.cc

@@ -87,7 +87,7 @@ struct AsyncWrapObject : public AsyncWrap {
   SET_SELF_SIZE(AsyncWrapObject)
 };
 
-void AsyncWrap::DestroyAsyncIdsCallback(Environment* env, void* data) {
+void AsyncWrap::DestroyAsyncIdsCallback(Environment* env) {
   Local<Function> fn = env->async_hooks_destroy_function();
 
   TryCatchScope try_catch(env, TryCatchScope::CatchMode::kFatal);
@@ -642,7 +642,7 @@ void AsyncWrap::EmitDestroy(Environment* env, double async_id) {
   }
 
   if (env->destroy_async_id_list()->empty()) {
-    env->SetUnrefImmediate(DestroyAsyncIdsCallback, nullptr);
+    env->SetUnrefImmediate(&DestroyAsyncIdsCallback);
   }
 
   env->destroy_async_id_list()->push_back(async_id);

src/async_wrap.h

@@ -154,7 +154,7 @@ class AsyncWrap : public BaseObject {
   static void EmitTraceEventAfter(ProviderType type, double async_id);
   void EmitTraceEventDestroy();
 
-  static void DestroyAsyncIdsCallback(Environment* env, void* data);
+  static void DestroyAsyncIdsCallback(Environment* env);
 
   inline ProviderType provider_type() const;
   inline ProviderType set_provider_type(ProviderType provider);

src/cares_wrap.cc

@@ -690,9 +690,9 @@ class QueryWrap : public AsyncWrap {
   }
 
   void QueueResponseCallback(int status) {
-    env()->SetImmediate([](Environment*, void* data) {
-      static_cast<QueryWrap*>(data)->AfterResponse();
-    }, this, object());
+    env()->SetImmediate([this](Environment*) {
+      AfterResponse();
+    }, object());
 
     channel_->set_query_last_ok(status != ARES_ECONNREFUSED);
     channel_->ModifyActivityQueryCount(-1);

src/env-inl.h

@@ -743,33 +743,66 @@ inline void IsolateData::set_options(
   options_ = std::move(options);
 }
 
-void Environment::CreateImmediate(native_immediate_callback cb,
-                                  void* data,
-                                  v8::Local<v8::Object> obj,
+template <typename Fn>
+void Environment::CreateImmediate(Fn&& cb,
+                                  v8::Local<v8::Object> keep_alive,
                                   bool ref) {
-  native_immediate_callbacks_.push_back({
-    cb,
-    data,
-    v8::Global<v8::Object>(isolate_, obj),
-    ref
-  });
+  auto callback = std::make_unique<NativeImmediateCallbackImpl<Fn>>(
+      std::move(cb),
+      v8::Global<v8::Object>(isolate(), keep_alive),
+      ref);
+  NativeImmediateCallback* prev_tail = native_immediate_callbacks_tail_;
+
+  native_immediate_callbacks_tail_ = callback.get();
+  if (prev_tail != nullptr)
+    prev_tail->set_next(std::move(callback));
+  else
+    native_immediate_callbacks_head_ = std::move(callback);
+
   immediate_info()->count_inc(1);
 }
 
-void Environment::SetImmediate(native_immediate_callback cb,
-                               void* data,
-                               v8::Local<v8::Object> obj) {
-  CreateImmediate(cb, data, obj, true);
+template <typename Fn>
+void Environment::SetImmediate(Fn&& cb, v8::Local<v8::Object> keep_alive) {
+  CreateImmediate(std::move(cb), keep_alive, true);
 
   if (immediate_info()->ref_count() == 0)
     ToggleImmediateRef(true);
   immediate_info()->ref_count_inc(1);
 }
 
-void Environment::SetUnrefImmediate(native_immediate_callback cb,
-                                    void* data,
-                                    v8::Local<v8::Object> obj) {
-  CreateImmediate(cb, data, obj, false);
+template <typename Fn>
+void Environment::SetUnrefImmediate(Fn&& cb, v8::Local<v8::Object> keep_alive) {
+  CreateImmediate(std::move(cb), keep_alive, false);
+}
+
+Environment::NativeImmediateCallback::NativeImmediateCallback(bool refed)
+  : refed_(refed) {}
+
+bool Environment::NativeImmediateCallback::is_refed() const {
+  return refed_;
+}
+
+std::unique_ptr<Environment::NativeImmediateCallback>
+Environment::NativeImmediateCallback::get_next() {
+  return std::move(next_);
+}
+
+void Environment::NativeImmediateCallback::set_next(
+    std::unique_ptr<NativeImmediateCallback> next) {
+  next_ = std::move(next);
+}
+
+template <typename Fn>
+Environment::NativeImmediateCallbackImpl<Fn>::NativeImmediateCallbackImpl(
+    Fn&& callback, v8::Global<v8::Object>&& keep_alive, bool refed)
+  : NativeImmediateCallback(refed),
+    callback_(std::move(callback)),
+    keep_alive_(std::move(keep_alive)) {}
+
+template <typename Fn>
+void Environment::NativeImmediateCallbackImpl<Fn>::Call(Environment* env) {
+  callback_(env);
 }
 
 inline bool Environment::can_call_into_js() const {

src/env.cc

@@ -339,7 +339,7 @@ Environment::Environment(IsolateData* isolate_data,
       [](void* arg) {
         Environment* env = static_cast<Environment*>(arg);
         if (!env->destroy_async_id_list()->empty())
-          AsyncWrap::DestroyAsyncIdsCallback(env, nullptr);
+          AsyncWrap::DestroyAsyncIdsCallback(env);
       },
       this);
 
@@ -641,42 +641,38 @@ void Environment::AtExit(void (*cb)(void* arg), void* arg) {
 void Environment::RunAndClearNativeImmediates() {
   TraceEventScope trace_scope(TRACING_CATEGORY_NODE1(environment),
                               "RunAndClearNativeImmediates", this);
-  size_t count = native_immediate_callbacks_.size();
-  if (count > 0) {
-    size_t ref_count = 0;
-    std::vector<NativeImmediateCallback> list;
-    native_immediate_callbacks_.swap(list);
-    auto drain_list = [&]() {
-      TryCatchScope try_catch(this);
-      for (auto it = list.begin(); it != list.end(); ++it) {
-        DebugSealHandleScope seal_handle_scope(isolate());
-        it->cb_(this, it->data_);
-        if (it->refed_)
-          ref_count++;
-        if (UNLIKELY(try_catch.HasCaught())) {
-          if (!try_catch.HasTerminated())
-            errors::TriggerUncaughtException(isolate(), try_catch);
-
-          // We are done with the current callback. Increase the counter so that
-          // the steps below make everything *after* the current item part of
-          // the new list.
-          it++;
-
-          // Bail out, remove the already executed callbacks from list
-          // and set up a new TryCatch for the other pending callbacks.
-          std::move_backward(it, list.end(), list.begin() + (list.end() - it));
-          list.resize(list.end() - it);
-          return true;
-        }
+  size_t ref_count = 0;
+  size_t count = 0;
+  std::unique_ptr<NativeImmediateCallback> head;
+  head.swap(native_immediate_callbacks_head_);
+  native_immediate_callbacks_tail_ = nullptr;
+
+  auto drain_list = [&]() {
+    TryCatchScope try_catch(this);
+    for (; head; head = head->get_next()) {
+      DebugSealHandleScope seal_handle_scope(isolate());
+      count++;
+      if (head->is_refed())
+        ref_count++;
+
+      head->Call(this);
+      if (UNLIKELY(try_catch.HasCaught())) {
+        if (!try_catch.HasTerminated())
+          errors::TriggerUncaughtException(isolate(), try_catch);
+
+        // We are done with the current callback. Move one iteration along,
+        // as if we had completed successfully.
+        head = head->get_next();
+        return true;
       }
-      return false;
-    };
-    while (drain_list()) {}
+    }
+    return false;
+  };
+  while (head && drain_list()) {}
 
-    DCHECK_GE(immediate_info()->count(), count);
-    immediate_info()->count_dec(count);
-    immediate_info()->ref_count_dec(ref_count);
-  }
+  DCHECK_GE(immediate_info()->count(), count);
+  immediate_info()->count_dec(count);
+  immediate_info()->ref_count_dec(ref_count);
 }
 
 

src/env.h

@@ -1172,15 +1172,15 @@ class Environment : public MemoryRetainer {
     return current_value;
   }
 
-  typedef void (*native_immediate_callback)(Environment* env, void* data);
-  // cb will be called as cb(env, data) on the next event loop iteration.
-  // obj will be kept alive between now and after the callback has run.
-  inline void SetImmediate(native_immediate_callback cb,
-                           void* data,
-                           v8::Local<v8::Object> obj = v8::Local<v8::Object>());
-  inline void SetUnrefImmediate(native_immediate_callback cb,
-                                void* data,
-                                v8::Local<v8::Object> obj =
+  // cb will be called as cb(env) on the next event loop iteration.
+  // keep_alive will be kept alive between now and after the callback has run.
+  template <typename Fn>
+  inline void SetImmediate(Fn&& cb,
+                           v8::Local<v8::Object> keep_alive =
+                               v8::Local<v8::Object>());
+  template <typename Fn>
+  inline void SetUnrefImmediate(Fn&& cb,
+                                v8::Local<v8::Object> keep_alive =
                                     v8::Local<v8::Object>());
   // This needs to be available for the JS-land setImmediate().
   void ToggleImmediateRef(bool ref);
@@ -1245,9 +1245,9 @@ class Environment : public MemoryRetainer {
 #endif  // HAVE_INSPECTOR
 
  private:
-  inline void CreateImmediate(native_immediate_callback cb,
-                              void* data,
-                              v8::Local<v8::Object> obj,
+  template <typename Fn>
+  inline void CreateImmediate(Fn&& cb,
+                              v8::Local<v8::Object> keep_alive,
                               bool ref);
 
   inline void ThrowError(v8::Local<v8::Value> (*fun)(v8::Local<v8::String>),
@@ -1370,13 +1370,38 @@ class Environment : public MemoryRetainer {
 
   std::list<ExitCallback> at_exit_functions_;
 
-  struct NativeImmediateCallback {
-    native_immediate_callback cb_;
-    void* data_;
-    v8::Global<v8::Object> keep_alive_;
+  class NativeImmediateCallback {
+   public:
+    explicit inline NativeImmediateCallback(bool refed);
+
+    virtual ~NativeImmediateCallback() = default;
+    virtual void Call(Environment* env) = 0;
+
+    inline bool is_refed() const;
+    inline std::unique_ptr<NativeImmediateCallback> get_next();
+    inline void set_next(std::unique_ptr<NativeImmediateCallback> next);
+
+   private:
     bool refed_;
+    std::unique_ptr<NativeImmediateCallback> next_;
+  };
+
+  template <typename Fn>
+  class NativeImmediateCallbackImpl final : public NativeImmediateCallback {
+   public:
+    NativeImmediateCallbackImpl(Fn&& callback,
+                                v8::Global<v8::Object>&& keep_alive,
+                                bool refed);
+    void Call(Environment* env) override;
+
+   private:
+    Fn callback_;
+    v8::Global<v8::Object> keep_alive_;
   };
-  std::vector<NativeImmediateCallback> native_immediate_callbacks_;
+
+  std::unique_ptr<NativeImmediateCallback> native_immediate_callbacks_head_;
+  NativeImmediateCallback* native_immediate_callbacks_tail_ = nullptr;
+
   void RunAndClearNativeImmediates();
   static void CheckImmediate(uv_check_t* handle);
 

src/node_api.cc

@@ -36,27 +36,33 @@ class BufferFinalizer : private Finalizer {
  public:
   // node::Buffer::FreeCallback
   static void FinalizeBufferCallback(char* data, void* hint) {
-    BufferFinalizer* finalizer = static_cast<BufferFinalizer*>(hint);
+    std::unique_ptr<BufferFinalizer, Deleter> finalizer{
+        static_cast<BufferFinalizer*>(hint)};
     finalizer->_finalize_data = data;
-    static_cast<node_napi_env>(finalizer->_env)->node_env()
-        ->SetImmediate([](node::Environment* env, void* hint) {
-      BufferFinalizer* finalizer = static_cast<BufferFinalizer*>(hint);
-
-      if (finalizer->_finalize_callback != nullptr) {
-        v8::HandleScope handle_scope(finalizer->_env->isolate);
-        v8::Context::Scope context_scope(finalizer->_env->context());
-
-        finalizer->_env->CallIntoModuleThrow([&](napi_env env) {
-          finalizer->_finalize_callback(
-              env,
-              finalizer->_finalize_data,
-              finalizer->_finalize_hint);
-        });
-      }
 
-      Delete(finalizer);
-    }, hint);
+    node::Environment* node_env =
+        static_cast<node_napi_env>(finalizer->_env)->node_env();
+    node_env->SetImmediate(
+        [finalizer = std::move(finalizer)](node::Environment* env) {
+      if (finalizer->_finalize_callback == nullptr) return;
+
+      v8::HandleScope handle_scope(finalizer->_env->isolate);
+      v8::Context::Scope context_scope(finalizer->_env->context());
+
+      finalizer->_env->CallIntoModuleThrow([&](napi_env env) {
+        finalizer->_finalize_callback(
+            env,
+            finalizer->_finalize_data,
+            finalizer->_finalize_hint);
+      });
+    });
   }
+
+  struct Deleter {
+    void operator()(BufferFinalizer* finalizer) {
+      Finalizer::Delete(finalizer);
+    }
+  };
 };
 
 static inline napi_env NewEnv(v8::Local<v8::Context> context) {

src/node_file.cc

@@ -170,35 +170,33 @@ inline void FileHandle::Close() {
 
   struct err_detail { int ret; int fd; };
 
-  err_detail* detail = new err_detail { ret, fd_ };
+  err_detail detail { ret, fd_ };
 
   if (ret < 0) {
     // Do not unref this
-    env()->SetImmediate([](Environment* env, void* data) {
+    env()->SetImmediate([detail](Environment* env) {
       char msg[70];
-      std::unique_ptr<err_detail> detail(static_cast<err_detail*>(data));
       snprintf(msg, arraysize(msg),
               "Closing file descriptor %d on garbage collection failed",
-              detail->fd);
+              detail.fd);
       // This exception will end up being fatal for the process because
       // it is being thrown from within the SetImmediate handler and
       // there is no JS stack to bubble it to. In other words, tearing
       // down the process is the only reasonable thing we can do here.
       HandleScope handle_scope(env->isolate());
-      env->ThrowUVException(detail->ret, "close", msg);
-    }, detail);
+      env->ThrowUVException(detail.ret, "close", msg);
+    });
     return;
   }
 
   // If the close was successful, we still want to emit a process warning
   // to notify that the file descriptor was gc'd. We want to be noisy about
   // this because not explicitly closing the FileHandle is a bug.
-  env()->SetUnrefImmediate([](Environment* env, void* data) {
-    std::unique_ptr<err_detail> detail(static_cast<err_detail*>(data));
+  env()->SetUnrefImmediate([detail](Environment* env) {
     ProcessEmitWarning(env,
                        "Closing file descriptor %d on garbage collection",
-                       detail->fd);
-  }, detail);
+                       detail.fd);
+  });
 }
 
 void FileHandle::CloseReq::Resolve() {