http2: empty DATA frame used instead of END_STREAM on last DATA frame

[clshortfuse]

• Version: v14.3.0
• Platform: Mac OS X
• Subsystem: http2

What steps will reproduce the bug?

Http2Stream.end('data');

How often does it reproduce? Is there a required condition?

It occurs as long as there are no trailers (which means always on ClientHttp2Stream)

What is the expected behavior?

The DATA frame with the last piece of data should have the END_STREAM flag.

What do you see instead?

node/src/node_http2.cc

Lines 2319 to 2326 in f46ca0f

	if (stream->available_outbound_length_ == 0 && !stream->is_writable()) {
	Debug(session, "no more data for stream %d", id);
	*flags |= NGHTTP2_DATA_FLAG_EOF;
	if (stream->has_trailers()) {
	*flags |= NGHTTP2_DATA_FLAG_NO_END_STREAM;
	stream->OnTrailers();
	}
	}

The NGHTTP2_DATA_FLAG_EOF is only set if the outbound stream is no long writable (eg: on shutdown), but not if the internal (Writable) stream has ended.

Additional information

This is how curl, Chrome, and Safari send DATA frames, wherein the last DATA frame contains the END_STREAM flag, instead of sending an empty frame. It makes it more difficult to test http2 if nodeJS applies different logic. Performance-wise, it's a wasted packet.

[clshortfuse]

Here's what the output looks like:

----JS: Calling stream.end(payload)-----
HttpStream 1 (119) [Http2Session server (111)] queuing 1 buffers to send
Http2Session server (111) scheduling write
----JS: Called stream.end(payload)-----
Http2Session server (111) sending pending data
Http2Session server (111) reading outbound data for stream 1
Http2Session server (111) stream 1 has pending outbound data
Http2Session server (111) sending 814 bytes for data frame on stream 1
Http2Session server (111) nghttp2 has 814 bytes to send directly
Http2Session server (111) reading outbound data for stream 1
Http2Session server (111) deferring stream 1
Http2Session server (111) wants read? 1
Http2Session server (111) write finished with status 0
STREAM 71059: onWriteComplete 0 undefined
HTTP2 71059: Http2Stream 1 [Http2Session server]: _final shutting down
HttpStream 1 (119) [Http2Session server (111)] writable side shutdown
Http2Session server (111) scheduling write
Http2Session server (111) sending pending data
Http2Session server (111) reading outbound data for stream 1
Http2Session server (111) no more data for stream 1
Http2Session server (111) stream 1 closed with code: 0
HttpStream 1 (119) [Http2Session server (111)] closed with code 0
HTTP2 71059: Http2Stream 1 [Http2Session server]: closed with code 0, closed false, readable false
HTTP2 71059: Http2Stream 1 [Http2Session server]: destroying stream
STREAM 71059: readableAddChunk null
STREAM 71059: onEofChunk
HttpStream 1 (119) [Http2Session server (111)] destroying stream
HttpStream 1 (119) [Http2Session server (111)] destroying stream
Http2Session server (111) nghttp2 has 9 bytes to send
Http2Session server (111) wants read? 1
HttpStream 1 (119) [Http2Session server (111)] tearing down stream
Http2Session server (111) write finished with status 0
Http2Session server (111) receiving -4095 bytes, offset 0
STREAM 71059: readableAddChunk null
STREAM 71059: onEofChunk
STREAM 71059: emitReadable_ false 0 true
STREAM 71059: flow null
STREAM 71059: read 0
STREAM 71059: endReadable false
STREAM 71059: endReadableNT false 0
HTTP2 71059: Http2Session server: socket closed
HTTP2 71059: Http2Session server: marking session closed
HTTP2 71059: Http2Session server: submitting goaway
Http2Session server (111) submitting goaway
Http2Session server (111) scheduling write
HTTP2 71059: Http2Session server: destroying
HTTP2 71059: Http2Session server: start closing/destroying
Http2Session server (111) destroying session
Http2Session server (111) closing session
Http2Session server (111) make done session callback
HTTP2 71059: Http2Session server: finishSessionClose
Http2Session server (111) sending pending data

The problem lies in two Http2Session server (111) scheduling write events instead of one signifying the stream is over. This change might be a little invasive, simply because the way StreamBase works. It's waiting for the write to complete before _final. And, I guess, if StreamBase doesn't have a way to signify a write chunk is final, neither can Http2Session.

[addaleax]

This change might be a little invasive, simply because the way StreamBase works. It's waiting for the write to complete before _final. And, I guess, if StreamBase doesn't have a way to signify a write chunk is final, neither can Http2Session.

@clshortfuse So … I can give you some context on this :)

Yes, StreamBase does not have a way to let its implementers know whether a chunk is final or not. Mostly, that is because Node.js streams don’t have that, either – for stream implementations, there is also no officially supported way to do this.

We have worked around this problem at least once, in zlib, where we look up streams internals to figure out whether the chunk is final or not:

node/lib/zlib.js

Lines 397 to 401 in 5536044

	// For the last chunk, also apply `_finishFlushFlag`.
	const ws = this._writableState;
	if ((ws.ending || ws.ended) && ws.length === chunk.byteLength) {
	flushFlag = maxFlush(flushFlag, this._finishFlushFlag);
	}

