http2: fix destroy() to send and handle remote NGHTTP2_CANCEL
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()
:
- Always call
closeStream()
with a non-zero code. This ensures that thedestroy()
is signalled when needed. - Emit an
'error'
if stream has been closed with code 8 by the remote, without an'aborted'
event being triggered. - 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), orvcbuild test
(Windows) passes -
tests and/or benchmarks are included -
commit message follows commit guidelines