Skip to content

Commit

Permalink
http2: fix end without read
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
apapirovski committed May 17, 2018
1 parent f9de6f5 commit 8d38288
Show file tree
Hide file tree
Showing 4 changed files with 66 additions and 15 deletions.
10 changes: 6 additions & 4 deletions lib/internal/http2/compat.js
Original file line number Diff line number Diff line change
Expand Up @@ -260,8 +260,6 @@ class Http2ServerRequest extends Readable {
stream[kRequest] = this;

// Pause the stream..
stream.pause();
stream.on('data', onStreamData);
stream.on('trailers', onStreamTrailers);
stream.on('end', onStreamEnd);
stream.on('error', onStreamError);
Expand Down Expand Up @@ -328,8 +326,12 @@ class Http2ServerRequest extends Readable {
_read(nread) {
const state = this[kState];
if (!state.closed) {
state.didRead = true;
process.nextTick(resumeStream, this[kStream]);
if (!state.didRead) {
state.didRead = true;
this[kStream].on('data', onStreamData);
} else {
process.nextTick(resumeStream, this[kStream]);
}
} else {
this.emit('error', new ERR_HTTP2_INVALID_STREAM());
}
Expand Down
12 changes: 7 additions & 5 deletions lib/internal/http2/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -349,11 +349,11 @@ function onStreamClose(code) {
// Push a null so the stream can end whenever the client consumes
// it completely.
stream.push(null);

// Same as net.
if (stream.readableLength === 0) {
stream.read(0);
}
// If the client hasn't tried to consume the stream and there is no
// resume scheduled (which would indicate they would consume in the future),
// then just dump the incoming data so that the stream can be destroyed.
if (!stream[kState].didRead && !stream._readableState.resumeScheduled)
stream.resume();
}
}

Expand Down Expand Up @@ -1795,6 +1795,8 @@ class Http2Stream extends Duplex {
const ret = this[kHandle].trailers(headersList);
if (ret < 0)
this.destroy(new NghttpError(ret));
else
this[kMaybeDestroy]();
}

get closed() {
Expand Down
15 changes: 9 additions & 6 deletions test/parallel/test-http2-client-upload-reject.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,15 @@ fs.readFile(loc, common.mustCall((err, data) => {
const server = http2.createServer();

server.on('stream', common.mustCall((stream) => {
stream.on('close', common.mustCall(() => {
assert.strictEqual(stream.rstCode, 0);
}));

stream.respond({ ':status': 400 });
stream.end();
// Wait for some data to come through.
setImmediate(() => {
stream.on('close', common.mustCall(() => {
assert.strictEqual(stream.rstCode, 0);
}));

stream.respond({ ':status': 400 });
stream.end();
});
}));

server.listen(0, common.mustCall(() => {
Expand Down
44 changes: 44 additions & 0 deletions test/parallel/test-http2-compat-client-upload-reject.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
'use strict';

// Verifies that uploading data from a client works

const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');
const assert = require('assert');
const http2 = require('http2');
const fs = require('fs');
const fixtures = require('../common/fixtures');

const loc = fixtures.path('person-large.jpg');

assert(fs.existsSync(loc));

fs.readFile(loc, common.mustCall((err, data) => {
assert.ifError(err);

const server = http2.createServer(common.mustCall((req, res) => {
setImmediate(() => {
res.writeHead(400);
res.end();
});
}));

server.listen(0, common.mustCall(() => {
const client = http2.connect(`http://localhost:${server.address().port}`);

const req = client.request({ ':method': 'POST' });
req.on('response', common.mustCall((headers) => {
assert.strictEqual(headers[':status'], 400);
}));

req.resume();
req.on('end', common.mustCall(() => {
server.close();
client.close();
}));

const str = fs.createReadStream(loc);
str.pipe(req);
}));
}));

0 comments on commit 8d38288

Please sign in to comment.