From 316354e3d35db1a14f505a30bbbfcb7e72c49829 Mon Sep 17 00:00:00 2001 From: Santiago Gimeno Date: Wed, 26 Oct 2022 11:16:00 +0200 Subject: [PATCH] http2: improve session close/destroy procedures Don't destroy the socket when closing the session but let it end gracefully. Also, when destroying the session, on Windows, we would get ECONNRESET errors, make sure we take those into account in our tests. PR-URL: https://github.com/nodejs/node/pull/45115 Reviewed-By: Matteo Collina Reviewed-By: Rafael Gonzaga Reviewed-By: James M Snell Reviewed-By: Colin Ihrig --- lib/internal/http2/core.js | 26 ++++++++++++------- src/node_http2.cc | 14 +++++++++- .../test-http2-server-close-callback.js | 2 +- .../test-http2-server-sessionerror.js | 18 ++++++++----- test/parallel/test-http2-too-many-settings.js | 8 +++--- 5 files changed, 46 insertions(+), 22 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 3ca6504d934dcc..000bc1ba85d7e4 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -1109,19 +1109,25 @@ function finishSessionClose(session, error) { cleanupSession(session); if (socket && !socket.destroyed) { + socket.on('close', () => { + emitClose(session, error); + }); + if (session.closed) { + // If we're gracefully closing the socket, call resume() so we can detect + // the peer closing in case binding.Http2Session is already gone. + socket.resume(); + } + // Always wait for writable side to finish. socket.end((err) => { debugSessionObj(session, 'finishSessionClose socket end', err, error); - // Due to the way the underlying stream is handled in Http2Session we - // won't get graceful Readable end from the other side even if it was sent - // as the stream is already considered closed and will neither be read - // from nor keep the event loop alive. - // Therefore destroy the socket immediately. - // Fixing this would require some heavy juggling of ReadStart/ReadStop - // mostly on Windows as on Unix it will be fine with just ReadStart - // after this 'ondone' callback. - socket.destroy(error); - emitClose(session, error); + // If session.destroy() was called, destroy the underlying socket. Delay + // it a bit to try to avoid ECONNRESET on Windows. + if (!session.closed) { + setImmediate(() => { + socket.destroy(error); + }); + } }); } else { process.nextTick(emitClose, session, error); diff --git a/src/node_http2.cc b/src/node_http2.cc index f178874bc0f9f2..db841e137095fe 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -703,6 +703,11 @@ void Http2Session::Close(uint32_t code, bool socket_closed) { Debug(this, "make done session callback"); HandleScope scope(env()->isolate()); MakeCallback(env()->ondone_string(), 0, nullptr); + if (stream_ != nullptr) { + // Start reading again to detect the other end finishing. + set_reading_stopped(false); + stream_->ReadStart(); + } } // If there are outstanding pings, those will need to be canceled, do @@ -1592,6 +1597,11 @@ void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) { if (is_destroyed()) { HandleScope scope(env()->isolate()); MakeCallback(env()->ondone_string(), 0, nullptr); + if (stream_ != nullptr) { + // Start reading again to detect the other end finishing. + set_reading_stopped(false); + stream_->ReadStart(); + } return; } @@ -1640,7 +1650,9 @@ void Http2Session::MaybeScheduleWrite() { } void Http2Session::MaybeStopReading() { - if (is_reading_stopped()) return; + // If the session is already closing we don't want to stop reading as we want + // to detect when the other peer is actually closed. + if (is_reading_stopped() || is_closing()) return; int want_read = nghttp2_session_want_read(session_.get()); Debug(this, "wants read? %d", want_read); if (want_read == 0 || is_write_in_progress()) { diff --git a/test/parallel/test-http2-server-close-callback.js b/test/parallel/test-http2-server-close-callback.js index 5b0b8116a147fd..e4cd24ce209782 100644 --- a/test/parallel/test-http2-server-close-callback.js +++ b/test/parallel/test-http2-server-close-callback.js @@ -13,7 +13,7 @@ let session; const countdown = new Countdown(2, () => { server.close(common.mustSucceed()); - session.destroy(); + session.close(); }); server.listen(0, common.mustCall(() => { diff --git a/test/parallel/test-http2-server-sessionerror.js b/test/parallel/test-http2-server-sessionerror.js index e266661b0cc2c3..b71b9df80f083f 100644 --- a/test/parallel/test-http2-server-sessionerror.js +++ b/test/parallel/test-http2-server-sessionerror.js @@ -44,17 +44,21 @@ server.on('sessionError', common.mustCall((err, session) => { server.listen(0, common.mustCall(() => { const url = `http://localhost:${server.address().port}`; http2.connect(url) - .on('error', common.expectsError({ - code: 'ERR_HTTP2_SESSION_ERROR', - message: 'Session closed with error code 2', + .on('error', common.mustCall((err) => { + if (err.code !== 'ECONNRESET') { + assert.strictEqual(err.code, 'ERR_HTTP2_SESSION_ERROR'); + assert.strictEqual(err.message, 'Session closed with error code 2'); + } })) .on('close', () => { server.removeAllListeners('error'); http2.connect(url) - .on('error', common.expectsError({ - code: 'ERR_HTTP2_SESSION_ERROR', - message: 'Session closed with error code 2', - })) + .on('error', common.mustCall((err) => { + if (err.code !== 'ECONNRESET') { + assert.strictEqual(err.code, 'ERR_HTTP2_SESSION_ERROR'); + assert.strictEqual(err.message, 'Session closed with error code 2'); + } + })) .on('close', () => server.close()); }); })); diff --git a/test/parallel/test-http2-too-many-settings.js b/test/parallel/test-http2-too-many-settings.js index b87e921a915e0e..da5d5865792a83 100644 --- a/test/parallel/test-http2-too-many-settings.js +++ b/test/parallel/test-http2-too-many-settings.js @@ -29,9 +29,11 @@ function doTest(session) { server.listen(0, common.mustCall(() => { const client = h2.connect(`http://localhost:${server.address().port}`); - client.on('error', common.expectsError({ - code: 'ERR_HTTP2_SESSION_ERROR', - message: 'Session closed with error code 2', + client.on('error', common.mustCall((err) => { + if (err.code !== 'ECONNRESET') { + assert.strictEqual(err.code, 'ERR_HTTP2_SESSION_ERROR'); + assert.strictEqual(err.message, 'Session closed with error code 2'); + } })); client.on('close', common.mustCall(() => server.close())); }));