From 4745ac4fd721713fc2a9e2123450bde75bf3d084 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 29 Sep 2019 01:29:16 +0200 Subject: [PATCH] http2: use custom BaseObject smart pointers Refs: https://github.com/nodejs/quic/pull/141 Reviewed-By: James M Snell Backport-PR-URL: https://github.com/nodejs/node/pull/32301 PR-URL: https://github.com/nodejs/node/pull/30374 Refs: https://github.com/nodejs/quic/pull/149 Refs: https://github.com/nodejs/quic/pull/165 Reviewed-By: David Carlier --- 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 5876701f0627c8..8b2b30021a8520 100644 --- a/src/base_object-inl.h +++ b/src/base_object-inl.h @@ -71,10 +71,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 150237709bc31e..be6c7f5cd0efba 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -243,7 +243,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(); } @@ -586,8 +585,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); } @@ -695,7 +692,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) { @@ -1451,7 +1448,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 @@ -1490,7 +1487,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; @@ -1951,12 +1948,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 { @@ -2164,8 +2160,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; } @@ -2832,7 +2830,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. @@ -2861,16 +2860,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(); @@ -2880,7 +2879,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; @@ -2891,8 +2890,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(); @@ -2902,7 +2901,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; @@ -2917,7 +2916,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 c6b9938d2d8ef8..045bdfd716da03 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -460,8 +460,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(); @@ -614,7 +614,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 @@ -846,11 +846,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; @@ -1027,10 +1027,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_;