http2: move uv_prepare handle to Http2Session

As far as I understand it, `Nghttp2Session` should not be concerned about how its consumers handle I/O. PR-URL: nodejs/node#16461 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
ayojs · Nov 2, 2017 · b60918b · b60918b
1 parent 4ad11b5
commit b60918b
Show file tree

Hide file tree

Showing 4 changed files with 55 additions and 51 deletions.
diff --git a/src/node_http2.cc b/src/node_http2.cc
@@ -69,8 +69,45 @@ Http2Options::Http2Options(Environment* env) {
   }
 }
 
-void Http2Session::OnFreeSession() {
-  ::delete this;
+
+Http2Session::Http2Session(Environment* env,
+                           Local<Object> wrap,
+                           nghttp2_session_type type)
+    : AsyncWrap(env, wrap, AsyncWrap::PROVIDER_HTTP2SESSION),
+      StreamBase(env) {
+  Wrap(object(), this);
+
+  Http2Options opts(env);
+
+  padding_strategy_ = opts.GetPaddingStrategy();
+
+  Init(type, *opts);
+
+  // For every node::Http2Session instance, there is a uv_prepare_t handle
+  // whose callback is triggered on every tick of the event loop. When
+  // run, nghttp2 is prompted to send any queued data it may have stored.
+  prep_ = new uv_prepare_t();
+  uv_prepare_init(env->event_loop(), prep_);
+  prep_->data = static_cast<void*>(this);
+  uv_prepare_start(prep_, [](uv_prepare_t* t) {
+    Http2Session* session = static_cast<Http2Session*>(t->data);
+    session->SendPendingData();
+  });
+}
+
+Http2Session::~Http2Session() {
+  CHECK_EQ(false, persistent().IsEmpty());
+  ClearWrap(object());
+  persistent().Reset();
+  CHECK_EQ(true, persistent().IsEmpty());
+
+  // Stop the loop
+  CHECK_EQ(uv_prepare_stop(prep_), 0);
+  auto prep_close = [](uv_handle_t* handle) {
+    delete reinterpret_cast<uv_prepare_t*>(handle);
+  };
+  uv_close(reinterpret_cast<uv_handle_t*>(prep_), prep_close);
+  prep_ = nullptr;
 }
 
 ssize_t Http2Session::OnMaxFrameSizePadding(size_t frameLen,

diff --git a/src/node_http2.h b/src/node_http2.h
@@ -343,24 +343,8 @@ class Http2Session : public AsyncWrap,
  public:
   Http2Session(Environment* env,
                Local<Object> wrap,
-               nghttp2_session_type type) :
-               AsyncWrap(env, wrap, AsyncWrap::PROVIDER_HTTP2SESSION),
-               StreamBase(env) {
-    Wrap(object(), this);
-
-    Http2Options opts(env);
-
-    padding_strategy_ = opts.GetPaddingStrategy();
-
-    Init(env->event_loop(), type, *opts);
-  }
-
-  ~Http2Session() override {
-    CHECK_EQ(false, persistent().IsEmpty());
-    ClearWrap(object());
-    persistent().Reset();
-    CHECK_EQ(true, persistent().IsEmpty());
-  }
+               nghttp2_session_type type);
+  ~Http2Session() override;
 
   static void OnStreamAllocImpl(size_t suggested_size,
                                 uv_buf_t* buf,
@@ -369,9 +353,8 @@ class Http2Session : public AsyncWrap,
                                const uv_buf_t* bufs,
                                uv_handle_type pending,
                                void* ctx);
- protected:
-  void OnFreeSession() override;
 
+ protected:
   ssize_t OnMaxFrameSizePadding(size_t frameLength,
                                 size_t maxPayloadLen);
 
@@ -449,6 +432,9 @@ class Http2Session : public AsyncWrap,
     return 0;
   }
 
+  uv_loop_t* event_loop() const override {
+    return env()->event_loop();
+  }
  public:
   void Consume(Local<External> external);
   void Unconsume();
@@ -496,6 +482,7 @@ class Http2Session : public AsyncWrap,
 
   // use this to allow timeout tracking during long-lasting writes
   uint32_t chunks_sent_since_last_write_ = 0;
+  uv_prepare_t* prep_ = nullptr;
 
   char stream_buf_[kAllocBufferSize];
 };

