http2: fix endless loop when writing empty string

[addaleax]

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

[apapirovski]

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

[addaleax]

@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?)

[apapirovski]

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.

[XadillaX]

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

[addaleax]

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

[BridgeAR]

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

[addaleax]

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

Thanks for the reviews!