http2: Client can return partial response (instead of error emit)

[kanongil]

• Version: v14.10.1
• Platform: macOS 10.15.6
• Subsystem: http2

What steps will reproduce the bug?

1. Setup a http2 server that will:
   a. Send a response containing a content-length header.
   b. Stall after transmitting part of the data (or just the headers).
   c. Stop the transmission with a RST_STREAM frame with error_code = 0.
2. Create a http2 client (new or old api), and call the server, trying to receive the content.
3. Validate that content-length bytes have been received.

FYI, I don't know how to setup such a server, though it can occur naturally with envoy in http2 mode.

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

100%. It requires the stream to be closed with a RST_STREAM frame containing error_code = 0.

What is the expected behavior?

http2 client emits an error.

What do you see instead?

No error. Just a partial response, with less than content-length bytes.

Additional information

This is caused by an nghttp2 bug: nghttp2/nghttp2#1508

The bug causes this callback to be called with code = NGHTTP2_NO_ERROR when the server sends a RST_STREAM frame with error_code = 0 for an active stream.

More details in my initial reported issue here: sindresorhus/got#1462

[nejat-njonjo]

I will take a look into this

[kanongil]

I made a test case:

'use strict';

const Http2 = require('http2');

const server = Http2.createServer();

server.on('stream', (stream, headers) => {
  stream.respond({
    'content-length': '4000',
    ':status': 200
  });
  stream.write(Buffer.alloc(2000));
  stream.on('error', (err) => console.error('server error', err));
  stream.destroy();  // or stream.close(0) if https://github.com/nodejs/node/pull/35378 is merged
});

server.listen(4000, () => {

  const client = Http2.connect('http://localhost:4000');
  client.on('error', (err) => console.error('client error', err));

  const req = client.request();
  req.on('error', (err) => console.error('req error', err));

  req.on('response', async (headers, flags) => {
    for (const name in headers) {
      console.log(`HEADER ${name}: ${headers[name]}`);
    }

    let bytes = 0;
    for await (const chunk of req) {
      bytes += chunk.length;
    }

    console.log('DONE with', bytes);
  });
});

Output with transferred bytes vs. content-length mismatch:

HEADER :status: 200
HEADER content-length: 4000
HEADER date: Mon, 28 Sep 2020 19:51:08 GMT
DONE with 2000

This is fixed by my nghttp2 bug fix, but could also be handled by node in the interim.

[kanongil]

Even worse, changing stream.destroy() to stream.close(8) will trigger another bug, and return a successful response though the server clearly sent an error (code 8 / NGHTTP2_CANCEL). The later issue is fixed with my PR in #35378.

[nwtgck]

Hi,

One of the issues mentioned in this issue: #33537 was fixed. Here is the details: #33537 (comment)

[szmarczak]

Finally I have the time to fix this. Expect a PR soon.