diff --git a/src/node_http2_core-inl.h b/src/node_http2_core-inl.h
@@ -193,7 +193,8 @@ inline ssize_t Nghttp2Session::OnStreamReadFD(nghttp2_session* session,
   uv_fs_t read_req;
 
   if (length > 0) {
-    numchars = uv_fs_read(handle->loop_,
+    // TODO(addaleax): Never use synchronous I/O on the main thread.
+    numchars = uv_fs_read(handle->event_loop(),
                           &read_req,
                           fd, &data, 1,
                           offset, nullptr);
@@ -541,11 +542,9 @@ inline void Nghttp2Session::SendPendingData() {
 // Initialize the Nghttp2Session handle by creating and
 // assigning the Nghttp2Session instance and associated
 // uv_loop_t.
-inline int Nghttp2Session::Init(uv_loop_t* loop,
-                               const nghttp2_session_type type,
-                               nghttp2_option* options,
-                               nghttp2_mem* mem) {
-  loop_ = loop;
+inline int Nghttp2Session::Init(const nghttp2_session_type type,
+                                nghttp2_option* options,
+                                nghttp2_mem* mem) {
   session_type_ = type;
   DEBUG_HTTP2("Nghttp2Session %s: initializing session\n", TypeName());
   destroying_ = false;
@@ -581,14 +580,6 @@ inline int Nghttp2Session::Init(uv_loop_t* loop,
     nghttp2_option_del(opts);
   }
 
-  // For every node::Http2Session instance, there is a uv_prep_t handle
-  // whose callback is triggered on every tick of the event loop. When
-  // run, nghttp2 is prompted to send any queued data it may have stored.
-  uv_prepare_init(loop_, &prep_);
-  uv_prepare_start(&prep_, [](uv_prepare_t* t) {
-    Nghttp2Session* session = ContainerOf(&Nghttp2Session::prep_, t);
-    session->SendPendingData();
-  });
   return ret;
 }
 
@@ -601,19 +592,9 @@ inline int Nghttp2Session::Free() {
   CHECK(session_ != nullptr);
 #endif
   DEBUG_HTTP2("Nghttp2Session %s: freeing session\n", TypeName());
-  // Stop the loop
-  CHECK_EQ(uv_prepare_stop(&prep_), 0);
-  auto PrepClose = [](uv_handle_t* handle) {
-    Nghttp2Session* session =
-        ContainerOf(&Nghttp2Session::prep_,
-                    reinterpret_cast<uv_prepare_t*>(handle));
-    session->OnFreeSession();
-  };
-  uv_close(reinterpret_cast<uv_handle_t*>(&prep_), PrepClose);
   nghttp2_session_terminate_session(session_, NGHTTP2_NO_ERROR);
   nghttp2_session_del(session_);
   session_ = nullptr;
-  loop_ = nullptr;
   DEBUG_HTTP2("Nghttp2Session %s: session freed\n", TypeName());
   return 1;
 }

diff --git a/src/node_http2_core.h b/src/node_http2_core.h
@@ -92,7 +92,6 @@ class Nghttp2Session {
  public:
   // Initializes the session instance
   inline int Init(
-      uv_loop_t*,
       const nghttp2_session_type type = NGHTTP2_SESSION_SERVER,
       nghttp2_option* options = nullptr,
       nghttp2_mem* mem = nullptr);
@@ -175,7 +174,6 @@ class Nghttp2Session {
                             int error_code) {}
   virtual ssize_t GetPadding(size_t frameLength,
                              size_t maxFrameLength) { return 0; }
-  virtual void OnFreeSession() {}
   virtual void AllocateSend(uv_buf_t* buf) = 0;
 
   virtual bool HasGetPaddingCallback() { return false; }
@@ -199,8 +197,11 @@ class Nghttp2Session {
   virtual void OnTrailers(Nghttp2Stream* stream,
                           const SubmitTrailers& submit_trailers) {}
 
- private:
   inline void SendPendingData();
+
+  virtual uv_loop_t* event_loop() const = 0;
+
+ private:
   inline void HandleHeadersFrame(const nghttp2_frame* frame);
   inline void HandlePriorityFrame(const nghttp2_frame* frame);
   inline void HandleDataFrame(const nghttp2_frame* frame);
@@ -281,8 +282,6 @@ class Nghttp2Session {
   static const Callbacks callback_struct_saved[2];
 
   nghttp2_session* session_;
-  uv_loop_t* loop_;
-  uv_prepare_t prep_;
   nghttp2_session_type session_type_;
   std::unordered_map<int32_t, Nghttp2Stream*> streams_;
   bool destroying_ = false;