Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit b45878b

Browse filesBrowse files
santigimenodanielleadams
santigimeno
authored and
danielleadams
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 3fb44f9 commit b45878b
Copy full SHA for b45878b

File tree

Expand file treeCollapse file tree

5 files changed

+46
-22
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

5 files changed

+46
-22
lines changed
Open diff view settings
Collapse file

‎lib/internal/http2/core.js‎

Copy file name to clipboardExpand all lines: lib/internal/http2/core.js
+16-10Lines 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);
Collapse file

‎src/node_http2.cc‎

Copy file name to clipboardExpand all lines: src/node_http2.cc
+13-1Lines 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()) {
Collapse file

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

Copy file name to clipboardExpand all lines: test/parallel/test-http2-server-close-callback.js
+1-1Lines 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(() => {
Collapse file

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

Copy file name to clipboardExpand all lines: test/parallel/test-http2-server-sessionerror.js
+11-7Lines 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
}));
Collapse file

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

Copy file name to clipboardExpand all lines: test/parallel/test-http2-too-many-settings.js
+5-3Lines 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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.