src: remove keep alive option from SetImmediate()

This is no longer necessary now that the copyable `BaseObjectPtr`
is available (as opposed to the only-movable `v8::Global`).

PR-URL: #30374
Refs: nodejs/quic#141
Refs: nodejs/quic#149
Refs: nodejs/quic#141
Refs: nodejs/quic#165
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>

Author: addaleax

src/cares_wrap.cc

@@ -627,8 +627,6 @@ class QueryWrap : public AsyncWrap {
     } else {
       Parse(response_data_->host.get());
     }
-
-    delete this;
   }
 
   void* MakeCallbackPointer() {
@@ -686,9 +684,13 @@ class QueryWrap : public AsyncWrap {
   }
 
   void QueueResponseCallback(int status) {
-    env()->SetImmediate([this](Environment*) {
+    BaseObjectPtr<QueryWrap> strong_ref{this};
+    env()->SetImmediate([this, strong_ref](Environment*) {
       AfterResponse();
-    }, object());
+
+      // Delete once strong_ref goes out of scope.
+      Detach();
+    });
 
     channel_->set_query_last_ok(status != ARES_ECONNREFUSED);
     channel_->ModifyActivityQueryCount(-1);

src/env-inl.h

@@ -746,13 +746,9 @@ inline void IsolateData::set_options(
 }
 
 template <typename Fn>
-void Environment::CreateImmediate(Fn&& cb,
-                                  v8::Local<v8::Object> keep_alive,
-                                  bool ref) {
+void Environment::CreateImmediate(Fn&& cb, bool ref) {
   auto callback = std::make_unique<NativeImmediateCallbackImpl<Fn>>(
-      std::move(cb),
-      v8::Global<v8::Object>(isolate(), keep_alive),
-      ref);
+      std::move(cb), ref);
   NativeImmediateCallback* prev_tail = native_immediate_callbacks_tail_;
 
   native_immediate_callbacks_tail_ = callback.get();
@@ -765,17 +761,17 @@ void Environment::CreateImmediate(Fn&& cb,
 }
 
 template <typename Fn>
-void Environment::SetImmediate(Fn&& cb, v8::Local<v8::Object> keep_alive) {
-  CreateImmediate(std::move(cb), keep_alive, true);
+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, v8::Local<v8::Object> keep_alive) {
-  CreateImmediate(std::move(cb), keep_alive, false);
+void Environment::SetUnrefImmediate(Fn&& cb) {
+  CreateImmediate(std::move(cb), false);
 }
 
 Environment::NativeImmediateCallback::NativeImmediateCallback(bool refed)
@@ -797,10 +793,9 @@ void Environment::NativeImmediateCallback::set_next(
 
 template <typename Fn>
 Environment::NativeImmediateCallbackImpl<Fn>::NativeImmediateCallbackImpl(
-    Fn&& callback, v8::Global<v8::Object>&& keep_alive, bool refed)
+    Fn&& callback, bool refed)
   : NativeImmediateCallback(refed),
-    callback_(std::move(callback)),
-    keep_alive_(std::move(keep_alive)) {}
+    callback_(std::move(callback)) {}
 
 template <typename Fn>
 void Environment::NativeImmediateCallbackImpl<Fn>::Call(Environment* env) {

src/env.h

@@ -1183,13 +1183,9 @@ class Environment : public MemoryRetainer {
   // 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>());
+  inline void SetImmediate(Fn&& cb);
   template <typename Fn>
-  inline void SetUnrefImmediate(Fn&& cb,
-                                v8::Local<v8::Object> keep_alive =
-                                    v8::Local<v8::Object>());
+  inline void SetUnrefImmediate(Fn&& cb);
   // This needs to be available for the JS-land setImmediate().
   void ToggleImmediateRef(bool ref);
 
@@ -1260,9 +1256,7 @@ class Environment : public MemoryRetainer {
 
  private:
   template <typename Fn>
-  inline void CreateImmediate(Fn&& cb,
-                              v8::Local<v8::Object> keep_alive,
-                              bool ref);
+  inline void CreateImmediate(Fn&& cb, bool ref);
 
   inline void ThrowError(v8::Local<v8::Value> (*fun)(v8::Local<v8::String>),
                          const char* errmsg);
@@ -1410,14 +1404,11 @@ class Environment : public MemoryRetainer {
   template <typename Fn>
   class NativeImmediateCallbackImpl final : public NativeImmediateCallback {
    public:
-    NativeImmediateCallbackImpl(Fn&& callback,
-                                v8::Global<v8::Object>&& keep_alive,
-                                bool refed);
+    NativeImmediateCallbackImpl(Fn&& callback, bool refed);
     void Call(Environment* env) override;
 
    private:
     Fn callback_;
-    v8::Global<v8::Object> keep_alive_;
   };
 
   std::unique_ptr<NativeImmediateCallback> native_immediate_callbacks_head_;

src/node_http2.cc

@@ -1582,7 +1582,8 @@ void Http2Session::MaybeScheduleWrite() {
     HandleScope handle_scope(env()->isolate());
     Debug(this, "scheduling write");
     flags_ |= SESSION_STATE_WRITE_SCHEDULED;
-    env()->SetImmediate([this](Environment* env) {
+    BaseObjectPtr<Http2Session> strong_ref{this};
+    env()->SetImmediate([this, strong_ref](Environment* env) {
       if (session_ == nullptr || !(flags_ & SESSION_STATE_WRITE_SCHEDULED)) {
         // This can happen e.g. when a stream was reset before this turn
         // of the event loop, in which case SendPendingData() is called early,
@@ -1595,7 +1596,7 @@ void Http2Session::MaybeScheduleWrite() {
       HandleScope handle_scope(env->isolate());
       InternalCallbackScope callback_scope(this);
       SendPendingData();
-    }, object());
+    });
   }
 }
 
@@ -2043,7 +2044,8 @@ void Http2Stream::Destroy() {
 
   // Wait until the start of the next loop to delete because there
   // may still be some pending operations queued for this stream.
-  env()->SetImmediate([this](Environment* env) {
+  BaseObjectPtr<Http2Stream> strong_ref{this};
+  env()->SetImmediate([this, strong_ref](Environment* env) {
     // Free any remaining outgoing data chunks here. This should be done
     // here because it's possible for destroy to have been called while
     // we still have queued outbound writes.
@@ -2057,9 +2059,11 @@ void Http2Stream::Destroy() {
     // We can destroy the stream now if there are no writes for it
     // already on the socket. Otherwise, we'll wait for the garbage collector
     // to take care of cleaning up.
-    if (session() == nullptr || !session()->HasWritesOnSocketForStream(this))
-      delete this;
-  }, object());
+    if (session() == nullptr || !session()->HasWritesOnSocketForStream(this)) {
+      // Delete once strong_ref goes out of scope.
+      Detach();
+    }
+  });
 
   statistics_.end_time = uv_hrtime();
   session_->statistics_.stream_average_duration =

src/stream_pipe.cc

@@ -71,6 +71,7 @@ void StreamPipe::Unpipe() {
   // Delay the JS-facing part with SetImmediate, because this might be from
   // inside the garbage collector, so we can’t run JS here.
   HandleScope handle_scope(env()->isolate());
+  BaseObjectPtr<StreamPipe> strong_ref{this};
   env()->SetImmediate([this](Environment* env) {
     HandleScope handle_scope(env->isolate());
     Context::Scope context_scope(env->context());
@@ -105,7 +106,7 @@ void StreamPipe::Unpipe() {
             .IsNothing()) {
       return;
     }
-  }, object());
+  });
 }
 
 uv_buf_t StreamPipe::ReadableListener::OnStreamAlloc(size_t suggested_size) {

src/tls_wrap.cc

@@ -316,9 +316,10 @@ void TLSWrap::EncOut() {
         // its not clear if it is always correct. Not calling Done() could block
         // data flow, so for now continue to call Done(), just do it in the next
         // tick.
-        env()->SetImmediate([this](Environment* env) {
+        BaseObjectPtr<TLSWrap> strong_ref{this};
+        env()->SetImmediate([this, strong_ref](Environment* env) {
           InvokeQueued(0);
-        }, object());
+        });
       }
     }
     return;
@@ -349,9 +350,10 @@ void TLSWrap::EncOut() {
     HandleScope handle_scope(env()->isolate());
 
     // Simulate asynchronous finishing, TLS cannot handle this at the moment.
-    env()->SetImmediate([this](Environment* env) {
+    BaseObjectPtr<TLSWrap> strong_ref{this};
+    env()->SetImmediate([this, strong_ref](Environment* env) {
       OnStreamAfterWrite(nullptr, 0);
-    }, object());
+    });
   }
 }
 
@@ -718,9 +720,10 @@ int TLSWrap::DoWrite(WriteWrap* w,
       StreamWriteResult res =
           underlying_stream()->Write(bufs, count, send_handle);
       if (!res.async) {
-        env()->SetImmediate([this](Environment* env) {
+        BaseObjectPtr<TLSWrap> strong_ref{this};
+        env()->SetImmediate([this, strong_ref](Environment* env) {
           OnStreamAfterWrite(current_empty_write_, 0);
-        }, object());
+        });
       }
       return 0;
     }