http2: fix destroy() to send and handle remote NGHTTP2_CANCEL

[kanongil]

This patch fixes the HTTP2 stream close behaviour to be more inline with HTTP1.

Most importantly, it allows consumers to detect that the remote server/client has closed the stream prematurely with code 8 (NGHTTP2_CANCEL). Without this patch, the consumer just receives an 'end' event, as if nothing bad happened!

Secondly, and also very important, it ensures that a stream.destroy() called on an active stream, will signal that the stream ended prematurely with code 8 (NGHTTP2_CANCEL), instead of code 0 (NGHTTP2_NO_ERROR).

This fixes #35302 and appears to fix #33537 as well. It also helps with #35209, since node will send the proper RST_STREAM code, and clients should not need to check for 'content-length' to know something is wrong.

How it is done

Inside Http2Stream._destroy():

1. Always call closeStream() with a non-zero code. This ensures that the destroy() is signalled when needed.
2. Emit an 'error' if stream has been closed with code 8 by the remote, without an 'aborted' event being triggered.
3. Avoid sending 'end' event for streams that are going to error.

While the patch itself is somewhat simple, it requires a lot of updated test cases. At face value, it would appear that this is a bad patch, but I would like to argue that all the modified tests are just inherently wrong. As such, this should not be seen as a breaking change, but a long coming bug fix.

Most of the modified tests are just about changing a wrongly expected 'end' to an 'error'. For some of the tests that didn't explicitly test destroy(), I changed it to end the stream more gracefully instead. Feel free to challenge me on any of the changes, as I have put a lot of thought into it, and believe they are all appropriate bug fixes.

Checklist

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

[nodejs-github-bot]

Review requested:

• @nodejs/http2
• @nodejs/net

[kanongil]

No interest in fixing http2 to actually error on partially transferred streams?

The alternative "fix" is to document that any http2 data transfers might only contain part of the request data, and application level validation is needed (eg. checksum validation) for all incoming and outgoing streams, and for all clients. Though, that would be a breaking change.

FYI, I see that some tests fail on macOS, but I won't look further into it until I get some feedback.

[Trott]

@nodejs/http2 @nodejs/streams This could use some reviews.

[mcollina]

No interest in fixing http2 to actually error on partially transferred streams?

Unfortunately everybody is a bit busy in this period and focused on Shipping v15. We'll get to review asap.

[mcollina]

lgtm

[mcollina]

I'm a bit unsure to flag this as a bugfix/patch or mark it as semver-major.

cc @ronag @jasnell wdyt?

[jasnell]

I'm fine with this as a semver-patch

[nodejs-github-bot]

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

[aduh95]

@kanongil can you take a look at the CI failures please?

[Trott]

Is this something we want to rebase and land? Or is this fixed elsewhere or otherwise obsolete?

[github-actions]

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

[github-actions]

Closing this because it has stalled. Feel free to reopen if this issue/PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.