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 b2fb1d7

Browse filesBrowse files
apapirovskiMylesBorins
authored andcommitted
http2: fix end without read
Adjust http2 behaviour to allow ending a stream even after some data comes in (when the user has no intention of reading that data). Also correctly end a stream when trailers are present. PR-URL: #20621 Fixes: #20060 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
1 parent 9e432ca commit b2fb1d7
Copy full SHA for b2fb1d7

File tree

Expand file treeCollapse file tree

4 files changed

+66
-15
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

4 files changed

+66
-15
lines changed
Open diff view settings
Collapse file

‎lib/internal/http2/compat.js‎

Copy file name to clipboardExpand all lines: lib/internal/http2/compat.js
+6-4Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -260,8 +260,6 @@ class Http2ServerRequest extends Readable {
260260
stream[kRequest] = this;
261261

262262
// Pause the stream..
263-
stream.pause();
264-
stream.on('data', onStreamData);
265263
stream.on('trailers', onStreamTrailers);
266264
stream.on('end', onStreamEnd);
267265
stream.on('error', onStreamError);
@@ -328,8 +326,12 @@ class Http2ServerRequest extends Readable {
328326
_read(nread) {
329327
const state = this[kState];
330328
if (!state.closed) {
331-
state.didRead = true;
332-
process.nextTick(resumeStream, this[kStream]);
329+
if (!state.didRead) {
330+
state.didRead = true;
331+
this[kStream].on('data', onStreamData);
332+
} else {
333+
process.nextTick(resumeStream, this[kStream]);
334+
}
333335
} else {
334336
this.emit('error', new ERR_HTTP2_INVALID_STREAM());
335337
}
Collapse file

‎lib/internal/http2/core.js‎

Copy file name to clipboardExpand all lines: lib/internal/http2/core.js
+7-5Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -349,11 +349,11 @@ function onStreamClose(code) {
349349
// Push a null so the stream can end whenever the client consumes
350350
// it completely.
351351
stream.push(null);
352-
353-
// Same as net.
354-
if (stream.readableLength === 0) {
355-
stream.read(0);
356-
}
352+
// If the client hasn't tried to consume the stream and there is no
353+
// resume scheduled (which would indicate they would consume in the future),
354+
// then just dump the incoming data so that the stream can be destroyed.
355+
if (!stream[kState].didRead && !stream._readableState.resumeScheduled)
356+
stream.resume();
357357
}
358358
}
359359

@@ -1795,6 +1795,8 @@ class Http2Stream extends Duplex {
17951795
const ret = this[kHandle].trailers(headersList);
17961796
if (ret < 0)
17971797
this.destroy(new NghttpError(ret));
1798+
else
1799+
this[kMaybeDestroy]();
17981800
}
17991801

18001802
get closed() {
Collapse file

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

Copy file name to clipboardExpand all lines: test/parallel/test-http2-client-upload-reject.js
+9-6Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,15 @@ fs.readFile(loc, common.mustCall((err, data) => {
2020
const server = http2.createServer();
2121

2222
server.on('stream', common.mustCall((stream) => {
23-
stream.on('close', common.mustCall(() => {
24-
assert.strictEqual(stream.rstCode, 0);
25-
}));
26-
27-
stream.respond({ ':status': 400 });
28-
stream.end();
23+
// Wait for some data to come through.
24+
setImmediate(() => {
25+
stream.on('close', common.mustCall(() => {
26+
assert.strictEqual(stream.rstCode, 0);
27+
}));
28+
29+
stream.respond({ ':status': 400 });
30+
stream.end();
31+
});
2932
}));
3033

3134
server.listen(0, common.mustCall(() => {
Collapse file
+44Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
'use strict';
2+
3+
// Verifies that uploading data from a client works
4+
5+
const common = require('../common');
6+
if (!common.hasCrypto)
7+
common.skip('missing crypto');
8+
const assert = require('assert');
9+
const http2 = require('http2');
10+
const fs = require('fs');
11+
const fixtures = require('../common/fixtures');
12+
13+
const loc = fixtures.path('person-large.jpg');
14+
15+
assert(fs.existsSync(loc));
16+
17+
fs.readFile(loc, common.mustCall((err, data) => {
18+
assert.ifError(err);
19+
20+
const server = http2.createServer(common.mustCall((req, res) => {
21+
setImmediate(() => {
22+
res.writeHead(400);
23+
res.end();
24+
});
25+
}));
26+
27+
server.listen(0, common.mustCall(() => {
28+
const client = http2.connect(`http://localhost:${server.address().port}`);
29+
30+
const req = client.request({ ':method': 'POST' });
31+
req.on('response', common.mustCall((headers) => {
32+
assert.strictEqual(headers[':status'], 400);
33+
}));
34+
35+
req.resume();
36+
req.on('end', common.mustCall(() => {
37+
server.close();
38+
client.close();
39+
}));
40+
41+
const str = fs.createReadStream(loc);
42+
str.pipe(req);
43+
}));
44+
}));

0 commit comments

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