Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
4 changes: 4 additions & 0 deletions lib/_http_incoming.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ exports.readStop = readStop;
function IncomingMessage(socket) {
Stream.Readable.call(this);

this._readableState.readingMore = true;

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

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


IncomingMessage.prototype.read = function(n) {
if (!this._consuming)
this._readableState.readingMore = false;
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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();
}));