From 7404a4dd42a45ae4d4dfb3474ce10cc5fef09e7c Mon Sep 17 00:00:00 2001 From: Eugene Ostroukhov Date: Wed, 25 Jul 2018 16:46:45 -0700 Subject: [PATCH] inspector: tie objects lifetime to the thread they belong to PR-URL: https://github.com/nodejs/node/pull/22242 Reviewed-By: James M Snell Reviewed-By: Aleksei Koziatinskii --- src/inspector/main_thread_interface.cc | 185 ++++++++++++++++--------- src/inspector/main_thread_interface.h | 25 +++- 2 files changed, 143 insertions(+), 67 deletions(-) diff --git a/src/inspector/main_thread_interface.cc b/src/inspector/main_thread_interface.cc index da43c95bea5fe3..d8ee737ce2fce0 100644 --- a/src/inspector/main_thread_interface.cc +++ b/src/inspector/main_thread_interface.cc @@ -3,6 +3,7 @@ #include "node_mutex.h" #include "v8-inspector.h" +#include #include namespace node { @@ -13,56 +14,72 @@ using v8_inspector::StringView; using v8_inspector::StringBuffer; template -class DeleteRequest : public Request { +class DeletableWrapper : public Deletable { public: - explicit DeleteRequest(T* object) : object_(object) {} - void Call() override { - delete object_; + explicit DeletableWrapper(std::unique_ptr object) + : object_(std::move(object)) {} + ~DeletableWrapper() override = default; + + static T* get(MainThreadInterface* thread, int id) { + return + static_cast*>(thread->GetObject(id))->object_.get(); } private: - T* object_; + std::unique_ptr object_; }; -template -class SingleArgumentFunctionCall : public Request { - public: - using Fn = void (Target::*)(Arg); +template +std::unique_ptr WrapInDeletable(std::unique_ptr object) { + return std::unique_ptr>( + new DeletableWrapper(std::move(object))); +} - SingleArgumentFunctionCall(Target* target, Fn fn, Arg argument) - : target_(target), - fn_(fn), - arg_(std::move(argument)) {} +template +class CreateObjectRequest : public Request { + public: + CreateObjectRequest(int object_id, Factory factory) + : object_id_(object_id), factory_(std::move(factory)) {} - void Call() override { - Apply(target_, fn_, std::move(arg_)); + void Call(MainThreadInterface* thread) { + thread->AddObject(object_id_, WrapInDeletable(factory_(thread))); } private: - template - void Apply(Element* target, Fn fn, Arg arg) { - (target->*fn)(std::move(arg)); + int object_id_; + Factory factory_; +}; + +template +std::unique_ptr NewCreateRequest(int object_id, Factory factory) { + return std::unique_ptr( + new CreateObjectRequest(object_id, std::move(factory))); +} + +class DeleteRequest : public Request { + public: + explicit DeleteRequest(int object_id) : object_id_(object_id) {} + + void Call(MainThreadInterface* thread) override { + thread->RemoveObject(object_id_); } - Target* target_; - Fn fn_; - Arg arg_; + private: + int object_id_; }; -class PostMessageRequest : public Request { +template +class CallRequest : public Request { public: - PostMessageRequest(InspectorSessionDelegate* delegate, - StringView message) - : delegate_(delegate), - message_(StringBuffer::create(message)) {} + CallRequest(int id, Fn fn) : id_(id), fn_(std::move(fn)) {} - void Call() override { - delegate_->SendMessageToFrontend(message_->string()); + void Call(MainThreadInterface* thread) override { + fn_(DeletableWrapper::get(thread, id_)); } private: - InspectorSessionDelegate* delegate_; - std::unique_ptr message_; + int id_; + Fn fn_; }; class DispatchMessagesTask : public v8::Task { @@ -88,45 +105,63 @@ void DisposePairCallback(uv_handle_t* ref) { template class AnotherThreadObjectReference { public: - // We create it on whatever thread, just make sure it gets disposed on the - // proper thread. - AnotherThreadObjectReference(std::shared_ptr thread, - T* object) - : thread_(thread), object_(object) { + AnotherThreadObjectReference( + std::shared_ptr thread, int object_id) + : thread_(thread), object_id_(object_id) {} + + template + AnotherThreadObjectReference( + std::shared_ptr thread, Factory factory) + : AnotherThreadObjectReference(thread, thread->newObjectId()) { + thread_->Post(NewCreateRequest(object_id_, std::move(factory))); } AnotherThreadObjectReference(AnotherThreadObjectReference&) = delete; ~AnotherThreadObjectReference() { // Disappearing thread may cause a memory leak - CHECK(thread_->Post( - std::unique_ptr>(new DeleteRequest(object_)))); - object_ = nullptr; + thread_->Post( + std::unique_ptr(new DeleteRequest(object_id_))); } - template - void Post(Fn fn, Arg argument) const { - using R = SingleArgumentFunctionCall; - thread_->Post(std::unique_ptr(new R(object_, fn, std::move(argument)))); + template + void Call(Fn fn) const { + using Request = CallRequest; + thread_->Post(std::unique_ptr( + new Request(object_id_, std::move(fn)))); } - T* get() const { - return object_; + template + void Call(void (T::*fn)(Arg), Arg argument) const { + Call(std::bind(Apply, std::placeholders::_1, fn, std::move(argument))); } private: + // This has to use non-const reference to support std::bind with non-copyable + // types + template + static void Apply(T* target, void (T::*fn)(Argument), + /* NOLINT (runtime/references) */ Argument& argument) { + (target->*fn)(std::move(argument)); + } + std::shared_ptr thread_; - T* object_; + const int object_id_; }; class MainThreadSessionState { public: - MainThreadSessionState( - std::shared_ptr thread, - bool prevent_shutdown) : thread_(thread), - prevent_shutdown_(prevent_shutdown) {} + MainThreadSessionState(MainThreadInterface* thread, bool prevent_shutdown) + : thread_(thread), + prevent_shutdown_(prevent_shutdown) {} + + static std::unique_ptr Create( + MainThreadInterface* thread, bool prevent_shutdown) { + return std::unique_ptr( + new MainThreadSessionState(thread, prevent_shutdown)); + } void Connect(std::unique_ptr delegate) { - Agent* agent = thread_->GetInspectorAgent(); + Agent* agent = thread_->inspector_agent(); if (agent != nullptr) session_ = agent->Connect(std::move(delegate), prevent_shutdown_); } @@ -136,7 +171,7 @@ class MainThreadSessionState { } private: - std::shared_ptr thread_; + MainThreadInterface* thread_; bool prevent_shutdown_; std::unique_ptr session_; }; @@ -148,12 +183,14 @@ class CrossThreadInspectorSession : public InspectorSession { std::shared_ptr thread, std::unique_ptr delegate, bool prevent_shutdown) - : state_(thread, new MainThreadSessionState(thread, prevent_shutdown)) { - state_.Post(&MainThreadSessionState::Connect, std::move(delegate)); + : state_(thread, std::bind(MainThreadSessionState::Create, + std::placeholders::_1, + prevent_shutdown)) { + state_.Call(&MainThreadSessionState::Connect, std::move(delegate)); } void Dispatch(const StringView& message) override { - state_.Post(&MainThreadSessionState::Dispatch, + state_.Call(&MainThreadSessionState::Dispatch, StringBuffer::create(message)); } @@ -163,13 +200,15 @@ class CrossThreadInspectorSession : public InspectorSession { class ThreadSafeDelegate : public InspectorSessionDelegate { public: - ThreadSafeDelegate(std::shared_ptr thread, - std::unique_ptr delegate) - : thread_(thread), delegate_(thread, delegate.release()) {} + ThreadSafeDelegate(std::shared_ptr thread, int object_id) + : thread_(thread), delegate_(thread, object_id) {} void SendMessageToFrontend(const v8_inspector::StringView& message) override { - thread_->Post(std::unique_ptr( - new PostMessageRequest(delegate_.get(), message))); + delegate_.Call( + [m = StringBuffer::create(message)] + (InspectorSessionDelegate* delegate) { + delegate->SendMessageToFrontend(m->string()); + }); } private: @@ -252,7 +291,7 @@ void MainThreadInterface::DispatchMessages() { MessageQueue::value_type task; std::swap(dispatching_message_queue_.front(), task); dispatching_message_queue_.pop_front(); - task->Call(); + task->Call(this); } } while (had_messages); dispatching_messages_ = false; @@ -264,6 +303,26 @@ std::shared_ptr MainThreadInterface::GetHandle() { return handle_; } +void MainThreadInterface::AddObject(int id, + std::unique_ptr object) { + CHECK_NE(nullptr, object); + managed_objects_[id] = std::move(object); +} + +void MainThreadInterface::RemoveObject(int id) { + CHECK_EQ(1, managed_objects_.erase(id)); +} + +Deletable* MainThreadInterface::GetObject(int id) { + auto iterator = managed_objects_.find(id); + // This would mean the object is requested after it was disposed, which is + // a coding error. + CHECK_NE(managed_objects_.end(), iterator); + Deletable* pointer = iterator->second.get(); + CHECK_NE(nullptr, pointer); + return pointer; +} + std::unique_ptr Utf8ToStringView(const std::string& message) { icu::UnicodeString utf16 = icu::UnicodeString::fromUTF8( icu::StringPiece(message.data(), message.length())); @@ -303,10 +362,12 @@ Agent* MainThreadHandle::GetInspectorAgent() { } std::unique_ptr -MainThreadHandle::MakeThreadSafeDelegate( +MainThreadHandle::MakeDelegateThreadSafe( std::unique_ptr delegate) { + int id = newObjectId(); + main_thread_->AddObject(id, WrapInDeletable(std::move(delegate))); return std::unique_ptr( - new ThreadSafeDelegate(shared_from_this(), std::move(delegate))); + new ThreadSafeDelegate(shared_from_this(), id)); } bool MainThreadHandle::Expired() { diff --git a/src/inspector/main_thread_interface.h b/src/inspector/main_thread_interface.h index 75df5ffe809048..db79db43821a89 100644 --- a/src/inspector/main_thread_interface.h +++ b/src/inspector/main_thread_interface.h @@ -9,10 +9,10 @@ #include "inspector_agent.h" #include "node_mutex.h" +#include #include #include #include -#include namespace v8_inspector { class StringBuffer; @@ -21,31 +21,41 @@ class StringView; namespace node { namespace inspector { +class MainThreadInterface; + class Request { public: - virtual void Call() = 0; + virtual void Call(MainThreadInterface*) = 0; virtual ~Request() {} }; +class Deletable { + public: + virtual ~Deletable() {} +}; + std::unique_ptr Utf8ToStringView( const std::string& message); using MessageQueue = std::deque>; -class MainThreadInterface; class MainThreadHandle : public std::enable_shared_from_this { public: explicit MainThreadHandle(MainThreadInterface* main_thread) - : main_thread_(main_thread) {} + : main_thread_(main_thread) { + } ~MainThreadHandle() { CHECK_NULL(main_thread_); // main_thread_ should have called Reset } std::unique_ptr Connect( std::unique_ptr delegate, bool prevent_shutdown); + int newObjectId() { + return ++next_object_id_; + } bool Post(std::unique_ptr request); Agent* GetInspectorAgent(); - std::unique_ptr MakeThreadSafeDelegate( + std::unique_ptr MakeDelegateThreadSafe( std::unique_ptr delegate); bool Expired(); @@ -55,6 +65,7 @@ class MainThreadHandle : public std::enable_shared_from_this { MainThreadInterface* main_thread_; Mutex block_lock_; int next_session_id_ = 0; + std::atomic_int next_object_id_ = {1}; friend class MainThreadInterface; }; @@ -72,6 +83,9 @@ class MainThreadInterface { Agent* inspector_agent() { return agent_; } + void AddObject(int handle, std::unique_ptr object); + Deletable* GetObject(int id); + void RemoveObject(int handle); private: using AsyncAndInterface = std::pair; @@ -92,6 +106,7 @@ class MainThreadInterface { v8::Platform* const platform_; DeleteFnPtr main_thread_request_; std::shared_ptr handle_; + std::unordered_map> managed_objects_; }; } // namespace inspector