http2: fix endless loop when write an empty string

[XadillaX]

If we do req.write("") (an empty string) on Http2Stream, the gather
logic will be messed up:

while ((src_length = nghttp2_session_mem_send(session_, &src)) > 0) {
  DEBUG_HTTP2SESSION2(this, "nghttp2 has %d bytes to send", src_length);
  CopyDataIntoOutgoing(src, src_length);
}

The logic above is in function Http2Session::SendPendingData under
src/node_http2.cc. This while will be endless when an empty string is
inside because src_length will always be 9.

And then the main event loop thread will be blocked and the memory used
will be larger and larger.

This pull request is to ignore empty string or buffer in _write() and
_writev() of Http2Stream.

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 passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

http2

[addaleax]

The first piece of the conditional is always true; Also, you’d still need to call cb.

[XadillaX]

I'm confused that is the logic below the same?

req.write('');
req.end();

vs

req.end();

[XadillaX]

That means can I actully ignore empty string stream?

[addaleax]

(Basically the same comment, except that the first piece of the conditional is never true; data is an array)

[XadillaX]

@addaleax I've updated the code.

[addaleax]

@XadillaX But this still wouldn’t send out an empty frame, like we’d expect, right?

I think this line is the reason why we don’t tell nghttp2 to defer the frames:

node/src/node_http2.cc

Line 2232 in a1bab82

if (amount == 0 && stream->IsWritable() && stream->queue_.empty()) {

The queue isn’t empty when we schedule an empty write, even though we don’t have any data available. Maybe we can just drop the last check in that conditional?

[XadillaX]

@addaleax I found that module http ignores empty stream (but write header) either.

node/lib/_http_outgoing.js

Lines 650 to 652 in a1bab82

	// If we get an empty string or buffer, then just do nothing, and
	// signal the user to keep writing.
	if (chunk.length === 0) return true;

[addaleax]

@XadillaX You’re right, and this way is certainly an approach to fix this bug. I’d have a preference for fixing it on the native level, though:

• One of the next things I’ll be working on wrt refactoring Node’s native streams is that I’ll try to abstract everything from net.Socket and Http2Stream so that there’ll be a common JS interface for the native StreamBase handles. That means that the _write() functions also should be merged, which would probably require undoing this change as it is now. :/
• In a similar vein, the bug here is not part of the JS code that uses the StreamBase interface; it’s specific to how HTTP/2 implements that interface, so the implementation should be fixed rather than the JS code that just provides bindings.
  • At least one case in which we rely on empty writes being meaningful is the TLS implementation; they are explicitly used to flush the underlying protocol’s state. I think it’s reasonable to make the same assumptions about HTTP/2 (and for HTTP/1 there’s no equivalent of flushing underlying state).
• The code line I linked above seems to just be buggy, it should be fixed anyway, and doing so should fully resolve this bug.
• The current approach does not fix the bug fully: Replacing the req.write(""); in the original reproduction with two calls, i.e. req.write(""); req.write("");, still makes the bug appear even with this PR applied.

If you want, you can still make the case for not sending empty data frames on the HTTP/2 level, but that should still be done as part of the implementation, not part of the streams API.

[addaleax]

Hm … @XadillaX any further thoughts on this one?

[addaleax]

@XadillaX I’ve spelled out my thoughts in #18924. I have to admit the solution is not quite as elegant as I had thought, but I still think the C++ level is the right place to address this issue

[XadillaX]

@addaleax Sorry for late reply because of Chinese Spring Festival. I think you're right, if we can solve this issue in C++, then in C++.

[BridgeAR]

I am closing this since @XadillaX gave a LG in #18924.

If anyone feels like that is wrong, please go ahead and reopen.