From 666588143e65482a6b509271de40ca19a795546e 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 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 4fc9210b39bf89..f35cd6734edf0b 100644 --- a/src/base_object-inl.h +++ b/src/base_object-inl.h @@ -63,10 +63,6 @@ BaseObject::~BaseObject() { } } -void BaseObject::RemoveCleanupHook() { - env_->RemoveCleanupHook(DeleteMe, static_cast(this)); -} - void BaseObject::Detach() { CHECK_GT(pointer_data()->strong_ptr_count, 0); pointer_data()->is_detached = true; diff --git a/src/base_object.h b/src/base_object.h index 38afe11a761e26..daf40b7c1eb7b4 100644 --- a/src/base_object.h +++ b/src/base_object.h @@ -97,7 +97,6 @@ class BaseObject : public MemoryRetainer { inline void Detach(); protected: - inline void RemoveCleanupHook(); // TODO(addaleax): Remove. virtual inline void OnGCCollect(); private: diff --git a/src/node_http2.cc b/src/node_http2.cc index f3ef8363e4ebcf..9eea9c257fccb9 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -239,7 +239,6 @@ Http2Session::Http2Settings::Http2Settings(Environment* env, : AsyncWrap(env, obj, PROVIDER_HTTP2SETTINGS), session_(session), startTime_(start_time) { - RemoveCleanupHook(); // This object is owned by the Http2Session. Init(); } @@ -658,8 +657,6 @@ Http2Session::Http2Session(Environment* env, Http2Session::~Http2Session() { CHECK_EQ(flags_ & SESSION_STATE_HAS_SCOPE, 0); Debug(this, "freeing nghttp2 session"); - for (const auto& iter : streams_) - iter.second->session_ = nullptr; nghttp2_session_del(session_); CHECK_EQ(current_nghttp2_memory_, 0); } @@ -767,7 +764,7 @@ void Http2Session::Close(uint32_t code, bool socket_closed) { // If there are outstanding pings, those will need to be canceled, do // so on the next iteration of the event loop to avoid calling out into // javascript since this may be called during garbage collection. - while (std::unique_ptr ping = PopPing()) { + while (BaseObjectPtr ping = PopPing()) { ping->DetachFromSession(); env()->SetImmediate( [ping = std::move(ping)](Environment* env) { @@ -1483,7 +1480,7 @@ void Http2Session::HandlePingFrame(const nghttp2_frame* frame) { Local arg; bool ack = frame->hd.flags & NGHTTP2_FLAG_ACK; if (ack) { - std::unique_ptr ping = PopPing(); + BaseObjectPtr ping = PopPing(); if (!ping) { // PING Ack is unsolicited. Treat as a connection error. The HTTP/2 @@ -1522,7 +1519,7 @@ void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) { // If this is an acknowledgement, we should have an Http2Settings // object for it. - std::unique_ptr settings = PopSettings(); + BaseObjectPtr settings = PopSettings(); if (settings) { settings->Done(true); return; @@ -1982,12 +1979,11 @@ Http2Stream::~Http2Stream() { nghttp2_rcbuf_decref(header.value); } - if (session_ == nullptr) + if (!session_) return; Debug(this, "tearing down stream"); session_->DecrementCurrentSessionMemory(current_headers_length_); session_->RemoveStream(this); - session_ = nullptr; } std::string Http2Stream::diagnostic_name() const { @@ -2189,8 +2185,10 @@ Http2Stream* Http2Stream::SubmitPushPromise(nghttp2_nv* nva, id_, nva, len, nullptr); CHECK_NE(*ret, NGHTTP2_ERR_NOMEM); Http2Stream* stream = nullptr; - if (*ret > 0) - stream = Http2Stream::New(session_, *ret, NGHTTP2_HCAT_HEADERS, options); + if (*ret > 0) { + stream = Http2Stream::New( + session_.get(), *ret, NGHTTP2_HCAT_HEADERS, options); + } return stream; } @@ -2855,7 +2853,8 @@ void Http2Session::Ping(const FunctionCallbackInfo& args) { if (obj->Set(env->context(), env->ondone_string(), args[1]).IsNothing()) return; - Http2Ping* ping = session->AddPing(std::make_unique(session, obj)); + Http2Ping* ping = session->AddPing( + MakeDetachedBaseObject(session, obj)); // To prevent abuse, we strictly limit the number of unacknowledged PING // frames that may be sent at any given time. This is configurable in the // Options when creating a Http2Session. @@ -2884,16 +2883,16 @@ void Http2Session::Settings(const FunctionCallbackInfo& args) { if (obj->Set(env->context(), env->ondone_string(), args[0]).IsNothing()) return; - Http2Session::Http2Settings* settings = session->AddSettings( - std::make_unique(session->env(), session, obj, 0)); + Http2Settings* settings = session->AddSettings( + MakeDetachedBaseObject(session->env(), session, obj, 0)); if (settings == nullptr) return args.GetReturnValue().Set(false); settings->Send(); args.GetReturnValue().Set(true); } -std::unique_ptr Http2Session::PopPing() { - std::unique_ptr ping; +BaseObjectPtr Http2Session::PopPing() { + BaseObjectPtr ping; if (!outstanding_pings_.empty()) { ping = std::move(outstanding_pings_.front()); outstanding_pings_.pop(); @@ -2903,7 +2902,7 @@ std::unique_ptr Http2Session::PopPing() { } Http2Session::Http2Ping* Http2Session::AddPing( - std::unique_ptr ping) { + BaseObjectPtr ping) { if (outstanding_pings_.size() == max_outstanding_pings_) { ping->Done(false); return nullptr; @@ -2914,8 +2913,8 @@ Http2Session::Http2Ping* Http2Session::AddPing( return ptr; } -std::unique_ptr Http2Session::PopSettings() { - std::unique_ptr settings; +BaseObjectPtr Http2Session::PopSettings() { + BaseObjectPtr settings; if (!outstanding_settings_.empty()) { settings = std::move(outstanding_settings_.front()); outstanding_settings_.pop(); @@ -2925,7 +2924,7 @@ std::unique_ptr Http2Session::PopSettings() { } Http2Session::Http2Settings* Http2Session::AddSettings( - std::unique_ptr settings) { + BaseObjectPtr settings) { if (outstanding_settings_.size() == max_outstanding_settings_) { settings->Done(false); return nullptr; @@ -2940,7 +2939,6 @@ Http2Session::Http2Ping::Http2Ping(Http2Session* session, Local obj) : AsyncWrap(session->env(), obj, AsyncWrap::PROVIDER_HTTP2PING), session_(session), startTime_(uv_hrtime()) { - RemoveCleanupHook(); // This object is owned by the Http2Session. } void Http2Session::Http2Ping::Send(const uint8_t* payload) { diff --git a/src/node_http2.h b/src/node_http2.h index 6aeb69fa848827..1444738470f9c7 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -456,8 +456,8 @@ class Http2Stream : public AsyncWrap, nghttp2_stream* operator*(); - Http2Session* session() { return session_; } - const Http2Session* session() const { return session_; } + Http2Session* session() { return session_.get(); } + const Http2Session* session() const { return session_.get(); } void EmitStatistics(); @@ -609,7 +609,7 @@ class Http2Stream : public AsyncWrap, nghttp2_headers_category category, int options); - Http2Session* session_ = nullptr; // The Parent HTTP/2 Session + BaseObjectWeakPtr session_; // The Parent HTTP/2 Session int32_t id_ = 0; // The Stream Identifier int32_t code_ = NGHTTP2_NO_ERROR; // The RST_STREAM code (if any) int flags_ = NGHTTP2_STREAM_FLAG_NONE; // Internal state flags @@ -822,11 +822,11 @@ class Http2Session : public AsyncWrap, public StreamListener { return env()->event_loop(); } - std::unique_ptr PopPing(); - Http2Ping* AddPing(std::unique_ptr ping); + BaseObjectPtr PopPing(); + Http2Ping* AddPing(BaseObjectPtr ping); - std::unique_ptr PopSettings(); - Http2Settings* AddSettings(std::unique_ptr settings); + BaseObjectPtr PopSettings(); + Http2Settings* AddSettings(BaseObjectPtr settings); void IncrementCurrentSessionMemory(uint64_t amount) { current_session_memory_ += amount; @@ -1001,10 +1001,10 @@ class Http2Session : public AsyncWrap, public StreamListener { size_t stream_buf_offset_ = 0; size_t max_outstanding_pings_ = DEFAULT_MAX_PINGS; - std::queue> outstanding_pings_; + std::queue> outstanding_pings_; size_t max_outstanding_settings_ = DEFAULT_MAX_SETTINGS; - std::queue> outstanding_settings_; + std::queue> outstanding_settings_; std::vector outgoing_buffers_; std::vector outgoing_storage_;