From 723d1af5e7a201571e4185059aa8b0674b6d43de Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 3 Aug 2017 13:57:28 -0700 Subject: [PATCH] http2: fix flakiness in timeout Backport-PR-URL: https://github.com/nodejs/node/pull/14813 Backport-Reviewed-By: Anna Henningsen Backport-Reviewed-By: Timothy Gu PR-URL: https://github.com/nodejs/node/pull/14239 Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: Matteo Collina --- lib/internal/http2/core.js | 63 +++++++++++++--------- src/node_http2.cc | 8 ++- src/node_http2_core.cc | 2 +- test/parallel/test-http2-server-timeout.js | 3 +- 4 files changed, 47 insertions(+), 29 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index aebd5de049039d..da8edb18cfdef7 100755 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -673,15 +673,23 @@ function submitShutdown(options) { } function finishSessionDestroy(socket) { + const state = this[kState]; + if (state.destroyed) + return; + if (!socket.destroyed) socket.destroy(); + state.destroying = false; + state.destroyed = true; + // Destroy the handle const handle = this[kHandle]; if (handle !== undefined) { - handle.destroy(); + handle.destroy(state.skipUnconsume); debug(`[${sessionName(this[kType])}] nghttp2session handle destroyed`); } + this[kHandle] = undefined; process.nextTick(emit.bind(this, 'close')); debug(`[${sessionName(this[kType])}] nghttp2session destroyed`); @@ -825,7 +833,7 @@ class Http2Session extends EventEmitter { // Submits a SETTINGS frame to be sent to the remote peer. settings(settings) { - if (this[kState].destroyed) + if (this[kState].destroyed || this[kState].destroying) throw new errors.Error('ERR_HTTP2_INVALID_SESSION'); // Validate the input first @@ -871,7 +879,7 @@ class Http2Session extends EventEmitter { // Submits a PRIORITY frame to be sent to the remote peer. priority(stream, options) { - if (this[kState].destroyed) + if (this[kState].destroyed || this[kState].destroying) throw new errors.Error('ERR_HTTP2_INVALID_SESSION'); if (!(stream instanceof Http2Stream)) { @@ -905,6 +913,8 @@ class Http2Session extends EventEmitter { // Submits an RST-STREAM frame to be sent to the remote peer. This will // cause the stream to be closed. rstStream(stream, code = NGHTTP2_NO_ERROR) { + // Do not check destroying here, as the rstStream may be sent while + // the session is in the middle of being destroyed. if (this[kState].destroyed) throw new errors.Error('ERR_HTTP2_INVALID_SESSION'); @@ -946,7 +956,6 @@ class Http2Session extends EventEmitter { const state = this[kState]; if (state.destroyed || state.destroying) return; - debug(`[${sessionName(this[kType])}] destroying nghttp2session`); state.destroying = true; @@ -963,8 +972,8 @@ class Http2Session extends EventEmitter { delete this[kSocket]; delete this[kServer]; - state.destroyed = true; - state.destroying = false; + state.destroyed = false; + state.destroying = true; if (this[kHandle] !== undefined) this[kHandle].destroying(); @@ -975,7 +984,7 @@ class Http2Session extends EventEmitter { // Graceful or immediate shutdown of the Http2Session. Graceful shutdown // is only supported on the server-side shutdown(options, callback) { - if (this[kState].destroyed) + if (this[kState].destroyed || this[kState].destroying) throw new errors.Error('ERR_HTTP2_INVALID_SESSION'); if (this[kState].shutdown || this[kState].shuttingDown) @@ -1037,7 +1046,7 @@ class Http2Session extends EventEmitter { } _onTimeout() { - this.emit('timeout'); + process.nextTick(emit.bind(this, 'timeout')); } } @@ -1061,7 +1070,7 @@ class ClientHttp2Session extends Http2Session { // Submits a new HTTP2 request to the connected peer. Returns the // associated Http2Stream instance. request(headers, options) { - if (this[kState].destroyed) + if (this[kState].destroyed || this[kState].destroying) throw new errors.Error('ERR_HTTP2_INVALID_SESSION'); debug(`[${sessionName(this[kType])}] initiating request`); _unrefActive(this); @@ -1317,7 +1326,7 @@ class Http2Stream extends Duplex { } _onTimeout() { - this.emit('timeout'); + process.nextTick(emit.bind(this, 'timeout')); } // true if the Http2Stream was aborted abornomally. @@ -2104,7 +2113,7 @@ const onTimeout = { configurable: false, enumerable: false, value: function() { - this.emit('timeout'); + process.nextTick(emit.bind(this, 'timeout')); } }; @@ -2191,20 +2200,22 @@ function socketOnError(error) { // of the session. function socketOnTimeout() { debug('socket timeout'); - const server = this[kServer]; - 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 - }, - this.destroy.bind(this)); - } + process.nextTick(() => { + const server = this[kServer]; + 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 + }, + this.destroy.bind(this)); + } + }); } // Handles the on('stream') event for a session and forwards @@ -2346,6 +2357,8 @@ function setupCompat(ev) { function socketOnClose(hadError) { const session = this[kSession]; if (session !== undefined && !session.destroyed) { + // Skip unconsume because the socket is destroyed. + session[kState].skipUnconsume = true; session.destroy(); } } diff --git a/src/node_http2.cc b/src/node_http2.cc index 9bde7b4b5670f7..2b873595a89b9d 100755 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -407,7 +407,13 @@ void Http2Session::Destroy(const FunctionCallbackInfo& args) { Http2Session* session; ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); DEBUG_HTTP2("Http2Session: destroying session %d\n", session->type()); - session->Unconsume(); + Environment* env = Environment::GetCurrent(args); + Local context = env->context(); + + bool skipUnconsume = args[0]->BooleanValue(context).ToChecked(); + + if (!skipUnconsume) + session->Unconsume(); session->Free(); } diff --git a/src/node_http2_core.cc b/src/node_http2_core.cc index fc0c9be3075b9a..c71c2f286efdc5 100644 --- a/src/node_http2_core.cc +++ b/src/node_http2_core.cc @@ -176,7 +176,7 @@ void Nghttp2Session::GetTrailers(nghttp2_session* session, handle->OnTrailers(stream, &trailers); if (trailers.length() > 0) { DEBUG_HTTP2("Nghttp2Session %d: sending trailers for stream %d, " - "count: %d\n", handle->session_type_, id, + "count: %d\n", handle->session_type_, stream->id(), trailers.length()); *flags |= NGHTTP2_DATA_FLAG_NO_END_STREAM; nghttp2_submit_trailer(session, diff --git a/test/parallel/test-http2-server-timeout.js b/test/parallel/test-http2-server-timeout.js index df2ea1c3fe43e2..e56e79fc8623e8 100755 --- a/test/parallel/test-http2-server-timeout.js +++ b/test/parallel/test-http2-server-timeout.js @@ -9,11 +9,10 @@ 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.once('timeout', onServerTimeout); server.listen(0, common.mustCall(() => { const url = `http://localhost:${server.address().port}`;