http2: use custom BaseObject smart pointers

Refs: nodejs/quic#141 Reviewed-By: James M Snell <jasnell@gmail.com> PR-URL: #30374 Refs: nodejs/quic#149 Refs: nodejs/quic#165 Reviewed-By: David Carlier <devnexen@gmail.com>
nodejs · Nov 21, 2019 · 666588143e65482a6b509271de40ca19a795546e · 6665881
1 parent a2dbadc
commit 666588143e65482a6b509271de40ca19a795546e
Showing with 27 additions and 34 deletions.

+0 −4 src/base_object-inl.h

+0 −1 src/base_object.h

+18 −20 src/node_http2.cc

+9 −9 src/node_http2.h
diff --git 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<void*>(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
@@ -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
@@ -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<Http2Ping> ping = PopPing()) {
+  while (BaseObjectPtr<Http2Ping> ping = PopPing()) {
     ping->DetachFromSession();
     env()->SetImmediate(
         [ping = std::move(ping)](Environment* env) {
@@ -1483,7 +1480,7 @@ void Http2Session::HandlePingFrame(const nghttp2_frame* frame) {
   Local<Value> arg;
   bool ack = frame->hd.flags & NGHTTP2_FLAG_ACK;
   if (ack) {
-    std::unique_ptr<Http2Ping> ping = PopPing();
+    BaseObjectPtr<Http2Ping> 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<Http2Settings> settings = PopSettings();
+  BaseObjectPtr<Http2Settings> 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<Value>& args) {
   if (obj->Set(env->context(), env->ondone_string(), args[1]).IsNothing())
     return;
 
-  Http2Ping* ping = session->AddPing(std::make_unique<Http2Ping>(session, obj));
+  Http2Ping* ping = session->AddPing(
+      MakeDetachedBaseObject<Http2Ping>(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<Value>& args) {
   if (obj->Set(env->context(), env->ondone_string(), args[0]).IsNothing())
     return;
 
-  Http2Session::Http2Settings* settings = session->AddSettings(
-      std::make_unique<Http2Settings>(session->env(), session, obj, 0));
+  Http2Settings* settings = session->AddSettings(
+      MakeDetachedBaseObject<Http2Settings>(session->env(), session, obj, 0));
   if (settings == nullptr) return args.GetReturnValue().Set(false);
 
   settings->Send();
   args.GetReturnValue().Set(true);
 }
 
-std::unique_ptr<Http2Session::Http2Ping> Http2Session::PopPing() {
-  std::unique_ptr<Http2Ping> ping;
+BaseObjectPtr<Http2Session::Http2Ping> Http2Session::PopPing() {
+  BaseObjectPtr<Http2Ping> ping;
   if (!outstanding_pings_.empty()) {
     ping = std::move(outstanding_pings_.front());
     outstanding_pings_.pop();
@@ -2903,7 +2902,7 @@ std::unique_ptr<Http2Session::Http2Ping> Http2Session::PopPing() {
 }
 
 Http2Session::Http2Ping* Http2Session::AddPing(
-    std::unique_ptr<Http2Session::Http2Ping> ping) {
+    BaseObjectPtr<Http2Session::Http2Ping> 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::Http2Settings> Http2Session::PopSettings() {
-  std::unique_ptr<Http2Settings> settings;
+BaseObjectPtr<Http2Session::Http2Settings> Http2Session::PopSettings() {
+  BaseObjectPtr<Http2Settings> settings;
   if (!outstanding_settings_.empty()) {
     settings = std::move(outstanding_settings_.front());
     outstanding_settings_.pop();
@@ -2925,7 +2924,7 @@ std::unique_ptr<Http2Session::Http2Settings> Http2Session::PopSettings() {
 }
 
 Http2Session::Http2Settings* Http2Session::AddSettings(
-    std::unique_ptr<Http2Session::Http2Settings> settings) {
+    BaseObjectPtr<Http2Session::Http2Settings> settings) {
   if (outstanding_settings_.size() == max_outstanding_settings_) {
     settings->Done(false);
     return nullptr;
@@ -2940,7 +2939,6 @@ Http2Session::Http2Ping::Http2Ping(Http2Session* session, Local<Object> 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
@@ -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<Http2Session> 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<Http2Ping> PopPing();
-  Http2Ping* AddPing(std::unique_ptr<Http2Ping> ping);
+  BaseObjectPtr<Http2Ping> PopPing();
+  Http2Ping* AddPing(BaseObjectPtr<Http2Ping> ping);
 
-  std::unique_ptr<Http2Settings> PopSettings();
-  Http2Settings* AddSettings(std::unique_ptr<Http2Settings> settings);
+  BaseObjectPtr<Http2Settings> PopSettings();
+  Http2Settings* AddSettings(BaseObjectPtr<Http2Settings> 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<std::unique_ptr<Http2Ping>> outstanding_pings_;
+  std::queue<BaseObjectPtr<Http2Ping>> outstanding_pings_;
 
   size_t max_outstanding_settings_ = DEFAULT_MAX_SETTINGS;
-  std::queue<std::unique_ptr<Http2Settings>> outstanding_settings_;
+  std::queue<BaseObjectPtr<Http2Settings>> outstanding_settings_;
 
   std::vector<nghttp2_stream_write> outgoing_buffers_;
   std::vector<uint8_t> outgoing_storage_;