http2: fix endless loop when writing empty string by addaleax · Pull Request #18924 · nodejs/node · GitHub

Alternative to #18673 that addresses the problem on the HTTP/2 level rather than the streams level itself. The test is taken from @XadillaX, whom I’d be very thankful if he could take a look at this :)

Refs: #18673
Fixes: #18169
Refs: https://github.com/nodejs/node/blob/v9.5.0/src/node_http2.cc#L1481-L1484
Refs: https://github.com/nodejs/node/blob/v9.5.0/lib/_http_outgoing.js#L659-L661

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

http2

Refs: nodejs#18673 Fixes: nodejs#18169 Refs: https://github.com/nodejs/node/blob/v9.5.0/src/node_http2.cc#L1481-L1484 Refs: https://github.com/nodejs/node/blob/v9.5.0/lib/_http_outgoing.js#L659-L661

Refs: nodejs#18673

Is there a way to standardize the behaviour between the various StreamBase implementations? As far as I can tell, the only place where we truly need these empty writes is TLSSocket#renegotiate...

(One side-effect of having a unique implementation of this logic for each StreamBase implementation is that http, for example, doesn't actually call the user-provided callback in the case of an empty write. That actually seems like a bug...)

@apapirovski I think the fact that TLS uses them makes “empty writes flush the underlying protocol” part of the StreamBase API, and that’s a meaningful thing to do in the context of HTTP/2.

I’d be open to changing that. In reality, TLS should probably work more like HTTP/2 and flush state automatically after making changes to the protocol state, like what we do with Http2Scope right now.

That actually seems like a bug

Yeah, but it’s part of the write() method in _http_outgoing.js – it’s not related to our native streams impl, is it? (I also thought somebody had opened a PR for that. But I can’t find it, so I guess I’m wrong?)

Yeah, but it’s part of the write() method in _http_outgoing.js – it’s not related to our native streams impl, is it? (I also thought somebody had opened a PR for that. But I can’t find it, so I guess I’m wrong?)

It's not, just commenting that the way it works is kind of problematic. We probably can't do much about it now but the http implementation makes a lot of non-obvious state assumptions. Like it's not at all clear that doing an empty write won't send the headers or trigger any of the follow-up logic that a non-empty write triggers.

I’d be open to changing that. In reality, TLS should probably work more like HTTP/2 and flush state automatically after making changes to the protocol state, like what we do with Http2Scope right now.

Yeah, that would be nice. Anyway, it's a bit hard for me to find time to think about much of this right now but will revisit when I have a bit more time.

In relation to this PR, I don't know how I feel about it vs the earlier one. There seem to be trade-offs with both.

One question, if we remove the empty buffer and do nothing, will the header be sent?

Refs: https://github.com/nodejs/node/pull/18673/files#diff-696b2cc418addca5f3fe5020058f8b15R1622

@XadillaX Hm – I’m not sure I can wrap my head around everything, but this code here is only ever called by nghttp2 after the headers were sent anyway… does that answer your question?

So, yes, writing an empty buffer would trigger the headers to be sent, as I understand it.

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

Landed in 9bc333c, and the test from @XadillaX in 16facd7.

Thanks for the reviews!

PR-URL: #18924 Fixes: #18169 Refs: #18673 Refs: https://github.com/nodejs/node/blob/v9.5.0/src/node_http2.cc#L1481-L1484 Refs: https://github.com/nodejs/node/blob/v9.5.0/lib/_http_outgoing.js#L659-L661 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Refs: #18673 PR-URL: #18924 Refs: #18673 Refs: https://github.com/nodejs/node/blob/v9.5.0/src/node_http2.cc#L1481-L1484 Refs: https://github.com/nodejs/node/blob/v9.5.0/lib/_http_outgoing.js#L659-L661 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: nodejs#18924 Fixes: nodejs#18169 Refs: nodejs#18673 Refs: https://github.com/nodejs/node/blob/v9.5.0/src/node_http2.cc#L1481-L1484 Refs: https://github.com/nodejs/node/blob/v9.5.0/lib/_http_outgoing.js#L659-L661 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Refs: nodejs#18673 PR-URL: nodejs#18924 Refs: nodejs#18673 Refs: https://github.com/nodejs/node/blob/v9.5.0/src/node_http2.cc#L1481-L1484 Refs: https://github.com/nodejs/node/blob/v9.5.0/lib/_http_outgoing.js#L659-L661 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: nodejs#18924 Fixes: nodejs#18169 Refs: nodejs#18673 Refs: https://github.com/nodejs/node/blob/v9.5.0/src/node_http2.cc#L1481-L1484 Refs: https://github.com/nodejs/node/blob/v9.5.0/lib/_http_outgoing.js#L659-L661 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Refs: nodejs#18673 PR-URL: nodejs#18924 Refs: nodejs#18673 Refs: https://github.com/nodejs/node/blob/v9.5.0/src/node_http2.cc#L1481-L1484 Refs: https://github.com/nodejs/node/blob/v9.5.0/lib/_http_outgoing.js#L659-L661 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Backport-PR-URL: #20456 PR-URL: #18924 Fixes: #18169 Refs: #18673 Refs: https://github.com/nodejs/node/blob/v9.5.0/src/node_http2.cc#L1481-L1484 Refs: https://github.com/nodejs/node/blob/v9.5.0/lib/_http_outgoing.js#L659-L661 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Refs: #18673 Backport-PR-URL: #20456 PR-URL: #18924 Refs: #18673 Refs: https://github.com/nodejs/node/blob/v9.5.0/src/node_http2.cc#L1481-L1484 Refs: https://github.com/nodejs/node/blob/v9.5.0/lib/_http_outgoing.js#L659-L661 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: nodejs#18924 Fixes: nodejs#18169 Refs: nodejs#18673 Refs: https://github.com/nodejs/node/blob/v9.5.0/src/node_http2.cc#L1481-L1484 Refs: https://github.com/nodejs/node/blob/v9.5.0/lib/_http_outgoing.js#L659-L661 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Refs: nodejs#18673 PR-URL: nodejs#18924 Refs: nodejs#18673 Refs: https://github.com/nodejs/node/blob/v9.5.0/src/node_http2.cc#L1481-L1484 Refs: https://github.com/nodejs/node/blob/v9.5.0/lib/_http_outgoing.js#L659-L661 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Backport-PR-URL: #20456 PR-URL: #18924 Fixes: #18169 Refs: #18673 Refs: https://github.com/nodejs/node/blob/v9.5.0/src/node_http2.cc#L1481-L1484 Refs: https://github.com/nodejs/node/blob/v9.5.0/lib/_http_outgoing.js#L659-L661 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Refs: #18673 Backport-PR-URL: #20456 PR-URL: #18924 Refs: #18673 Refs: https://github.com/nodejs/node/blob/v9.5.0/src/node_http2.cc#L1481-L1484 Refs: https://github.com/nodejs/node/blob/v9.5.0/lib/_http_outgoing.js#L659-L661 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Backport-PR-URL: #20456 PR-URL: #18924 Fixes: #18169 Refs: #18673 Refs: https://github.com/nodejs/node/blob/v9.5.0/src/node_http2.cc#L1481-L1484 Refs: https://github.com/nodejs/node/blob/v9.5.0/lib/_http_outgoing.js#L659-L661 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Refs: #18673 Backport-PR-URL: #20456 PR-URL: #18924 Refs: #18673 Refs: https://github.com/nodejs/node/blob/v9.5.0/src/node_http2.cc#L1481-L1484 Refs: https://github.com/nodejs/node/blob/v9.5.0/lib/_http_outgoing.js#L659-L661 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v4.x http2 Issues or PRs related to the http2 subsystem. labels Feb 22, 2018

addaleax mentioned this pull request Feb 22, 2018

http2: fix endless loop when write an empty string #18673

Closed

3 tasks

addaleax and others added 3 commits February 22, 2018 03:52

http2: fix endless loop when writing empty string

9b72053

Refs: nodejs#18673 Fixes: nodejs#18169 Refs: https://github.com/nodejs/node/blob/v9.5.0/src/node_http2.cc#L1481-L1484 Refs: https://github.com/nodejs/node/blob/v9.5.0/lib/_http_outgoing.js#L659-L661

test: check endless loop while writing empty string

74f489c

Refs: nodejs#18673

[squash] extend test case

b702ba3

addaleax force-pushed the http2-fix-endless-loop branch from 27baa0a to b702ba3 Compare February 22, 2018 02:52

XadillaX reviewed Feb 24, 2018

View reviewed changes

jasnell approved these changes Feb 26, 2018

View reviewed changes

XadillaX approved these changes Feb 27, 2018

View reviewed changes

BridgeAR approved these changes Mar 2, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 2, 2018

addaleax closed this Mar 2, 2018

addaleax removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 2, 2018

addaleax deleted the http2-fix-endless-loop branch March 2, 2018 11:50

MylesBorins mentioned this pull request Mar 6, 2018

v9.8.0 proposal #19181

Merged

MylesBorins mentioned this pull request May 2, 2018

v8.11.2 proposal #20478

Merged

+                // This is done here so that .write('', cb) is still a meaningful way to
+                // find out when the HTTP2 stream wants to consume data, and because the
+                // StreamBase API allows empty input chunks.
+                while (!stream->queue_.empty() && stream->queue_.front().buf.len == 0) {

http2: fix endless loop when writing empty string by addaleax · Pull Request #18924 · nodejs/node · GitHub

Uh oh!

Uh oh!

http2: fix endless loop when writing empty string - #18924

http2: fix endless loop when writing empty string#18924
addaleax wants to merge 3 commits into
nodejs:masterfrom
addaleax:http2-fix-endless-loop

addaleax commented Feb 22, 2018

Uh oh!

apapirovski commented Feb 23, 2018

Uh oh!

addaleax commented Feb 23, 2018

Uh oh!

apapirovski commented Feb 23, 2018

Uh oh!

XadillaX Feb 24, 2018

Uh oh!

addaleax Feb 24, 2018

Uh oh!

BridgeAR commented Mar 2, 2018

Uh oh!

addaleax commented Mar 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

http2: fix endless loop when writing empty string by addaleax · Pull Request #18924 · nodejs/node · GitHub

Uh oh!

Uh oh!

http2: fix endless loop when writing empty string - #18924

http2: fix endless loop when writing empty string#18924addaleax wants to merge 3 commits intonodejs:masternodejs/node:masterfrom addaleax:http2-fix-endless-loopaddaleax/node:http2-fix-endless-loopCopy head branch name to clipboard

Conversation

addaleax commented Feb 22, 2018

Checklist

Affected core subsystem(s)

Uh oh!

apapirovski commented Feb 23, 2018

Uh oh!

addaleax commented Feb 23, 2018

Uh oh!

apapirovski commented Feb 23, 2018

Uh oh!

XadillaX Feb 24, 2018

Choose a reason for hiding this comment

Uh oh!

addaleax Feb 24, 2018

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Mar 2, 2018

Uh oh!

addaleax commented Mar 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

http2: fix endless loop when writing empty string#18924
addaleax wants to merge 3 commits into
nodejs:masterfrom
addaleax:http2-fix-endless-loop