From bae75ef39129c972b4f3dd707d692661ef070e57 Mon Sep 17 00:00:00 2001 From: Santiago Gimeno Date: Thu, 27 Oct 2022 11:34:45 +0200 Subject: [PATCH] src: let http2 streams end after session close After the stream has been marked as closed by the nghttp2 stack, there might be still pending data to be sent: trailing headers is an example of this. In that case, avoid reentering the nghttp2 stack synchronously to allow the data to be written before actually closing the stream. Fixes: https://github.com/nodejs/node/issues/42713 PR-URL: https://github.com/nodejs/node/pull/45153 Backport-PR-URL: https://github.com/nodejs/node/pull/45660 Reviewed-By: Colin Ihrig Reviewed-By: Paolo Insogna Reviewed-By: Matteo Collina Reviewed-By: Rafael Gonzaga --- src/node_http2.cc | 11 +++++ ...test-http2-trailers-after-session-close.js | 49 +++++++++++++++++++ 2 files changed, 60 insertions(+) create mode 100644 test/parallel/test-http2-trailers-after-session-close.js diff --git a/src/node_http2.cc b/src/node_http2.cc index 53216dcee8b..42694cccdbe 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1115,6 +1115,17 @@ int Http2Session::OnStreamClose(nghttp2_session* handle, if (!stream || stream->is_destroyed()) return 0; + // Don't close synchronously in case there's pending data to be written. This + // may happen when writing trailing headers. + if (code == NGHTTP2_NO_ERROR && nghttp2_session_want_write(handle) && + !env->is_stopping()) { + env->SetImmediate([handle, id, code, user_data](Environment* env) { + OnStreamClose(handle, id, code, user_data); + }); + + return 0; + } + stream->Close(code); // It is possible for the stream close to occur before the stream is diff --git a/test/parallel/test-http2-trailers-after-session-close.js b/test/parallel/test-http2-trailers-after-session-close.js new file mode 100644 index 00000000000..d08f8754941 --- /dev/null +++ b/test/parallel/test-http2-trailers-after-session-close.js @@ -0,0 +1,49 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const assert = require('assert'); +const http2 = require('http2'); + +const { + HTTP2_HEADER_PATH, + HTTP2_HEADER_STATUS, + HTTP2_HEADER_METHOD, +} = http2.constants; + +const server = http2.createServer(); +server.on('stream', common.mustCall((stream) => { + server.close(); + stream.session.close(); + stream.on('wantTrailers', common.mustCall(() => { + stream.sendTrailers({ xyz: 'abc' }); + })); + + stream.respond({ [HTTP2_HEADER_STATUS]: 200 }, { waitForTrailers: true }); + stream.write('some data'); + stream.end(); +})); + +server.listen(0, common.mustCall(() => { + const port = server.address().port; + const client = http2.connect(`http://localhost:${port}`); + client.socket.on('close', common.mustCall()); + const req = client.request({ + [HTTP2_HEADER_PATH]: '/', + [HTTP2_HEADER_METHOD]: 'POST' + }); + req.end(); + req.on('response', common.mustCall()); + let data = ''; + req.on('data', (chunk) => { + data += chunk; + }); + req.on('end', common.mustCall(() => { + assert.strictEqual(data, 'some data'); + })); + req.on('trailers', common.mustCall((headers) => { + assert.strictEqual(headers.xyz, 'abc'); + })); + req.on('close', common.mustCall()); +}));