src: make ELDHistogram a HandleWrap

This simplifies the implementation of ELDHistogram a bit, and more generally allows us to have weak JS references associated with `HandleWrap`s. PR-URL: #29317 Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs · Sep 25, 2019 · 67aa5ef · 67aa5ef
1 parent 7dd897f
commit 67aa5ef
Show file tree

Hide file tree

Showing 6 changed files with 33 additions and 34 deletions.
diff --git a/src/async_wrap.h b/src/async_wrap.h
@@ -34,6 +34,7 @@ namespace node {
 #define NODE_ASYNC_NON_CRYPTO_PROVIDER_TYPES(V)                               \
   V(NONE)                                                                     \
   V(DNSCHANNEL)                                                               \
+  V(ELDHISTOGRAM)                                                             \
   V(FILEHANDLE)                                                               \
   V(FILEHANDLECLOSEREQ)                                                       \
   V(FSEVENTWRAP)                                                              \

diff --git a/src/handle_wrap.cc b/src/handle_wrap.cc
@@ -84,6 +84,17 @@ void HandleWrap::Close(Local<Value> close_callback) {
 }
 
 
+void HandleWrap::MakeWeak() {
+  persistent().SetWeak(
+      this,
+      [](const v8::WeakCallbackInfo<HandleWrap>& data) {
+        HandleWrap* handle_wrap = data.GetParameter();
+        handle_wrap->persistent().Reset();
+        handle_wrap->Close();
+      }, v8::WeakCallbackType::kParameter);
+}
+
+
 void HandleWrap::MarkAsInitialized() {
   env()->handle_wrap_queue()->PushBack(this);
   state_ = kInitialized;
@@ -115,15 +126,14 @@ void HandleWrap::OnClose(uv_handle_t* handle) {
   HandleScope scope(env->isolate());
   Context::Scope context_scope(env->context());
 
-  // The wrap object should still be there.
-  CHECK_EQ(wrap->persistent().IsEmpty(), false);
   CHECK_EQ(wrap->state_, kClosing);
 
   wrap->state_ = kClosed;
 
   wrap->OnClose();
 
-  if (wrap->object()->Has(env->context(), env->handle_onclose_symbol())
+  if (!wrap->persistent().IsEmpty() &&
+      wrap->object()->Has(env->context(), env->handle_onclose_symbol())
       .FromMaybe(false)) {
     wrap->MakeCallback(env->handle_onclose_symbol(), 0, nullptr);
   }

diff --git a/src/handle_wrap.h b/src/handle_wrap.h
@@ -76,6 +76,8 @@ class HandleWrap : public AsyncWrap {
   static v8::Local<v8::FunctionTemplate> GetConstructorTemplate(
       Environment* env);
 
+  void MakeWeak();  // This hides BaseObject::MakeWeak()
+
  protected:
   HandleWrap(Environment* env,
              v8::Local<v8::Object> object,

diff --git a/src/node_perf.cc b/src/node_perf.cc
@@ -477,31 +477,18 @@ static void ELDHistogramNew(const FunctionCallbackInfo<Value>& args) {
 ELDHistogram::ELDHistogram(
     Environment* env,
     Local<Object> wrap,
-    int32_t resolution) : BaseObject(env, wrap),
+    int32_t resolution) : HandleWrap(env,
+                                     wrap,
+                                     reinterpret_cast<uv_handle_t*>(&timer_),
+                                     AsyncWrap::PROVIDER_ELDHISTOGRAM),
                           Histogram(1, 3.6e12),
                           resolution_(resolution) {
   MakeWeak();
-  timer_ = new uv_timer_t();
-  uv_timer_init(env->event_loop(), timer_);
-  timer_->data = this;
+  uv_timer_init(env->event_loop(), &timer_);
 }
 
-void ELDHistogram::CloseTimer() {
-  if (timer_ == nullptr)
-    return;
-
-  env()->CloseHandle(timer_, [](uv_timer_t* handle) { delete handle; });
-  timer_ = nullptr;
-}
-
-ELDHistogram::~ELDHistogram() {
-  Disable();
-  CloseTimer();
-}
-
-void ELDHistogramDelayInterval(uv_timer_t* req) {
-  ELDHistogram* histogram =
-    reinterpret_cast<ELDHistogram*>(req->data);
+void ELDHistogram::DelayIntervalCallback(uv_timer_t* req) {
+  ELDHistogram* histogram = ContainerOf(&ELDHistogram::timer_, req);
   histogram->RecordDelta();
   TRACE_COUNTER1(TRACING_CATEGORY_NODE2(perf, event_loop),
                  "min", histogram->Min());
@@ -537,21 +524,21 @@ bool ELDHistogram::RecordDelta() {
 }
 
 bool ELDHistogram::Enable() {
-  if (enabled_) return false;
+  if (enabled_ || IsHandleClosing()) return false;
   enabled_ = true;
   prev_ = 0;
-  uv_timer_start(timer_,
-                 ELDHistogramDelayInterval,
+  uv_timer_start(&timer_,
+                 DelayIntervalCallback,
                  resolution_,
                  resolution_);
-  uv_unref(reinterpret_cast<uv_handle_t*>(timer_));
+  uv_unref(reinterpret_cast<uv_handle_t*>(&timer_));
   return true;
 }
 
 bool ELDHistogram::Disable() {
-  if (!enabled_) return false;
+  if (!enabled_ || IsHandleClosing()) return false;
   enabled_ = false;
-  uv_timer_stop(timer_);
+  uv_timer_stop(&timer_);
   return true;
 }
 

diff --git a/src/node_perf.h b/src/node_perf.h
@@ -123,14 +123,12 @@ class GCPerformanceEntry : public PerformanceEntry {
   PerformanceGCKind gckind_;
 };
 
-class ELDHistogram : public BaseObject, public Histogram {
+class ELDHistogram : public HandleWrap, public Histogram {
  public:
   ELDHistogram(Environment* env,
                Local<Object> wrap,
                int32_t resolution);
 
-  ~ELDHistogram() override;
-
   bool RecordDelta();
   bool Enable();
   bool Disable();
@@ -149,13 +147,13 @@ class ELDHistogram : public BaseObject, public Histogram {
   SET_SELF_SIZE(ELDHistogram)
 
  private:
-  void CloseTimer();
+  static void DelayIntervalCallback(uv_timer_t* req);
 
   bool enabled_ = false;
   int32_t resolution_ = 0;
   int64_t exceeds_ = 0;
   uint64_t prev_ = 0;
-  uv_timer_t* timer_;
+  uv_timer_t timer_;
 };
 
 }  // namespace performance

diff --git a/test/sequential/test-async-wrap-getasyncid.js b/test/sequential/test-async-wrap-getasyncid.js
@@ -50,6 +50,7 @@ const { getSystemErrorName } = require('util');
     delete providers.KEYPAIRGENREQUEST;
     delete providers.HTTPCLIENTREQUEST;
     delete providers.HTTPINCOMINGMESSAGE;
+    delete providers.ELDHISTOGRAM;
 
     const objKeys = Object.keys(providers);
     if (objKeys.length > 0)