From cd0f4c6652ed0bd4740cb85611b71821f79a9ee6 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Tue, 18 Jul 2017 17:24:41 -0700 Subject: [PATCH] http2: fix abort when client.destroy inside end event 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 | 68 ++++++++++--------- src/node_http2.cc | 0 src/node_http2.h | 6 +- src/node_http2_core-inl.h | 7 +- src/node_http2_core.h | 8 +-- ...test-http2-options-max-reserved-streams.js | 17 +++-- .../parallel/test-http2-response-splitting.js | 3 +- .../test-http2-server-socket-destroy.js | 7 +- 8 files changed, 58 insertions(+), 58 deletions(-) mode change 100644 => 100755 lib/internal/http2/core.js mode change 100644 => 100755 src/node_http2.cc mode change 100644 => 100755 src/node_http2.h mode change 100644 => 100755 src/node_http2_core-inl.h mode change 100644 => 100755 src/node_http2_core.h mode change 100644 => 100755 test/parallel/test-http2-options-max-reserved-streams.js mode change 100644 => 100755 test/parallel/test-http2-response-splitting.js mode change 100644 => 100755 test/parallel/test-http2-server-socket-destroy.js diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js old mode 100644 new mode 100755 index 1bdd57926c4e62..5c99dbc897ba6c --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -289,11 +289,9 @@ function onSessionRead(nread, buf, handle) { _unrefActive(this); // Reset the session timeout timer _unrefActive(stream); // Reset the stream timeout timer - if (nread >= 0) { + if (nread >= 0 && !stream.destroyed) { if (!stream.push(buf)) { - assert(this.streamReadStop(id) === undefined, - `HTTP/2 Stream ${id} does not exist. Please report this as ' + - 'a bug in Node.js`); + this.streamReadStop(id); state.reading = false; } } else { @@ -1475,44 +1473,48 @@ class Http2Stream extends Duplex { this.once('ready', this._destroy.bind(this, err, callback)); return; } - debug(`[${sessionName(session[kType])}] destroying stream ${this[kID]}`); - - // Submit RST-STREAM frame if one hasn't been sent already and the - // stream hasn't closed normally... - if (!this[kState].rst) { - const code = - err instanceof Error ? - NGHTTP2_INTERNAL_ERROR : NGHTTP2_NO_ERROR; - this[kSession].rstStream(this, code); - } - + process.nextTick(() => { + debug(`[${sessionName(session[kType])}] destroying stream ${this[kID]}`); + + // Submit RST-STREAM frame if one hasn't been sent already and the + // stream hasn't closed normally... + if (!this[kState].rst && !session.destroyed) { + const code = + err instanceof Error ? + NGHTTP2_INTERNAL_ERROR : NGHTTP2_NO_ERROR; + this[kSession].rstStream(this, code); + } - // Remove the close handler on the session - session.removeListener('close', this[kState].closeHandler); + // Remove the close handler on the session + session.removeListener('close', this[kState].closeHandler); - // Unenroll the timer - unenroll(this); + // Unenroll the timer + unenroll(this); - setImmediate(finishStreamDestroy.bind(this, handle)); - session[kState].streams.delete(this[kID]); - delete this[kSession]; + setImmediate(finishStreamDestroy.bind(this, handle)); - // All done - const rst = this[kState].rst; - const code = rst ? this[kState].rstCode : NGHTTP2_NO_ERROR; - if (code !== NGHTTP2_NO_ERROR) { - const err = new errors.Error('ERR_HTTP2_STREAM_ERROR', code); - process.nextTick(() => this.emit('error', err)); - } - process.nextTick(emit.bind(this, 'streamClosed', code)); - debug(`[${sessionName(session[kType])}] stream ${this[kID]} destroyed`); - callback(err); + // All done + const rst = this[kState].rst; + const code = rst ? this[kState].rstCode : NGHTTP2_NO_ERROR; + if (code !== NGHTTP2_NO_ERROR) { + const err = new errors.Error('ERR_HTTP2_STREAM_ERROR', code); + process.nextTick(() => this.emit('error', err)); + } + process.nextTick(emit.bind(this, 'streamClosed', code)); + debug(`[${sessionName(session[kType])}] stream ${this[kID]} destroyed`); + callback(err); + }); } } function finishStreamDestroy(handle) { + const id = this[kID]; + const session = this[kSession]; + session[kState].streams.delete(id); + delete this[kSession]; if (handle !== undefined) - handle.destroyStream(this[kID]); + handle.destroyStream(id); + this.emit('destroy'); } function processHeaders(headers) { diff --git a/src/node_http2.cc b/src/node_http2.cc old mode 100644 new mode 100755 diff --git a/src/node_http2.h b/src/node_http2.h old mode 100644 new mode 100755 index f6ccad29846d4a..c2dcd82e35948c --- a/src/node_http2.h +++ b/src/node_http2.h @@ -329,7 +329,6 @@ class Http2Session : public AsyncWrap, padding_strategy_ = opts.GetPaddingStrategy(); Init(env->event_loop(), type, *opts); - stream_buf_.AllocateSufficientStorage(kAllocBufferSize); } ~Http2Session() override { @@ -456,7 +455,7 @@ class Http2Session : public AsyncWrap, } char* stream_alloc() { - return *stream_buf_; + return stream_buf_; } private: @@ -464,7 +463,8 @@ class Http2Session : public AsyncWrap, StreamResource::Callback prev_alloc_cb_; StreamResource::Callback prev_read_cb_; padding_strategy_type padding_strategy_ = PADDING_STRATEGY_NONE; - MaybeStackBuffer stream_buf_; + + char stream_buf_[kAllocBufferSize]; }; class ExternalHeader : diff --git a/src/node_http2_core-inl.h b/src/node_http2_core-inl.h old mode 100644 new mode 100755 index 49ec63b59bd581..0659cb65a36940 --- a/src/node_http2_core-inl.h +++ b/src/node_http2_core-inl.h @@ -221,10 +221,7 @@ inline int Nghttp2Session::Free() { Nghttp2Session* session = ContainerOf(&Nghttp2Session::prep_, reinterpret_cast(handle)); - session->OnFreeSession(); - DEBUG_HTTP2("Nghttp2Session %d: session is free\n", - session->session_type_); }; uv_close(reinterpret_cast(&prep_), PrepClose); @@ -302,9 +299,9 @@ inline void Nghttp2Stream::ResetState( inline void Nghttp2Stream::Destroy() { DEBUG_HTTP2("Nghttp2Stream %d: destroying stream\n", id_); // Do nothing if this stream instance is already destroyed - if (IsDestroyed() || IsDestroying()) + if (IsDestroyed()) return; - flags_ |= NGHTTP2_STREAM_DESTROYING; + flags_ |= NGHTTP2_STREAM_DESTROYED; Nghttp2Session* session = this->session_; if (session != nullptr) { diff --git a/src/node_http2_core.h b/src/node_http2_core.h old mode 100644 new mode 100755 index 10acd7736b419f..3efeda69b58135 --- a/src/node_http2_core.h +++ b/src/node_http2_core.h @@ -65,9 +65,7 @@ enum nghttp2_stream_flags { // Stream is closed NGHTTP2_STREAM_CLOSED = 0x8, // Stream is destroyed - NGHTTP2_STREAM_DESTROYED = 0x10, - // Stream is being destroyed - NGHTTP2_STREAM_DESTROYING = 0x20 + NGHTTP2_STREAM_DESTROYED = 0x10 }; @@ -290,10 +288,6 @@ class Nghttp2Stream { return (flags_ & NGHTTP2_STREAM_DESTROYED) == NGHTTP2_STREAM_DESTROYED; } - inline bool IsDestroying() const { - return (flags_ & NGHTTP2_STREAM_DESTROYING) == NGHTTP2_STREAM_DESTROYING; - } - // Queue outbound chunks of data to be sent on this stream inline int Write( nghttp2_stream_write_t* req, diff --git a/test/parallel/test-http2-options-max-reserved-streams.js b/test/parallel/test-http2-options-max-reserved-streams.js old mode 100644 new mode 100755 index 1173b58e287de2..b01ed89de0a384 --- a/test/parallel/test-http2-options-max-reserved-streams.js +++ b/test/parallel/test-http2-options-max-reserved-streams.js @@ -51,6 +51,14 @@ server.on('listening', common.mustCall(() => { const client = h2.connect(`http://localhost:${server.address().port}`, options); + let remaining = 2; + function maybeClose() { + if (--remaining === 0) { + server.close(); + client.destroy(); + } + } + const req = client.request({ ':path': '/' }); // Because maxReservedRemoteStream is 1, the stream event @@ -59,15 +67,12 @@ server.on('listening', common.mustCall(() => { client.on('stream', common.mustCall((stream) => { stream.resume(); stream.on('end', common.mustCall()); + stream.on('streamClosed', common.mustCall(maybeClose)); })); req.on('response', common.mustCall()); req.resume(); - req.on('end', common.mustCall(() => { - server.close(); - client.destroy(); - })); - req.end(); - + req.on('end', common.mustCall()); + req.on('streamClosed', common.mustCall(maybeClose)); })); diff --git a/test/parallel/test-http2-response-splitting.js b/test/parallel/test-http2-response-splitting.js old mode 100644 new mode 100755 index 088c675389f5ba..cd3a5d39d7af01 --- a/test/parallel/test-http2-response-splitting.js +++ b/test/parallel/test-http2-response-splitting.js @@ -65,7 +65,8 @@ server.listen(0, common.mustCall(() => { assert.strictEqual(headers.location, undefined); })); req.resume(); - req.on('end', common.mustCall(maybeClose)); + req.on('end', common.mustCall()); + req.on('streamClosed', common.mustCall(maybeClose)); } doTest(str, 'location', str); diff --git a/test/parallel/test-http2-server-socket-destroy.js b/test/parallel/test-http2-server-socket-destroy.js old mode 100644 new mode 100755 index c10bbd0ccbe0c5..15b19ca1786f53 --- a/test/parallel/test-http2-server-socket-destroy.js +++ b/test/parallel/test-http2-server-socket-destroy.js @@ -36,7 +36,9 @@ function onStream(stream) { assert.notStrictEqual(stream.session, undefined); socket.destroy(); - assert.strictEqual(stream.session, undefined); + stream.on('destroy', common.mustCall(() => { + assert.strictEqual(stream.session, undefined); + })); } server.listen(0); @@ -49,9 +51,8 @@ server.on('listening', common.mustCall(() => { [HTTP2_HEADER_METHOD]: HTTP2_METHOD_POST }); req.on('aborted', common.mustCall()); + req.resume(); req.on('end', common.mustCall()); - req.on('response', common.mustCall()); - req.on('data', common.mustCall()); client.on('close', common.mustCall()); }));