diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 04acb5d30f6..d3695145115 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -348,6 +348,7 @@ const STREAM_FLAGS_HEADERS_SENT = 0x4; const STREAM_FLAGS_HEAD_REQUEST = 0x8; const STREAM_FLAGS_ABORTED = 0x10; const STREAM_FLAGS_HAS_TRAILERS = 0x20; +const STREAM_FLAGS_RST_DEFERRED = 0x40; const SESSION_FLAGS_PENDING = 0x0; const SESSION_FLAGS_READY = 0x1; @@ -644,6 +645,8 @@ function onStreamClose(code, peerReset) { stream[kState].fd = -1; if (locallyInitiated) { + // nghttp2 has already closed the stream, a deferred RST_STREAM is moot. + stream[kState].flags &= ~STREAM_FLAGS_RST_DEFERRED; stream.destroy(); return true; } @@ -2166,6 +2169,9 @@ function closeStream(stream, code, rstStreamStatus = kSubmitRstStream) { rstStreamStatus === kForceRstStream) { finishCloseStream.call(stream, code); } else { + // Remember that the RST_STREAM is still owed, so that destroying the + // stream before 'finish' can submit it. See Http2Stream#_destroy(). + state.flags |= STREAM_FLAGS_RST_DEFERRED; stream.once('finish', finishCloseStream.bind(stream, code)); } } @@ -2188,6 +2194,7 @@ function closeStream(stream, code, rstStreamStatus = kSubmitRstStream) { } function finishCloseStream(code) { + this[kState].flags &= ~STREAM_FLAGS_RST_DEFERRED; const rstStreamFn = submitRstStream.bind(this, code); // Defer until the stream has an ID assigned. if (this.pending) { @@ -2681,8 +2688,16 @@ class Http2Stream extends Duplex { } const hasHandle = handle !== undefined; - if (!this.closed) + if (!this.closed) { closeStream(this, code, hasHandle ? kForceRstStream : kNoRstStream); + } else if (state.flags & STREAM_FLAGS_RST_DEFERRED) { + // close() deferred the RST_STREAM until 'finish', which will never be + // emitted now. Submit it here, otherwise nghttp2 keeps the stream open + // with a data provider that no longer has a stream to read from. + state.flags &= ~STREAM_FLAGS_RST_DEFERRED; + if (hasHandle) + handle.rstStream(code); + } this.push(null); diff --git a/src/node_http2.cc b/src/node_http2.cc index a6779610ddb..f865da0accc 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -2840,7 +2840,10 @@ ssize_t Http2Stream::Provider::Stream::OnRead(nghttp2_session* handle, Http2Session* session = static_cast(user_data); Debug(session, "reading outbound data for stream %d", id); BaseObjectPtr stream = session->FindStream(id); - if (!stream) return 0; + // The stream was destroyed without nghttp2 being told. Returning 0 here + // would make nghttp2 send empty DATA frames forever, so have it reset + // the stream instead. + if (!stream) return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; if (stream->statistics_.first_byte_sent == 0) stream->statistics_.first_byte_sent = uv_hrtime(); CHECK_EQ(id, stream->id()); diff --git a/test/parallel/test-http2-close-destroy-queued-body.js b/test/parallel/test-http2-close-destroy-queued-body.js new file mode 100644 index 00000000000..68cc846ffbc --- /dev/null +++ b/test/parallel/test-http2-close-destroy-queued-body.js @@ -0,0 +1,46 @@ +'use strict'; + +// Destroying a stream that was closed with NO_ERROR while its request body +// was still queued behind flow control must still send RST_STREAM. +// Refs: https://github.com/nodejs/node/issues/67363 + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const assert = require('assert'); +const http2 = require('http2'); +const { NGHTTP2_NO_ERROR } = http2.constants; + +// A small stream window makes sure that most of the body stays queued. +const server = http2.createServer({ settings: { initialWindowSize: 16384 } }); + +let req; + +server.on('stream', common.mustCall((stream) => { + // The stream is not being read yet, so the request body is stuck behind + // flow control when the client closes and destroys the request. + req.close(); + req.destroy(); + + stream.on('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ABORTED', + })); + stream.on('close', common.mustCall(() => { + assert.strictEqual(stream.rstCode, NGHTTP2_NO_ERROR); + server.close(); + })); + // Open the window again. Without the RST_STREAM this is what makes the + // client loop. + stream.resume(); +})); + +server.listen(0, common.mustCall(() => { + const client = http2.connect(`http://localhost:${server.address().port}`); + req = client.request({ ':method': 'POST' }); + req.on('error', common.mustNotCall()); + req.on('close', common.mustCall(() => { + assert.strictEqual(req.rstCode, NGHTTP2_NO_ERROR); + client.close(); + })); + req.end(Buffer.alloc(1024 * 1024)); +})); diff --git a/test/parallel/test-http2-server-close-destroy-queued-body.js b/test/parallel/test-http2-server-close-destroy-queued-body.js new file mode 100644 index 00000000000..635b8fe5241 --- /dev/null +++ b/test/parallel/test-http2-server-close-destroy-queued-body.js @@ -0,0 +1,51 @@ +'use strict'; + +// Destroying a stream that was closed with NO_ERROR while its response body +// was still queued behind flow control must still send RST_STREAM. +// Refs: https://github.com/nodejs/node/issues/67363 + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const assert = require('assert'); +const http2 = require('http2'); +const { NGHTTP2_NO_ERROR } = http2.constants; + +const server = http2.createServer(); + +let serverStream; + +server.on('stream', common.mustCall((stream) => { + serverStream = stream; + stream.on('error', common.mustNotCall()); + stream.on('close', common.mustCall(() => { + assert.strictEqual(stream.rstCode, NGHTTP2_NO_ERROR); + })); + stream.respond(); + stream.end(Buffer.alloc(1024 * 1024)); +})); + +server.listen(0, common.mustCall(() => { + // A small stream window makes sure that most of the body stays queued. + const client = http2.connect(`http://localhost:${server.address().port}`, + { settings: { initialWindowSize: 16384 } }); + const req = client.request(); + req.on('response', common.mustCall(() => { + // The response is not being read yet, so the response body is stuck + // behind flow control when the server closes and destroys the stream. + serverStream.close(); + serverStream.destroy(); + // Open the window again. Without the RST_STREAM this is what makes the + // server loop. + req.resume(); + })); + req.on('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ABORTED', + })); + req.on('close', common.mustCall(() => { + assert.strictEqual(req.rstCode, NGHTTP2_NO_ERROR); + client.close(); + server.close(); + })); + req.end(); +}));