From 224ea159aea56cc56b822fe3f75285f899572b1a Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 24 Oct 2017 20:09:01 +0200 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/16461 Reviewed-By: Anatoli Papirovski Reviewed-By: James M Snell Reviewed-By: Franziska Hinkelmann --- src/node_http2.cc | 41 +++++++++++++++++++++++++++++++++++++-- src/node_http2.h | 27 +++++++------------------- src/node_http2_core-inl.h | 29 +++++---------------------- src/node_http2_core.h | 9 ++++----- 4 files changed, 55 insertions(+), 51 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index d12b9ca000de39..9c8f1293accb3d 100644 --- 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 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(this); + uv_prepare_start(prep_, [](uv_prepare_t* t) { + Http2Session* session = static_cast(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(handle); + }; + uv_close(reinterpret_cast(prep_), prep_close); + prep_ = nullptr; } ssize_t Http2Session::OnMaxFrameSizePadding(size_t frameLen, diff --git a/src/node_http2.h b/src/node_http2.h index 1b24b97ff048f0..65cb12578fe071 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -343,24 +343,8 @@ class Http2Session : public AsyncWrap, public: Http2Session(Environment* env, Local 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); 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 index 67bacfbbf07842..38deb4943b9580 100644 --- 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(handle)); - session->OnFreeSession(); - }; - uv_close(reinterpret_cast(&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 index a029366a9d433e..8724b03e76cba6 100644 --- 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 Callbacks callback_struct_saved[2]; nghttp2_session* session_; - uv_loop_t* loop_; - uv_prepare_t prep_; nghttp2_session_type session_type_; std::unordered_map streams_; bool destroying_ = false;