Skip to content

Commit

Permalink
http: fix _dump regression
Browse files Browse the repository at this point in the history
A recent set of changes removed _consuming tracking from server
incoming messages which ensures that _dump only runs if the
user has never attempted to read the incoming data. Fix by
reintroducing _consuming which tracks whether _read was ever
successfully called.

PR-URL: #20088
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
  • Loading branch information
apapirovski authored and jasnell committed Apr 19, 2018
1 parent c64632e commit 299da1f
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 1 deletion.
8 changes: 8 additions & 0 deletions lib/_http_incoming.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ function readStop(socket) {
function IncomingMessage(socket) {
Stream.Readable.call(this);

this._readableState.readingMore = true;

this.socket = socket;
this.connection = socket;

Expand All @@ -63,6 +65,7 @@ function IncomingMessage(socket) {
this.statusMessage = null;
this.client = socket;

this._consuming = false;
// flag for when we decide that this message cannot possibly be
// read by the user, so there's no point continuing to handle it.
this._dumped = false;
Expand All @@ -79,6 +82,11 @@ IncomingMessage.prototype.setTimeout = function setTimeout(msecs, callback) {


IncomingMessage.prototype._read = function _read(n) {
if (!this._consuming) {
this._readableState.readingMore = false;
this._consuming = true;
}

// We actually do almost nothing here, because the parserOnBody
// function fills up our internal buffer directly. However, we
// do need to unpause the underlying socket so that it flows.
Expand Down
2 changes: 1 addition & 1 deletion lib/_http_server.js
Original file line number Diff line number Diff line change
Expand Up @@ -560,7 +560,7 @@ function resOnFinish(req, res, socket, state, server) {
// if the user never called req.read(), and didn't pipe() or
// .resume() or .on('data'), then we call req._dump() so that the
// bytes will be pulled off the wire.
if (!req._readableState.resumeScheduled)
if (!req._consuming && !req._readableState.resumeScheduled)
req._dump();

res.detachSocket(socket);
Expand Down
33 changes: 33 additions & 0 deletions test/parallel/test-http-pause-no-dump.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const http = require('http');

const server = http.createServer(common.mustCall(function(req, res) {
req.once('data', common.mustCall(() => {
req.pause();
res.writeHead(200);
res.end();
res.on('finish', common.mustCall(() => {
assert(!req._dumped);
}));
}));
}));
server.listen(0);

server.on('listening', common.mustCall(function() {
const req = http.request({
port: this.address().port,
method: 'POST',
path: '/'
}, common.mustCall(function(res) {
assert.strictEqual(res.statusCode, 200);
res.resume();
res.on('end', common.mustCall(() => {
server.close();
}));
}));

req.end(Buffer.allocUnsafe(1024));
}));

0 comments on commit 299da1f

Please sign in to comment.