Without that, I think it’s generally not possible to tell the following patterns apart from inside the stream:

stream.write(data, () => stream.end());  // vs.
stream.end(data);

So, my understanding is that, in Http2Stream::DoShutdown(), we should send out an empty data frame with NGHTTP2_DATA_FLAG_EOF set? And that we could optimize that away by setting it on the final chunk in a similar way to how we deal with this in zlib? Is that correct?

[clshortfuse]

So, my understanding is that, in Http2Stream::DoShutdown(), we should send out an empty data frame with NGHTTP2_DATA_FLAG_EOF set? And that we could optimize that away by setting it on the final chunk in a similar way to how we deal with this in zlib? Is that correct?

In Http2Stream::DoShutdown(), we are currently sending out an empty data frame with NGHTTP2_DATA_FLAG_EOF set. We want to avoid sending an empty DATA frame, and instead flag the final frame. This would make the client on par with how all browsers I've seen send http2 data. At a glance, what we want to do looks similar to what is done in zlib.

But, there is a problem. zlib works because it's a Transform and as data is pushed out asynchronously, it can detect ending and set a flag. But Http2Stream is essentially a proxy for nghttp2 stream. It's based on Duplex and .end(), as adopted by Writable, doesn't change .ending or .ended until after the write happens.

node/lib/_stream_writable.js

Line 590 in 5536044

this.write(chunk, encoding);

The path of least resistance seems to queue up the buffers and use DoShutdown to signal set_not_writable(). It seems as though we kinda have this built into node_http2.cc already. The buffer queue is already setup on the C++ side:

https://github.com/nodejs/node/blob/master/src/node_http2.cc#L2191

The problem is the next interaction nearly immediately tells nghttp2 to consume the stream, without letting _final signal the stream. That means either the C++ side is going too fast, or the JS side is going too slow. By the time _final is called, all packets have already been sent. So we want to signal to the C++ side that all write commands have been sent (ie: set_not_writable()) somewhere between Http2Stream::DoWrite and Http2Stream::Provider::Stream::OnRead.

---

I encountered this "was end called?"/"is this the final chunk?" issue, personally while trying to transparently calculate Content-Length while writing to a stream. I tried hijacking .end(), but found it to be too unpredictable at an application-level. So I opted for buffering with Transform._flush() using a process.nextTick(() => setImmediate(() => write delay.

[clshortfuse]

So this works:

[kWriteGeneric](writev, data, encoding, cb) {
    // When the Http2Stream is first created, it is corked until the
    // handle and the stream ID is assigned. However, if the user calls
    // uncork() before that happens, the Duplex will attempt to pass
    // writes through. Those need to be queued up here.
    if (this.pending) {
      this.once(
        'ready',
        this[kWriteGeneric].bind(this, writev, data, encoding, cb)
      );
      return;
    }

    // If the stream has been destroyed, there's nothing else we can do
    // because the handle has been destroyed. This should only be an
    // issue if a write occurs before the 'ready' event in the case where
    // the duplex is uncorked before the stream is ready to go. In that
    // case, drop the data on the floor. An error should have already been
    // emitted.
    if (this.destroyed)
      return;

    this[kUpdateTimer]();
    if (!this.headersSent)
      this[kProceed]();

    let req;

    let waitingForWriteCallback = true;
    let waitingForEndCheck = true;
    let writeCallbackErr;
    let endCheckCallbackErr;
    const done = () => {
      if (waitingForEndCheck || waitingForWriteCallback) return;
      const err = writeCallbackErr || endCheckCallbackErr;
      if (err) {
        // writeGeneric does not destroy on error and we cannot enable autoDestroy,
        // so make sure to destroy on error.
        this.destroy(err);
      }
      cb(err);
    }
    const writeCallback = (err) => {
      waitingForWriteCallback = false;
      writeCallbackErr = err;
      done();
    };
    const endCheckCallback = (err) => {
      waitingForEndCheck = false;
      endCheckCallbackErr = err;
      done()
    }
    process.nextTick(() => {
      if (!this._writableState.ending) return endCheckCallback();
      if (this._writableState.buffered.length) return endCheckCallback();
      const handle = this[kHandle];
      if (!handle) return endCheckCallback();
      debugStreamObj(this, 'fast shutting down');
      const req = new ShutdownWrap();
      req.oncomplete = afterShutdown;
      req.callback = endCheckCallback;
      req.handle = handle;
      const err = handle.shutdown(req);
      if (err === 1)  // synchronous finish
        return afterShutdown.call(req, 0);
    });

    if (writev)
      req = writevGeneric(this, data, writeCallback);
    else
      req = writeGeneric(this, data, encoding, writeCallback);

    trackWriteState(this, req.bytes);
  }

With code injection here:

node/lib/internal/http2/core.js

Line 1981 in 272b46e

// writeGeneric does not destroy on error and we cannot enable autoDestroy,

Basically, after sending a write or writev, check for .ending on the nextTick(). If it's set, then shut down the write stream. That means that it'll batch stream.write(payload); stream.end() into one DATA frame. And if you want to send an empty frame you'll have to do stream.write(payload, () => stream.end());. The shutdown then would be covered by _final().

There might be a flaw with backpressured streams, but that's generally the concept. Fixed