Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: fix no dumping after maybeReadMore #7211

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, would there be a behavioral difference if you dropped the if statement and wrote it as this._readableState.readingMore = this._consuming;?

Also, this could use a comment explaining why it's necessary to fiddle with this._readableState and why this._consuming should be true when monkey-patching this.read..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I believe it would make a difference. _consuming can flip only into true, and can't really go back. This is a hack to detect following scenario:

http.createServer((req, res) => {
  res.end('123');
});

Clearly in this case no one has attempted to read from req. Thus _consuming is false here, and in _http_server.js the request can be _dumped. However, initial version (prior to this patch) of this hack wasn't working as expected, because _stream_readable.js may call .read() by itself. Luckily it calls it only from maybeReadMore, so we could test it here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, does that mean that in practice this._readableState.readingMore can be false when this._consuming is true?

A comment in the source explaining all that would definitely be appreciated. :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it can be false. _stream_readable.js flips it on and off.

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();
}));