From aeed4a1fe9a2eb4a685579a50b101a36dc3011f5 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 24 Oct 2017 20:09:01 +0200 Subject: [PATCH 1/5] 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; From 41aaacb6b19d047edf71559c3adf48a20aa1b6b0 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 24 Oct 2017 23:22:07 +0200 Subject: [PATCH 2/5] src: add `InternalCallbackScope` util constructor Add an utility constructor for `AsyncWrap` classes that wish to leverage `InternalCallbackScope`s. PR-URL: https://github.com/nodejs/node/pull/16461 Reviewed-By: Anatoli Papirovski Reviewed-By: James M Snell Reviewed-By: Franziska Hinkelmann --- src/node.cc | 6 ++++++ src/node_internals.h | 2 ++ 2 files changed, 8 insertions(+) diff --git a/src/node.cc b/src/node.cc index 9f6c8084cfd3c9..33bf5199d63b01 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1373,6 +1373,12 @@ CallbackScope::~CallbackScope() { delete private_; } +InternalCallbackScope::InternalCallbackScope(AsyncWrap* async_wrap) + : InternalCallbackScope(async_wrap->env(), + async_wrap->object(), + { async_wrap->get_async_id(), + async_wrap->get_trigger_async_id() }) {} + InternalCallbackScope::InternalCallbackScope(Environment* env, Local object, const async_context& asyncContext, diff --git a/src/node_internals.h b/src/node_internals.h index 610347364d5eab..5d2f996cc8c1c5 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -303,6 +303,8 @@ class InternalCallbackScope { v8::Local object, const async_context& asyncContext, ResourceExpectation expect = kRequireResource); + // Utility that can be used by AsyncWrap classes. + explicit InternalCallbackScope(AsyncWrap* async_wrap); ~InternalCallbackScope(); void Close(); From 0c90c9c9438f58bf5819abd77b6de1cfa12ce7c2 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 24 Oct 2017 23:25:02 +0200 Subject: [PATCH 3/5] http2: track async state for sending Sending pending data may involve running arbitrary JavaScript code. Therefore, it should involve a callback scope. 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 | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/node_http2.cc b/src/node_http2.cc index 9c8f1293accb3d..9f5273c550600a 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -91,6 +91,12 @@ Http2Session::Http2Session(Environment* env, prep_->data = static_cast(this); uv_prepare_start(prep_, [](uv_prepare_t* t) { Http2Session* session = static_cast(t->data); + HandleScope scope(session->env()->isolate()); + Context::Scope context_scope(session->env()->context()); + + // Sending data may call arbitrary JS code, so keep track of + // async context. + InternalCallbackScope callback_scope(session); session->SendPendingData(); }); } From 3ddd15161184d92c892b49446f7884a6e26016f3 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 24 Oct 2017 23:27:20 +0200 Subject: [PATCH 4/5] http2: remove unused assignment When assigning to a union twice, the first assignment is a no-op. 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_core-inl.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/node_http2_core-inl.h b/src/node_http2_core-inl.h index 38deb4943b9580..11f48381ef22ca 100644 --- a/src/node_http2_core-inl.h +++ b/src/node_http2_core-inl.h @@ -794,7 +794,6 @@ inline int Nghttp2Stream::SubmitFile(int fd, DEBUG_HTTP2("Nghttp2Stream %d: submitting file\n", id_); getTrailers_ = options & STREAM_OPTION_GET_TRAILERS; nghttp2_data_provider prov; - prov.source.ptr = this; prov.source.fd = fd; prov.read_callback = Nghttp2Session::OnStreamReadFD; From c8a00fdf62ff2645fe4be515811429d08ba3d641 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 24 Oct 2017 23:41:48 +0200 Subject: [PATCH 5/5] http2: make sessions garbage-collectible Use weak handles to track the lifetime of an `Http2Session` instance. Refs: https://github.com/nodejs/http2/issues/140 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 | 15 ++++++++++----- src/node_http2.h | 2 ++ src/node_http2_core-inl.h | 12 +++++++----- src/node_http2_core.h | 6 +++++- 4 files changed, 24 insertions(+), 11 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 9f5273c550600a..744d55078501a8 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -75,7 +75,7 @@ Http2Session::Http2Session(Environment* env, nghttp2_session_type type) : AsyncWrap(env, wrap, AsyncWrap::PROVIDER_HTTP2SESSION), StreamBase(env) { - Wrap(object(), this); + MakeWeak(this); Http2Options opts(env); @@ -102,11 +102,16 @@ Http2Session::Http2Session(Environment* env, } Http2Session::~Http2Session() { - CHECK_EQ(false, persistent().IsEmpty()); - ClearWrap(object()); + CHECK(persistent().IsEmpty()); + Close(); +} + +void Http2Session::Close() { + if (!object().IsEmpty()) + ClearWrap(object()); persistent().Reset(); - CHECK_EQ(true, persistent().IsEmpty()); + this->Nghttp2Session::Close(); // Stop the loop CHECK_EQ(uv_prepare_stop(prep_), 0); auto prep_close = [](uv_handle_t* handle) { @@ -407,7 +412,7 @@ void Http2Session::Destroy(const FunctionCallbackInfo& args) { if (!skipUnconsume) session->Unconsume(); - session->Free(); + session->Close(); } void Http2Session::Destroying(const FunctionCallbackInfo& args) { diff --git a/src/node_http2.h b/src/node_http2.h index 65cb12578fe071..1006dff3ced70b 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -474,6 +474,8 @@ class Http2Session : public AsyncWrap, return stream_buf_; } + void Close() override; + private: StreamBase* stream_; StreamResource::Callback prev_alloc_cb_; diff --git a/src/node_http2_core-inl.h b/src/node_http2_core-inl.h index 11f48381ef22ca..fa365fee059b4d 100644 --- a/src/node_http2_core-inl.h +++ b/src/node_http2_core-inl.h @@ -587,16 +587,18 @@ inline void Nghttp2Session::MarkDestroying() { destroying_ = true; } -inline int Nghttp2Session::Free() { -#if defined(DEBUG) && DEBUG - CHECK(session_ != nullptr); -#endif +inline Nghttp2Session::~Nghttp2Session() { + Close(); +} + +inline void Nghttp2Session::Close() { + if (IsClosed()) + return; DEBUG_HTTP2("Nghttp2Session %s: freeing session\n", TypeName()); nghttp2_session_terminate_session(session_, NGHTTP2_NO_ERROR); nghttp2_session_del(session_); session_ = nullptr; DEBUG_HTTP2("Nghttp2Session %s: session freed\n", TypeName()); - return 1; } // Write data received from the socket to the underlying nghttp2_session. diff --git a/src/node_http2_core.h b/src/node_http2_core.h index 8724b03e76cba6..2e885b73dab8f5 100644 --- a/src/node_http2_core.h +++ b/src/node_http2_core.h @@ -97,7 +97,7 @@ class Nghttp2Session { nghttp2_mem* mem = nullptr); // Frees this session instance - inline int Free(); + inline ~Nghttp2Session(); inline void MarkDestroying(); bool IsDestroying() { return destroying_; @@ -140,6 +140,8 @@ class Nghttp2Session { // Returns the nghttp2 library session inline nghttp2_session* session() const { return session_; } + inline bool IsClosed() const { return session_ == nullptr; } + nghttp2_session_type type() const { return session_type_; } @@ -201,6 +203,8 @@ class Nghttp2Session { virtual uv_loop_t* event_loop() const = 0; + virtual void Close(); + private: inline void HandleHeadersFrame(const nghttp2_frame* frame); inline void HandlePriorityFrame(const nghttp2_frame* frame);