Skip to content

Commit

Permalink
http: fix no dumping after maybeReadMore
Browse files Browse the repository at this point in the history
When `maybeReadMore` kicks in on a first bytes of incoming data, the
`req.read(0)` will be invoked and the `req._consuming` will be set to
`true`. This seemingly harmless property leads to a dire consequences:
the server won't call `req._dump()` and the whole HTTP/1.1 pipeline will
hang (single connection).

PR-URL: #7211
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
  • Loading branch information
indutny authored and evanlucas committed Jun 16, 2016
1 parent caa6718 commit ad1045c
Show file tree
Hide file tree
Showing 2 changed files with 60 additions and 0 deletions.
9 changes: 9 additions & 0 deletions lib/_http_incoming.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,13 @@ exports.readStop = readStop;
function IncomingMessage(socket) {
Stream.Readable.call(this);

// Set this to `true` so that stream.Readable won't attempt to read more
// data on `IncomingMessage#push` (see `maybeReadMore` in
// `_stream_readable.js`). This is important for proper tracking of
// `IncomingMessage#_consuming` which is used to dump requests that users
// haven't attempted to read.
this._readableState.readingMore = true;

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

Expand Down Expand Up @@ -67,6 +74,8 @@ IncomingMessage.prototype.setTimeout = function(msecs, callback) {


IncomingMessage.prototype.read = function(n) {
if (!this._consuming)
this._readableState.readingMore = false;
this._consuming = true;
this.read = Stream.Readable.prototype.read;
return this.read(n);
Expand Down
51 changes: 51 additions & 0 deletions test/parallel/test-http-no-read-no-dump.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
'use strict';
const common = require('../common');
const http = require('http');

let onPause = null;

const server = http.createServer((req, res) => {
if (req.method === 'GET')
return res.end();

res.writeHead(200);
res.flushHeaders();

req.connection.on('pause', () => {
res.end();
onPause();
});
}).listen(common.PORT, common.mustCall(() => {
const agent = new http.Agent({
maxSockets: 1,
keepAlive: true
});

const post = http.request({
agent: agent,
method: 'POST',
port: common.PORT,
}, common.mustCall((res) => {
res.resume();

post.write(Buffer.alloc(16 * 1024).fill('X'));
onPause = () => {
post.end('something');
};
}));

/* What happens here is that the server `end`s the response before we send
* `something`, and the client thought that this is a green light for sending
* next GET request
*/
post.write('initial');

http.request({
agent: agent,
method: 'GET',
port: common.PORT,
}, common.mustCall((res) => {
server.close();
res.connection.end();
})).end();
}));

0 comments on commit ad1045c

Please sign in to comment.