FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

http2: improve session close/destroy procedures · nodejs/node@316354e · GitHub

/ node Public

Commit 316354e

Browse files
santigimeno
authored and
RafaelGSS
committed
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: #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>
1 parent 3a1ee94 commit 316354e

5 files changed

Lines changed: 46 additions & 22 deletions

File tree

‎lib/internal/http2/core.js‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1109,19 +1109,25 @@ function finishSessionClose(session, error) {
11091109
cleanupSession(session);
11101110

11111111
if (socket && !socket.destroyed) {
1112+
socket.on('close', () => {
1113+
emitClose(session, error);
1114+
});
1115+
if (session.closed) {
1116+
// If we're gracefully closing the socket, call resume() so we can detect
1117+
// the peer closing in case binding.Http2Session is already gone.
1118+
socket.resume();
1119+
}
1120+
11121121
// Always wait for writable side to finish.
11131122
socket.end((err) => {
11141123
debugSessionObj(session, 'finishSessionClose socket end', err, error);
1115-
// Due to the way the underlying stream is handled in Http2Session we
1116-
// won't get graceful Readable end from the other side even if it was sent
1117-
// as the stream is already considered closed and will neither be read
1118-
// from nor keep the event loop alive.
1119-
// Therefore destroy the socket immediately.
1120-
// Fixing this would require some heavy juggling of ReadStart/ReadStop
1121-
// mostly on Windows as on Unix it will be fine with just ReadStart
1122-
// after this 'ondone' callback.
1123-
socket.destroy(error);
1124-
emitClose(session, error);
1124+
// If session.destroy() was called, destroy the underlying socket. Delay
1125+
// it a bit to try to avoid ECONNRESET on Windows.
1126+
if (!session.closed) {
1127+
setImmediate(() => {
1128+
socket.destroy(error);
1129+
});
1130+
}
11251131
});
11261132
} else {
11271133
process.nextTick(emitClose, session, error);

‎src/node_http2.cc‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -703,6 +703,11 @@ void Http2Session::Close(uint32_t code, bool socket_closed) {
703703
Debug(this, "make done session callback");
704704
HandleScope scope(env()->isolate());
705705
MakeCallback(env()->ondone_string(), 0, nullptr);
706+
if (stream_ != nullptr) {
707+
// Start reading again to detect the other end finishing.
708+
set_reading_stopped(false);
709+
stream_->ReadStart();
710+
}
706711
}
707712

708713
// If there are outstanding pings, those will need to be canceled, do
@@ -1592,6 +1597,11 @@ void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) {
15921597
if (is_destroyed()) {
15931598
HandleScope scope(env()->isolate());
15941599
MakeCallback(env()->ondone_string(), 0, nullptr);
1600+
if (stream_ != nullptr) {
1601+
// Start reading again to detect the other end finishing.
1602+
set_reading_stopped(false);
1603+
stream_->ReadStart();
1604+
}
15951605
return;
15961606
}
15971607

@@ -1640,7 +1650,9 @@ void Http2Session::MaybeScheduleWrite() {
16401650
}
16411651

16421652
void Http2Session::MaybeStopReading() {
1643-
if (is_reading_stopped()) return;
1653+
// If the session is already closing we don't want to stop reading as we want
1654+
// to detect when the other peer is actually closed.
1655+
if (is_reading_stopped() || is_closing()) return;
16441656
int want_read = nghttp2_session_want_read(session_.get());
16451657
Debug(this, "wants read? %d", want_read);
16461658
if (want_read == 0 || is_write_in_progress()) {

‎test/parallel/test-http2-server-close-callback.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ let session;
1313

1414
const countdown = new Countdown(2, () => {
1515
server.close(common.mustSucceed());
16-
session.destroy();
16+
session.close();
1717
});
1818

1919
server.listen(0, common.mustCall(() => {

‎test/parallel/test-http2-server-sessionerror.js‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -44,17 +44,21 @@ server.on('sessionError', common.mustCall((err, session) => {
4444
server.listen(0, common.mustCall(() => {
4545
const url = `http://localhost:${server.address().port}`;
4646
http2.connect(url)
47-
.on('error', common.expectsError({
48-
code: 'ERR_HTTP2_SESSION_ERROR',
49-
message: 'Session closed with error code 2',
47+
.on('error', common.mustCall((err) => {
48+
if (err.code !== 'ECONNRESET') {
49+
assert.strictEqual(err.code, 'ERR_HTTP2_SESSION_ERROR');
50+
assert.strictEqual(err.message, 'Session closed with error code 2');
51+
}
5052
}))
5153
.on('close', () => {
5254
server.removeAllListeners('error');
5355
http2.connect(url)
54-
.on('error', common.expectsError({
55-
code: 'ERR_HTTP2_SESSION_ERROR',
56-
message: 'Session closed with error code 2',
57-
}))
56+
.on('error', common.mustCall((err) => {
57+
if (err.code !== 'ECONNRESET') {
58+
assert.strictEqual(err.code, 'ERR_HTTP2_SESSION_ERROR');
59+
assert.strictEqual(err.message, 'Session closed with error code 2');
60+
}
61+
}))
5862
.on('close', () => server.close());
5963
});
6064
}));

‎test/parallel/test-http2-too-many-settings.js‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,9 +29,11 @@ function doTest(session) {
2929

3030
server.listen(0, common.mustCall(() => {
3131
const client = h2.connect(`http://localhost:${server.address().port}`);
32-
client.on('error', common.expectsError({
33-
code: 'ERR_HTTP2_SESSION_ERROR',
34-
message: 'Session closed with error code 2',
32+
client.on('error', common.mustCall((err) => {
33+
if (err.code !== 'ECONNRESET') {
34+
assert.strictEqual(err.code, 'ERR_HTTP2_SESSION_ERROR');
35+
assert.strictEqual(err.message, 'Session closed with error code 2');
36+
}
3537
}));
3638
client.on('close', common.mustCall(() => server.close()));
3739
}));

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL