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 ef8f90f

Browse filesBrowse files
mcollinaaddaleax
authored andcommitted
http2: fix condition where data is lost
PR-URL: #18895 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
1 parent e74e422 commit ef8f90f
Copy full SHA for ef8f90f

File tree

Expand file treeCollapse file tree

3 files changed

+146
-14
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

3 files changed

+146
-14
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
+41-14Lines changed: 41 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -306,8 +306,23 @@ function onStreamClose(code) {
306306

307307
if (state.fd !== undefined)
308308
tryClose(state.fd);
309-
stream.push(null);
310-
stream[kMaybeDestroy](null, code);
309+
310+
// Defer destroy we actually emit end.
311+
if (stream._readableState.endEmitted || code !== NGHTTP2_NO_ERROR) {
312+
// If errored or ended, we can destroy immediately.
313+
stream[kMaybeDestroy](null, code);
314+
} else {
315+
// Wait for end to destroy.
316+
stream.on('end', stream[kMaybeDestroy]);
317+
// Push a null so the stream can end whenever the client consumes
318+
// it completely.
319+
stream.push(null);
320+
321+
// Same as net.
322+
if (stream.readableLength === 0) {
323+
stream.read(0);
324+
}
325+
}
311326
}
312327

313328
// Receives a chunk of data for a given stream and forwards it on
@@ -325,11 +340,19 @@ function onStreamRead(nread, buf) {
325340
}
326341
return;
327342
}
343+
328344
// Last chunk was received. End the readable side.
329345
debug(`Http2Stream ${stream[kID]} [Http2Session ` +
330346
`${sessionName(stream[kSession][kType])}]: ending readable.`);
331-
stream.push(null);
332-
stream[kMaybeDestroy]();
347+
348+
// defer this until we actually emit end
349+
if (stream._readableState.endEmitted) {
350+
stream[kMaybeDestroy]();
351+
} else {
352+
stream.on('end', stream[kMaybeDestroy]);
353+
stream.push(null);
354+
stream.read(0);
355+
}
333356
}
334357

335358
// Called when the remote peer settings have been updated.
@@ -1825,21 +1848,25 @@ class Http2Stream extends Duplex {
18251848
session[kMaybeDestroy]();
18261849
process.nextTick(emit, this, 'close', code);
18271850
callback(err);
1828-
}
18291851

1852+
}
18301853
// The Http2Stream can be destroyed if it has closed and if the readable
18311854
// side has received the final chunk.
18321855
[kMaybeDestroy](error, code = NGHTTP2_NO_ERROR) {
1833-
if (error == null) {
1834-
if (code === NGHTTP2_NO_ERROR &&
1835-
(!this._readableState.ended ||
1836-
!this._writableState.ended ||
1837-
this._writableState.pendingcb > 0 ||
1838-
!this.closed)) {
1839-
return;
1840-
}
1856+
if (error || code !== NGHTTP2_NO_ERROR) {
1857+
this.destroy(error);
1858+
return;
1859+
}
1860+
1861+
// TODO(mcollina): remove usage of _*State properties
1862+
if (this._readableState.ended &&
1863+
this._writableState.ended &&
1864+
this._writableState.pendingcb === 0 &&
1865+
this.closed) {
1866+
this.destroy();
1867+
// This should return, but eslint complains.
1868+
// return
18411869
}
1842-
this.destroy(error);
18431870
}
18441871
}
18451872

Collapse file
+50Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
if (!common.hasCrypto)
5+
common.skip('missing crypto');
6+
const assert = require('assert');
7+
const http2 = require('http2');
8+
const { Readable } = require('stream');
9+
10+
const server = http2.createServer(common.mustCall((req, res) => {
11+
res.setHeader('content-type', 'text/html');
12+
const input = new Readable({
13+
read() {
14+
this.push('test');
15+
this.push(null);
16+
}
17+
});
18+
input.pipe(res);
19+
}));
20+
21+
server.listen(0, common.mustCall(() => {
22+
const port = server.address().port;
23+
const client = http2.connect(`http://localhost:${port}`);
24+
25+
const req = client.request();
26+
27+
req.on('response', common.mustCall((headers) => {
28+
assert.strictEqual(headers[':status'], 200);
29+
assert.strictEqual(headers['content-type'], 'text/html');
30+
}));
31+
32+
let data = '';
33+
34+
const notCallClose = common.mustNotCall();
35+
36+
setTimeout(() => {
37+
req.setEncoding('utf8');
38+
req.removeListener('close', notCallClose);
39+
req.on('close', common.mustCall(() => {
40+
server.close();
41+
client.close();
42+
}));
43+
req.on('data', common.mustCallAtLeast((d) => data += d));
44+
req.on('end', common.mustCall(() => {
45+
assert.strictEqual(data, 'test');
46+
}));
47+
}, common.platformTimeout(100));
48+
49+
req.on('close', notCallClose);
50+
}));
Collapse file
+55Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
if (!common.hasCrypto)
5+
common.skip('missing crypto');
6+
const assert = require('assert');
7+
const http2 = require('http2');
8+
const { Readable } = require('stream');
9+
10+
const server = http2.createServer();
11+
server.on('stream', common.mustCall((stream) => {
12+
stream.respond({
13+
':status': 200,
14+
'content-type': 'text/html'
15+
});
16+
const input = new Readable({
17+
read() {
18+
this.push('test');
19+
this.push(null);
20+
}
21+
});
22+
input.pipe(stream);
23+
}));
24+
25+
26+
server.listen(0, common.mustCall(() => {
27+
const port = server.address().port;
28+
const client = http2.connect(`http://localhost:${port}`);
29+
30+
const req = client.request();
31+
32+
req.on('response', common.mustCall((headers) => {
33+
assert.strictEqual(headers[':status'], 200);
34+
assert.strictEqual(headers['content-type'], 'text/html');
35+
}));
36+
37+
let data = '';
38+
39+
const notCallClose = common.mustNotCall();
40+
41+
setTimeout(() => {
42+
req.setEncoding('utf8');
43+
req.removeListener('close', notCallClose);
44+
req.on('close', common.mustCall(() => {
45+
server.close();
46+
client.close();
47+
}));
48+
req.on('data', common.mustCallAtLeast((d) => data += d));
49+
req.on('end', common.mustCall(() => {
50+
assert.strictEqual(data, 'test');
51+
}));
52+
}, common.platformTimeout(100));
53+
54+
req.on('close', notCallClose);
55+
}));

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.