http: fix no dumping after maybeReadMore#7211
Conversation
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 commented Jun 8, 2016
cc @nodejs/http @bnoordhuis |
indutny commented Jun 8, 2016
rvagg commented Jun 8, 2016
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 |
@rvagg I guess it can do |
indutny commented Jun 8, 2016
cc @nodejs/collaborators just in case. |
ronkorving commented Jun 8, 2016
When was this issue introduced? |
rvagg commented Jun 8, 2016
market as lts-watch-v0.10, will defer the backporting pain for now |
rvagg commented Jun 8, 2016
Confirmed that v0.10 exhibits the same behaviour but 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 |
indutny commented Jun 8, 2016
@ronkorving I think at the introduction of Streams2. |
| IncomingMessage.prototype.read=function(n){ | ||
| if(!this._consuming) | ||
| this._readableState.readingMore=false; |
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. :-)
There was a problem hiding this comment.
Yes, it can be false. _stream_readable.js flips it on and off.
indutny commented Jun 13, 2016
@bnoordhuis how about this? ^ |
bnoordhuis commented Jun 13, 2016
LGTM but all this stream state hacking makes it very brittle, evidently so. |
mcollina commented Jun 13, 2016
LGTM, and I agree with @bnoordhuis. |
indutny commented Jun 15, 2016
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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
indutny commented Jun 15, 2016
MylesBorins commented Jun 15, 2016
indutny commented Jun 15, 2016
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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
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) #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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
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]>
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]>
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]>
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]>
7 participants
Checklist
make -j4 test(UNIX) orvcbuild test nosign(Windows) passesAffected core subsystem(s)
http
Description of change
When
maybeReadMorekicks in on a first bytes of incoming data, thereq.read(0)will be invoked and thereq._consumingwill be set totrue. This seemingly harmless property leads to a dire consequences:the server won't call
req._dump()and the whole HTTP/1.1 pipeline willhang (single connection).