From a4cfde03cf444b75f921e72f2c3248d1ceeb67cd Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 8 Jul 2020 01:42:07 +0200 Subject: [PATCH 1/2] src: remove user_data from TimerWrap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There’s no point in having an opaque user data pointer when we’re already using `std::function`. --- src/inspector_agent.cc | 2 +- src/quic/node_quic_session.cc | 4 ++-- src/timer_wrap.cc | 12 +++++------- src/timer_wrap.h | 8 +++----- 4 files changed, 11 insertions(+), 15 deletions(-) diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index a3c0e42375a9e1..13ebffdc04935a 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -525,7 +525,7 @@ class NodeInspectorClient : public V8InspectorClient { void* data) override { auto result = timers_.emplace(std::piecewise_construct, std::make_tuple(data), - std::make_tuple(env_, callback, data)); + std::make_tuple(env_, [=]() { callback(data); })); CHECK(result.second); uint64_t interval = 1000 * interval_s; result.first->second.Update(interval, interval); diff --git a/src/quic/node_quic_session.cc b/src/quic/node_quic_session.cc index 75a1cc2b7f77b2..6c4d2fd47c6ccc 100644 --- a/src/quic/node_quic_session.cc +++ b/src/quic/node_quic_session.cc @@ -1431,8 +1431,8 @@ QuicSession::QuicSession( socket_(socket), alpn_(alpn), hostname_(hostname), - idle_(socket->env(), [this](void* data) { OnIdleTimeout(); }), - retransmit_(socket->env(), [this](void* data) { MaybeTimeout(); }), + idle_(socket->env(), [this]() { OnIdleTimeout(); }), + retransmit_(socket->env(), [this]() { MaybeTimeout(); }), dcid_(dcid), state_(env()->isolate()), quic_state_(socket->quic_state()) { diff --git a/src/timer_wrap.cc b/src/timer_wrap.cc index 618c50c82e2cbd..919b629348c2bf 100644 --- a/src/timer_wrap.cc +++ b/src/timer_wrap.cc @@ -5,10 +5,9 @@ namespace node { -TimerWrap::TimerWrap(Environment* env, TimerCb fn, void* user_data) +TimerWrap::TimerWrap(Environment* env, const TimerCb& fn) : env_(env), - fn_(fn), - user_data_(user_data) { + fn_(fn) { uv_timer_init(env->event_loop(), &timer_); timer_.data = this; } @@ -45,14 +44,13 @@ void TimerWrap::Unref() { void TimerWrap::OnTimeout(uv_timer_t* timer) { TimerWrap* t = ContainerOf(&TimerWrap::timer_, timer); - t->fn_(t->user_data_); + t->fn_(); } TimerWrapHandle::TimerWrapHandle( Environment* env, - TimerWrap::TimerCb fn, - void* user_data) { - timer_ = new TimerWrap(env, fn, user_data); + const TimerWrap::TimerCb& fn) { + timer_ = new TimerWrap(env, fn); env->AddCleanupHook(CleanupHook, this); } diff --git a/src/timer_wrap.h b/src/timer_wrap.h index c75e17b776beef..263562f2a135bd 100644 --- a/src/timer_wrap.h +++ b/src/timer_wrap.h @@ -14,9 +14,9 @@ namespace node { // Utility class that makes working with libuv timers a bit easier. class TimerWrap final : public MemoryRetainer { public: - using TimerCb = std::function; + using TimerCb = std::function; - TimerWrap(Environment* env, TimerCb fn, void* user_data); + TimerWrap(Environment* env, const TimerCb& fn); TimerWrap(const TimerWrap&) = delete; inline Environment* env() const { return env_; } @@ -43,7 +43,6 @@ class TimerWrap final : public MemoryRetainer { Environment* env_; TimerCb fn_; uv_timer_t timer_; - void* user_data_ = nullptr; friend std::unique_ptr::deleter_type; }; @@ -52,8 +51,7 @@ class TimerWrapHandle : public MemoryRetainer { public: TimerWrapHandle( Environment* env, - TimerWrap::TimerCb fn, - void* user_data = nullptr); + const TimerWrap::TimerCb& fn); TimerWrapHandle(const TimerWrapHandle&) = delete; From 76c6697e66ca439c246d5a06486055f0c99e8176 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 8 Jul 2020 02:06:40 +0200 Subject: [PATCH 2/2] 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(&timer_), TimerClosedCb); - } +} + +void TimerWrap::Close() { + timer_.data = nullptr; + env_->CloseHandle(reinterpret_cast(&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(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(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_; };