From 754d713ca2d5fe09184531cdfc6549dd0f945984 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Wed, 19 Jul 2017 12:59:39 -0700 Subject: [PATCH] http2: fix socketOnTimeout and a segfault Fixes: https://github.com/nodejs/http2/issues/179 Was fixing issue #179 and encountered a segault that was happening somewhat randomly on session destruction. Both should be fixed --- lib/internal/http2/core.js | 29 +++++++++++++++++----- src/node_http2.cc | 14 +++++++++-- src/node_http2.h | 1 + src/node_http2_core-inl.h | 12 ++++++++- src/node_http2_core.h | 9 +++++++ test/parallel/test-http2-server-timeout.js | 27 ++++++++++++++++++++ 6 files changed, 83 insertions(+), 9 deletions(-) create mode 100755 test/parallel/test-http2-server-timeout.js diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 1dfbe608842084..38cd3160298b73 100755 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -670,7 +670,7 @@ function finishSessionDestroy(socket) { debug(`[${sessionName(this[kType])}] nghttp2session handle destroyed`); } - this.emit('close'); + process.nextTick(emit.bind(this, 'close')); debug(`[${sessionName(this[kType])}] nghttp2session destroyed`); } @@ -953,6 +953,9 @@ class Http2Session extends EventEmitter { state.destroyed = true; state.destroying = false; + if (this[kHandle] !== undefined) + this[kHandle].destroying(); + setImmediate(finishSessionDestroy.bind(this, socket)); } @@ -1985,6 +1988,7 @@ function socketDestroy(error) { const type = this[kSession][kType]; debug(`[${sessionName(type)}] socket destroy called`); delete this[kServer]; + this.removeListener('timeout', socketOnTimeout); // destroy the session first so that it will stop trying to // send data while we close the socket. this[kSession].destroy(); @@ -2046,14 +2050,18 @@ function socketOnError(error) { this.destroy(error); } -// When the socket times out, attempt a graceful shutdown -// of the session +// When the socket times out on the server, attempt a graceful shutdown +// of the session. function socketOnTimeout() { debug('socket timeout'); const server = this[kServer]; - // server can be null if the socket is a client - if (server === undefined || !server.emit('timeout', this)) { - this[kSession].shutdown( + const session = this[kSession]; + // If server or session are undefined, then we're already in the process of + // shutting down, do nothing. + if (server === undefined || session === undefined) + return; + if (!server.emit('timeout', session, this)) { + session.shutdown( { graceful: true, errorCode: NGHTTP2_NO_ERROR @@ -2105,6 +2113,7 @@ function connectionListener(socket) { socket.on('resume', socketOnResume); socket.on('pause', socketOnPause); socket.on('drain', socketOnDrain); + socket.on('close', socketOnClose); // Set up the Session const session = new ServerHttp2Session(options, socket, this); @@ -2197,6 +2206,13 @@ function setupCompat(ev) { } } +function socketOnClose(hadError) { + const session = this[kSession]; + if (session !== undefined && !session.destroyed) { + session.destroy(); + } +} + // If the session emits an error, forward it to the socket as a sessionError; // failing that, destroy the session, remove the listener and re-emit the error function clientSessionOnError(error) { @@ -2244,6 +2260,7 @@ function connect(authority, options, listener) { socket.on('resume', socketOnResume); socket.on('pause', socketOnPause); socket.on('drain', socketOnDrain); + socket.on('close', socketOnClose); const session = new ClientHttp2Session(options, socket); diff --git a/src/node_http2.cc b/src/node_http2.cc index 5ad1352cc108dd..61a98758b1cc9e 100755 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -401,13 +401,21 @@ void Http2Session::Consume(const FunctionCallbackInfo& args) { } void Http2Session::Destroy(const FunctionCallbackInfo& args) { - DEBUG_HTTP2("Http2Session: destroying session\n"); Http2Session* session; ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); + DEBUG_HTTP2("Http2Session: destroying session %d\n", session->type()); session->Unconsume(); session->Free(); } +void Http2Session::Destroying(const FunctionCallbackInfo& args) { + Http2Session* session; + ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); + DEBUG_HTTP2("Http2Session: preparing to destroy session %d\n", + session->type()); + session->MarkDestroying(); +} + void Http2Session::SubmitPriority(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Http2Session* session; @@ -816,11 +824,11 @@ void Http2Session::AllocateSend(size_t recommended, uv_buf_t* buf) { } void Http2Session::Send(uv_buf_t* buf, size_t length) { + DEBUG_HTTP2("Http2Session: Attempting to send data\n"); if (stream_ == nullptr || !stream_->IsAlive() || stream_->IsClosing()) { return; } HandleScope scope(env()->isolate()); - auto AfterWrite = [](WriteWrap* req_wrap, int status) { req_wrap->Dispose(); }; @@ -1191,6 +1199,8 @@ void Initialize(Local target, Http2Session::Consume); env->SetProtoMethod(session, "destroy", Http2Session::Destroy); + env->SetProtoMethod(session, "destroying", + Http2Session::Destroying); env->SetProtoMethod(session, "sendHeaders", Http2Session::SendHeaders); env->SetProtoMethod(session, "submitShutdownNotice", diff --git a/src/node_http2.h b/src/node_http2.h index c2dcd82e35948c..49f6e55ebf377e 100755 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -429,6 +429,7 @@ class Http2Session : public AsyncWrap, static void New(const FunctionCallbackInfo& args); static void Consume(const FunctionCallbackInfo& args); static void Unconsume(const FunctionCallbackInfo& args); + static void Destroying(const FunctionCallbackInfo& args); static void Destroy(const FunctionCallbackInfo& args); static void SubmitSettings(const FunctionCallbackInfo& args); static void SubmitRstStream(const FunctionCallbackInfo& args); diff --git a/src/node_http2_core-inl.h b/src/node_http2_core-inl.h index 0659cb65a36940..c79d412e6384cf 100755 --- a/src/node_http2_core-inl.h +++ b/src/node_http2_core-inl.h @@ -136,6 +136,12 @@ inline void Nghttp2Session::HandleGoawayFrame(const nghttp2_frame* frame) { // Prompts nghttp2 to flush the queue of pending data frames inline void Nghttp2Session::SendPendingData() { + DEBUG_HTTP2("Nghttp2Session %d: Sending pending data\n", session_type_); + // Do not attempt to send data on the socket if the destroying flag has + // been set. That means everything is shutting down and the socket + // will not be usable. + if (IsDestroying()) + return; const uint8_t* data; ssize_t len = 0; size_t ncopy = 0; @@ -167,6 +173,7 @@ inline int Nghttp2Session::Init(uv_loop_t* loop, DEBUG_HTTP2("Nghttp2Session %d: initializing session\n", type); loop_ = loop; session_type_ = type; + destroying_ = false; int ret = 0; nghttp2_session_callbacks* callbacks @@ -211,6 +218,9 @@ inline int Nghttp2Session::Init(uv_loop_t* loop, return ret; } +inline void Nghttp2Session::MarkDestroying() { + destroying_ = true; +} inline int Nghttp2Session::Free() { assert(session_ != nullptr); @@ -224,11 +234,11 @@ inline int Nghttp2Session::Free() { 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 %d: session freed\n", session_type_); return 1; } diff --git a/src/node_http2_core.h b/src/node_http2_core.h index 3efeda69b58135..4c3fc5549c9d6c 100755 --- a/src/node_http2_core.h +++ b/src/node_http2_core.h @@ -100,6 +100,10 @@ class Nghttp2Session { // Frees this session instance inline int Free(); + inline void MarkDestroying(); + bool IsDestroying() { + return destroying_; + } // Returns the pointer to the identified stream, or nullptr if // the stream does not exist @@ -128,6 +132,10 @@ class Nghttp2Session { // Returns the nghttp2 library session inline nghttp2_session* session() { return session_; } + nghttp2_session_type type() { + return session_type_; + } + protected: // Adds a stream instance to this session inline void AddStream(Nghttp2Stream* stream); @@ -240,6 +248,7 @@ class Nghttp2Session { uv_prepare_t prep_; nghttp2_session_type session_type_; std::unordered_map streams_; + bool destroying_ = false; friend class Nghttp2Stream; }; diff --git a/test/parallel/test-http2-server-timeout.js b/test/parallel/test-http2-server-timeout.js new file mode 100755 index 00000000000000..df2ea1c3fe43e2 --- /dev/null +++ b/test/parallel/test-http2-server-timeout.js @@ -0,0 +1,27 @@ +// Flags: --expose-http2 +'use strict'; + +const common = require('../common'); +const http2 = require('http2'); + +const server = http2.createServer(); +server.setTimeout(common.platformTimeout(1)); + +const onServerTimeout = common.mustCall((session) => { + session.destroy(); + server.removeListener('timeout', onServerTimeout); +}); + +server.on('stream', common.mustNotCall()); +server.on('timeout', onServerTimeout); + +server.listen(0, common.mustCall(() => { + const url = `http://localhost:${server.address().port}`; + const client = http2.connect(url); + client.on('close', common.mustCall(() => { + + const client2 = http2.connect(url); + client2.on('close', common.mustCall(() => server.close())); + + })); +}));