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

http2: wait for session socket writable end on close/destroy · nodejs/node@ba0682e · GitHub

/ node Public

Commit ba0682e

Browse files
lundibundi
authored and
BridgeAR
committed
http2: wait for session socket writable end on close/destroy
This slightly alters the behaviour of session close by first using .end() on a session socket to finish writing the data and only then calls .destroy() to make sure the Readable side is closed. This allows the socket to finish transmitting data, receive proper FIN packet and avoid ECONNRESET errors upon graceful close. onStreamClose now directly calls stream.destroy() instead of kMaybeDestroy because the latter will first check that the stream has writableFinished set. And that may not be true as we have just (synchronously) called .end() on the stream if it was not closed and that doesn't give it enough time to finish. Furthermore there is no point in waiting for 'finish' as the other party have already closed the stream and we won't be able to write anyway. This also changes a few tests to correctly handle graceful session close. This includes: * not reading request data (on client side) * not reading push stream data (on client side) * relying on socket.destroy() (on client) to finish server session due to the destroy of the socket without closing the server session. As the goaway itself is *not* a session close. Added few 'close' event mustCall checks. PR-URL: #30854 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
1 parent 86f2e86 commit ba0682e

10 files changed

Lines changed: 101 additions & 45 deletions

‎lib/internal/http2/core.js‎

