From 76c6697e66ca439c246d5a06486055f0c99e8176 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Wed, 8 Jul 2020 02:06:40 +0200
Subject: [PATCH] src: refactor TimerWrap lifetime management

Split `Stop(true)` and `Stop(false)` into separate methods since the
actions performed by these are fully distinct.
---
 src/quic/node_quic_session-inl.h |  4 ++--
 src/timer_wrap.cc                | 30 ++++++++++++++++--------------
 src/timer_wrap.h                 | 14 ++++++++------
 3 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/src/quic/node_quic_session-inl.h b/src/quic/node_quic_session-inl.h
index 94f84c5c793edf..2544bef1c1d5c4 100644
--- a/src/quic/node_quic_session-inl.h
+++ b/src/quic/node_quic_session-inl.h
@@ -511,11 +511,11 @@ void QuicSession::set_remote_transport_params() {
 }
 
 void QuicSession::StopIdleTimer() {
-  idle_.Stop();
+  idle_.Close();
 }
 
 void QuicSession::StopRetransmitTimer() {
-  retransmit_.Stop();
+  retransmit_.Close();
 }
 
 // Called by the OnVersionNegotiation callback when a version
diff --git a/src/timer_wrap.cc b/src/timer_wrap.cc
index 919b629348c2bf..4b6350c4cb33b5 100644
--- a/src/timer_wrap.cc
+++ b/src/timer_wrap.cc
@@ -12,13 +12,14 @@ TimerWrap::TimerWrap(Environment* env, const TimerCb& fn)
   timer_.data = this;
 }
 
-void TimerWrap::Stop(bool close) {
+void TimerWrap::Stop() {
   if (timer_.data == nullptr) return;
   uv_timer_stop(&timer_);
-  if (LIKELY(close)) {
-    timer_.data = nullptr;
-    env_->CloseHandle(reinterpret_cast<uv_handle_t*>(&timer_), TimerClosedCb);
-  }
+}
+
+void TimerWrap::Close() {
+  timer_.data = nullptr;
+  env_->CloseHandle(reinterpret_cast<uv_handle_t*>(&timer_), TimerClosedCb);
 }
 
 void TimerWrap::TimerClosedCb(uv_handle_t* handle) {
@@ -54,13 +55,14 @@ TimerWrapHandle::TimerWrapHandle(
   env->AddCleanupHook(CleanupHook, this);
 }
 
-void TimerWrapHandle::Stop(bool close) {
-  if (UNLIKELY(!close))
-    return timer_->Stop(close);
+void TimerWrapHandle::Stop() {
+  return timer_->Stop();
+}
 
+void TimerWrapHandle::Close() {
   if (timer_ != nullptr) {
     timer_->env()->RemoveCleanupHook(CleanupHook, this);
-    timer_->Stop();
+    timer_->Close();
   }
   timer_ = nullptr;
 }
@@ -80,13 +82,13 @@ void TimerWrapHandle::Update(uint64_t interval, uint64_t repeat) {
     timer_->Update(interval, repeat);
 }
 
-void TimerWrapHandle::CleanupHook(void* data) {
-  static_cast<TimerWrapHandle*>(data)->Stop();
-}
-
-void TimerWrapHandle::MemoryInfo(node::MemoryTracker* tracker) const {
+void TimerWrapHandle::MemoryInfo(MemoryTracker* tracker) const {
   if (timer_ != nullptr)
     tracker->TrackField("timer", *timer_);
 }
 
+void TimerWrapHandle::CleanupHook(void* data) {
+  static_cast<TimerWrapHandle*>(data)->Close();
+}
+
 }  // namespace node
diff --git a/src/timer_wrap.h b/src/timer_wrap.h
index 263562f2a135bd..b2c20bf24d8746 100644
--- a/src/timer_wrap.h
+++ b/src/timer_wrap.h
@@ -21,14 +21,15 @@ class TimerWrap final : public MemoryRetainer {
 
   inline Environment* env() const { return env_; }
 
-  // Completely stops the timer, making it no longer usable.
-  void Stop(bool close = true);
+  // Stop calling the timer callback.
+  void Stop();
+  // Render the timer unusable and delete this object.
+  void Close();
 
   // Starts / Restarts the Timer
   void Update(uint64_t interval, uint64_t repeat = 0);
 
   void Ref();
-
   void Unref();
 
   SET_NO_MEMORY_INFO();
@@ -55,15 +56,15 @@ class TimerWrapHandle : public MemoryRetainer  {
 
   TimerWrapHandle(const TimerWrapHandle&) = delete;
 
-  ~TimerWrapHandle() { Stop(); }
+  ~TimerWrapHandle() { Close(); }
 
   void Update(uint64_t interval, uint64_t repeat = 0);
 
   void Ref();
-
   void Unref();
 
-  void Stop(bool close = true);
+  void Stop();
+  void Close();
 
   void MemoryInfo(node::MemoryTracker* tracker) const override;
 
@@ -72,6 +73,7 @@ class TimerWrapHandle : public MemoryRetainer  {
 
  private:
   static void CleanupHook(void* data);
+
   TimerWrap* timer_;
 };