src: use enum for refed flag on native immediates

Refs: nodejs#33320 (comment) PR-URL: nodejs#33444 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Zeyu Yang <himself65@outlook.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Trott · May 22, 2020 · b6b82cb · b6b82cb
1 parent 6f6bf01
commit b6b82cb
Show file tree

Hide file tree

Showing 11 changed files with 52 additions and 54 deletions.
diff --git a/src/async_wrap.cc b/src/async_wrap.cc
@@ -753,7 +753,7 @@ void AsyncWrap::EmitDestroy(Environment* env, double async_id) {
   }
 
   if (env->destroy_async_id_list()->empty()) {
-    env->SetUnrefImmediate(&DestroyAsyncIdsCallback);
+    env->SetImmediate(&DestroyAsyncIdsCallback, CallbackFlags::kUnrefed);
   }
 
   env->destroy_async_id_list()->push_back(async_id);

diff --git a/src/callback_queue-inl.h b/src/callback_queue-inl.h
@@ -10,8 +10,8 @@ namespace node {
 template <typename R, typename... Args>
 template <typename Fn>
 std::unique_ptr<typename CallbackQueue<R, Args...>::Callback>
-CallbackQueue<R, Args...>::CreateCallback(Fn&& fn, bool refed) {
-  return std::make_unique<CallbackImpl<Fn>>(std::move(fn), refed);
+CallbackQueue<R, Args...>::CreateCallback(Fn&& fn, CallbackFlags::Flags flags) {
+  return std::make_unique<CallbackImpl<Fn>>(std::move(fn), flags);
 }
 
 template <typename R, typename... Args>
@@ -57,12 +57,12 @@ size_t CallbackQueue<R, Args...>::size() const {
 }
 
 template <typename R, typename... Args>
-CallbackQueue<R, Args...>::Callback::Callback(bool refed)
-  : refed_(refed) {}
+CallbackQueue<R, Args...>::Callback::Callback(CallbackFlags::Flags flags)
+  : flags_(flags) {}
 
 template <typename R, typename... Args>
-bool CallbackQueue<R, Args...>::Callback::is_refed() const {
-  return refed_;
+CallbackFlags::Flags CallbackQueue<R, Args...>::Callback::flags() const {
+  return flags_;
 }
 
 template <typename R, typename... Args>
@@ -80,8 +80,8 @@ void CallbackQueue<R, Args...>::Callback::set_next(
 template <typename R, typename... Args>
 template <typename Fn>
 CallbackQueue<R, Args...>::CallbackImpl<Fn>::CallbackImpl(
-    Fn&& callback, bool refed)
-  : Callback(refed),
+    Fn&& callback, CallbackFlags::Flags flags)
+  : Callback(flags),
     callback_(std::move(callback)) {}
 
 template <typename R, typename... Args>

diff --git a/src/callback_queue.h b/src/callback_queue.h
@@ -7,6 +7,13 @@
 
 namespace node {
 
+namespace CallbackFlags {
+enum Flags {
+  kUnrefed = 0,
+  kRefed = 1,
+};
+}
+
 // A queue of C++ functions that take Args... as arguments and return R
 // (this is similar to the signature of std::function).
 // New entries are added using `CreateCallback()`/`Push()`, and removed using
@@ -18,25 +25,26 @@ class CallbackQueue {
  public:
   class Callback {
    public:
-    explicit inline Callback(bool refed);
+    explicit inline Callback(CallbackFlags::Flags flags);
 
     virtual ~Callback() = default;
     virtual R Call(Args... args) = 0;
 
-    inline bool is_refed() const;
+    inline CallbackFlags::Flags flags() const;
 
    private:
     inline std::unique_ptr<Callback> get_next();
     inline void set_next(std::unique_ptr<Callback> next);
 
-    bool refed_;
+    CallbackFlags::Flags flags_;
     std::unique_ptr<Callback> next_;
 
     friend class CallbackQueue;
   };
 
   template <typename Fn>
-  inline std::unique_ptr<Callback> CreateCallback(Fn&& fn, bool refed);
+  inline std::unique_ptr<Callback> CreateCallback(
+      Fn&& fn, CallbackFlags::Flags);
 
   inline std::unique_ptr<Callback> Shift();
   inline void Push(std::unique_ptr<Callback> cb);
@@ -51,7 +59,7 @@ class CallbackQueue {
   template <typename Fn>
   class CallbackImpl final : public Callback {
    public:
-    CallbackImpl(Fn&& callback, bool refed);
+    CallbackImpl(Fn&& callback, CallbackFlags::Flags flags);
     R Call(Args... args) override;
 
    private:

diff --git a/src/env-inl.h b/src/env-inl.h
@@ -707,29 +707,21 @@ inline void IsolateData::set_options(
 }
 
 template <typename Fn>
-void Environment::CreateImmediate(Fn&& cb, bool ref) {
-  auto callback = native_immediates_.CreateCallback(std::move(cb), ref);
+void Environment::SetImmediate(Fn&& cb, CallbackFlags::Flags flags) {
+  auto callback = native_immediates_.CreateCallback(std::move(cb), flags);
   native_immediates_.Push(std::move(callback));
-}
-
-template <typename Fn>
-void Environment::SetImmediate(Fn&& cb) {
-  CreateImmediate(std::move(cb), true);
 
-  if (immediate_info()->ref_count() == 0)
-    ToggleImmediateRef(true);
-  immediate_info()->ref_count_inc(1);
-}
-
-template <typename Fn>
-void Environment::SetUnrefImmediate(Fn&& cb) {
-  CreateImmediate(std::move(cb), false);
+  if (flags & CallbackFlags::kRefed) {
+    if (immediate_info()->ref_count() == 0)
+      ToggleImmediateRef(true);
+    immediate_info()->ref_count_inc(1);
+  }
 }
 
 template <typename Fn>
-void Environment::SetImmediateThreadsafe(Fn&& cb, bool refed) {
-  auto callback =
-      native_immediates_threadsafe_.CreateCallback(std::move(cb), refed);
+void Environment::SetImmediateThreadsafe(Fn&& cb, CallbackFlags::Flags flags) {
+  auto callback = native_immediates_threadsafe_.CreateCallback(
+      std::move(cb), flags);
   {
     Mutex::ScopedLock lock(native_immediates_threadsafe_mutex_);
     native_immediates_threadsafe_.Push(std::move(callback));
@@ -740,8 +732,8 @@ void Environment::SetImmediateThreadsafe(Fn&& cb, bool refed) {
 
 template <typename Fn>
 void Environment::RequestInterrupt(Fn&& cb) {
-  auto callback =
-      native_immediates_interrupts_.CreateCallback(std::move(cb), false);
+  auto callback = native_immediates_interrupts_.CreateCallback(
+      std::move(cb), CallbackFlags::kRefed);
   {
     Mutex::ScopedLock lock(native_immediates_threadsafe_mutex_);
     native_immediates_interrupts_.Push(std::move(callback));

diff --git a/src/env.cc b/src/env.cc
@@ -587,7 +587,7 @@ void Environment::CleanupHandles() {
   Isolate::DisallowJavascriptExecutionScope disallow_js(isolate(),
       Isolate::DisallowJavascriptExecutionScope::THROW_ON_FAILURE);
 
-  RunAndClearNativeImmediates(true /* skip SetUnrefImmediate()s */);
+  RunAndClearNativeImmediates(true /* skip unrefed SetImmediate()s */);
 
   for (ReqWrapBase* request : req_wrap_queue_)
     request->Cancel();
@@ -730,10 +730,11 @@ void Environment::RunAndClearNativeImmediates(bool only_refed) {
     TryCatchScope try_catch(this);
     DebugSealHandleScope seal_handle_scope(isolate());
     while (auto head = queue->Shift()) {
-      if (head->is_refed())
+      bool is_refed = head->flags() & CallbackFlags::kRefed;
+      if (is_refed)
         ref_count++;
 
-      if (head->is_refed() || !only_refed)
+      if (is_refed || !only_refed)
         head->Call(this);
 
       head.reset();  // Destroy now so that this is also observed by try_catch.

diff --git a/src/env.h b/src/env.h
@@ -1165,12 +1165,12 @@ class Environment : public MemoryRetainer {
   // Unlike the JS setImmediate() function, nested SetImmediate() calls will
   // be run without returning control to the event loop, similar to nextTick().
   template <typename Fn>
-  inline void SetImmediate(Fn&& cb);
-  template <typename Fn>
-  inline void SetUnrefImmediate(Fn&& cb);
+  inline void SetImmediate(
+      Fn&& cb, CallbackFlags::Flags flags = CallbackFlags::kRefed);
   template <typename Fn>
   // This behaves like SetImmediate() but can be called from any thread.
-  inline void SetImmediateThreadsafe(Fn&& cb, bool refed = true);
+  inline void SetImmediateThreadsafe(
+      Fn&& cb, CallbackFlags::Flags flags = CallbackFlags::kRefed);
   // This behaves like V8's Isolate::RequestInterrupt(), but also accounts for
   // the event loop (i.e. combines the V8 function with SetImmediate()).
   // The passed callback may not throw exceptions.
@@ -1253,9 +1253,6 @@ class Environment : public MemoryRetainer {
   void RunAndClearInterrupts();
 
  private:
-  template <typename Fn>
-  inline void CreateImmediate(Fn&& cb, bool ref);
-
   inline void ThrowError(v8::Local<v8::Value> (*fun)(v8::Local<v8::String>),
                          const char* errmsg);
 

diff --git a/src/node_dir.cc b/src/node_dir.cc
@@ -126,10 +126,10 @@ inline void DirHandle::GCClose() {
   // to notify that the file descriptor was gc'd. We want to be noisy about
   // this because not explicitly closing the DirHandle is a bug.
 
-  env()->SetUnrefImmediate([](Environment* env) {
+  env()->SetImmediate([](Environment* env) {
     ProcessEmitWarning(env,
                        "Closing directory handle on garbage collection");
-  });
+  }, CallbackFlags::kUnrefed);
 }
 
 void AfterClose(uv_fs_t* req) {

diff --git a/src/node_file.cc b/src/node_file.cc
@@ -226,7 +226,7 @@ inline void FileHandle::Close() {
   // 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([detail](Environment* env) {
+  env()->SetImmediate([detail](Environment* env) {
     ProcessEmitWarning(env,
                        "Closing file descriptor %d on garbage collection",
                        detail.fd);
@@ -240,7 +240,7 @@ inline void FileHandle::Close() {
           "thrown if a file descriptor is closed during garbage collection.",
           "DEP0137").IsNothing();
     }
-  });
+  }, CallbackFlags::kUnrefed);
 }
 
 void FileHandle::CloseReq::Resolve() {

diff --git a/src/node_perf.cc b/src/node_perf.cc
@@ -282,9 +282,9 @@ void MarkGarbageCollectionEnd(Isolate* isolate,
       static_cast<PerformanceGCFlags>(flags),
       state->performance_last_gc_start_mark,
       PERFORMANCE_NOW());
-  env->SetUnrefImmediate([entry = std::move(entry)](Environment* env) mutable {
+  env->SetImmediate([entry = std::move(entry)](Environment* env) mutable {
     PerformanceGCCallback(env, std::move(entry));
-  });
+  }, CallbackFlags::kUnrefed);
 }
 
 void GarbageCollectionCleanupHook(void* data) {

diff --git a/src/node_worker.cc b/src/node_worker.cc
@@ -759,7 +759,7 @@ void Worker::TakeHeapSnapshot(const FunctionCallbackInfo<Value>& args) {
               env, std::move(snapshot));
           Local<Value> args[] = { stream->object() };
           taker->MakeCallback(env->ondone_string(), arraysize(args), args);
-        }, /* refed */ false);
+        }, CallbackFlags::kUnrefed);
   });
   args.GetReturnValue().Set(scheduled ? taker->object() : Local<Object>());
 }

diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc
@@ -266,11 +266,11 @@ TEST_F(EnvironmentTest, SetImmediateCleanup) {
     (*env)->SetImmediate([&](node::Environment* env_arg) {
       EXPECT_EQ(env_arg, *env);
       called++;
-    });
-    (*env)->SetUnrefImmediate([&](node::Environment* env_arg) {
+    }, node::CallbackFlags::kRefed);
+    (*env)->SetImmediate([&](node::Environment* env_arg) {
       EXPECT_EQ(env_arg, *env);
       called_unref++;
-    });
+    }, node::CallbackFlags::kUnrefed);
   }
 
   EXPECT_EQ(called, 1);