Skip to content

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

Rodrigo Muino Tomonari requested to merge github/fork/kanongil/http2-destroy-fix into main

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 (closed) 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

Merge request reports