HTTP/2 streams cancelled with an AbortSignal should close with NGHTTP2_CANCEL instead of NGHTTP2_INTERNAL_ERROR

[timostamm]

Version

v18.15.0

Platform

Darwin XXX 22.3.0 Darwin Kernel Version 22.3.0: Mon Jan 30 20:38:37 PST 2023; root:xnu-8792.81.3~2/RELEASE_ARM64_T6000 arm64

Subsystem

http2

What steps will reproduce the bug?

With an AbortSignal, it's possible to somewhat conveniently cancel an HTTP/2 request. The HTTP/2 spec actually defines an error code to be used for RST_STREAM frame in this case (RFC7540, section 7):

CANCEL (0x8): Used by the endpoint to indicate that the stream is no longer needed.

But with a request from the Node.js http2 module, an AbortSignal will cause the stream to be closed with:

INTERNAL_ERROR (0x2): The endpoint encountered an unexpected internal error.

The behavior can be reproduced with the attached script, which outputs stream closed with RST_STREAM error code 2.

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

No response

What is the expected behavior? Why is that the expected behavior?

I would expect an AbortSignal to send code CANCEL (NGHTTP2_CANCEL), and see the output stream closed with RST_STREAM error code 8 from the attached script.

Using an AbortSignal seems like the best choice today to cancel requests (for example in an application that reaches out to a server for autocomplete suggestions). In such a use-case, having streams close with a code that indicates an unexpected internal error causes issues for observability and metrics.

Of course applications can switch to closing streams manually, but it seems reasonable for the http2 module to use the appropriate HTTP/2 code instead, and let users continue to use the more convenient AbortSignal.

I propose to make a minimal change to lib/internal/http2/core.js - basically:

    const code = (err != null ?
-      (sessionCode || NGHTTP2_INTERNAL_ERROR) :
+      (sessionCode || (err instanceof AbortError ? NGHTTP2_CANCEL : NGHTTP2_INTERNAL_ERROR)) :
      (this.closed ? this.rstCode : sessionCode)
    );

What do you see instead?

Code INTERNAL_ERROR - stream closed with RST_STREAM error code 2 from the attached script.

Additional information

const http2 = require("http2");
const net = require("net");

const server = http2
  .createServer()
  .on("stream", (stream) => {
    stream
      .on("error", () => {})
      .on("close", () => {
        console.log("stream closed with RST_STREAM error code", stream.rstCode);
        server.close();
      });
  })
  .listen(0, () => {
    http2.connect(
      `http://localhost:${server.address().port}`,
      (session) => {
        const abortController = new AbortController();
        session
          .request(
            {
              ":method": "POST",
              ":path": "/foo",
            },
            {
              signal: abortController.signal,
            }
          )
          .on("error", () => {});
        setTimeout(() => abortController.abort(), 50);
        setTimeout(() => session.close(), 150);
      }
    );
  });

[bnoordhuis]

I don't see a problem with that, pull request welcome. Please break it up in separate statements though; your diff exceeds the ternary threshold. :-)

[devm33]

Hello :) I've run into this same issue. If it's alright with you @timostamm I'd like to move forward with the fix you suggested.

[timostamm]

I have not found time to look into it yet, @devm33, please feel free to take it!

[devm33]

Thanks @timostamm, opened #48573

@bnoordhuis could this fix be eligible for backporting? It looks like it should merge cleanly into at least 18-staging.