http: fix no dumping after `maybeReadMore` #7211

indutny · 2016-06-08T04:54:48Z

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

indutny · 2016-06-08T04:55:30Z

cc @nodejs/http @bnoordhuis

indutny · 2016-06-08T04:56:53Z

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

rvagg · 2016-06-08T06:07:03Z

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?

indutny · 2016-06-08T07:04:49Z

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

indutny · 2016-06-08T07:05:49Z

cc @nodejs/collaborators just in case.

ronkorving · 2016-06-08T07:27:34Z

When was this issue introduced?

rvagg · 2016-06-08T10:56:18Z

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

rvagg · 2016-06-08T12:37:35Z

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?

indutny · 2016-06-08T13:24:09Z

@ronkorving I think at the introduction of Streams2.

bnoordhuis · 2016-06-11T10:49:11Z

lib/_http_incoming.js

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


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


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.

indutny · 2016-06-13T01:18:58Z

@bnoordhuis how about this? ^

bnoordhuis · 2016-06-13T06:06:42Z

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

mcollina · 2016-06-13T13:49:10Z

LGTM, and I agree with @bnoordhuis.

indutny · 2016-06-15T16:49:04Z

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>

indutny · 2016-06-15T16:50:51Z

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

MylesBorins · 2016-06-15T22:01:18Z

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

Just want to make sure that is what you expected

indutny · 2016-06-15T23:45:20Z

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: fix no dumping after `maybeReadMore` #7211

http: fix no dumping after `maybeReadMore` #7211

indutny commented Jun 8, 2016 •

edited

Loading

indutny commented Jun 8, 2016

indutny commented Jun 8, 2016

rvagg commented Jun 8, 2016

indutny commented Jun 8, 2016 •

edited

Loading

indutny commented Jun 8, 2016

ronkorving commented Jun 8, 2016

rvagg commented Jun 8, 2016

rvagg commented Jun 8, 2016

indutny commented Jun 8, 2016

bnoordhuis Jun 11, 2016

indutny Jun 11, 2016

bnoordhuis Jun 12, 2016

indutny Jun 12, 2016

indutny commented Jun 13, 2016

bnoordhuis commented Jun 13, 2016

mcollina commented Jun 13, 2016

indutny commented Jun 15, 2016

indutny commented Jun 15, 2016

MylesBorins commented Jun 15, 2016

indutny commented Jun 15, 2016

http: fix no dumping after maybeReadMore #7211

http: fix no dumping after maybeReadMore #7211

Conversation

indutny commented Jun 8, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

indutny commented Jun 8, 2016

indutny commented Jun 8, 2016

rvagg commented Jun 8, 2016

indutny commented Jun 8, 2016 • edited Loading

indutny commented Jun 8, 2016

ronkorving commented Jun 8, 2016

rvagg commented Jun 8, 2016

rvagg commented Jun 8, 2016

indutny commented Jun 8, 2016

bnoordhuis Jun 11, 2016

Choose a reason for hiding this comment

indutny Jun 11, 2016

Choose a reason for hiding this comment

bnoordhuis Jun 12, 2016

Choose a reason for hiding this comment

indutny Jun 12, 2016

Choose a reason for hiding this comment

indutny commented Jun 13, 2016

bnoordhuis commented Jun 13, 2016

mcollina commented Jun 13, 2016

indutny commented Jun 15, 2016

indutny commented Jun 15, 2016

MylesBorins commented Jun 15, 2016

indutny commented Jun 15, 2016

http: fix no dumping after `maybeReadMore` #7211

http: fix no dumping after `maybeReadMore` #7211

indutny commented Jun 8, 2016 •

edited

Loading

indutny commented Jun 8, 2016 •

edited

Loading