From cad1a62d325b011887349687962700dfe7510525 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 26 Apr 2016 12:01:46 +0200 Subject: [PATCH 1/3] src: use libuv's refcounting directly Don't implement an additional reference counting scheme on top of libuv. Libuv is the canonical source for that information so use it directly. PR-URL: https://github.com/nodejs/node/pull/6395 Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: James M Snell --- src/handle_wrap.cc | 42 ++++++++++++++++++------------------------ src/handle_wrap.h | 17 +++++++++++------ src/node.cc | 2 +- 3 files changed, 30 insertions(+), 31 deletions(-) diff --git a/src/handle_wrap.cc b/src/handle_wrap.cc index 8421d694a991bb..85c8d9e9215cd8 100644 --- a/src/handle_wrap.cc +++ b/src/handle_wrap.cc @@ -20,28 +20,25 @@ using v8::Value; void HandleWrap::Ref(const FunctionCallbackInfo& args) { HandleWrap* wrap = Unwrap(args.Holder()); - if (IsAlive(wrap)) { - uv_ref(wrap->handle__); - wrap->flags_ &= ~kUnref; - } + if (IsAlive(wrap)) + uv_ref(wrap->GetHandle()); } void HandleWrap::Unref(const FunctionCallbackInfo& args) { HandleWrap* wrap = Unwrap(args.Holder()); - if (IsAlive(wrap)) { - uv_unref(wrap->handle__); - wrap->flags_ |= kUnref; - } + if (IsAlive(wrap)) + uv_unref(wrap->GetHandle()); } void HandleWrap::Unrefed(const FunctionCallbackInfo& args) { HandleWrap* wrap = Unwrap(args.Holder()); - - bool unrefed = wrap->flags_ & kUnref == 1; - args.GetReturnValue().Set(unrefed); + // XXX(bnoordhuis) It's debatable whether a nullptr wrap should count + // as having a reference count but it's compatible with the logic that + // it replaces. + args.GetReturnValue().Set(wrap == nullptr || !HasRef(wrap)); } @@ -50,17 +47,17 @@ void HandleWrap::Close(const FunctionCallbackInfo& args) { HandleWrap* wrap = Unwrap(args.Holder()); - // guard against uninitialized handle or double close + // Guard against uninitialized handle or double close. if (!IsAlive(wrap)) return; CHECK_EQ(false, wrap->persistent().IsEmpty()); uv_close(wrap->handle__, OnClose); - wrap->handle__ = nullptr; + wrap->state_ = kClosing; if (args[0]->IsFunction()) { wrap->object()->Set(env->onclose_string(), args[0]); - wrap->flags_ |= kCloseCallback; + wrap->state_ = kClosingWithCallback; } } @@ -71,7 +68,7 @@ HandleWrap::HandleWrap(Environment* env, AsyncWrap::ProviderType provider, AsyncWrap* parent) : AsyncWrap(env, object, provider, parent), - flags_(0), + state_(kInitialized), handle__(handle) { handle__->data = this; HandleScope scope(env->isolate()); @@ -89,22 +86,19 @@ void HandleWrap::OnClose(uv_handle_t* handle) { HandleWrap* wrap = static_cast(handle->data); Environment* env = wrap->env(); HandleScope scope(env->isolate()); + Context::Scope context_scope(env->context()); // The wrap object should still be there. CHECK_EQ(wrap->persistent().IsEmpty(), false); + CHECK(wrap->state_ >= kClosing && wrap->state_ <= kClosingWithCallback); - // But the handle pointer should be gone. - CHECK_EQ(wrap->handle__, nullptr); + const bool have_close_callback = (wrap->state_ == kClosingWithCallback); + wrap->state_ = kClosed; - HandleScope handle_scope(env->isolate()); - Context::Scope context_scope(env->context()); - Local object = wrap->object(); - - if (wrap->flags_ & kCloseCallback) { + if (have_close_callback) wrap->MakeCallback(env->onclose_string(), 0, nullptr); - } - object->SetAlignedPointerInInternalField(0, nullptr); + wrap->object()->SetAlignedPointerInInternalField(0, nullptr); wrap->persistent().Reset(); delete wrap; } diff --git a/src/handle_wrap.h b/src/handle_wrap.h index d945143d31a952..fe3c5a8d9dc243 100644 --- a/src/handle_wrap.h +++ b/src/handle_wrap.h @@ -38,7 +38,15 @@ class HandleWrap : public AsyncWrap { static void Unrefed(const v8::FunctionCallbackInfo& args); static inline bool IsAlive(const HandleWrap* wrap) { - return wrap != nullptr && wrap->GetHandle() != nullptr; + // XXX(bnoordhuis) It's debatable whether only kInitialized should + // count as alive but it's compatible with the check that it replaces. + return wrap != nullptr && wrap->state_ == kInitialized; + } + + static inline bool HasRef(const HandleWrap* wrap) { + return wrap != nullptr && + wrap->state_ != kClosed && + uv_has_ref(wrap->GetHandle()); } inline uv_handle_t* GetHandle() const { return handle__; } @@ -56,13 +64,10 @@ class HandleWrap : public AsyncWrap { friend void GetActiveHandles(const v8::FunctionCallbackInfo&); static void OnClose(uv_handle_t* handle); ListNode handle_wrap_queue_; - unsigned int flags_; + enum { kInitialized, kClosing, kClosingWithCallback, kClosed } state_; // Using double underscore due to handle_ member in tcp_wrap. Probably // tcp_wrap should rename it's member to 'handle'. - uv_handle_t* handle__; - - static const unsigned int kUnref = 1; - static const unsigned int kCloseCallback = 2; + uv_handle_t* const handle__; }; diff --git a/src/node.cc b/src/node.cc index 2cfb2866b898bf..479130230b7c6a 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1723,7 +1723,7 @@ void GetActiveHandles(const FunctionCallbackInfo& args) { Local owner_sym = env->owner_string(); for (auto w : *env->handle_wrap_queue()) { - if (w->persistent().IsEmpty() || (w->flags_ & HandleWrap::kUnref)) + if (w->persistent().IsEmpty() || !HandleWrap::HasRef(w)) continue; Local object = w->object(); Local owner = object->Get(owner_sym); From a58d4839af6dfcb493ead8771cbff21b5ef9f18d Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 26 Apr 2016 13:32:44 +0200 Subject: [PATCH 2/3] src: simplify handlewrap state tracking logic This also updates the tests to expect that a closed handle has no reference count. PR-URL: https://github.com/nodejs/node/pull/6395 Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: James M Snell --- src/handle_wrap.cc | 8 ++++---- src/handle_wrap.h | 8 ++------ test/parallel/test-handle-wrap-isrefed-tty.js | 2 +- test/parallel/test-handle-wrap-isrefed.js | 16 ++++++++++------ 4 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/handle_wrap.cc b/src/handle_wrap.cc index 85c8d9e9215cd8..b82449989dd46e 100644 --- a/src/handle_wrap.cc +++ b/src/handle_wrap.cc @@ -35,10 +35,7 @@ void HandleWrap::Unref(const FunctionCallbackInfo& args) { void HandleWrap::Unrefed(const FunctionCallbackInfo& args) { HandleWrap* wrap = Unwrap(args.Holder()); - // XXX(bnoordhuis) It's debatable whether a nullptr wrap should count - // as having a reference count but it's compatible with the logic that - // it replaces. - args.GetReturnValue().Set(wrap == nullptr || !HasRef(wrap)); + args.GetReturnValue().Set(!HasRef(wrap)); } @@ -51,6 +48,9 @@ void HandleWrap::Close(const FunctionCallbackInfo& args) { if (!IsAlive(wrap)) return; + if (wrap->state_ != kInitialized) + return; + CHECK_EQ(false, wrap->persistent().IsEmpty()); uv_close(wrap->handle__, OnClose); wrap->state_ = kClosing; diff --git a/src/handle_wrap.h b/src/handle_wrap.h index fe3c5a8d9dc243..ef37cf9e3413d7 100644 --- a/src/handle_wrap.h +++ b/src/handle_wrap.h @@ -38,15 +38,11 @@ class HandleWrap : public AsyncWrap { static void Unrefed(const v8::FunctionCallbackInfo& args); static inline bool IsAlive(const HandleWrap* wrap) { - // XXX(bnoordhuis) It's debatable whether only kInitialized should - // count as alive but it's compatible with the check that it replaces. - return wrap != nullptr && wrap->state_ == kInitialized; + return wrap != nullptr && wrap->state_ != kClosed; } static inline bool HasRef(const HandleWrap* wrap) { - return wrap != nullptr && - wrap->state_ != kClosed && - uv_has_ref(wrap->GetHandle()); + return IsAlive(wrap) && uv_has_ref(wrap->GetHandle()); } inline uv_handle_t* GetHandle() const { return handle__; } diff --git a/test/parallel/test-handle-wrap-isrefed-tty.js b/test/parallel/test-handle-wrap-isrefed-tty.js index 4c31d63b52c568..c40a6b09132a76 100644 --- a/test/parallel/test-handle-wrap-isrefed-tty.js +++ b/test/parallel/test-handle-wrap-isrefed-tty.js @@ -19,7 +19,7 @@ if (process.argv[2] === 'child') { assert(tty._handle.unrefed(), false); tty.unref(); assert(tty._handle.unrefed(), true); - tty._handle.close(); + tty._handle.close(common.mustCall(() => assert(tty._handle.unrefed(), true))); assert(tty._handle.unrefed(), true); return; } diff --git a/test/parallel/test-handle-wrap-isrefed.js b/test/parallel/test-handle-wrap-isrefed.js index c88534117c3c9b..d3ea56a46f3911 100644 --- a/test/parallel/test-handle-wrap-isrefed.js +++ b/test/parallel/test-handle-wrap-isrefed.js @@ -22,7 +22,7 @@ function makeAssert(message) { assert(cp._handle.unrefed(), true); cp.ref(); assert(cp._handle.unrefed(), false); - cp._handle.close(); + cp._handle.close(common.mustCall(() => assert(cp._handle.unrefed(), true))); assert(cp._handle.unrefed(), false); } @@ -39,7 +39,8 @@ function makeAssert(message) { assert(sock4._handle.unrefed(), true); sock4.ref(); assert(sock4._handle.unrefed(), false); - sock4._handle.close(); + sock4._handle.close( + common.mustCall(() => assert(sock4._handle.unrefed(), true))); assert(sock4._handle.unrefed(), false); const sock6 = dgram.createSocket('udp6'); @@ -49,7 +50,8 @@ function makeAssert(message) { assert(sock6._handle.unrefed(), true); sock6.ref(); assert(sock6._handle.unrefed(), false); - sock6._handle.close(); + sock6._handle.close( + common.mustCall(() => assert(sock6._handle.unrefed(), true))); assert(sock6._handle.unrefed(), false); } @@ -65,7 +67,7 @@ function makeAssert(message) { assert(handle.unrefed(), true); handle.ref(); assert(handle.unrefed(), false); - handle.close(); + handle.close(common.mustCall(() => assert(handle.unrefed(), true))); assert(handle.unrefed(), false); } @@ -84,7 +86,8 @@ function makeAssert(message) { server.ref(); assert(server._handle.unrefed(), false); assert(server._unref, false); - server._handle.close(); + server._handle.close( + common.mustCall(() => assert(server._handle.unrefed(), true))); assert(server._handle.unrefed(), false); } @@ -98,6 +101,7 @@ function makeAssert(message) { assert(timer._handle.unrefed(), true); timer.ref(); assert(timer._handle.unrefed(), false); - timer.close(); + timer._handle.close( + common.mustCall(() => assert(timer._handle.unrefed(), true))); assert(timer._handle.unrefed(), false); } From 428240519c3da4c9f75ef47e2259cfc7b33d5c4d Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 26 Apr 2016 13:35:42 +0200 Subject: [PATCH 3/3] test: check that 2nd handle.close() call is a nop Verify that a second call to handle.close() is a no-op. PR-URL: https://github.com/nodejs/node/pull/6395 Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: James M Snell --- test/parallel/test-handle-wrap-isrefed-tty.js | 1 + test/parallel/test-handle-wrap-isrefed.js | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/test/parallel/test-handle-wrap-isrefed-tty.js b/test/parallel/test-handle-wrap-isrefed-tty.js index c40a6b09132a76..3eca28e18a8d29 100644 --- a/test/parallel/test-handle-wrap-isrefed-tty.js +++ b/test/parallel/test-handle-wrap-isrefed-tty.js @@ -20,6 +20,7 @@ if (process.argv[2] === 'child') { tty.unref(); assert(tty._handle.unrefed(), true); tty._handle.close(common.mustCall(() => assert(tty._handle.unrefed(), true))); + tty._handle.close(common.fail); assert(tty._handle.unrefed(), true); return; } diff --git a/test/parallel/test-handle-wrap-isrefed.js b/test/parallel/test-handle-wrap-isrefed.js index d3ea56a46f3911..544729c2210955 100644 --- a/test/parallel/test-handle-wrap-isrefed.js +++ b/test/parallel/test-handle-wrap-isrefed.js @@ -23,6 +23,7 @@ function makeAssert(message) { cp.ref(); assert(cp._handle.unrefed(), false); cp._handle.close(common.mustCall(() => assert(cp._handle.unrefed(), true))); + cp._handle.close(common.fail); assert(cp._handle.unrefed(), false); } @@ -41,6 +42,7 @@ function makeAssert(message) { assert(sock4._handle.unrefed(), false); sock4._handle.close( common.mustCall(() => assert(sock4._handle.unrefed(), true))); + sock4._handle.close(common.fail); assert(sock4._handle.unrefed(), false); const sock6 = dgram.createSocket('udp6'); @@ -52,6 +54,7 @@ function makeAssert(message) { assert(sock6._handle.unrefed(), false); sock6._handle.close( common.mustCall(() => assert(sock6._handle.unrefed(), true))); + sock6._handle.close(common.fail); assert(sock6._handle.unrefed(), false); } @@ -68,6 +71,7 @@ function makeAssert(message) { handle.ref(); assert(handle.unrefed(), false); handle.close(common.mustCall(() => assert(handle.unrefed(), true))); + handle.close(common.fail); assert(handle.unrefed(), false); } @@ -88,6 +92,7 @@ function makeAssert(message) { assert(server._unref, false); server._handle.close( common.mustCall(() => assert(server._handle.unrefed(), true))); + server._handle.close(common.fail); assert(server._handle.unrefed(), false); } @@ -103,5 +108,6 @@ function makeAssert(message) { assert(timer._handle.unrefed(), false); timer._handle.close( common.mustCall(() => assert(timer._handle.unrefed(), true))); + timer._handle.close(common.fail); assert(timer._handle.unrefed(), false); }