Lines changed: 71 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -488,7 +488,10 @@ function onStreamClose(code) {
488488
if (!stream || stream.destroyed)
489489
return false;
490490

491-
debugStreamObj(stream, 'closed with code %d', code);
491+
debugStreamObj(
492+
stream, 'closed with code %d, closed %s, readable %s',
493+
code, stream.closed, stream.readable
494+
);
492495

493496
if (!stream.closed)
494497
closeStream(stream, code, kNoRstStream);
@@ -497,7 +500,7 @@ function onStreamClose(code) {
497500
// Defer destroy we actually emit end.
498501
if (!stream.readable || code !== NGHTTP2_NO_ERROR) {
499502
// If errored or ended, we can destroy immediately.
500-
stream[kMaybeDestroy](code);
503+
stream.destroy();
501504
} else {
502505
// Wait for end to destroy.
503506
stream.on('end', stream[kMaybeDestroy]);
@@ -985,22 +988,76 @@ function emitClose(self, error) {
985988
self.emit('close');
986989
}
987990

988-
function finishSessionDestroy(session, error) {
989-
debugSessionObj(session, 'finishSessionDestroy');
990-
991+
function cleanupSession(session) {
991992
const socket = session[kSocket];
992-
if (!socket.destroyed)
993-
socket.destroy(error);
994-
993+
const handle = session[kHandle];
995994
session[kProxySocket] = undefined;
996995
session[kSocket] = undefined;
997996
session[kHandle] = undefined;
998997
session[kNativeFields] = new Uint8Array(kSessionUint8FieldCount);
999-
socket[kSession] = undefined;
1000-
socket[kServer] = undefined;
998+
if (handle)
999+
handle.ondone = null;
1000+
if (socket) {
1001+
socket[kSession] = undefined;
1002+
socket[kServer] = undefined;
1003+
}
1004+
}
1005+
1006+
function finishSessionClose(session, error) {
1007+
debugSessionObj(session, 'finishSessionClose');
1008+
1009+
const socket = session[kSocket];
1010+
cleanupSession(session);
1011+
1012+
if (socket && !socket.destroyed) {
1013+
// Always wait for writable side to finish.
1014+
socket.end((err) => {
1015+
debugSessionObj(session, 'finishSessionClose socket end', err);
1016+
// Due to the way the underlying stream is handled in Http2Session we
1017+
// won't get graceful Readable end from the other side even if it was sent
1018+
// as the stream is already considered closed and will neither be read
1019+
// from nor keep the event loop alive.
1020+
// Therefore destroy the socket immediately.
1021+
// Fixing this would require some heavy juggling of ReadStart/ReadStop
1022+
// mostly on Windows as on Unix it will be fine with just ReadStart
1023+
// after this 'ondone' callback.
1024+
socket.destroy(error);
1025+
emitClose(session, error);
1026+
});
1027+
} else {
1028+
process.nextTick(emitClose, session, error);
1029+
}
1030+
}
1031+
1032+
function closeSession(session, code, error) {
1033+
debugSessionObj(session, 'start closing/destroying');
1034+
1035+
const state = session[kState];
1036+
state.flags |= SESSION_FLAGS_DESTROYED;
1037+
state.destroyCode = code;
1038+
1039+
// Clear timeout and remove timeout listeners.
1040+
session.setTimeout(0);
1041+
session.removeAllListeners('timeout');
1042+
1043+
// Destroy any pending and open streams
1044+
if (state.pendingStreams.size > 0 || state.streams.size > 0) {
1045+
const cancel = new ERR_HTTP2_STREAM_CANCEL(error);
1046+
state.pendingStreams.forEach((stream) => stream.destroy(cancel));
1047+
state.streams.forEach((stream) => stream.destroy(error));
1048+
}
10011049

1002-
// Finally, emit the close and error events (if necessary) on next tick.
1003-
process.nextTick(emitClose, session, error);
1050+
// Disassociate from the socket and server.
1051+
const socket = session[kSocket];
1052+
const handle = session[kHandle];
1053+
1054+
// Destroy the handle if it exists at this point.
1055+
if (handle !== undefined) {
1056+
handle.ondone = finishSessionClose.bind(null, session, error);
1057+
handle.destroy(code, socket.destroyed);
1058+
} else {
1059+
finishSessionClose(session, error);
1060+
}
10041061
}
10051062

10061063
// Upon creation, the Http2Session takes ownership of the socket. The session
@@ -1327,6 +1384,7 @@ class Http2Session extends EventEmitter {
13271384
destroy(error = NGHTTP2_NO_ERROR, code) {
13281385
if (this.destroyed)
13291386
return;
1387+
13301388
debugSessionObj(this, 'destroying');
13311389

13321390
if (typeof error === 'number') {
@@ -1338,30 +1396,7 @@ class Http2Session extends EventEmitter {
13381396
if (code === undefined && error != null)
13391397
code = NGHTTP2_INTERNAL_ERROR;
13401398

1341-
const state = this[kState];
1342-
state.flags |= SESSION_FLAGS_DESTROYED;
1343-
state.destroyCode = code;
1344-
1345-
// Clear timeout and remove timeout listeners
1346-
this.setTimeout(0);
1347-
this.removeAllListeners('timeout');
1348-
1349-
// Destroy any pending and open streams
1350-
const cancel = new ERR_HTTP2_STREAM_CANCEL(error);
1351-
state.pendingStreams.forEach((stream) => stream.destroy(cancel));
1352-
state.streams.forEach((stream) => stream.destroy(error));
1353-
1354-
// Disassociate from the socket and server
1355-
const socket = this[kSocket];
1356-
const handle = this[kHandle];
1357-
1358-
// Destroy the handle if it exists at this point
1359-
if (handle !== undefined) {
1360-
handle.ondone = finishSessionDestroy.bind(null, this, error);
1361-
handle.destroy(code, socket.destroyed);
1362-
} else {
1363-
finishSessionDestroy(this, error);
1364-
}
1399+
closeSession(this, code, error);
13651400
}
13661401

13671402
// Closing the session will:

‎src/node_http2.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1782,7 +1782,7 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) {
17821782
Context::Scope context_scope(env()->context());
17831783
Http2Scope h2scope(this);
17841784
CHECK_NOT_NULL(stream_);
1785-
Debug(this, "receiving %d bytes", nread);
1785+
Debug(this, "receiving %d bytes, offset %d", nread, stream_buf_offset_);
17861786
AllocatedBuffer buf(env(), buf_);
17871787

17881788
// Only pass data on if nread > 0

‎test/parallel/test-http2-capture-rejection.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,6 @@ events.captureRejections = true;
7272
}));
7373
}
7474

75-
7675
{
7776
// Test error thrown in 'request' event
7877

@@ -136,6 +135,7 @@ events.captureRejections = true;
136135
const session = connect(`http://localhost:${port}`);
137136

138137
const req = session.request();
138+
req.resume();
139139

140140
session.on('stream', common.mustCall(async (stream) => {
141141
session.close();

‎test/parallel/test-http2-client-destroy.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,7 @@ const Countdown = require('../common/countdown');
145145
server.on('stream', common.mustNotCall());
146146
server.listen(0, common.mustCall(() => {
147147
const client = h2.connect(`http://localhost:${server.address().port}`);
148+
client.on('close', common.mustCall());
148149
const socket = client[kSocket];
149150
socket.on('close', common.mustCall(() => {
150151
assert(socket.destroyed);

‎test/parallel/test-http2-client-stream-destroy-before-connect.js‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ if (!common.hasCrypto)
66
const assert = require('assert');
77
const h2 = require('http2');
88
const NGHTTP2_INTERNAL_ERROR = h2.constants.NGHTTP2_INTERNAL_ERROR;
9+
const Countdown = require('../common/countdown');
910

1011
const server = h2.createServer();
1112

@@ -27,6 +28,11 @@ server.on('stream', (stream) => {
2728

2829
server.listen(0, common.mustCall(() => {
2930
const client = h2.connect(`http://localhost:${server.address().port}`);
31+
const countdown = new Countdown(2, () => {
32+
server.close();
33+
client.close();
34+
});
35+
client.on('connect', () => countdown.dec());
3036

3137
const req = client.request();
3238
req.destroy(new Error('test'));
@@ -39,8 +45,7 @@ server.listen(0, common.mustCall(() => {
3945
req.on('close', common.mustCall(() => {
4046
assert.strictEqual(req.rstCode, NGHTTP2_INTERNAL_ERROR);
4147
assert.strictEqual(req.rstCode, NGHTTP2_INTERNAL_ERROR);
42-
server.close();
43-
client.close();
48+
countdown.dec();
4449
}));
4550

4651
req.on('response', common.mustNotCall());

‎test/parallel/test-http2-compat-client-upload-reject.js‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,11 @@ fs.readFile(loc, common.mustCall((err, data) => {
2323
res.end();
2424
});
2525
}));
26+
server.on('close', common.mustCall());
2627

2728
server.listen(0, common.mustCall(() => {
2829
const client = http2.connect(`http://localhost:${server.address().port}`);
30+
client.on('close', common.mustCall());
2931

3032
const req = client.request({ ':method': 'POST' });
3133
req.on('response', common.mustCall((headers) => {

‎test/parallel/test-http2-create-client-connect.js‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,11 +38,13 @@ const URL = url.URL;
3838
const client =
3939
h2.connect.apply(null, i)
4040
.on('connect', common.mustCall(() => maybeClose(client)));
41+
client.on('close', common.mustCall());
4142
});
4243

4344
// Will fail because protocol does not match the server.
44-
h2.connect({ port: port, protocol: 'https:' })
45+
const client = h2.connect({ port: port, protocol: 'https:' })
4546
.on('error', common.mustCall(() => serverClose.dec()));
47+
client.on('close', common.mustCall());
4648
}));
4749
}
4850

‎test/parallel/test-http2-goaway-opaquedata.js‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,20 +8,24 @@ const http2 = require('http2');
88

99
const server = http2.createServer();
1010
const data = Buffer.from([0x1, 0x2, 0x3, 0x4, 0x5]);
11+
let session;
1112

1213
server.on('stream', common.mustCall((stream) => {
13-
stream.session.goaway(0, 0, data);
14+
session = stream.session;
15+
session.on('close', common.mustCall());
16+
session.goaway(0, 0, data);
1417
stream.respond();
1518
stream.end();
1619
}));
20+
server.on('close', common.mustCall());
1721

1822
server.listen(0, () => {
19-
2023
const client = http2.connect(`http://localhost:${server.address().port}`);
2124
client.once('goaway', common.mustCall((code, lastStreamID, buf) => {
2225
assert.deepStrictEqual(code, 0);
2326
assert.deepStrictEqual(lastStreamID, 1);
2427
assert.deepStrictEqual(data, buf);
28+
session.close();
2529
server.close();
2630
}));
2731
const req = client.request();

‎test/parallel/test-http2-ping-settings-heapdump.js‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,12 @@ for (const variant of ['ping', 'settings']) {
3030
}));
3131

3232
server.listen(0, common.mustCall(() => {
33-
http2.connect(`http://localhost:${server.address().port}`,
34-
common.mustCall());
33+
const client = http2.connect(`http://localhost:${server.address().port}`,
34+
common.mustCall());
35+
client.on('error', (err) => {
36+
// We destroy the session so it's possible to get ECONNRESET here.
37+
if (err.code !== 'ECONNRESET')
38+
throw err;
39+
});
3540
}));
3641
}

‎test/parallel/test-http2-server-push-stream.js‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,8 @@ server.listen(0, common.mustCall(() => {
5555
assert.strictEqual(headers['x-push-data'], 'pushed by server');
5656
}));
5757
stream.on('aborted', common.mustNotCall());
58+
// We have to read the data of the push stream to end gracefully.
59+
stream.resume();
5860
}));
5961

6062
let data = '';

0 commit comments

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