From f787f883c91fa6142fbd97735a08e18aea62b826 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 25 Aug 2019 19:35:22 +0200 Subject: [PATCH 1/9] 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: https://github.com/nodejs/node/pull/29317 Reviewed-By: James M Snell --- src/async_wrap.h | 1 + src/handle_wrap.cc | 16 ++++++-- src/handle_wrap.h | 2 + src/node_perf.cc | 39 +++++++------------ src/node_perf.h | 8 ++-- test/sequential/test-async-wrap-getasyncid.js | 1 + 6 files changed, 33 insertions(+), 34 deletions(-) diff --git a/src/async_wrap.h b/src/async_wrap.h index a76c856fc5..9aacf57c54 100644 --- 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 index cf32aa78cf..55988867a4 100644 --- a/src/handle_wrap.cc +++ b/src/handle_wrap.cc @@ -84,6 +84,17 @@ 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::MarkAsInitialized() { env()->handle_wrap_queue()->PushBack(this); state_ = kInitialized; @@ -119,15 +130,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 index e045ea79a4..e126726f79 100644 --- a/src/handle_wrap.h +++ b/src/handle_wrap.h @@ -76,6 +76,8 @@ class HandleWrap : public AsyncWrap { static v8::Local GetConstructorTemplate( Environment* env); + void MakeWeak(); // This hides BaseObject::MakeWeak() + protected: HandleWrap(Environment* env, v8::Local object, diff --git a/src/node_perf.cc b/src/node_perf.cc index 3efaca6065..5d907bf7af 100644 --- a/src/node_perf.cc +++ b/src/node_perf.cc @@ -467,31 +467,18 @@ static void ELDHistogramNew(const FunctionCallbackInfo& args) { ELDHistogram::ELDHistogram( Environment* env, Local wrap, - int32_t resolution) : BaseObject(env, wrap), + int32_t resolution) : HandleWrap(env, + wrap, + reinterpret_cast(&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(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()); @@ -527,21 +514,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(timer_)); + uv_unref(reinterpret_cast(&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 index 4c7585d65f..e8441e3bb7 100644 --- 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 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 index b2683a8b68..0c664df42f 100644 --- a/test/sequential/test-async-wrap-getasyncid.js +++ b/test/sequential/test-async-wrap-getasyncid.js @@ -55,6 +55,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) From 1af37034426399df5bd8c5c5d6c9992c5305ae85 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 21 Sep 2019 02:15:32 +0200 Subject: [PATCH 2/9] src: fix closing weak `HandleWrap`s on GC In 0af62aae07ccbb3783030367ffe4, this was overlooked, with it possibly leading to hard crashes. Refs: https://github.com/nodejs/node/pull/29317 PR-URL: https://github.com/nodejs/node/pull/29640 Reviewed-By: Ben Coe Reviewed-By: Rich Trott --- src/handle_wrap.cc | 4 ++-- test/sequential/test-performance-eventloopdelay.js | 5 +++++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/handle_wrap.cc b/src/handle_wrap.cc index 55988867a4..26107d6e51 100644 --- a/src/handle_wrap.cc +++ b/src/handle_wrap.cc @@ -72,11 +72,11 @@ void HandleWrap::Close(Local close_callback) { if (state_ != kInitialized) return; - CHECK_EQ(false, persistent().IsEmpty()); uv_close(handle_, OnClose); state_ = kClosing; - if (!close_callback.IsEmpty() && close_callback->IsFunction()) { + if (!close_callback.IsEmpty() && close_callback->IsFunction() && + !persistent().IsEmpty()) { object()->Set(env()->context(), env()->handle_onclose_symbol(), close_callback).Check(); diff --git a/test/sequential/test-performance-eventloopdelay.js b/test/sequential/test-performance-eventloopdelay.js index 82f47b6fb2..7b9527b386 100644 --- a/test/sequential/test-performance-eventloopdelay.js +++ b/test/sequential/test-performance-eventloopdelay.js @@ -1,3 +1,4 @@ +// Flags: --expose-gc 'use strict'; const common = require('../common'); @@ -97,3 +98,7 @@ const { } spinAWhile(); } + +// Make sure that the histogram instances can be garbage-collected without +// and not just implictly destroyed when the Environment is torn down. +process.on('exit', global.gc); From aef2c95f4ac199c1cd99f030fbcae397c34aca34 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 29 Sep 2019 01:29:07 +0200 Subject: [PATCH 3/9] 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. --- node.gyp | 1 + src/base_object-inl.h | 198 ++++++++++++++++++- src/base_object.h | 110 ++++++++++- 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 | 6 + test/cctest/node_test_fixture.h | 2 +- test/cctest/test_base_object_ptr.cc | 118 +++++++++++ test/cctest/test_node_postmortem_metadata.cc | 9 +- 13 files changed, 458 insertions(+), 33 deletions(-) create mode 100644 test/cctest/test_base_object_ptr.cc diff --git a/node.gyp b/node.gyp index 6429e9a197..12beec99e7 100644 --- a/node.gyp +++ b/node.gyp @@ -1127,6 +1127,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 af69084f4a..b8614988db 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,159 @@ 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) { + MakeWeak(); + } + } +} + +void BaseObject::increase_refcount() { + unsigned int prev_refcount = pointer_data()->strong_ptr_count++; + if (prev_refcount == 0) + 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 +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 0b202cd3a5..7f7ea53988 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,62 @@ 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 constructors. Note that the templated version is not a copy + // constructor in the C++ sense of the word, so an identical untemplated + // version is provided. + // TODO(addaleax): Add move variants. + 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 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 d6d80b3045..979a2f978e 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -1156,6 +1156,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 257bf78519..d642f0e15f 100644 --- a/src/env.cc +++ b/src/env.cc @@ -430,6 +430,8 @@ Environment::~Environment() { addon.Close(); } } + + CHECK_EQ(base_object_count(), 0); } void Environment::InitializeLibuv(bool start_profiler_idle_notifier) { @@ -1071,6 +1073,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 d1b719405e..f9136c3345 100644 --- a/src/env.h +++ b/src/env.h @@ -1264,6 +1264,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); @@ -1473,6 +1479,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 26107d6e51..bd9a030368 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(); } @@ -125,7 +119,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 e126726f79..dd9890d33d 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 da37f72c73..4a01a174aa 100644 --- a/src/memory_tracker-inl.h +++ b/src/memory_tracker-inl.h @@ -109,6 +109,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 d22116918a..99af4c458e 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,6 +140,10 @@ class MemoryTracker { inline void TrackField(const char* edge_name, const std::unique_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. diff --git a/test/cctest/node_test_fixture.h b/test/cctest/node_test_fixture.h index f6b80c860c..ac0701d094 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 0000000000..61759440d0 --- /dev/null +++ b/test/cctest/test_base_object_ptr.cc @@ -0,0 +1,118 @@ +#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 BaseObjectPtr NewDetached(Environment* env) { + Local obj = BaseObject::MakeLazilyInitializedJSTemplate(env) + ->GetFunction(env->context()).ToLocalChecked() + ->NewInstance(env->context()).ToLocalChecked(); + return MakeDetachedBaseObject(env, obj); + } + + static BaseObjectPtr New(Environment* env) { + Local obj = BaseObject::MakeLazilyInitializedJSTemplate(env) + ->GetFunction(env->context()).ToLocalChecked() + ->NewInstance(env->context()).ToLocalChecked(); + 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); +} diff --git a/test/cctest/test_node_postmortem_metadata.cc b/test/cctest/test_node_postmortem_metadata.cc index 79b766939b..af5376ec33 100644 --- a/test/cctest/test_node_postmortem_metadata.cc +++ b/test/cctest/test_node_postmortem_metadata.cc @@ -91,14 +91,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 9a3f0dd60ee97b435c975bd2296dee675174c65b Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 29 Sep 2019 01:29:16 +0200 Subject: [PATCH 4/9] http2: use custom BaseObject smart pointers --- 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 b8614988db..1157b19ba0 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 7f7ea53988..dec93944c6 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 7728165b48..8762404224 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -240,7 +240,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(); } @@ -575,8 +574,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); } @@ -682,7 +679,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) { @@ -1394,7 +1391,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 @@ -1433,7 +1430,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; @@ -1892,12 +1889,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 { @@ -2099,8 +2095,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; } @@ -2765,7 +2763,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. @@ -2794,16 +2793,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(); @@ -2813,7 +2812,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; @@ -2824,8 +2823,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(); @@ -2835,7 +2834,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; @@ -2850,7 +2849,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 c27e3dba4b..8e21617c11 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 @@ -828,11 +828,11 @@ class Http2Session : public AsyncWrap, 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; @@ -1007,10 +1007,10 @@ class Http2Session : public AsyncWrap, 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 6eccf461725b43ea5b71a1baa9716061864d58cc Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 29 Sep 2019 01:29:29 +0200 Subject: [PATCH 5/9] quic: use custom BaseObject smart pointers --- src/node_quic_session-inl.h | 13 ++-- src/node_quic_session.cc | 118 +++++++++++++++++++----------------- src/node_quic_session.h | 17 +++--- src/node_quic_socket.cc | 24 ++++---- src/node_quic_socket.h | 14 ++--- src/node_quic_stream.cc | 15 ++--- src/node_quic_stream.h | 12 ++-- 7 files changed, 109 insertions(+), 104 deletions(-) diff --git a/src/node_quic_session-inl.h b/src/node_quic_session-inl.h index 4a90a76f42..11f0b89f46 100644 --- a/src/node_quic_session-inl.h +++ b/src/node_quic_session-inl.h @@ -4,6 +4,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS #include "node_quic_session.h" +#include "node_quic_socket.h" #include @@ -47,14 +48,6 @@ bool QuicSession::IsInDrainingPeriod() { return ngtcp2_conn_is_in_draining_period(Connection()); } -// Locate the QuicStream with the given id or return nullptr -QuicStream* QuicSession::FindStream(int64_t id) { - auto it = streams_.find(id); - if (it == std::end(streams_)) - return nullptr; - return it->second.get(); -} - bool QuicSession::HasStream(int64_t id) { return streams_.find(id) != std::end(streams_); } @@ -74,6 +67,10 @@ void QuicSession::StartGracefulClose() { session_stats_.closing_at = uv_hrtime(); } +QuicSocket* QuicSession::Socket() const { + return socket_.get(); +} + } // namespace quic } // namespace node diff --git a/src/node_quic_session.cc b/src/node_quic_session.cc index 9812f56698..63ddb3f716 100644 --- a/src/node_quic_session.cc +++ b/src/node_quic_session.cc @@ -284,6 +284,14 @@ std::string QuicSession::diagnostic_name() const { " (" + std::to_string(static_cast(get_async_id())) + ")"; } +// Locate the QuicStream with the given id or return nullptr +QuicStream* QuicSession::FindStream(int64_t id) { + auto it = streams_.find(id); + if (it == std::end(streams_)) + return nullptr; + return it->second.get(); +} + void QuicSession::AckedCryptoOffset(size_t datalen) { // It is possible for the QuicSession to have been destroyed but not yet // deconstructed. In such cases, we want to ignore the callback as there @@ -331,7 +339,7 @@ void QuicSession::AckedStreamDataOffset( // Add the given QuicStream to this QuicSession's collection of streams. All // streams added must be removed before the QuicSession instance is freed. -void QuicSession::AddStream(std::shared_ptr stream) { +void QuicSession::AddStream(BaseObjectPtr stream) { DCHECK(!IsFlagSet(QUICSESSION_FLAG_GRACEFUL_CLOSING)); Debug(this, "Adding stream %" PRId64 " to session.", stream->GetID()); streams_.emplace(stream->GetID(), stream); @@ -400,7 +408,7 @@ void QuicSession::ImmediateClose() { // Grab a shared pointer to this to prevent the QuicSession // from being freed while the MakeCallback is running. - std::shared_ptr ptr(shared_from_this()); + BaseObjectPtr ptr(this); MakeCallback(env()->quic_on_session_close_function(), arraysize(argv), argv); } @@ -411,8 +419,8 @@ QuicStream* QuicSession::CreateStream(int64_t stream_id) { CHECK(!IsFlagSet(QUICSESSION_FLAG_GRACEFUL_CLOSING)); CHECK(!IsFlagSet(QUICSESSION_FLAG_CLOSING)); - std::shared_ptr stream = QuicStream::New(this, stream_id); - CHECK_NOT_NULL(stream); + BaseObjectPtr stream = QuicStream::New(this, stream_id); + CHECK(stream); Local argv[] = { stream->object(), Number::New(env()->isolate(), static_cast(stream_id)) @@ -420,7 +428,7 @@ QuicStream* QuicSession::CreateStream(int64_t stream_id) { // Grab a shared pointer to this to prevent the QuicSession // from being freed while the MakeCallback is running. - std::shared_ptr ptr(shared_from_this()); + BaseObjectPtr ptr(this); MakeCallback(env()->quic_on_stream_ready_function(), arraysize(argv), argv); return stream.get(); } @@ -462,7 +470,7 @@ void QuicSession::Destroy() { StopRetransmitTimer(); // The QuicSession instances are kept alive using - // std::shared_ptr. The only persistent shared_ptr + // BaseObjectPtr. The only persistent BaseObjectPtr // is the map in the associated QuicSocket. Removing // the QuicSession from the QuicSocket will free // that pointer, allowing the QuicSession to be @@ -726,7 +734,7 @@ void QuicSession::HandshakeCompleted() { // Grab a shared pointer to this to prevent the QuicSession // from being freed while the MakeCallback is running. - std::shared_ptr ptr(shared_from_this()); + BaseObjectPtr ptr(this); MakeCallback(env()->quic_on_session_handshake_function(), arraysize(argv), argv); @@ -789,7 +797,7 @@ void QuicSession::Keylog(const char* line) { // Grab a shared pointer to this to prevent the QuicSession // from being freed while the MakeCallback is running. - std::shared_ptr ptr(shared_from_this()); + BaseObjectPtr ptr(this); MakeCallback(env()->quic_on_session_keylog_function(), 1, &line_bf); } @@ -966,7 +974,7 @@ void QuicSession::PathValidation( }; // Grab a shared pointer to this to prevent the QuicSession // from being freed while the MakeCallback is running. - std::shared_ptr ptr(shared_from_this()); + BaseObjectPtr ptr(this); MakeCallback( env()->quic_on_session_path_validation_function(), arraysize(argv), @@ -1244,7 +1252,7 @@ void QuicSession::RemoveConnectionID(const ngtcp2_cid* cid) { // Removes the QuicSession from the current socket. This is // done with when the session is being destroyed or being // migrated to another QuicSocket. It is important to keep in mind -// that the QuicSocket uses a shared_ptr for the QuicSession. +// that the QuicSocket uses a BaseObjectPtr for the QuicSession. // If the session is removed and there are no other references held, // the session object will be destroyed automatically. void QuicSession::RemoveFromSocket() { @@ -1259,7 +1267,7 @@ void QuicSession::RemoveFromSocket() { Debug(this, "Removed from the QuicSocket."); QuicCID scid(scid_); socket_->RemoveSession(&scid, **GetRemoteAddress()); - socket_ = nullptr; + socket_.reset(); } // Removes the given stream from the QuicSession. All streams must @@ -1472,7 +1480,7 @@ bool QuicSession::SendPacket(const char* diagnostic_label) { int err = Socket()->SendPacket( *remote_address_, &txbuf_, - shared_from_this(), + BaseObjectPtr(this), diagnostic_label); if (err != 0) { SetLastError(QUIC_ERROR_SESSION, err); @@ -1597,7 +1605,7 @@ void QuicSession::SilentClose(bool stateless_reset) { // Grab a shared pointer to this to prevent the QuicSession // from being freed while the MakeCallback is running. - std::shared_ptr ptr(shared_from_this()); + BaseObjectPtr ptr(this); MakeCallback( env()->quic_on_session_silent_close_function(), arraysize(argv), argv); } @@ -1625,7 +1633,7 @@ void QuicSession::StreamClose(int64_t stream_id, uint64_t app_error_code) { // Grab a shared pointer to this to prevent the QuicSession // from being freed while the MakeCallback is running. - std::shared_ptr ptr(shared_from_this()); + BaseObjectPtr ptr(this); MakeCallback(env()->quic_on_stream_close_function(), arraysize(argv), argv); } @@ -1699,7 +1707,7 @@ void QuicSession::StreamReset( }; // Grab a shared pointer to this to prevent the QuicSession // from being freed while the MakeCallback is running. - std::shared_ptr ptr(shared_from_this()); + BaseObjectPtr ptr(this); MakeCallback(env()->quic_on_stream_reset_function(), arraysize(argv), argv); } @@ -1924,7 +1932,7 @@ QuicServerSession::QuicServerSession( Init(config, addr, dcid, ocid, version); } -std::shared_ptr QuicServerSession::New( +BaseObjectPtr QuicServerSession::New( QuicSocket* socket, QuicSessionConfig* config, const ngtcp2_cid* rcid, @@ -1941,18 +1949,19 @@ std::shared_ptr QuicServerSession::New( ->NewInstance(socket->env()->context()).ToLocal(&obj)) { return {}; } - std::shared_ptr session { new QuicServerSession( - socket, - config, - obj, - rcid, - addr, - dcid, - ocid, - version, - alpn, - options, - initial_connection_close) }; + BaseObjectPtr session = + MakeDetachedBaseObject( + socket, + config, + obj, + rcid, + addr, + dcid, + ocid, + version, + alpn, + options, + initial_connection_close); session->AddToSocket(socket); return session; @@ -1962,7 +1971,7 @@ std::shared_ptr QuicServerSession::New( void QuicServerSession::AddToSocket(QuicSocket* socket) { QuicCID scid(scid_); QuicCID rcid(rcid_); - socket->AddSession(&scid, shared_from_this()); + socket->AddSession(&scid, BaseObjectPtr(this)); socket->AssociateCID(&rcid, &scid); if (pscid_.datalen) { @@ -2121,7 +2130,7 @@ int QuicServerSession::OnClientHello() { // Grab a shared pointer to this to prevent the QuicSession // from being freed while the MakeCallback is running. - std::shared_ptr ptr(shared_from_this()); + BaseObjectPtr ptr(this); MakeCallback( env()->quic_on_session_client_hello_function(), arraysize(argv), argv); @@ -2223,7 +2232,7 @@ int QuicServerSession::OnCert() { // Grab a shared pointer to this to prevent the QuicSession // from being freed while the MakeCallback is running. - std::shared_ptr ptr(shared_from_this()); + BaseObjectPtr ptr(this); MakeCallback(env()->quic_on_session_cert_function(), arraysize(argv), argv); return IsFlagSet(QUICSESSION_FLAG_CERT_CB_RUNNING) ? -1 : 1; @@ -2268,7 +2277,7 @@ void QuicSession::UpdateRecoveryStats() { recovery_stats_.smoothed_rtt = static_cast(stat.smoothed_rtt); } -// The QuicSocket maintains a map of std::shared_ptr's that keep +// The QuicSocket maintains a map of BaseObjectPtr's that keep // the QuicSession instance alive. Once socket_->RemoveSession() // is called, the QuicSession instance will be freed if there are // no other references being held. @@ -2402,7 +2411,7 @@ QuicClientSession::QuicClientSession( CHECK(Init(addr, version, early_transport_params, session_ticket, dcid)); } -std::shared_ptr QuicClientSession::New( +BaseObjectPtr QuicClientSession::New( QuicSocket* socket, const struct sockaddr* addr, uint32_t version, @@ -2422,20 +2431,21 @@ std::shared_ptr QuicClientSession::New( return {}; } - std::shared_ptr session { new QuicClientSession( - socket, - obj, - addr, - version, - context, - hostname, - port, - early_transport_params, - session_ticket, - dcid, - select_preferred_address_policy, - alpn, - options) }; + BaseObjectPtr session = + MakeDetachedBaseObject( + socket, + obj, + addr, + version, + context, + hostname, + port, + early_transport_params, + session_ticket, + dcid, + select_preferred_address_policy, + alpn, + options); session->AddToSocket(socket); session->TLSHandshake(); @@ -2444,7 +2454,7 @@ std::shared_ptr QuicClientSession::New( void QuicClientSession::AddToSocket(QuicSocket* socket) { QuicCID scid(scid_); - socket->AddSession(&scid, shared_from_this()); + socket->AddSession(&scid, BaseObjectPtr(this)); std::vector cids(ngtcp2_conn_get_num_scid(Connection())); ngtcp2_conn_get_scid(Connection(), cids.data()); @@ -2484,7 +2494,7 @@ void QuicClientSession::VersionNegotiation( // Grab a shared pointer to this to prevent the QuicSession // from being freed while the MakeCallback is running. - std::shared_ptr ptr(shared_from_this()); + BaseObjectPtr ptr(this); MakeCallback( env()->quic_on_session_version_negotiation_function(), arraysize(argv), argv); @@ -2642,7 +2652,7 @@ int QuicClientSession::SetSession(SSL_SESSION* session) { } // Grab a shared pointer to this to prevent the QuicSession // from being freed while the MakeCallback is running. - std::shared_ptr ptr(shared_from_this()); + BaseObjectPtr ptr(this); MakeCallback(env()->quic_on_session_ticket_function(), arraysize(argv), argv); return 1; @@ -2651,7 +2661,7 @@ int QuicClientSession::SetSession(SSL_SESSION* session) { bool QuicClientSession::SetSocket(QuicSocket* socket, bool nat_rebinding) { CHECK(!IsFlagSet(QUICSESSION_FLAG_DESTROYED)); CHECK(!IsFlagSet(QUICSESSION_FLAG_GRACEFUL_CLOSING)); - if (socket == nullptr || socket == socket_) + if (socket == nullptr || socket == socket_.get()) return true; // Step 1: Add this Session to the given Socket @@ -2661,7 +2671,7 @@ bool QuicClientSession::SetSocket(QuicSocket* socket, bool nat_rebinding) { RemoveFromSocket(); // Step 3: Update the internal references - socket_ = socket; + socket_.reset(socket); socket->ReceiveStart(); // Step 4: Update ngtcp2 @@ -2737,7 +2747,7 @@ int QuicClientSession::OnTLSStatus() { } // Grab a shared pointer to this to prevent the QuicSession // from being freed while the MakeCallback is running. - std::shared_ptr ptr(shared_from_this()); + BaseObjectPtr ptr(this); MakeCallback(env()->quic_on_session_status_function(), 1, &arg); return 1; } @@ -3616,7 +3626,7 @@ void NewQuicClientSession(const FunctionCallbackInfo& args) { socket->ReceiveStart(); - std::shared_ptr session = + BaseObjectPtr session = QuicClientSession::New( socket, const_cast(reinterpret_cast(&addr)), diff --git a/src/node_quic_session.h b/src/node_quic_session.h index b473cabfe7..2162b779aa 100644 --- a/src/node_quic_session.h +++ b/src/node_quic_session.h @@ -182,7 +182,6 @@ enum QuicSessionState : int { // control. In other words, there's quite a bit going on within // a QuicSession object. class QuicSession : public AsyncWrap, - public std::enable_shared_from_this, public mem::NgLibMemoryManager { public: static const int kInitialClientBufferLength = 4096; @@ -240,13 +239,13 @@ class QuicSession : public AsyncWrap, const ngtcp2_cid* scid() const { return &scid_; } - QuicSocket* Socket() { return socket_; } + inline QuicSocket* Socket() const; SSL* ssl() { return ssl_.get(); } ngtcp2_conn* Connection() { return connection_.get(); } - void AddStream(std::shared_ptr stream); + void AddStream(BaseObjectPtr stream); // Immediately discards the state of the QuicSession // and renders the QuicSession instance completely @@ -382,7 +381,7 @@ class QuicSession : public AsyncWrap, // the idle timeout period elapses or until the // QuicSession is explicitly destroyed. inline bool IsInDrainingPeriod(); - inline QuicStream* FindStream(int64_t id); + QuicStream* FindStream(int64_t id); inline bool HasStream(int64_t id); bool IsHandshakeSuspended() const { @@ -827,7 +826,7 @@ class QuicSession : public AsyncWrap, ngtcp2_mem alloc_info_; ngtcp2_crypto_side side_; - QuicSocket* socket_; + BaseObjectWeakPtr socket_; std::string alpn_; ngtcp2_crypto_level rx_crypto_level_ = NGTCP2_CRYPTO_LEVEL_INITIAL; @@ -874,7 +873,7 @@ class QuicSession : public AsyncWrap, // Temporary holding for inbound TLS handshake data. std::vector peer_handshake_; - std::map> streams_; + std::map> streams_; AliasedFloat64Array state_; @@ -1031,7 +1030,7 @@ class QuicServerSession : public QuicSession { v8::Local target, v8::Local context); - static std::shared_ptr New( + static BaseObjectPtr New( QuicSocket* socket, QuicSessionConfig* config, const ngtcp2_cid* rcid, @@ -1068,7 +1067,6 @@ class QuicServerSession : public QuicSession { SET_MEMORY_INFO_NAME(QuicServerSession) SET_SELF_SIZE(QuicServerSession) - private: QuicServerSession( QuicSocket* socket, QuicSessionConfig* config, @@ -1082,6 +1080,7 @@ class QuicServerSession : public QuicSession { uint32_t options, uint64_t initial_connection_close); + private: void DisassociateCID(const ngtcp2_cid* cid) override; void InitTLS_Post() override; void RemoveFromSocket() override; @@ -1129,7 +1128,7 @@ class QuicClientSession : public QuicSession { v8::Local target, v8::Local context); - static std::shared_ptr New( + static BaseObjectPtr New( QuicSocket* socket, const struct sockaddr* addr, uint32_t version, diff --git a/src/node_quic_socket.cc b/src/node_quic_socket.cc index 688d094223..f7d808df26 100644 --- a/src/node_quic_socket.cc +++ b/src/node_quic_socket.cc @@ -125,7 +125,7 @@ void QuicSocket::MemoryInfo(MemoryTracker* tracker) const { void QuicSocket::AddSession( QuicCID* cid, - std::shared_ptr session) { + BaseObjectPtr session) { sessions_[cid->ToStr()] = session; IncrementSocketAddressCounter(**session->GetRemoteAddress()); IncrementSocketStat( @@ -361,7 +361,7 @@ void QuicSocket::Receive( // desconstructing while we're still using it. The session may // end up being destroyed, however, so we have to make sure // we're checking for that. - std::shared_ptr session; + BaseObjectPtr session; // Identify the appropriate handler auto session_it = sessions_.find(dcid_str); @@ -408,15 +408,15 @@ void QuicSocket::Receive( return; } } else { - session_it = sessions_.find((*scid_it).second); - session = (*session_it).second; + session_it = sessions_.find(scid_it->second); + session = session_it->second; CHECK_NE(session_it, std::end(sessions_)); } } else { - session = (*session_it).second; + session = session_it->second; } - CHECK_NOT_NULL(session); + CHECK(session); // If the packet could not successfully processed for any reason (possibly // due to being malformed or malicious in some way) we ignore it completely. @@ -625,7 +625,7 @@ bool QuicSocket::IsValidatedAddress(const sockaddr* addr) const { return false; } -std::shared_ptr QuicSocket::AcceptInitialPacket( +BaseObjectPtr QuicSocket::AcceptInitialPacket( uint32_t version, QuicCID* dcid, QuicCID* scid, @@ -709,7 +709,7 @@ std::shared_ptr QuicSocket::AcceptInitialPacket( } } - std::shared_ptr session { + BaseObjectPtr session = QuicServerSession::New( this, &server_session_config_, @@ -720,7 +720,7 @@ std::shared_ptr QuicSocket::AcceptInitialPacket( version, server_alpn_, server_options_, - initial_connection_close) }; + initial_connection_close); Local arg = session->object(); MakeCallback(env()->quic_on_session_ready_function(), 1, &arg); @@ -801,7 +801,7 @@ int QuicSocket::DropMembership(const char* address, const char* iface) { int QuicSocket::SendPacket( const sockaddr* dest, QuicBuffer* buffer, - std::shared_ptr session, + BaseObjectPtr session, const char* diagnostic_label) { // If there is no data in the buffer, // or no data remaining to be read, @@ -922,7 +922,7 @@ QuicSocket::SendWrap::SendWrap( QuicSocket* socket, SocketAddress* dest, QuicBuffer* buffer, - std::shared_ptr session, + BaseObjectPtr session, const char* diagnostic_label) : SendWrap(socket, **dest, buffer, session, diagnostic_label) {} @@ -930,7 +930,7 @@ QuicSocket::SendWrap::SendWrap( QuicSocket* socket, const sockaddr* dest, QuicBuffer* buffer, - std::shared_ptr session, + BaseObjectPtr session, const char* diagnostic_label) : SendWrapBase(socket, dest, diagnostic_label), buffer_(buffer), diff --git a/src/node_quic_socket.h b/src/node_quic_socket.h index 4b3070d621..cccc0b4eb2 100644 --- a/src/node_quic_socket.h +++ b/src/node_quic_socket.h @@ -71,7 +71,7 @@ class QuicSocket : public HandleWrap, const char* iface); void AddSession( QuicCID* cid, - std::shared_ptr session); + BaseObjectPtr session); void AssociateCID( QuicCID* cid, QuicCID* scid); @@ -110,7 +110,7 @@ class QuicSocket : public HandleWrap, int SendPacket( const sockaddr* dest, QuicBuffer* buf, - std::shared_ptr session, + BaseObjectPtr session, const char* diagnostic_label = nullptr); void SetServerBusy(bool on); void SetDiagnosticPacketLoss(double rx = 0.0, double tx = 0.0); @@ -170,7 +170,7 @@ class QuicSocket : public HandleWrap, void SetValidatedAddress(const sockaddr* addr); bool IsValidatedAddress(const sockaddr* addr) const; - std::shared_ptr AcceptInitialPacket( + BaseObjectPtr AcceptInitialPacket( uint32_t version, QuicCID* dcid, QuicCID* scid, @@ -257,7 +257,7 @@ class QuicSocket : public HandleWrap, QuicSessionConfig server_session_config_; crypto::SecureContext* server_secure_context_ = nullptr; std::string server_alpn_; - std::unordered_map> sessions_; + std::unordered_map> sessions_; std::unordered_map dcid_to_scid_; std::array token_secret_; @@ -377,14 +377,14 @@ class QuicSocket : public HandleWrap, QuicSocket* socket, SocketAddress* dest, QuicBuffer* buffer, - std::shared_ptr session, + BaseObjectPtr session, const char* diagnostic_label = nullptr); SendWrap( QuicSocket* socket, const sockaddr* dest, QuicBuffer* buffer, - std::shared_ptr session, + BaseObjectPtr session, const char* diagnostic_label = nullptr); void Done(int status) override; @@ -395,7 +395,7 @@ class QuicSocket : public HandleWrap, private: QuicBuffer* buffer_; - std::shared_ptr session_; + BaseObjectPtr session_; size_t length_ = 0; }; diff --git a/src/node_quic_stream.cc b/src/node_quic_stream.cc index aacb3d611c..dd6c2574a4 100644 --- a/src/node_quic_stream.cc +++ b/src/node_quic_stream.cc @@ -140,7 +140,7 @@ int QuicStream::DoShutdown(ShutdownWrap* req_wrap) { // If we're not currently within an ngtcp2 callback, then we need to // tell the QuicSession to initiate serialization and sending of any // pending frames. - if (!QuicSession::Ngtcp2CallbackScope::InNgtcp2CallbackScope(session_)) + if (!QuicSession::Ngtcp2CallbackScope::InNgtcp2CallbackScope(session_.get())) session_->SendStreamData(this); return 1; @@ -190,7 +190,7 @@ int QuicStream::DoWrite( // the pending stream data. Otherwise, the data will be flushed // once the ngtcp2 callback scope exits and all streams with // data pending are flushed. - if (!QuicSession::Ngtcp2CallbackScope::InNgtcp2CallbackScope(session_)) + if (!QuicSession::Ngtcp2CallbackScope::InNgtcp2CallbackScope(session_.get())) session_->SendStreamData(this); // IncrementAvailableOutboundLength(len); @@ -365,15 +365,16 @@ void QuicStream::Shutdown(uint64_t app_error_code) { session_->ShutdownStream(GetID(), app_error_code); } -std::shared_ptr QuicStream::New( +BaseObjectPtr QuicStream::New( QuicSession* session, int64_t stream_id) { Local obj; if (!session->env() ->quicserverstream_constructor_template() ->NewInstance(session->env()->context()).ToLocal(&obj)) { - return nullptr; + return {}; } - std::shared_ptr stream {new QuicStream(session, obj, stream_id)}; + BaseObjectPtr stream = + MakeDetachedBaseObject(session, obj, stream_id); session->AddStream(stream); return stream; } @@ -396,7 +397,7 @@ void OpenUnidirectionalStream(const FunctionCallbackInfo& args) { if (!session->OpenUnidirectionalStream(&stream_id)) return; - std::shared_ptr stream = QuicStream::New(session, stream_id); + BaseObjectPtr stream = QuicStream::New(session, stream_id); args.GetReturnValue().Set(stream->object()); } @@ -410,7 +411,7 @@ void OpenBidirectionalStream(const FunctionCallbackInfo& args) { if (!session->OpenBidirectionalStream(&stream_id)) return; - std::shared_ptr stream = QuicStream::New(session, stream_id); + BaseObjectPtr stream = QuicStream::New(session, stream_id); args.GetReturnValue().Set(stream->object()); } diff --git a/src/node_quic_stream.h b/src/node_quic_stream.h index 2549efedf0..b441a17659 100644 --- a/src/node_quic_stream.h +++ b/src/node_quic_stream.h @@ -85,9 +85,7 @@ class QuicServerSession; // This causes all queued data and pending JavaScript writes to be // abandoned, and causes the QuicStream to be immediately closed at the // ngtcp2 level. -class QuicStream : public AsyncWrap, - public StreamBase, - public std::enable_shared_from_this { +class QuicStream : public AsyncWrap, public StreamBase { public: enum QuicStreamStates : uint32_t { // QuicStream is fully open. Readable and Writable @@ -144,7 +142,7 @@ class QuicStream : public AsyncWrap, v8::Local target, v8::Local context); - static std::shared_ptr New( + static BaseObjectPtr New( QuicSession* session, int64_t stream_id); std::string diagnostic_name() const override; @@ -260,7 +258,7 @@ class QuicStream : public AsyncWrap, return HasSentFin() && streambuf_.Length() == 0; } - QuicSession* Session() const { return session_; } + QuicSession* Session() const { return session_.get(); } virtual void AckedDataOffset(uint64_t offset, size_t datalen); @@ -305,12 +303,12 @@ class QuicStream : public AsyncWrap, SET_MEMORY_INFO_NAME(QuicStream) SET_SELF_SIZE(QuicStream) - private: QuicStream( QuicSession* session, v8::Local target, int64_t stream_id); + private: // Called only when a final stream frame has been received from // the peer. This has the side effect of marking the readable // side of the stream closed. No additional data will be received @@ -351,7 +349,7 @@ class QuicStream : public AsyncWrap, inline void IncrementStats(size_t datalen); - QuicSession* session_; + BaseObjectWeakPtr session_; int64_t stream_id_; uint64_t max_offset_ = 0; uint64_t max_offset_ack_ = 0; From 1428c19a6e85f124da6a30aad4a9f2db08121e54 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 29 Sep 2019 16:03:04 +0200 Subject: [PATCH 6/9] src: use custom smart pointers for HistogramBase --- src/histogram-inl.h | 15 +++++++++------ src/histogram.h | 4 ++-- src/node_quic_session.h | 4 ++-- src/node_quic_stream.h | 6 +++--- 4 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/histogram-inl.h b/src/histogram-inl.h index f341fab6d6..e02b619b3c 100644 --- a/src/histogram-inl.h +++ b/src/histogram-inl.h @@ -63,9 +63,11 @@ HistogramBase::HistogramBase( v8::Local wrap, int64_t lowest, int64_t highest, - int figures) : - BaseObject(env, wrap), - Histogram(lowest, highest, figures) {} + int figures) + : BaseObject(env, wrap), + Histogram(lowest, highest, figures) { + MakeWeak(); +} bool HistogramBase::RecordDelta() { uint64_t time = uv_hrtime(); @@ -92,7 +94,7 @@ void HistogramBase::ResetState() { prev_ = 0; } -HistogramBase* HistogramBase::New( +BaseObjectPtr HistogramBase::New( Environment* env, int64_t lowest, int64_t highest, @@ -102,9 +104,10 @@ HistogramBase* HistogramBase::New( v8::Local obj; auto tmpl = env->histogram_ctor_template(); if (!tmpl->NewInstance(env->context()).ToLocal(&obj)) - return nullptr; + return {}; - return new HistogramBase(env, obj, lowest, highest, figures); + return MakeDetachedBaseObject( + env, obj, lowest, highest, figures); } } // namespace node diff --git a/src/histogram.h b/src/histogram.h index 117b214e87..e1ee75f5f0 100644 --- a/src/histogram.h +++ b/src/histogram.h @@ -64,13 +64,12 @@ class HistogramBase : public BaseObject, public Histogram { static void HistogramReset(const v8::FunctionCallbackInfo& args); static void Initialize(Environment* env); - static inline HistogramBase* New( + static inline BaseObjectPtr New( Environment* env, int64_t lowest, int64_t highest, int figures = 3); - private: inline HistogramBase( Environment* env, v8::Local wrap, @@ -78,6 +77,7 @@ class HistogramBase : public BaseObject, public Histogram { int64_t highest, int figures = 3); + private: int64_t exceeds_ = 0; uint64_t prev_ = 0; }; diff --git a/src/node_quic_session.h b/src/node_quic_session.h index 2162b779aa..1602a7a7ca 100644 --- a/src/node_quic_session.h +++ b/src/node_quic_session.h @@ -931,13 +931,13 @@ class QuicSession : public AsyncWrap, // crypto_rx_ack_ measures the elapsed time between crypto acks // for this stream. This data can be used to detect peers that are // generally taking too long to acknowledge crypto data. - std::unique_ptr crypto_rx_ack_; + BaseObjectPtr crypto_rx_ack_; // crypto_handshake_rate_ measures the elapsed time between // crypto continuation steps. This data can be used to detect // peers that are generally taking too long to carry out the // handshake - std::unique_ptr crypto_handshake_rate_; + BaseObjectPtr crypto_handshake_rate_; struct recovery_stats { double min_rtt; diff --git a/src/node_quic_stream.h b/src/node_quic_stream.h index b441a17659..3fbe524638 100644 --- a/src/node_quic_stream.h +++ b/src/node_quic_stream.h @@ -384,7 +384,7 @@ class QuicStream : public AsyncWrap, public StreamBase { // for the stream. Specifically, this can be used to detect // potentially bad acting peers that are sending many small chunks // of data too slowly in an attempt to DOS the peer. - std::unique_ptr data_rx_rate_; + BaseObjectPtr data_rx_rate_; // data_rx_size_ measures the size of data packets for this stream // over time. When used in combination with the data_rx_rate_, @@ -392,12 +392,12 @@ class QuicStream : public AsyncWrap, public StreamBase { // for the stream. Specifically, this can be used to detect // potentially bad acting peers that are sending many small chunks // of data too slowly in an attempt to DOS the peer. - std::unique_ptr data_rx_size_; + BaseObjectPtr data_rx_size_; // data_rx_ack_ measures the elapsed time between data acks // for this stream. This data can be used to detect peers that are // generally taking too long to acknowledge sent stream data. - std::unique_ptr data_rx_ack_; + BaseObjectPtr data_rx_ack_; AliasedBigUint64Array stats_buffer_; }; From bd66e347260ea4dd23be991db47e82af76797cc2 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 29 Sep 2019 17:40:49 +0200 Subject: [PATCH 7/9] quic: test cleanup when session is detached from QuicSocket MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, the QuicSocket instance would always be destroyed first during cleanup (because it is a `HandleWrap`), destroying the other native objects associated with it in the process. This makes sure that cleanup also works when the session is detached from the “real” network socket. In particular, this is useful for the future possibility of fully detaching sessions from sockets. --- lib/internal/quic/core.js | 6 ++++++ src/node_quic_session.cc | 11 ++++++++++- test/parallel/test-quic-process-cleanup.js | 10 ++++++++-- 3 files changed, 24 insertions(+), 3 deletions(-) diff --git a/lib/internal/quic/core.js b/lib/internal/quic/core.js index 3877f66d6c..46c0d924ac 100644 --- a/lib/internal/quic/core.js +++ b/lib/internal/quic/core.js @@ -1762,6 +1762,12 @@ class QuicSession extends EventEmitter { get handshakeContinuationHistogram() { return this.#handshakeContinuationHistogram; } + + // TODO(addaleax): This is a temporary solution for testing and should be + // removed later. + removeFromSocket() { + return this[kHandle].removeFromSocket(); + } } class QuicServerSession extends QuicSession { diff --git a/src/node_quic_session.cc b/src/node_quic_session.cc index 63ddb3f716..9a43b59161 100644 --- a/src/node_quic_session.cc +++ b/src/node_quic_session.cc @@ -1472,7 +1472,7 @@ bool QuicSession::SendPacket(const char* diagnostic_label) { txbuf_ += std::move(sendbuf_); } // There's nothing to send, so let's not try - if (txbuf_.Length() == 0) + if (txbuf_.Length() == 0 || Socket() == nullptr) return true; Debug(this, "There are %" PRIu64 " bytes in txbuf_ to send", txbuf_.Length()); session_stats_.session_sent_at = uv_hrtime(); @@ -3577,6 +3577,14 @@ void QuicSessionPing(const FunctionCallbackInfo& args) { session->Ping(); } +// TODO(addaleax): This is a temporary solution for testing and should be +// removed later. +void QuicSessionRemoveFromSocket(const FunctionCallbackInfo& args) { + QuicSession* session; + ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); + session->RemoveFromSocket(); +} + void QuicSessionUpdateKey(const FunctionCallbackInfo& args) { QuicSession* session; ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); @@ -3658,6 +3666,7 @@ void AddMethods(Environment* env, Local session) { env->SetProtoMethod(session, "gracefulClose", QuicSessionGracefulClose); env->SetProtoMethod(session, "updateKey", QuicSessionUpdateKey); env->SetProtoMethod(session, "ping", QuicSessionPing); + env->SetProtoMethod(session, "removeFromSocket", QuicSessionRemoveFromSocket); env->SetProtoMethod(session, "onClientHelloDone", QuicSessionOnClientHelloDone); env->SetProtoMethod(session, "onCertDone", QuicSessionOnCertDone); diff --git a/test/parallel/test-quic-process-cleanup.js b/test/parallel/test-quic-process-cleanup.js index 2d335c4034..4396cf0b36 100644 --- a/test/parallel/test-quic-process-cleanup.js +++ b/test/parallel/test-quic-process-cleanup.js @@ -8,9 +8,13 @@ if (!common.hasQuic) // sequence and we can stop execution at any point. const quic = require('quic'); -const { isMainThread, Worker } = require('worker_threads'); +const { isMainThread, Worker, workerData } = require('worker_threads'); -if (isMainThread) return new Worker(__filename); +if (isMainThread) { + new Worker(__filename, { workerData: { removeFromSocket: true } }); + new Worker(__filename, { workerData: { removeFromSocket: false } }); + return; +} const fixtures = require('../common/fixtures'); const key = fixtures.readKey('agent1-key.pem', 'binary'); @@ -58,6 +62,8 @@ server.on('ready', common.mustCall(() => { }); req.on('stream', common.mustCall(() => { + if (workerData.removeFromSocket) + req.removeFromSocket(); process.exit(); })); From 290902197698cee52e77d8b13bb4d3ac72ca7692 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 1 Oct 2019 15:19:09 +0200 Subject: [PATCH 8/9] fixup! src: introduce custom smart pointers for `BaseObject`s --- src/base_object-inl.h | 17 ++++++++ src/base_object.h | 9 ++-- test/cctest/test_base_object_ptr.cc | 68 ++++++++++++++++++++++++++--- 3 files changed, 85 insertions(+), 9 deletions(-) diff --git a/src/base_object-inl.h b/src/base_object-inl.h index 1157b19ba0..af0a7d707b 100644 --- a/src/base_object-inl.h +++ b/src/base_object-inl.h @@ -280,6 +280,23 @@ BaseObjectPtrImpl& BaseObjectPtrImpl::operator=( 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); diff --git a/src/base_object.h b/src/base_object.h index dec93944c6..daf40b7c1e 100644 --- a/src/base_object.h +++ b/src/base_object.h @@ -176,16 +176,17 @@ class BaseObjectPtrImpl final { inline ~BaseObjectPtrImpl(); inline explicit BaseObjectPtrImpl(T* target); - // Copy constructors. Note that the templated version is not a copy - // constructor in the C++ sense of the word, so an identical untemplated - // version is provided. - // TODO(addaleax): Add move variants. + // 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; diff --git a/test/cctest/test_base_object_ptr.cc b/test/cctest/test_base_object_ptr.cc index 61759440d0..18e27edba8 100644 --- a/test/cctest/test_base_object_ptr.cc +++ b/test/cctest/test_base_object_ptr.cc @@ -20,17 +20,19 @@ class DummyBaseObject : public BaseObject { public: DummyBaseObject(Environment* env, Local obj) : BaseObject(env, obj) {} - static BaseObjectPtr NewDetached(Environment* env) { - Local obj = BaseObject::MakeLazilyInitializedJSTemplate(env) + 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 = BaseObject::MakeLazilyInitializedJSTemplate(env) - ->GetFunction(env->context()).ToLocalChecked() - ->NewInstance(env->context()).ToLocalChecked(); + Local obj = MakeJSObject(env); return MakeBaseObject(env, obj); } @@ -116,3 +118,59 @@ TEST_F(BaseObjectPtrTest, GCWeak) { 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); +} From d0b63ad0eb6550aeb4d529c16a14110e376375c9 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 1 Oct 2019 22:01:31 +0200 Subject: [PATCH 9/9] fixup! quic: use custom BaseObject smart pointers --- src/node_quic_socket.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node_quic_socket.h b/src/node_quic_socket.h index cccc0b4eb2..85f5bf1c09 100644 --- a/src/node_quic_socket.h +++ b/src/node_quic_socket.h @@ -347,7 +347,7 @@ class QuicSocket : public HandleWrap, uv_udp_send_t* req() { return &req_; } - QuicSocket* Socket() { return socket_; } + QuicSocket* Socket() { return socket_.get(); } SocketAddress* Address() { return &address_; } @@ -363,7 +363,7 @@ class QuicSocket : public HandleWrap, private: uv_udp_send_t req_; - QuicSocket* socket_; + BaseObjectPtr socket_; SocketAddress address_; const char* diagnostic_label_; };