From 9707be185e01984948526aa12de64924b43f3e41 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 3 Jul 2020 07:42:45 -0700 Subject: [PATCH 1/3] src: add TimerWrap utility Consolidate uv_timer_t boilerplate code into a shared utility. There are several places throughout the code where we use uv_timer_t internally (inspector, perf, quic), with some code duplication. This eliminates the duplicated code, ensures that cleanup occurs correctly, and simplifies use of the timers. Signed-off-by: James M Snell --- node.gyp | 2 + src/timer_wrap.cc | 94 +++++++++++++++++++++++++++++++++++++++++++++++ src/timer_wrap.h | 84 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 180 insertions(+) create mode 100644 src/timer_wrap.cc create mode 100644 src/timer_wrap.h diff --git a/node.gyp b/node.gyp index 47ce95de75591a..23f9e2ec7ffe5c 100644 --- a/node.gyp +++ b/node.gyp @@ -638,6 +638,7 @@ 'src/string_decoder.cc', 'src/tcp_wrap.cc', 'src/timers.cc', + 'src/timer_wrap.cc', 'src/tracing/agent.cc', 'src/tracing/node_trace_buffer.cc', 'src/tracing/node_trace_writer.cc', @@ -741,6 +742,7 @@ 'src/tracing/trace_event.h', 'src/tracing/trace_event_common.h', 'src/tracing/traced_value.h', + 'src/timer_wrap.h', 'src/tty_wrap.h', 'src/udp_wrap.h', 'src/util.h', diff --git a/src/timer_wrap.cc b/src/timer_wrap.cc new file mode 100644 index 00000000000000..618c50c82e2cbd --- /dev/null +++ b/src/timer_wrap.cc @@ -0,0 +1,94 @@ +#include "env-inl.h" +#include "memory_tracker-inl.h" +#include "timer_wrap.h" +#include "uv.h" + +namespace node { + +TimerWrap::TimerWrap(Environment* env, TimerCb fn, void* user_data) + : env_(env), + fn_(fn), + user_data_(user_data) { + uv_timer_init(env->event_loop(), &timer_); + timer_.data = this; +} + +void TimerWrap::Stop(bool close) { + if (timer_.data == nullptr) return; + uv_timer_stop(&timer_); + if (LIKELY(close)) { + timer_.data = nullptr; + env_->CloseHandle(reinterpret_cast(&timer_), TimerClosedCb); + } +} + +void TimerWrap::TimerClosedCb(uv_handle_t* handle) { + std::unique_ptr ptr( + ContainerOf(&TimerWrap::timer_, + reinterpret_cast(handle))); +} + +void TimerWrap::Update(uint64_t interval, uint64_t repeat) { + if (timer_.data == nullptr) return; + uv_timer_start(&timer_, OnTimeout, interval, repeat); +} + +void TimerWrap::Ref() { + if (timer_.data == nullptr) return; + uv_ref(reinterpret_cast(&timer_)); +} + +void TimerWrap::Unref() { + if (timer_.data == nullptr) return; + uv_unref(reinterpret_cast(&timer_)); +} + +void TimerWrap::OnTimeout(uv_timer_t* timer) { + TimerWrap* t = ContainerOf(&TimerWrap::timer_, timer); + t->fn_(t->user_data_); +} + +TimerWrapHandle::TimerWrapHandle( + Environment* env, + TimerWrap::TimerCb fn, + void* user_data) { + timer_ = new TimerWrap(env, fn, user_data); + env->AddCleanupHook(CleanupHook, this); +} + +void TimerWrapHandle::Stop(bool close) { + if (UNLIKELY(!close)) + return timer_->Stop(close); + + if (timer_ != nullptr) { + timer_->env()->RemoveCleanupHook(CleanupHook, this); + timer_->Stop(); + } + timer_ = nullptr; +} + +void TimerWrapHandle::Ref() { + if (timer_ != nullptr) + timer_->Ref(); +} + +void TimerWrapHandle::Unref() { + if (timer_ != nullptr) + timer_->Unref(); +} + +void TimerWrapHandle::Update(uint64_t interval, uint64_t repeat) { + if (timer_ != nullptr) + timer_->Update(interval, repeat); +} + +void TimerWrapHandle::CleanupHook(void* data) { + static_cast(data)->Stop(); +} + +void TimerWrapHandle::MemoryInfo(node::MemoryTracker* tracker) const { + if (timer_ != nullptr) + tracker->TrackField("timer", *timer_); +} + +} // namespace node diff --git a/src/timer_wrap.h b/src/timer_wrap.h new file mode 100644 index 00000000000000..c75e17b776beef --- /dev/null +++ b/src/timer_wrap.h @@ -0,0 +1,84 @@ +#ifndef SRC_TIMER_WRAP_H_ +#define SRC_TIMER_WRAP_H_ + +#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#include "memory_tracker.h" +#include "env.h" +#include "uv.h" + +#include + +namespace node { + +// Utility class that makes working with libuv timers a bit easier. +class TimerWrap final : public MemoryRetainer { + public: + using TimerCb = std::function; + + TimerWrap(Environment* env, TimerCb fn, void* user_data); + TimerWrap(const TimerWrap&) = delete; + + inline Environment* env() const { return env_; } + + // Completely stops the timer, making it no longer usable. + void Stop(bool close = true); + + // Starts / Restarts the Timer + void Update(uint64_t interval, uint64_t repeat = 0); + + void Ref(); + + void Unref(); + + SET_NO_MEMORY_INFO(); + SET_MEMORY_INFO_NAME(TimerWrap) + SET_SELF_SIZE(TimerWrap) + + private: + static void TimerClosedCb(uv_handle_t* handle); + static void OnTimeout(uv_timer_t* timer); + ~TimerWrap() = default; + + Environment* env_; + TimerCb fn_; + uv_timer_t timer_; + void* user_data_ = nullptr; + + friend std::unique_ptr::deleter_type; +}; + +class TimerWrapHandle : public MemoryRetainer { + public: + TimerWrapHandle( + Environment* env, + TimerWrap::TimerCb fn, + void* user_data = nullptr); + + TimerWrapHandle(const TimerWrapHandle&) = delete; + + ~TimerWrapHandle() { Stop(); } + + void Update(uint64_t interval, uint64_t repeat = 0); + + void Ref(); + + void Unref(); + + void Stop(bool close = true); + + void MemoryInfo(node::MemoryTracker* tracker) const override; + + SET_MEMORY_INFO_NAME(TimerWrapHandle) + SET_SELF_SIZE(TimerWrapHandle) + + private: + static void CleanupHook(void* data); + TimerWrap* timer_; +}; + +} // namespace node + +#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#endif // SRC_TIMER_WRAP_H_ From b4e7e573962b487f03dbaeb9df84deb604b53015 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 3 Jul 2020 09:03:02 -0700 Subject: [PATCH 2/3] src: replace InspectorTimer with TimerWrap utility Signed-off-by: James M Snell --- src/inspector_agent.cc | 92 ++++-------------------------------------- 1 file changed, 8 insertions(+), 84 deletions(-) diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index f128a197ca9726..a3c0e42375a9e1 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -15,6 +15,7 @@ #include "node_process.h" #include "node_url.h" #include "util-inl.h" +#include "timer_wrap.h" #include "v8-inspector.h" #include "v8-platform.h" @@ -326,86 +327,6 @@ class ChannelImpl final : public v8_inspector::V8Inspector::Channel, bool retaining_context_; }; -class InspectorTimer { - public: - InspectorTimer(Environment* env, - double interval_s, - V8InspectorClient::TimerCallback callback, - void* data) : env_(env), - callback_(callback), - data_(data) { - uv_timer_init(env->event_loop(), &timer_); - int64_t interval_ms = 1000 * interval_s; - uv_timer_start(&timer_, OnTimer, interval_ms, interval_ms); - timer_.data = this; - } - - InspectorTimer(const InspectorTimer&) = delete; - - void Stop() { - if (timer_.data == nullptr) return; - - timer_.data = nullptr; - uv_timer_stop(&timer_); - env_->CloseHandle(reinterpret_cast(&timer_), TimerClosedCb); - } - - inline Environment* env() const { return env_; } - - private: - static void OnTimer(uv_timer_t* uvtimer) { - InspectorTimer* timer = node::ContainerOf(&InspectorTimer::timer_, uvtimer); - timer->callback_(timer->data_); - } - - static void TimerClosedCb(uv_handle_t* uvtimer) { - std::unique_ptr timer( - node::ContainerOf(&InspectorTimer::timer_, - reinterpret_cast(uvtimer))); - // Unique_ptr goes out of scope here and pointer is deleted. - } - - ~InspectorTimer() = default; - - Environment* env_; - uv_timer_t timer_; - V8InspectorClient::TimerCallback callback_; - void* data_; - - friend std::unique_ptr::deleter_type; -}; - -class InspectorTimerHandle { - public: - InspectorTimerHandle(Environment* env, double interval_s, - V8InspectorClient::TimerCallback callback, void* data) { - timer_ = new InspectorTimer(env, interval_s, callback, data); - - env->AddCleanupHook(CleanupHook, this); - } - - InspectorTimerHandle(const InspectorTimerHandle&) = delete; - - ~InspectorTimerHandle() { - Stop(); - } - - private: - void Stop() { - if (timer_ != nullptr) { - timer_->env()->RemoveCleanupHook(CleanupHook, this); - timer_->Stop(); - } - timer_ = nullptr; - } - - static void CleanupHook(void* data) { - static_cast(data)->Stop(); - } - - InspectorTimer* timer_; -}; - class SameThreadInspectorSession : public InspectorSession { public: SameThreadInspectorSession( @@ -602,9 +523,12 @@ class NodeInspectorClient : public V8InspectorClient { void startRepeatingTimer(double interval_s, TimerCallback callback, void* data) override { - timers_.emplace(std::piecewise_construct, std::make_tuple(data), - std::make_tuple(env_, interval_s, callback, - data)); + auto result = + timers_.emplace(std::piecewise_construct, std::make_tuple(data), + std::make_tuple(env_, callback, data)); + CHECK(result.second); + uint64_t interval = 1000 * interval_s; + result.first->second.Update(interval, interval); } void cancelTimer(void* data) override { @@ -724,7 +648,7 @@ class NodeInspectorClient : public V8InspectorClient { bool running_nested_loop_ = false; std::unique_ptr client_; // Note: ~ChannelImpl may access timers_ so timers_ has to come first. - std::unordered_map timers_; + std::unordered_map timers_; std::unordered_map> channels_; int next_session_id_ = 1; bool waiting_for_resume_ = false; From f7ad0bfff8a18c3e76db9bd3340fad8d827b47c8 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 3 Jul 2020 09:42:46 -0700 Subject: [PATCH 3/3] quic: use TimerWrap for idle and retransmit timers Signed-off-by: James M Snell --- src/quic/node_quic_session-inl.h | 9 +++---- src/quic/node_quic_session.cc | 7 +++--- src/quic/node_quic_session.h | 5 ++-- src/quic/node_quic_util-inl.h | 43 -------------------------------- src/quic/node_quic_util.h | 32 ------------------------ 5 files changed, 9 insertions(+), 87 deletions(-) diff --git a/src/quic/node_quic_session-inl.h b/src/quic/node_quic_session-inl.h index 935c8d024d55c9..94f84c5c793edf 100644 --- a/src/quic/node_quic_session-inl.h +++ b/src/quic/node_quic_session-inl.h @@ -375,8 +375,7 @@ QuicCID QuicSession::dcid() const { // timer to actually monitor. Here we take the calculated timeout // and extend out the libuv timer. void QuicSession::UpdateRetransmitTimer(uint64_t timeout) { - DCHECK_NOT_NULL(retransmit_); - retransmit_->Update(timeout); + retransmit_.Update(timeout, timeout); } void QuicSession::CheckAllocatedSize(size_t previous_size) const { @@ -512,13 +511,11 @@ void QuicSession::set_remote_transport_params() { } void QuicSession::StopIdleTimer() { - CHECK_NOT_NULL(idle_); - idle_->Stop(); + idle_.Stop(); } void QuicSession::StopRetransmitTimer() { - CHECK_NOT_NULL(retransmit_); - retransmit_->Stop(); + retransmit_.Stop(); } // Called by the OnVersionNegotiation callback when a version diff --git a/src/quic/node_quic_session.cc b/src/quic/node_quic_session.cc index fe30411d0def31..75a1cc2b7f77b2 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_(new Timer(socket->env(), [this]() { OnIdleTimeout(); })), - retransmit_(new Timer(socket->env(), [this]() { MaybeTimeout(); })), + idle_(socket->env(), [this](void* data) { OnIdleTimeout(); }), + retransmit_(socket->env(), [this](void* data) { MaybeTimeout(); }), dcid_(dcid), state_(env()->isolate()), quic_state_(socket->quic_state()) { @@ -2461,14 +2461,13 @@ void QuicSession::UpdateConnectionID( // will be silently closed. It is important to update this as activity // occurs to keep the idle timer from firing. void QuicSession::UpdateIdleTimer() { - CHECK_NOT_NULL(idle_); uint64_t now = uv_hrtime(); uint64_t expiry = ngtcp2_conn_get_idle_expiry(connection()); // nano to millis uint64_t timeout = expiry > now ? (expiry - now) / 1000000ULL : 1; if (timeout == 0) timeout = 1; Debug(this, "Updating idle timeout to %" PRIu64, timeout); - idle_->Update(timeout); + idle_.Update(timeout, timeout); } diff --git a/src/quic/node_quic_session.h b/src/quic/node_quic_session.h index 9cbaa317ae5876..bac8bcd1b9a1a9 100644 --- a/src/quic/node_quic_session.h +++ b/src/quic/node_quic_session.h @@ -18,6 +18,7 @@ #include "node_quic_util.h" #include "node_sockaddr.h" #include "stream_base.h" +#include "timer_wrap.h" #include "v8.h" #include "uv.h" @@ -1471,8 +1472,8 @@ class QuicSession : public AsyncWrap, QuicSessionListener* listener_ = nullptr; JSQuicSessionListener default_listener_; - TimerPointer idle_; - TimerPointer retransmit_; + TimerWrapHandle idle_; + TimerWrapHandle retransmit_; QuicCID scid_; QuicCID dcid_; diff --git a/src/quic/node_quic_util-inl.h b/src/quic/node_quic_util-inl.h index d51dc8d7e63d23..65b2e08823da54 100644 --- a/src/quic/node_quic_util-inl.h +++ b/src/quic/node_quic_util-inl.h @@ -73,49 +73,6 @@ size_t GetMaxPktLen(const SocketAddress& addr) { NGTCP2_MAX_PKTLEN_IPV4; } -Timer::Timer(Environment* env, std::function fn) - : env_(env), - fn_(fn) { - uv_timer_init(env_->event_loop(), &timer_); - timer_.data = this; -} - -void Timer::Stop() { - if (stopped_) - return; - stopped_ = true; - - if (timer_.data == this) { - uv_timer_stop(&timer_); - timer_.data = nullptr; - } -} - -// If the timer is not currently active, interval must be either 0 or greater. -// If the timer is already active, interval is ignored. -void Timer::Update(uint64_t interval) { - if (stopped_) - return; - uv_timer_start(&timer_, OnTimeout, interval, interval); - uv_unref(reinterpret_cast(&timer_)); -} - -void Timer::Free(Timer* timer) { - timer->env_->CloseHandle( - reinterpret_cast(&timer->timer_), - [&](uv_handle_t* timer) { - Timer* t = ContainerOf( - &Timer::timer_, - reinterpret_cast(timer)); - delete t; - }); -} - -void Timer::OnTimeout(uv_timer_t* timer) { - Timer* t = ContainerOf(&Timer::timer_, timer); - t->fn_(); -} - QuicError::QuicError( int32_t family_, uint64_t code_) : diff --git a/src/quic/node_quic_util.h b/src/quic/node_quic_util.h index d698a6df0b4041..b3afcb9483d940 100644 --- a/src/quic/node_quic_util.h +++ b/src/quic/node_quic_util.h @@ -338,38 +338,6 @@ class QuicCID : public MemoryRetainer { const ngtcp2_cid* ptr_; }; -// Simple timer wrapper that is used to implement the internals -// for idle and retransmission timeouts. Call Update to start or -// reset the timer; Stop to halt the timer. -class Timer final : public MemoryRetainer { - public: - inline explicit Timer(Environment* env, std::function fn); - - // Stops the timer with the side effect of the timer no longer being usable. - // It will be cleaned up and the Timer object will be destroyed. - inline void Stop(); - - // If the timer is not currently active, interval must be either 0 or greater. - // If the timer is already active, interval is ignored. - inline void Update(uint64_t interval); - - static inline void Free(Timer* timer); - - SET_NO_MEMORY_INFO() - SET_MEMORY_INFO_NAME(Timer) - SET_SELF_SIZE(Timer) - - private: - static inline void OnTimeout(uv_timer_t* timer); - - bool stopped_ = false; - Environment* env_; - std::function fn_; - uv_timer_t timer_; -}; - -using TimerPointer = DeleteFnPtr; - // A Stateless Reset Token is a mechanism by which a QUIC // endpoint can discreetly signal to a peer that it has // lost all state associated with a connection. This