From 3ff2122d6e3e323b6e0da0be0cdc02ade30eb075 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 29 Sep 2019 01:29:07 +0200 Subject: [PATCH 1/5] src: introduce custom smart pointers for `BaseObject`s MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Referring to `BaseObject` instances using standard C++ smart pointers can interfere with BaseObject’s own cleanup mechanisms (explicit delete, delete-on-GC and delete-on-cleanup). Introducing custom smart pointers allows referring to `BaseObject`s safely while keeping those mechanisms intact. Refs: https://github.com/nodejs/quic/pull/141 Refs: https://github.com/nodejs/quic/pull/149 Reviewed-By: James M Snell --- node.gyp | 1 + src/base_object-inl.h | 215 ++++++++++++++++++- src/base_object.h | 111 +++++++++- src/env-inl.h | 8 + src/env.cc | 6 + src/env.h | 8 + src/handle_wrap.cc | 14 +- src/handle_wrap.h | 3 +- src/memory_tracker-inl.h | 8 + src/memory_tracker.h | 8 + test/cctest/node_test_fixture.h | 2 +- test/cctest/test_base_object_ptr.cc | 176 +++++++++++++++ test/cctest/test_node_postmortem_metadata.cc | 9 +- 13 files changed, 536 insertions(+), 33 deletions(-) create mode 100644 test/cctest/test_base_object_ptr.cc diff --git a/node.gyp b/node.gyp index d37be3141bd317..7548a4220c5925 100644 --- a/node.gyp +++ b/node.gyp @@ -1099,6 +1099,7 @@ 'test/cctest/node_test_fixture.h', 'test/cctest/test_aliased_buffer.cc', 'test/cctest/test_base64.cc', + 'test/cctest/test_base_object_ptr.cc', 'test/cctest/test_node_postmortem_metadata.cc', 'test/cctest/test_environment.cc', 'test/cctest/test_linked_binding.cc', diff --git a/src/base_object-inl.h b/src/base_object-inl.h index af69084f4a5595..4fc9210b39bf89 100644 --- a/src/base_object-inl.h +++ b/src/base_object-inl.h @@ -32,16 +32,25 @@ namespace node { BaseObject::BaseObject(Environment* env, v8::Local object) - : persistent_handle_(env->isolate(), object), - env_(env) { + : persistent_handle_(env->isolate(), object), env_(env) { CHECK_EQ(false, object.IsEmpty()); CHECK_GT(object->InternalFieldCount(), 0); object->SetAlignedPointerInInternalField(0, static_cast(this)); - env_->AddCleanupHook(DeleteMe, static_cast(this)); + env->AddCleanupHook(DeleteMe, static_cast(this)); + env->modify_base_object_count(1); } BaseObject::~BaseObject() { - RemoveCleanupHook(); + env()->modify_base_object_count(-1); + env()->RemoveCleanupHook(DeleteMe, static_cast(this)); + + if (UNLIKELY(has_pointer_data())) { + PointerData* metadata = pointer_data(); + CHECK_EQ(metadata->strong_ptr_count, 0); + metadata->self = nullptr; + if (metadata->weak_ptr_count == 0) + delete metadata; + } if (persistent_handle_.IsEmpty()) { // This most likely happened because the weak callback below cleared it. @@ -49,7 +58,7 @@ BaseObject::~BaseObject() { } { - v8::HandleScope handle_scope(env_->isolate()); + v8::HandleScope handle_scope(env()->isolate()); object()->SetAlignedPointerInInternalField(0, nullptr); } } @@ -58,20 +67,25 @@ void BaseObject::RemoveCleanupHook() { env_->RemoveCleanupHook(DeleteMe, static_cast(this)); } +void BaseObject::Detach() { + CHECK_GT(pointer_data()->strong_ptr_count, 0); + pointer_data()->is_detached = true; +} + v8::Global& BaseObject::persistent() { return persistent_handle_; } v8::Local BaseObject::object() const { - return PersistentToLocal::Default(env_->isolate(), persistent_handle_); + return PersistentToLocal::Default(env()->isolate(), persistent_handle_); } v8::Local BaseObject::object(v8::Isolate* isolate) const { v8::Local handle = object(); DCHECK_EQ(handle->CreationContext()->GetIsolate(), isolate); - DCHECK_EQ(env_->isolate(), isolate); + DCHECK_EQ(env()->isolate(), isolate); return handle; } @@ -80,7 +94,6 @@ Environment* BaseObject::env() const { return env_; } - BaseObject* BaseObject::FromJSObject(v8::Local obj) { CHECK_GT(obj->InternalFieldCount(), 0); return static_cast(obj->GetAlignedPointerFromInternalField(0)); @@ -94,20 +107,34 @@ T* BaseObject::FromJSObject(v8::Local object) { void BaseObject::MakeWeak() { + if (has_pointer_data()) { + pointer_data()->wants_weak_jsobj = true; + if (pointer_data()->strong_ptr_count > 0) return; + } + persistent_handle_.SetWeak( this, [](const v8::WeakCallbackInfo& data) { - std::unique_ptr obj(data.GetParameter()); + BaseObject* obj = data.GetParameter(); // Clear the persistent handle so that ~BaseObject() doesn't attempt // to mess with internal fields, since the JS object may have // transitioned into an invalid state. // Refs: https://github.com/nodejs/node/issues/18897 obj->persistent_handle_.Reset(); + CHECK_IMPLIES(obj->has_pointer_data(), + obj->pointer_data()->strong_ptr_count == 0); + obj->OnGCCollect(); }, v8::WeakCallbackType::kParameter); } +void BaseObject::OnGCCollect() { + delete this; +} void BaseObject::ClearWeak() { + if (has_pointer_data()) + pointer_data()->wants_weak_jsobj = false; + persistent_handle_.ClearWeak(); } @@ -141,6 +168,176 @@ void BaseObject::InternalFieldSet(v8::Local property, info.This()->SetInternalField(Field, value); } +bool BaseObject::has_pointer_data() const { + return pointer_data_ != nullptr; +} + +BaseObject::PointerData* BaseObject::pointer_data() { + if (!has_pointer_data()) { + PointerData* metadata = new PointerData(); + metadata->wants_weak_jsobj = persistent_handle_.IsWeak(); + metadata->self = this; + pointer_data_ = metadata; + } + CHECK(has_pointer_data()); + return pointer_data_; +} + +void BaseObject::decrease_refcount() { + CHECK(has_pointer_data()); + PointerData* metadata = pointer_data(); + CHECK_GT(metadata->strong_ptr_count, 0); + unsigned int new_refcount = --metadata->strong_ptr_count; + if (new_refcount == 0) { + if (metadata->is_detached) { + delete this; + } else if (metadata->wants_weak_jsobj && !persistent_handle_.IsEmpty()) { + MakeWeak(); + } + } +} + +void BaseObject::increase_refcount() { + unsigned int prev_refcount = pointer_data()->strong_ptr_count++; + if (prev_refcount == 0 && !persistent_handle_.IsEmpty()) + persistent_handle_.ClearWeak(); +} + +template +BaseObject::PointerData* +BaseObjectPtrImpl::pointer_data() const { + if (kIsWeak) { + return data_.pointer_data; + } else { + if (get_base_object() == nullptr) return nullptr; + return get_base_object()->pointer_data(); + } +} + +template +BaseObject* BaseObjectPtrImpl::get_base_object() const { + if (kIsWeak) { + if (pointer_data() == nullptr) return nullptr; + return pointer_data()->self; + } else { + return data_.target; + } +} + +template +BaseObjectPtrImpl::~BaseObjectPtrImpl() { + if (get() == nullptr) return; + if (kIsWeak) { + if (--pointer_data()->weak_ptr_count == 0 && + pointer_data()->self == nullptr) { + delete pointer_data(); + } + } else { + get()->decrease_refcount(); + } +} + +template +BaseObjectPtrImpl::BaseObjectPtrImpl() { + data_.target = nullptr; +} + +template +BaseObjectPtrImpl::BaseObjectPtrImpl(T* target) + : BaseObjectPtrImpl() { + if (target == nullptr) return; + if (kIsWeak) { + data_.pointer_data = target->pointer_data(); + CHECK_NOT_NULL(pointer_data()); + pointer_data()->weak_ptr_count++; + } else { + data_.target = target; + CHECK_NOT_NULL(pointer_data()); + get()->increase_refcount(); + } +} + +template +template +BaseObjectPtrImpl::BaseObjectPtrImpl( + const BaseObjectPtrImpl& other) + : BaseObjectPtrImpl(other.get()) {} + +template +BaseObjectPtrImpl::BaseObjectPtrImpl(const BaseObjectPtrImpl& other) + : BaseObjectPtrImpl(other.get()) {} + +template +template +BaseObjectPtrImpl& BaseObjectPtrImpl::operator=( + const BaseObjectPtrImpl& other) { + if (other.get() == get()) return *this; + this->~BaseObjectPtrImpl(); + return *new (this) BaseObjectPtrImpl(other); +} + +template +BaseObjectPtrImpl& BaseObjectPtrImpl::operator=( + const BaseObjectPtrImpl& other) { + if (other.get() == get()) return *this; + this->~BaseObjectPtrImpl(); + return *new (this) BaseObjectPtrImpl(other); +} + +template +BaseObjectPtrImpl::BaseObjectPtrImpl(BaseObjectPtrImpl&& other) + : data_(other.data_) { + if (kIsWeak) + other.data_.target = nullptr; + else + other.data_.pointer_data = nullptr; +} + +template +BaseObjectPtrImpl& BaseObjectPtrImpl::operator=( + BaseObjectPtrImpl&& other) { + if (&other == this) return *this; + this->~BaseObjectPtrImpl(); + return *new (this) BaseObjectPtrImpl(std::move(other)); +} + +template +void BaseObjectPtrImpl::reset(T* ptr) { + *this = BaseObjectPtrImpl(ptr); +} + +template +T* BaseObjectPtrImpl::get() const { + return static_cast(get_base_object()); +} + +template +T& BaseObjectPtrImpl::operator*() const { + return *get(); +} + +template +T* BaseObjectPtrImpl::operator->() const { + return get(); +} + +template +BaseObjectPtrImpl::operator bool() const { + return get() != nullptr; +} + +template +BaseObjectPtr MakeBaseObject(Args&&... args) { + return BaseObjectPtr(new T(std::forward(args)...)); +} + +template +BaseObjectPtr MakeDetachedBaseObject(Args&&... args) { + BaseObjectPtr target = MakeBaseObject(std::forward(args)...); + target->Detach(); + return target; +} + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/base_object.h b/src/base_object.h index 0b202cd3a51324..38afe11a761e26 100644 --- a/src/base_object.h +++ b/src/base_object.h @@ -31,6 +31,8 @@ namespace node { class Environment; +template +class BaseObjectPtrImpl; class BaseObject : public MemoryRetainer { public: @@ -62,10 +64,12 @@ class BaseObject : public MemoryRetainer { static inline T* FromJSObject(v8::Local object); // Make the `v8::Global` a weak reference and, `delete` this object once - // the JS object has been garbage collected. + // the JS object has been garbage collected and there are no (strong) + // BaseObjectPtr references to it. inline void MakeWeak(); - // Undo `MakeWeak()`, i.e. turn this into a strong reference. + // Undo `MakeWeak()`, i.e. turn this into a strong reference that is a GC + // root and will not be touched by the garbage collector. inline void ClearWeak(); // Utility to create a FunctionTemplate with one internal field (used for @@ -86,11 +90,15 @@ class BaseObject : public MemoryRetainer { // This is a bit of a hack. See the override in async_wrap.cc for details. virtual bool IsDoneInitializing() const; + // Can be used to avoid this object keepling itself alive as a GC root + // indefinitely, for example when this object is owned and deleted by another + // BaseObject once that is torn down. This can only be called when there is + // a BaseObjectPtr to this object. + inline void Detach(); + protected: - // Can be used to avoid the automatic object deletion when the Environment - // exits, for example when this object is owned and deleted by another - // BaseObject at that point. - inline void RemoveCleanupHook(); + inline void RemoveCleanupHook(); // TODO(addaleax): Remove. + virtual inline void OnGCCollect(); private: v8::Local WrappedObject() const override; @@ -103,12 +111,44 @@ class BaseObject : public MemoryRetainer { // refer to `doc/guides/node-postmortem-support.md` friend int GenDebugSymbols(); friend class CleanupHookCallback; + template + friend class BaseObjectPtrImpl; v8::Global persistent_handle_; + + // Metadata that is associated with this BaseObject if there are BaseObjectPtr + // or BaseObjectWeakPtr references to it. + // This object is deleted when the BaseObject itself is destroyed, and there + // are no weak references to it. + struct PointerData { + // Number of BaseObjectPtr instances that refer to this object. If this + // is non-zero, the BaseObject is always a GC root and will not be destroyed + // during cleanup until the count drops to zero again. + unsigned int strong_ptr_count = 0; + // Number of BaseObjectWeakPtr instances that refer to this object. + unsigned int weak_ptr_count = 0; + // Indicates whether MakeWeak() has been called. + bool wants_weak_jsobj = false; + // Indicates whether Detach() has been called. If that is the case, this + // object will be destryoed once the strong pointer count drops to zero. + bool is_detached = false; + // Reference to the original BaseObject. This is used by weak pointers. + BaseObject* self = nullptr; + }; + + inline bool has_pointer_data() const; + // This creates a PointerData struct if none was associated with this + // BaseObject before. + inline PointerData* pointer_data(); + + // Functions that adjust the strong pointer count. + inline void decrease_refcount(); + inline void increase_refcount(); + Environment* env_; + PointerData* pointer_data_ = nullptr; }; - // Global alias for FromJSObject() to avoid churn. template inline T* Unwrap(v8::Local obj) { @@ -124,6 +164,63 @@ inline T* Unwrap(v8::Local obj) { return __VA_ARGS__; \ } while (0) +// Implementation of a generic strong or weak pointer to a BaseObject. +// If strong, this will keep the target BaseObject alive regardless of other +// circumstances such das GC or Environment cleanup. +// If weak, destruction behaviour is not affected, but the pointer will be +// reset to nullptr once the BaseObject is destroyed. +// The API matches std::shared_ptr closely. +template +class BaseObjectPtrImpl final { + public: + inline BaseObjectPtrImpl(); + inline ~BaseObjectPtrImpl(); + inline explicit BaseObjectPtrImpl(T* target); + + // Copy and move constructors. Note that the templated version is not a copy + // or move constructor in the C++ sense of the word, so an identical + // untemplated version is provided. + template + inline BaseObjectPtrImpl(const BaseObjectPtrImpl& other); + inline BaseObjectPtrImpl(const BaseObjectPtrImpl& other); + template + inline BaseObjectPtrImpl& operator=(const BaseObjectPtrImpl& other); + inline BaseObjectPtrImpl& operator=(const BaseObjectPtrImpl& other); + inline BaseObjectPtrImpl(BaseObjectPtrImpl&& other); + inline BaseObjectPtrImpl& operator=(BaseObjectPtrImpl&& other); + + inline void reset(T* ptr = nullptr); + inline T* get() const; + inline T& operator*() const; + inline T* operator->() const; + inline operator bool() const; + + private: + union { + BaseObject* target; // Used for strong pointers. + BaseObject::PointerData* pointer_data; // Used for weak pointers. + } data_; + + inline BaseObject* get_base_object() const; + inline BaseObject::PointerData* pointer_data() const; +}; + +template +using BaseObjectPtr = BaseObjectPtrImpl; +template +using BaseObjectWeakPtr = BaseObjectPtrImpl; + +// Create a BaseObject instance and return a pointer to it. +// This variant leaves the object as a GC root by default. +template +inline BaseObjectPtr MakeBaseObject(Args&&... args); +// Create a BaseObject instance and return a pointer to it. +// This variant detaches the object by default, meaning that the caller fully +// owns it, and once the last BaseObjectPtr to it is destroyed, the object +// itself is also destroyed. +template +inline BaseObjectPtr MakeDetachedBaseObject(Args&&... args); + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/env-inl.h b/src/env-inl.h index ee170b071503f8..c361c8fa63ee49 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -1151,6 +1151,14 @@ void Environment::ForEachBaseObject(T&& iterator) { } } +void Environment::modify_base_object_count(int64_t delta) { + base_object_count_ += delta; +} + +int64_t Environment::base_object_count() const { + return base_object_count_; +} + bool AsyncRequest::is_stopped() const { return stopped_.load(); } diff --git a/src/env.cc b/src/env.cc index c8704cb3be6dce..5f9a6acb461f97 100644 --- a/src/env.cc +++ b/src/env.cc @@ -431,6 +431,8 @@ Environment::~Environment() { addon.Close(); } } + + CHECK_EQ(base_object_count(), 0); } void Environment::InitializeLibuv(bool start_profiler_idle_notifier) { @@ -1088,6 +1090,10 @@ AsyncRequest::~AsyncRequest() { // Not really any better place than env.cc at this moment. void BaseObject::DeleteMe(void* data) { BaseObject* self = static_cast(data); + if (self->has_pointer_data() && + self->pointer_data()->strong_ptr_count > 0) { + return self->Detach(); + } delete self; } diff --git a/src/env.h b/src/env.h index 5f5188d49d6052..a0fc496cce6599 100644 --- a/src/env.h +++ b/src/env.h @@ -1216,6 +1216,12 @@ class Environment : public MemoryRetainer { inline AsyncRequest* thread_stopper() { return &thread_stopper_; } + // The BaseObject count is a debugging helper that makes sure that there are + // no memory leaks caused by BaseObjects staying alive longer than expected + // (in particular, no circular BaseObjectPtr references). + inline void modify_base_object_count(int64_t delta); + inline int64_t base_object_count() const; + #if HAVE_INSPECTOR void set_coverage_connection( std::unique_ptr connection); @@ -1427,6 +1433,8 @@ class Environment : public MemoryRetainer { uint64_t cleanup_hook_counter_ = 0; bool started_cleanup_ = false; + int64_t base_object_count_ = 0; + // A custom async abstraction (a pair of async handle and a state variable) // Used by embedders to shutdown running Node instance. AsyncRequest thread_stopper_; diff --git a/src/handle_wrap.cc b/src/handle_wrap.cc index 888640e9493d8e..32ab06edfc9a50 100644 --- a/src/handle_wrap.cc +++ b/src/handle_wrap.cc @@ -84,14 +84,8 @@ void HandleWrap::Close(Local close_callback) { } -void HandleWrap::MakeWeak() { - persistent().SetWeak( - this, - [](const v8::WeakCallbackInfo& data) { - HandleWrap* handle_wrap = data.GetParameter(); - handle_wrap->persistent().Reset(); - handle_wrap->Close(); - }, v8::WeakCallbackType::kParameter); +void HandleWrap::OnGCCollect() { + Close(); } @@ -121,7 +115,9 @@ HandleWrap::HandleWrap(Environment* env, void HandleWrap::OnClose(uv_handle_t* handle) { - std::unique_ptr wrap { static_cast(handle->data) }; + BaseObjectPtr wrap { static_cast(handle->data) }; + wrap->Detach(); + Environment* env = wrap->env(); HandleScope scope(env->isolate()); Context::Scope context_scope(env->context()); diff --git a/src/handle_wrap.h b/src/handle_wrap.h index fbcea4ae4487f5..612874aa2efb4f 100644 --- a/src/handle_wrap.h +++ b/src/handle_wrap.h @@ -76,14 +76,13 @@ class HandleWrap : public AsyncWrap { static v8::Local GetConstructorTemplate( Environment* env); - void MakeWeak(); // This hides BaseObject::MakeWeak() - protected: HandleWrap(Environment* env, v8::Local object, uv_handle_t* handle, AsyncWrap::ProviderType provider); virtual void OnClose() {} + void OnGCCollect() final; void MarkAsInitialized(); void MarkAsUninitialized(); diff --git a/src/memory_tracker-inl.h b/src/memory_tracker-inl.h index 14635aaf5e84c8..938aba1a7a8d11 100644 --- a/src/memory_tracker-inl.h +++ b/src/memory_tracker-inl.h @@ -119,6 +119,14 @@ void MemoryTracker::TrackField(const char* edge_name, TrackField(edge_name, value.get(), node_name); } +template +void MemoryTracker::TrackField(const char* edge_name, + const BaseObjectPtrImpl& value, + const char* node_name) { + if (value.get() == nullptr) return; + TrackField(edge_name, value.get(), node_name); +} + template void MemoryTracker::TrackField(const char* edge_name, const T& value, diff --git a/src/memory_tracker.h b/src/memory_tracker.h index 3bcbe97c99c4e2..7e39da5ecf6de9 100644 --- a/src/memory_tracker.h +++ b/src/memory_tracker.h @@ -30,6 +30,8 @@ namespace node { class MemoryTracker; class MemoryRetainerNode; +template +class BaseObjectPtrImpl; namespace crypto { class NodeBIO; @@ -138,11 +140,17 @@ class MemoryTracker { inline void TrackField(const char* edge_name, const std::unique_ptr& value, const char* node_name = nullptr); + template inline void TrackField(const char* edge_name, const std::shared_ptr& value, const char* node_name = nullptr); + template + void TrackField(const char* edge_name, + const BaseObjectPtrImpl& value, + const char* node_name = nullptr); + // For containers, the elements will be graphed as grandchildren nodes // if the container is not empty. // By default, we assume the parent count the stack size of the container diff --git a/test/cctest/node_test_fixture.h b/test/cctest/node_test_fixture.h index f6b80c860c1f58..ac0701d0942666 100644 --- a/test/cctest/node_test_fixture.h +++ b/test/cctest/node_test_fixture.h @@ -105,9 +105,9 @@ class NodeTestFixture : public ::testing::Test { } void TearDown() override { + platform->DrainTasks(isolate_); isolate_->Exit(); isolate_->Dispose(); - platform->DrainTasks(isolate_); platform->UnregisterIsolate(isolate_); isolate_ = nullptr; } diff --git a/test/cctest/test_base_object_ptr.cc b/test/cctest/test_base_object_ptr.cc new file mode 100644 index 00000000000000..18e27edba8cd53 --- /dev/null +++ b/test/cctest/test_base_object_ptr.cc @@ -0,0 +1,176 @@ +#include "gtest/gtest.h" +#include "node.h" +#include "base_object-inl.h" +#include "node_test_fixture.h" + +using node::BaseObject; +using node::BaseObjectPtr; +using node::BaseObjectWeakPtr; +using node::Environment; +using node::MakeBaseObject; +using node::MakeDetachedBaseObject; +using v8::HandleScope; +using v8::Isolate; +using v8::Local; +using v8::Object; + +class BaseObjectPtrTest : public EnvironmentTestFixture {}; + +class DummyBaseObject : public BaseObject { + public: + DummyBaseObject(Environment* env, Local obj) : BaseObject(env, obj) {} + + static Local MakeJSObject(Environment* env) { + return BaseObject::MakeLazilyInitializedJSTemplate(env) + ->GetFunction(env->context()).ToLocalChecked() + ->NewInstance(env->context()).ToLocalChecked(); + } + + static BaseObjectPtr NewDetached(Environment* env) { + Local obj = MakeJSObject(env); + return MakeDetachedBaseObject(env, obj); + } + + static BaseObjectPtr New(Environment* env) { + Local obj = MakeJSObject(env); + return MakeBaseObject(env, obj); + } + + SET_NO_MEMORY_INFO() + SET_MEMORY_INFO_NAME(DummyBaseObject) + SET_SELF_SIZE(DummyBaseObject) +}; + +TEST_F(BaseObjectPtrTest, ScopedDetached) { + const HandleScope handle_scope(isolate_); + const Argv argv; + Env env_{handle_scope, argv}; + Environment* env = *env_; + + EXPECT_EQ(env->base_object_count(), 0); + { + BaseObjectPtr ptr = DummyBaseObject::NewDetached(env); + EXPECT_EQ(env->base_object_count(), 1); + } + EXPECT_EQ(env->base_object_count(), 0); +} + +TEST_F(BaseObjectPtrTest, ScopedDetachedWithWeak) { + const HandleScope handle_scope(isolate_); + const Argv argv; + Env env_{handle_scope, argv}; + Environment* env = *env_; + + BaseObjectWeakPtr weak_ptr; + + EXPECT_EQ(env->base_object_count(), 0); + { + BaseObjectPtr ptr = DummyBaseObject::NewDetached(env); + weak_ptr = ptr; + EXPECT_EQ(env->base_object_count(), 1); + } + EXPECT_EQ(weak_ptr.get(), nullptr); + EXPECT_EQ(env->base_object_count(), 0); +} + +TEST_F(BaseObjectPtrTest, Undetached) { + const HandleScope handle_scope(isolate_); + const Argv argv; + Env env_{handle_scope, argv}; + Environment* env = *env_; + + node::AddEnvironmentCleanupHook(isolate_, [](void* arg) { + EXPECT_EQ(static_cast(arg)->base_object_count(), 0); + }, env); + + BaseObjectPtr ptr = DummyBaseObject::New(env); + EXPECT_EQ(env->base_object_count(), 1); +} + +TEST_F(BaseObjectPtrTest, GCWeak) { + const HandleScope handle_scope(isolate_); + const Argv argv; + Env env_{handle_scope, argv}; + Environment* env = *env_; + + BaseObjectWeakPtr weak_ptr; + + { + const HandleScope handle_scope(isolate_); + BaseObjectPtr ptr = DummyBaseObject::New(env); + weak_ptr = ptr; + ptr->MakeWeak(); + + EXPECT_EQ(env->base_object_count(), 1); + EXPECT_EQ(weak_ptr.get(), ptr.get()); + EXPECT_EQ(weak_ptr->persistent().IsWeak(), false); + + ptr.reset(); + } + + EXPECT_EQ(env->base_object_count(), 1); + EXPECT_NE(weak_ptr.get(), nullptr); + EXPECT_EQ(weak_ptr->persistent().IsWeak(), true); + + v8::V8::SetFlagsFromString("--expose-gc"); + isolate_->RequestGarbageCollectionForTesting(Isolate::kFullGarbageCollection); + + EXPECT_EQ(env->base_object_count(), 0); + EXPECT_EQ(weak_ptr.get(), nullptr); +} + +TEST_F(BaseObjectPtrTest, Moveable) { + const HandleScope handle_scope(isolate_); + const Argv argv; + Env env_{handle_scope, argv}; + Environment* env = *env_; + + BaseObjectPtr ptr = DummyBaseObject::NewDetached(env); + EXPECT_EQ(env->base_object_count(), 1); + BaseObjectWeakPtr weak_ptr { ptr }; + EXPECT_EQ(weak_ptr.get(), ptr.get()); + + BaseObjectPtr ptr2 = std::move(ptr); + EXPECT_EQ(weak_ptr.get(), ptr2.get()); + EXPECT_EQ(ptr.get(), nullptr); + + BaseObjectWeakPtr weak_ptr2 = std::move(weak_ptr); + EXPECT_EQ(weak_ptr2.get(), ptr2.get()); + EXPECT_EQ(weak_ptr.get(), nullptr); + EXPECT_EQ(env->base_object_count(), 1); + + ptr2.reset(); + + EXPECT_EQ(weak_ptr2.get(), nullptr); + EXPECT_EQ(env->base_object_count(), 0); +} + +TEST_F(BaseObjectPtrTest, NestedClasses) { + class ObjectWithPtr : public BaseObject { + public: + ObjectWithPtr(Environment* env, Local obj) : BaseObject(env, obj) {} + + BaseObjectPtr ptr1; + BaseObjectPtr ptr2; + + SET_NO_MEMORY_INFO() + SET_MEMORY_INFO_NAME(ObjectWithPtr) + SET_SELF_SIZE(ObjectWithPtr) + }; + + const HandleScope handle_scope(isolate_); + const Argv argv; + Env env_{handle_scope, argv}; + Environment* env = *env_; + + node::AddEnvironmentCleanupHook(isolate_, [](void* arg) { + EXPECT_EQ(static_cast(arg)->base_object_count(), 0); + }, env); + + ObjectWithPtr* obj = + new ObjectWithPtr(env, DummyBaseObject::MakeJSObject(env)); + obj->ptr1 = DummyBaseObject::NewDetached(env); + obj->ptr2 = DummyBaseObject::New(env); + + EXPECT_EQ(env->base_object_count(), 3); +} diff --git a/test/cctest/test_node_postmortem_metadata.cc b/test/cctest/test_node_postmortem_metadata.cc index f33d40eb5c23fe..3fb67ecbca265e 100644 --- a/test/cctest/test_node_postmortem_metadata.cc +++ b/test/cctest/test_node_postmortem_metadata.cc @@ -93,14 +93,13 @@ TEST_F(DebugSymbolsTest, BaseObjectPersistentHandle) { v8::Local object = obj_templ->NewInstance(env.context()).ToLocalChecked(); - DummyBaseObject obj(*env, object); + node::BaseObjectPtr obj = + node::MakeDetachedBaseObject(*env, object); - auto expected = reinterpret_cast(&obj.persistent()); - auto calculated = reinterpret_cast(&obj) + + auto expected = reinterpret_cast(&obj->persistent()); + auto calculated = reinterpret_cast(obj.get()) + nodedbg_offset_BaseObject__persistent_handle___v8_Persistent_v8_Object; EXPECT_EQ(expected, calculated); - - obj.persistent().Reset(); // ~BaseObject() expects an empty handle. } From c371a377e30cf23d6f4658625bfa345f4037e602 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 29 Sep 2019 01:29:16 +0200 Subject: [PATCH 2/5] http2: use custom BaseObject smart pointers Refs: https://github.com/nodejs/quic/pull/141 Reviewed-By: James M Snell --- src/base_object-inl.h | 4 ---- src/base_object.h | 1 - src/node_http2.cc | 38 ++++++++++++++++++-------------------- src/node_http2.h | 18 +++++++++--------- 4 files changed, 27 insertions(+), 34 deletions(-) diff --git a/src/base_object-inl.h b/src/base_object-inl.h index 4fc9210b39bf89..f35cd6734edf0b 100644 --- a/src/base_object-inl.h +++ b/src/base_object-inl.h @@ -63,10 +63,6 @@ BaseObject::~BaseObject() { } } -void BaseObject::RemoveCleanupHook() { - env_->RemoveCleanupHook(DeleteMe, static_cast(this)); -} - void BaseObject::Detach() { CHECK_GT(pointer_data()->strong_ptr_count, 0); pointer_data()->is_detached = true; diff --git a/src/base_object.h b/src/base_object.h index 38afe11a761e26..daf40b7c1eb7b4 100644 --- a/src/base_object.h +++ b/src/base_object.h @@ -97,7 +97,6 @@ class BaseObject : public MemoryRetainer { inline void Detach(); protected: - inline void RemoveCleanupHook(); // TODO(addaleax): Remove. virtual inline void OnGCCollect(); private: diff --git a/src/node_http2.cc b/src/node_http2.cc index f3ef8363e4ebcf..9eea9c257fccb9 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -239,7 +239,6 @@ Http2Session::Http2Settings::Http2Settings(Environment* env, : AsyncWrap(env, obj, PROVIDER_HTTP2SETTINGS), session_(session), startTime_(start_time) { - RemoveCleanupHook(); // This object is owned by the Http2Session. Init(); } @@ -658,8 +657,6 @@ Http2Session::Http2Session(Environment* env, Http2Session::~Http2Session() { CHECK_EQ(flags_ & SESSION_STATE_HAS_SCOPE, 0); Debug(this, "freeing nghttp2 session"); - for (const auto& iter : streams_) - iter.second->session_ = nullptr; nghttp2_session_del(session_); CHECK_EQ(current_nghttp2_memory_, 0); } @@ -767,7 +764,7 @@ void Http2Session::Close(uint32_t code, bool socket_closed) { // If there are outstanding pings, those will need to be canceled, do // so on the next iteration of the event loop to avoid calling out into // javascript since this may be called during garbage collection. - while (std::unique_ptr ping = PopPing()) { + while (BaseObjectPtr ping = PopPing()) { ping->DetachFromSession(); env()->SetImmediate( [ping = std::move(ping)](Environment* env) { @@ -1483,7 +1480,7 @@ void Http2Session::HandlePingFrame(const nghttp2_frame* frame) { Local arg; bool ack = frame->hd.flags & NGHTTP2_FLAG_ACK; if (ack) { - std::unique_ptr ping = PopPing(); + BaseObjectPtr ping = PopPing(); if (!ping) { // PING Ack is unsolicited. Treat as a connection error. The HTTP/2 @@ -1522,7 +1519,7 @@ void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) { // If this is an acknowledgement, we should have an Http2Settings // object for it. - std::unique_ptr settings = PopSettings(); + BaseObjectPtr settings = PopSettings(); if (settings) { settings->Done(true); return; @@ -1982,12 +1979,11 @@ Http2Stream::~Http2Stream() { nghttp2_rcbuf_decref(header.value); } - if (session_ == nullptr) + if (!session_) return; Debug(this, "tearing down stream"); session_->DecrementCurrentSessionMemory(current_headers_length_); session_->RemoveStream(this); - session_ = nullptr; } std::string Http2Stream::diagnostic_name() const { @@ -2189,8 +2185,10 @@ Http2Stream* Http2Stream::SubmitPushPromise(nghttp2_nv* nva, id_, nva, len, nullptr); CHECK_NE(*ret, NGHTTP2_ERR_NOMEM); Http2Stream* stream = nullptr; - if (*ret > 0) - stream = Http2Stream::New(session_, *ret, NGHTTP2_HCAT_HEADERS, options); + if (*ret > 0) { + stream = Http2Stream::New( + session_.get(), *ret, NGHTTP2_HCAT_HEADERS, options); + } return stream; } @@ -2855,7 +2853,8 @@ void Http2Session::Ping(const FunctionCallbackInfo& args) { if (obj->Set(env->context(), env->ondone_string(), args[1]).IsNothing()) return; - Http2Ping* ping = session->AddPing(std::make_unique(session, obj)); + Http2Ping* ping = session->AddPing( + MakeDetachedBaseObject(session, obj)); // To prevent abuse, we strictly limit the number of unacknowledged PING // frames that may be sent at any given time. This is configurable in the // Options when creating a Http2Session. @@ -2884,16 +2883,16 @@ void Http2Session::Settings(const FunctionCallbackInfo& args) { if (obj->Set(env->context(), env->ondone_string(), args[0]).IsNothing()) return; - Http2Session::Http2Settings* settings = session->AddSettings( - std::make_unique(session->env(), session, obj, 0)); + Http2Settings* settings = session->AddSettings( + MakeDetachedBaseObject(session->env(), session, obj, 0)); if (settings == nullptr) return args.GetReturnValue().Set(false); settings->Send(); args.GetReturnValue().Set(true); } -std::unique_ptr Http2Session::PopPing() { - std::unique_ptr ping; +BaseObjectPtr Http2Session::PopPing() { + BaseObjectPtr ping; if (!outstanding_pings_.empty()) { ping = std::move(outstanding_pings_.front()); outstanding_pings_.pop(); @@ -2903,7 +2902,7 @@ std::unique_ptr Http2Session::PopPing() { } Http2Session::Http2Ping* Http2Session::AddPing( - std::unique_ptr ping) { + BaseObjectPtr ping) { if (outstanding_pings_.size() == max_outstanding_pings_) { ping->Done(false); return nullptr; @@ -2914,8 +2913,8 @@ Http2Session::Http2Ping* Http2Session::AddPing( return ptr; } -std::unique_ptr Http2Session::PopSettings() { - std::unique_ptr settings; +BaseObjectPtr Http2Session::PopSettings() { + BaseObjectPtr settings; if (!outstanding_settings_.empty()) { settings = std::move(outstanding_settings_.front()); outstanding_settings_.pop(); @@ -2925,7 +2924,7 @@ std::unique_ptr Http2Session::PopSettings() { } Http2Session::Http2Settings* Http2Session::AddSettings( - std::unique_ptr settings) { + BaseObjectPtr settings) { if (outstanding_settings_.size() == max_outstanding_settings_) { settings->Done(false); return nullptr; @@ -2940,7 +2939,6 @@ Http2Session::Http2Ping::Http2Ping(Http2Session* session, Local obj) : AsyncWrap(session->env(), obj, AsyncWrap::PROVIDER_HTTP2PING), session_(session), startTime_(uv_hrtime()) { - RemoveCleanupHook(); // This object is owned by the Http2Session. } void Http2Session::Http2Ping::Send(const uint8_t* payload) { diff --git a/src/node_http2.h b/src/node_http2.h index 6aeb69fa848827..1444738470f9c7 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -456,8 +456,8 @@ class Http2Stream : public AsyncWrap, nghttp2_stream* operator*(); - Http2Session* session() { return session_; } - const Http2Session* session() const { return session_; } + Http2Session* session() { return session_.get(); } + const Http2Session* session() const { return session_.get(); } void EmitStatistics(); @@ -609,7 +609,7 @@ class Http2Stream : public AsyncWrap, nghttp2_headers_category category, int options); - Http2Session* session_ = nullptr; // The Parent HTTP/2 Session + BaseObjectWeakPtr session_; // The Parent HTTP/2 Session int32_t id_ = 0; // The Stream Identifier int32_t code_ = NGHTTP2_NO_ERROR; // The RST_STREAM code (if any) int flags_ = NGHTTP2_STREAM_FLAG_NONE; // Internal state flags @@ -822,11 +822,11 @@ class Http2Session : public AsyncWrap, public StreamListener { return env()->event_loop(); } - std::unique_ptr PopPing(); - Http2Ping* AddPing(std::unique_ptr ping); + BaseObjectPtr PopPing(); + Http2Ping* AddPing(BaseObjectPtr ping); - std::unique_ptr PopSettings(); - Http2Settings* AddSettings(std::unique_ptr settings); + BaseObjectPtr PopSettings(); + Http2Settings* AddSettings(BaseObjectPtr settings); void IncrementCurrentSessionMemory(uint64_t amount) { current_session_memory_ += amount; @@ -1001,10 +1001,10 @@ class Http2Session : public AsyncWrap, public StreamListener { size_t stream_buf_offset_ = 0; size_t max_outstanding_pings_ = DEFAULT_MAX_PINGS; - std::queue> outstanding_pings_; + std::queue> outstanding_pings_; size_t max_outstanding_settings_ = DEFAULT_MAX_SETTINGS; - std::queue> outstanding_settings_; + std::queue> outstanding_settings_; std::vector outgoing_buffers_; std::vector outgoing_storage_; From 697e719c8424dc1b98d6f99710eecaa02a3bfcf5 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 12 Nov 2019 13:47:08 +0000 Subject: [PATCH 3/5] src: use BaseObjectPtr for keeping channel alive in dns bindings --- src/cares_wrap.cc | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index 96062cb48199e3..ee521ce64a05d6 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -575,10 +575,6 @@ class QueryWrap : public AsyncWrap { : AsyncWrap(channel->env(), req_wrap_obj, AsyncWrap::PROVIDER_QUERYWRAP), channel_(channel), trace_name_(name) { - // Make sure the channel object stays alive during the query lifetime. - req_wrap_obj->Set(env()->context(), - env()->channel_string(), - channel->object()).Check(); } ~QueryWrap() override { @@ -735,7 +731,7 @@ class QueryWrap : public AsyncWrap { UNREACHABLE(); } - ChannelWrap* channel_; + BaseObjectPtr channel_; private: std::unique_ptr response_data_; From 82f06be40d89617f2d06de43161611d338fe8141 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 12 Nov 2019 14:01:08 +0000 Subject: [PATCH 4/5] src: remove keep alive option from SetImmediate() This is no longer necessary now that the copyable `BaseObjectPtr` is available (as opposed to the only-movable `v8::Global`). --- src/cares_wrap.cc | 10 ++++++---- src/env-inl.h | 21 ++++++++------------- src/env.h | 17 ++++------------- src/node_http2.cc | 16 ++++++++++------ src/stream_pipe.cc | 3 ++- src/tls_wrap.cc | 15 +++++++++------ 6 files changed, 39 insertions(+), 43 deletions(-) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index ee521ce64a05d6..1fb0f47dd80f08 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -627,8 +627,6 @@ class QueryWrap : public AsyncWrap { } else { Parse(response_data_->host.get()); } - - delete this; } void* MakeCallbackPointer() { @@ -686,9 +684,13 @@ class QueryWrap : public AsyncWrap { } void QueueResponseCallback(int status) { - env()->SetImmediate([this](Environment*) { + BaseObjectPtr strong_ref{this}; + env()->SetImmediate([this, strong_ref](Environment*) { AfterResponse(); - }, object()); + + // Delete once strong_ref goes out of scope. + Detach(); + }); channel_->set_query_last_ok(status != ARES_ECONNREFUSED); channel_->ModifyActivityQueryCount(-1); diff --git a/src/env-inl.h b/src/env-inl.h index c361c8fa63ee49..15b5010deb7c90 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -746,13 +746,9 @@ inline void IsolateData::set_options( } template -void Environment::CreateImmediate(Fn&& cb, - v8::Local keep_alive, - bool ref) { +void Environment::CreateImmediate(Fn&& cb, bool ref) { auto callback = std::make_unique>( - std::move(cb), - v8::Global(isolate(), keep_alive), - ref); + std::move(cb), ref); NativeImmediateCallback* prev_tail = native_immediate_callbacks_tail_; native_immediate_callbacks_tail_ = callback.get(); @@ -765,8 +761,8 @@ void Environment::CreateImmediate(Fn&& cb, } template -void Environment::SetImmediate(Fn&& cb, v8::Local keep_alive) { - CreateImmediate(std::move(cb), keep_alive, true); +void Environment::SetImmediate(Fn&& cb) { + CreateImmediate(std::move(cb), true); if (immediate_info()->ref_count() == 0) ToggleImmediateRef(true); @@ -774,8 +770,8 @@ void Environment::SetImmediate(Fn&& cb, v8::Local keep_alive) { } template -void Environment::SetUnrefImmediate(Fn&& cb, v8::Local keep_alive) { - CreateImmediate(std::move(cb), keep_alive, false); +void Environment::SetUnrefImmediate(Fn&& cb) { + CreateImmediate(std::move(cb), false); } Environment::NativeImmediateCallback::NativeImmediateCallback(bool refed) @@ -797,10 +793,9 @@ void Environment::NativeImmediateCallback::set_next( template Environment::NativeImmediateCallbackImpl::NativeImmediateCallbackImpl( - Fn&& callback, v8::Global&& keep_alive, bool refed) + Fn&& callback, bool refed) : NativeImmediateCallback(refed), - callback_(std::move(callback)), - keep_alive_(std::move(keep_alive)) {} + callback_(std::move(callback)) {} template void Environment::NativeImmediateCallbackImpl::Call(Environment* env) { diff --git a/src/env.h b/src/env.h index a0fc496cce6599..2df49a24a15255 100644 --- a/src/env.h +++ b/src/env.h @@ -1183,13 +1183,9 @@ class Environment : public MemoryRetainer { // cb will be called as cb(env) on the next event loop iteration. // keep_alive will be kept alive between now and after the callback has run. template - inline void SetImmediate(Fn&& cb, - v8::Local keep_alive = - v8::Local()); + inline void SetImmediate(Fn&& cb); template - inline void SetUnrefImmediate(Fn&& cb, - v8::Local keep_alive = - v8::Local()); + inline void SetUnrefImmediate(Fn&& cb); // This needs to be available for the JS-land setImmediate(). void ToggleImmediateRef(bool ref); @@ -1260,9 +1256,7 @@ class Environment : public MemoryRetainer { private: template - inline void CreateImmediate(Fn&& cb, - v8::Local keep_alive, - bool ref); + inline void CreateImmediate(Fn&& cb, bool ref); inline void ThrowError(v8::Local (*fun)(v8::Local), const char* errmsg); @@ -1410,14 +1404,11 @@ class Environment : public MemoryRetainer { template class NativeImmediateCallbackImpl final : public NativeImmediateCallback { public: - NativeImmediateCallbackImpl(Fn&& callback, - v8::Global&& keep_alive, - bool refed); + NativeImmediateCallbackImpl(Fn&& callback, bool refed); void Call(Environment* env) override; private: Fn callback_; - v8::Global keep_alive_; }; std::unique_ptr native_immediate_callbacks_head_; diff --git a/src/node_http2.cc b/src/node_http2.cc index 9eea9c257fccb9..7170907ced1b3e 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1582,7 +1582,8 @@ void Http2Session::MaybeScheduleWrite() { HandleScope handle_scope(env()->isolate()); Debug(this, "scheduling write"); flags_ |= SESSION_STATE_WRITE_SCHEDULED; - env()->SetImmediate([this](Environment* env) { + BaseObjectPtr strong_ref{this}; + env()->SetImmediate([this, strong_ref](Environment* env) { if (session_ == nullptr || !(flags_ & SESSION_STATE_WRITE_SCHEDULED)) { // This can happen e.g. when a stream was reset before this turn // of the event loop, in which case SendPendingData() is called early, @@ -1595,7 +1596,7 @@ void Http2Session::MaybeScheduleWrite() { HandleScope handle_scope(env->isolate()); InternalCallbackScope callback_scope(this); SendPendingData(); - }, object()); + }); } } @@ -2043,7 +2044,8 @@ void Http2Stream::Destroy() { // Wait until the start of the next loop to delete because there // may still be some pending operations queued for this stream. - env()->SetImmediate([this](Environment* env) { + BaseObjectPtr strong_ref{this}; + env()->SetImmediate([this, strong_ref](Environment* env) { // Free any remaining outgoing data chunks here. This should be done // here because it's possible for destroy to have been called while // we still have queued outbound writes. @@ -2057,9 +2059,11 @@ void Http2Stream::Destroy() { // We can destroy the stream now if there are no writes for it // already on the socket. Otherwise, we'll wait for the garbage collector // to take care of cleaning up. - if (session() == nullptr || !session()->HasWritesOnSocketForStream(this)) - delete this; - }, object()); + if (session() == nullptr || !session()->HasWritesOnSocketForStream(this)) { + // Delete once strong_ref goes out of scope. + Detach(); + } + }); statistics_.end_time = uv_hrtime(); session_->statistics_.stream_average_duration = diff --git a/src/stream_pipe.cc b/src/stream_pipe.cc index 832a20d324f0ea..6e339378ceb374 100644 --- a/src/stream_pipe.cc +++ b/src/stream_pipe.cc @@ -71,6 +71,7 @@ void StreamPipe::Unpipe() { // Delay the JS-facing part with SetImmediate, because this might be from // inside the garbage collector, so we can’t run JS here. HandleScope handle_scope(env()->isolate()); + BaseObjectPtr strong_ref{this}; env()->SetImmediate([this](Environment* env) { HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); @@ -105,7 +106,7 @@ void StreamPipe::Unpipe() { .IsNothing()) { return; } - }, object()); + }); } uv_buf_t StreamPipe::ReadableListener::OnStreamAlloc(size_t suggested_size) { diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 42b9469e38189f..4ec6dda6df70d7 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -316,9 +316,10 @@ void TLSWrap::EncOut() { // its not clear if it is always correct. Not calling Done() could block // data flow, so for now continue to call Done(), just do it in the next // tick. - env()->SetImmediate([this](Environment* env) { + BaseObjectPtr strong_ref{this}; + env()->SetImmediate([this, strong_ref](Environment* env) { InvokeQueued(0); - }, object()); + }); } } return; @@ -349,9 +350,10 @@ void TLSWrap::EncOut() { HandleScope handle_scope(env()->isolate()); // Simulate asynchronous finishing, TLS cannot handle this at the moment. - env()->SetImmediate([this](Environment* env) { + BaseObjectPtr strong_ref{this}; + env()->SetImmediate([this, strong_ref](Environment* env) { OnStreamAfterWrite(nullptr, 0); - }, object()); + }); } } @@ -718,9 +720,10 @@ int TLSWrap::DoWrite(WriteWrap* w, StreamWriteResult res = underlying_stream()->Write(bufs, count, send_handle); if (!res.async) { - env()->SetImmediate([this](Environment* env) { + BaseObjectPtr strong_ref{this}; + env()->SetImmediate([this, strong_ref](Environment* env) { OnStreamAfterWrite(current_empty_write_, 0); - }, object()); + }); } return 0; } From 360b843d040c568a03264aa5548a1627731fca93 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 12 Oct 2019 01:11:11 +0200 Subject: [PATCH 5/5] src: remove HandleWrap instances from list once closed This allows keeping `BaseObjectPtr`s to `HandleWrap` instances. Previously, the pointer kept the `HandleWrap` object alive, leaving the Environment cleanup code that waits for the handle list to drain in a busy loop, because only the `HandleWrap` destructor removed the item from the list. Refs: https://github.com/nodejs/quic/pull/165 Reviewed-By: James M Snell Reviewed-By: Daniel Bevenius --- src/handle_wrap.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/handle_wrap.cc b/src/handle_wrap.cc index 32ab06edfc9a50..5979ff7877a05f 100644 --- a/src/handle_wrap.cc +++ b/src/handle_wrap.cc @@ -127,6 +127,7 @@ void HandleWrap::OnClose(uv_handle_t* handle) { wrap->state_ = kClosed; wrap->OnClose(); + wrap->handle_wrap_queue_.Remove(); if (!wrap->persistent().IsEmpty() && wrap->object()->Has(env->context(), env->handle_onclose_symbol())