Skip to content

Commit

Permalink
http2: improve session close/destroy procedures
Browse files Browse the repository at this point in the history
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: #45115
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
  • Loading branch information
santigimeno authored and danielleadams committed Jan 3, 2023
1 parent 3fb44f9 commit b45878b
Show file tree
Hide file tree
Showing 5 changed files with 46 additions and 22 deletions.
26 changes: 16 additions & 10 deletions lib/internal/http2/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
14 changes: 13 additions & 1 deletion src/node_http2.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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()) {
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-http2-server-close-callback.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ let session;

const countdown = new Countdown(2, () => {
server.close(common.mustSucceed());
session.destroy();
session.close();
});

server.listen(0, common.mustCall(() => {
Expand Down
18 changes: 11 additions & 7 deletions test/parallel/test-http2-server-sessionerror.js
Original file line number Diff line number Diff line change
Expand Up @@ -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());
});
}));
8 changes: 5 additions & 3 deletions test/parallel/test-http2-too-many-settings.js
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
}));
Expand Down

0 comments on commit b45878b

Please sign in to comment.