node:http2: respondWithFD() never calls options.onError, like node

src/js/node/http2.ts

@@ -2955,6 +2955,7 @@ function tryClose(fd) {
 // it exactly once) and respondWithFD (the caller owns the descriptor: nothing here may close it,
 // matching node's doSendFD).
 function doSendFileFD(options, fd, headers, err, stat) {
+  // respondWithFD() clears onError in its options: node reads it only for respondWithFile().
   const onError = options.onError;
   const ownsFd = this[kOwnsFd] === true;
   if (err) {
@@ -2963,8 +2964,7 @@ function doSendFileFD(options, fd, headers, err, stat) {
     }
 
     if (onError) onError(err);
-    else {
-      this.respond(headers, options);
+    else if (respondOrDestroy.$call(this, headers, options)) {
       this.destroy(streamErrorFromCode(NGHTTP2_INTERNAL_ERROR));
     }
     return;
@@ -2982,10 +2982,7 @@ function doSendFileFD(options, fd, headers, err, stat) {
       const err = isDirectory ? $ERR_HTTP2_SEND_FILE() : $ERR_HTTP2_SEND_FILE_NOSEEK();
       if (ownsFd) tryClose(fd);
       if (onError) onError(err);
-      else {
-        this.respond(headers, options);
-        this.destroy(err);
-      }
+      else if (respondOrDestroy.$call(this, headers, options)) this.destroy(err);
       return;
     }
 
@@ -3097,6 +3094,16 @@ function doSendFileFD(options, fd, headers, err, stat) {
   });
   fileStream.pipe(sink);
 }
+// respond() can throw, and a throw from an fs callback is an uncaught exception.
+function respondOrDestroy(this: ServerHttp2Stream, headers, options) {
+  try {
+    this.respond(headers, options);
+    return true;
+  } catch (err) {
+    this.destroy(err);
+    return false;
+  }
+}
 function onFileStreamError(this: Http2Stream) {
   if (!this.destroyed && !this.closed) this.close(NGHTTP2_INTERNAL_ERROR);
 }
@@ -3416,6 +3423,8 @@ class ServerHttp2Stream extends Http2Stream {
     // The caller owns this fd; clear any stale flag left by a prior respondWithFile()
     // on the same stream so doSendFileFD will not close it (node semantics).
     this[kOwnsFd] = false;
+    // node never reads onError: https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/core.js#L2755-L2790
+    options.onError = undefined;
     if (options.statCheck === undefined) {
       // node's processRespondWithFD runs synchronously when no statCheck is given: the
       // user-facing writable side is already closed by the time respondWithFD() returns, so a

test/js/node/http2/node-http2.test.js

@@ -4938,6 +4938,125 @@ it("http2 hands out the cached name strings for pseudo-headers and known header
   }
 });
 
+// node calls options.onError only from respondWithFile(): for a failed open or fstat, for a
+// directory, and for another non-regular file with an offset or a length. respondWithFD() never
+// reads it (doSendFD and processRespondWithFD in lib/internal/http2/core.js, checked on node v26.3.0).
+it("http2 respondWithFD never calls options.onError, like Node.js", async () => {
+  const badFd = 2 ** 30; // never a valid descriptor: fstat fails with EBADF
+  const fileFd = fs.openSync(import.meta.path, "r");
+  const dirFd = fs.openSync(import.meta.dir, "r");
+  const ok = { ":status": 200 };
+  const rejected = { ":status": 200, ":method": "GET" }; // respond() rejects a request pseudo-header
+  const headerError = { status: null, serverError: "ERR_HTTP2_INVALID_PSEUDOHEADER" };
+  const statCheck = () => {};
+  const ignoresOnError = [
+    {
+      name: "respondWithFD(bad fd), statCheck",
+      call: (stream, options) => stream.respondWithFD(badFd, ok, { ...options, statCheck }),
+    },
+    { name: "respondWithFD(bad fd)", call: (stream, options) => stream.respondWithFD(badFd, ok, options) },
+    {
+      name: "respondWithFD(directory fd), statCheck",
+      call: (stream, options) => stream.respondWithFD(dirFd, ok, { ...options, statCheck }),
+    },
+    { name: "respondWithFD(directory fd)", call: (stream, options) => stream.respondWithFD(dirFd, ok, options) },
+    {
+      name: "respondWithFD(file fd), rejected headers, statCheck",
+      call: (stream, options) => stream.respondWithFD(fileFd, rejected, { ...options, statCheck }),
+      ...headerError,
+    },
+    {
+      name: "respondWithFD(file fd), rejected headers",
+      call: (stream, options) => stream.respondWithFD(fileFd, rejected, options),
+      ...headerError,
+    },
+    // Two failures at once: the descriptor is bad, and respond() rejects the headers in the fstat callback.
+    {
+      name: "respondWithFD(bad fd), rejected headers",
+      call: (stream, options) => stream.respondWithFD(badFd, rejected, options),
+      ...headerError,
+    },
+    {
+      name: "respondWithFD(directory fd), rejected headers",
+      call: (stream, options) => stream.respondWithFD(dirFd, rejected, options),
+      ...headerError,
+    },
+  ];
+  // respondWithFile() does call onError for these, and the handler's response arrives.
+  const callsOnError = {
+    ENOENT: (stream, options) => stream.respondWithFile(path.join(import.meta.dir, "does-not-exist"), ok, options),
+    ERR_HTTP2_SEND_FILE: (stream, options) => stream.respondWithFile(import.meta.dir, ok, options),
+  };
+
+  const server = http2.createServer();
+  await new Promise(resolve => server.listen(0, "127.0.0.1", resolve));
+  const client = http2.connect(`http://127.0.0.1:${server.address().port}`);
+
+  // One request. `onError` is undefined, "answers 404" (the handler the node docs show), or
+  // "must not run" (the handler destroys the stream, so a call ends the request and fails an
+  // assertion below).
+  async function request(call, { onError }) {
+    const events = { onError: [], serverError: null, status: null, body: "", clientError: null };
+    const serverStreamClosed = Promise.withResolvers();
+    server.once("stream", stream => {
+      stream.on("error", err => (events.serverError = err.code));
+      stream.on("close", serverStreamClosed.resolve);
+      const handler = err => {
+        events.onError.push(err.code);
+        if (onError === "must not run") return stream.destroy();
+        stream.respond({ ":status": 404 });
+        stream.end("from onError");
+      };
+      call(stream, onError === undefined ? {} : { onError: handler });
+    });
+    const req = client.request({ ":path": "/" });
+    req.setEncoding("utf8");
+    req.on("response", headers => (events.status = headers[":status"]));
+    req.on("data", chunk => (events.body += chunk));
+    req.on("error", err => (events.clientError = err.message));
+    req.end();
+    await Promise.all([new Promise(resolve => req.on("close", resolve)), serverStreamClosed.promise]);
+    return events;
+  }
+
+  try {
+    for (const { name, call, ...reported } of ignoresOnError) {
+      // Without onError the stream is destroyed with an error and the client sees a reset.
+      const withoutOnError = { name, ...(await request(call, {})) };
+      expect(withoutOnError).toMatchObject({
+        body: "",
+        clientError: "Stream closed with error code NGHTTP2_INTERNAL_ERROR",
+        ...reported,
+      });
+      expect(withoutOnError.serverError).toBeString();
+      // With onError the outcome is the same, and nothing calls the handler.
+      const withOnError = { name, ...(await request(call, { onError: "must not run" })) };
+      expect(withOnError).toEqual({ ...withoutOnError, onError: [] });
+    }
+    // onError belongs to one call. A respondWithFile() that starts on the same stream before the
+    // fstat of respondWithFD() returns does not hand it to that fstat callback.
+    const bothInFlight = (stream, options) => {
+      stream.respondWithFD(badFd, ok, { ...options, statCheck });
+      stream.respondWithFile(import.meta.path, ok);
+    };
+    expect(await request(bothInFlight, { onError: "must not run" })).toMatchObject({ onError: [] });
+    for (const [code, call] of Object.entries(callsOnError)) {
+      expect(await request(call, { onError: "answers 404" })).toEqual({
+        onError: [code],
+        serverError: null,
+        status: 404,
+        body: "from onError",
+        clientError: null,
+      });
+    }
+  } finally {
+    client.close();
+    server.close();
+    fs.closeSync(fileFd);
+    fs.closeSync(dirFd);
+  }
+});
+
 it("http2 option range error messages use the options. prefix", () => {
   for (const opt of ["maxSessionInvalidFrames", "maxSessionRejectedStreams", "unknownProtocolTimeout"]) {
     let error;