http: fix no dumping after `maybeReadMore` by indutny · Pull Request #7211 · nodejs/node · GitHub

Checklist

make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
a test and/or benchmark is included
documentation is changed or added
the commit message follows commit guidelines

Affected core subsystem(s)

http

Description of change

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).

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).

cc @nodejs/http @bnoordhuis

CI: https://ci.nodejs.org/job/node-test-pull-request/2953/

lgtm

Jenkins drama on one of the ARM machines, I can't decipher that unfortunately.

Marked for backporting to v0.12 as it suffers from the same problem and we should get on top of this there (it's close to critical but not quite). I don't think 0.10 does but can't verify because of the lack of flushHeaders(). @indutny do you have time to figure out an alternative test for v0.10? Is it good enough to just remove flushHeaders() from the test?

@rvagg I guess it can do res.write('') or something like this should make it work.

cc @nodejs/collaborators just in case.

When was this issue introduced?

market as lts-watch-v0.10, will defer the backporting pain for now

Confirmed that v0.10 exhibits the same behaviour but res.write('') or even res._send(new Buffer(0)) doesn't make the test pass, so we're going to need some help with v0.10 @indutny

Also, can you explore a way to reduce the time it takes for this test to fail instead of relying on a socket timeout? or perhaps we can shorten the socket timeout? Would req.setTimeout(100) have a negative impact on the reliability of the test?

@ronkorving I think at the introduction of Streams2.

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..

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.

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. :-)

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

@bnoordhuis how about this? ^

LGTM but all this stream state hacking makes it very brittle, evidently so.

LGTM, and I agree with @bnoordhuis.

I don't have any better ideas, though :(

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 <info@bnoordhuis.nl> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

@rvagg sorry, your LGTM was lowercase, didn't see it! Landed in 6842aa7, thank you everyone!

@indutny it would appear the fix landed in 357f904 as opposed to 6842aa7

Just want to make sure that is what you expected

Yikes, my mistake. 6842 was a local one that I cherry-picked, sorry about this!

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 <info@bnoordhuis.nl> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Notable changes: * **http**: - 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). (Fedor Indutny) [#7211](#7211) - When freeing the socket to be reused in keep-alive Agent wait for both prefinish and end events. Otherwise the next request may be written before the previous one has finished sending the body, leading to a parser errors. (Fedor Indutny) [#7149](#7149) * **npm**: upgrade npm to 3.9.5 (Kat Marchán) [#7139](#7139)

Notable changes: * **http**: - req.read(0) could cause incoming connections to stall and time out under certain conditions. (Fedor Indutny) [#7211](#7211) - When freeing the socket to be reused in keep-alive Agent wait for both prefinish and end events. Otherwise the next request may be written before the previous one has finished sending the body, leading to a parser errors. (Fedor Indutny) [#7149](#7149) * **npm**: upgrade npm to 3.9.5 (Kat Marchán) [#7139](#7139) #7323

Notable changes: * **http**: - req.read(0) could cause incoming connections to stall and time out under certain conditions. (Fedor Indutny) [#7211](#7211) - When freeing the socket to be reused in keep-alive Agent wait for both prefinish and end events. Otherwise the next request may be written before the previous one has finished sending the body, leading to a parser errors. (Fedor Indutny) [#7149](#7149) * **npm**: upgrade npm to 3.9.5 (Kat Marchán) [#7139](#7139) PR-URL: #7323

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 <info@bnoordhuis.nl> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jun 8, 2016

indutny added the lts-watch-v4.x label Jun 8, 2016

rvagg added the lts-watch-v0.12 label Jun 8, 2016

rvagg added the lts-watch-v0.10 label Jun 8, 2016

rvagg mentioned this pull request Jun 8, 2016

test: add note about duration_ms in TAP reporter #7216

Closed

bnoordhuis reviewed Jun 11, 2016
View reviewed changes

fix

961e878

indutny closed this Jun 15, 2016

indutny deleted the fix/http-no-read-no-dump branch June 15, 2016 16:50

evanlucas mentioned this pull request Jun 16, 2016

v6.2.2 proposal #7323

Merged

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jul 11, 2016

MylesBorins mentioned this pull request Jul 12, 2016

v4.5.0 proposal #7688

Merged

mscdex mentioned this pull request Dec 19, 2016

node 6 IncomingMessage events fix mscdex/busboy#134

Closed

               IncomingMessage.prototype.read = function(n) {
+                if (!this._consuming)
+                  this._readableState.readingMore = false;

http: fix no dumping after `maybeReadMore` by indutny · Pull Request #7211 · nodejs/node · GitHub

Uh oh!

Uh oh!

http: fix no dumping after maybeReadMore - #7211

http: fix no dumping after maybeReadMore#7211indutny wants to merge 2 commits intonodejs:masternodejs/node:masterfrom indutny:fix/http-no-read-no-dumpindutny/io.js:fix/http-no-read-no-dumpCopy head branch name to clipboard

Conversation

indutny commented Jun 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

indutny commented Jun 8, 2016

Uh oh!

indutny commented Jun 8, 2016

Uh oh!

rvagg commented Jun 8, 2016

Uh oh!

indutny commented Jun 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

indutny commented Jun 8, 2016

Uh oh!

ronkorving commented Jun 8, 2016

Uh oh!

rvagg commented Jun 8, 2016

Uh oh!

rvagg commented Jun 8, 2016

Uh oh!

indutny commented Jun 8, 2016

Uh oh!

bnoordhuis Jun 11, 2016

Choose a reason for hiding this comment

Uh oh!

indutny Jun 11, 2016

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Jun 12, 2016

Choose a reason for hiding this comment

Uh oh!

indutny Jun 12, 2016

Choose a reason for hiding this comment

Uh oh!

indutny commented Jun 13, 2016

Uh oh!

bnoordhuis commented Jun 13, 2016

Uh oh!

mcollina commented Jun 13, 2016

Uh oh!

indutny commented Jun 15, 2016

Uh oh!

indutny commented Jun 15, 2016

Uh oh!

MylesBorins commented Jun 15, 2016

Uh oh!

indutny commented Jun 15, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

http: fix no dumping after maybeReadMore#7211
indutny wants to merge 2 commits into
nodejs:masterfrom
indutny:fix/http-no-read-no-dump

indutny commented Jun 8, 2016 •
edited
Loading

indutny commented Jun 8, 2016 •
edited
Loading