diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 7eda063dde9966..fd101022c9dad7 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 f61c650de53a3e..bee122259e4da7 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())); }));