From 09efcccc153bd5fbfea859860367facd4e79b628 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 19 Sep 2026 18:35:48 +0000 Subject: [PATCH 1/5] node:http2: call options.onError only where node does doSendFileFD is shared by respondWithFile() and respondWithFD(). It called options.onError for both. Node reads onError only in respondWithFile() (afterOpen and doSendFileFD). Its doSendFD and processRespondWithFD never read it. respondWithFD() now clears onError on its own copy of the options. respondWithFD() without statCheck ends the writable side when it is called. A response that the onError handler sent after that never ended, so the client request did not close. A header error from respond() now destroys the stream for respondWithFile() too. Node's processRespondWithFD does the same for both entry points. --- src/js/node/http2.ts | 14 ++-- test/js/node/http2/node-http2.test.js | 107 ++++++++++++++++++++++++++ 2 files changed, 116 insertions(+), 5 deletions(-) diff --git a/src/js/node/http2.ts b/src/js/node/http2.ts index abfd52512f03..250575d49282 100644 --- a/src/js/node/http2.ts +++ b/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) { @@ -3044,12 +3045,10 @@ function doSendFileFD(options, fd, headers, err, stat) { } catch (err) { // respond() rejected the headers (e.g. a request pseudo-header in the response): the fd opened // for the file never reaches a read stream, so close it here before the stream is destroyed. + // node destroys the stream here for both entry points and never calls onError: + // https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/core.js#L2714-L2719 if (this[kOwnsFd] === true) tryClose(fd); - if (typeof onError === "function") { - onError(err); - } else { - this.destroy(err); - } + this.destroy(err); return; } @@ -3416,6 +3415,11 @@ 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's respondWithFD() never reads options.onError, only respondWithFile() does: + // https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/core.js#L2755-L2790 + // Cleared on this call's copy of the options: kOwnsFd is per stream and can change before + // fstat returns. + 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 diff --git a/test/js/node/http2/node-http2.test.js b/test/js/node/http2/node-http2.test.js index 7cf5c3e7ab3a..400c94ce8f4e 100644 --- a/test/js/node/http2/node-http2.test.js +++ b/test/js/node/http2/node-http2.test.js @@ -4938,6 +4938,113 @@ 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, and a header error destroys the stream for both methods (afterOpen, doSendFileFD, +// doSendFD and processRespondWithFD in lib/internal/http2/core.js, checked on node v26.3.0). +it("http2 respondWithFD never calls options.onError and a header error never reaches it, 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 = () => {}; + // Each statCheck case runs before its twin without statCheck. If onError is called, the + // statCheck case fails on an assertion. The twin has ended its writable side by then, so the + // handler's response would never end and the test would time out. + 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, + }, + { + name: "respondWithFile(file), rejected headers", + call: (stream, options) => stream.respondWithFile(import.meta.path, 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. The handler passed as onError answers with a 404, like the node docs show. + async function request(call, { withOnError }) { + 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 onError = err => { + events.onError.push(err.code); + stream.respond({ ":status": 404 }); + stream.end("from onError"); + }; + call(stream, withOnError ? { onError } : {}); + }); + 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 and the client sees a reset. + const withoutOnError = { name, ...(await request(call, { withOnError: false })) }; + expect(withoutOnError).toMatchObject({ + serverError: expect.any(String), + body: "", + clientError: "Stream closed with error code NGHTTP2_INTERNAL_ERROR", + ...reported, + }); + // With onError the outcome is the same, and nothing calls the handler. + const withOnError = { name, ...(await request(call, { withOnError: true })) }; + expect(withOnError).toEqual({ ...withoutOnError, onError: [] }); + } + for (const [code, call] of Object.entries(callsOnError)) { + expect(await request(call, { withOnError: true })).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; From 6b683720168043216fa251eaa78c174736f5122b Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 19 Sep 2026 22:37:46 +0000 Subject: [PATCH 2/5] node:http2: do not throw from the fstat callback when respond() fails doSendFileFD calls respond() in the two arms that run without onError. respond() throws when the stream is already destroyed and when it rejects the headers. The throw left the fstat callback as an uncaught exception. respondWithFD() callers that pass onError now reach these arms, so the error destroys the stream there, like the arm that sends the file. The test covers a bad descriptor together with rejected headers. Its onError handler for the cases that must not call it destroys the stream, so an unwanted call fails an assertion and does not wait for the timeout. --- src/js/node/http2.ts | 19 ++++++++++----- test/js/node/http2/node-http2.test.js | 35 ++++++++++++++++++--------- 2 files changed, 36 insertions(+), 18 deletions(-) diff --git a/src/js/node/http2.ts b/src/js/node/http2.ts index 250575d49282..01c2d6d88b4b 100644 --- a/src/js/node/http2.ts +++ b/src/js/node/http2.ts @@ -2964,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; @@ -2983,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; } @@ -3096,6 +3092,17 @@ function doSendFileFD(options, fd, headers, err, stat) { }); fileStream.pipe(sink); } +// respond() throws when the stream is already destroyed and when it rejects the headers. A throw +// from an fs callback is an uncaught exception, so the error destroys the stream instead. +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); } diff --git a/test/js/node/http2/node-http2.test.js b/test/js/node/http2/node-http2.test.js index 400c94ce8f4e..44d9a87e26b1 100644 --- a/test/js/node/http2/node-http2.test.js +++ b/test/js/node/http2/node-http2.test.js @@ -4950,9 +4950,6 @@ it("http2 respondWithFD never calls options.onError and a header error never rea const rejected = { ":status": 200, ":method": "GET" }; // respond() rejects a request pseudo-header const headerError = { status: null, serverError: "ERR_HTTP2_INVALID_PSEUDOHEADER" }; const statCheck = () => {}; - // Each statCheck case runs before its twin without statCheck. If onError is called, the - // statCheck case fails on an assertion. The twin has ended its writable side by then, so the - // handler's response would never end and the test would time out. const ignoresOnError = [ { name: "respondWithFD(bad fd), statCheck", @@ -4979,6 +4976,17 @@ it("http2 respondWithFD never calls options.onError and a header error never rea call: (stream, options) => stream.respondWithFile(import.meta.path, 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 = { @@ -4990,19 +4998,22 @@ it("http2 respondWithFD never calls options.onError and a header error never rea 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. The handler passed as onError answers with a 404, like the node docs show. - async function request(call, { withOnError }) { + // 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 onError = err => { + 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, withOnError ? { onError } : {}); + call(stream, onError === undefined ? {} : { onError: handler }); }); const req = client.request({ ":path": "/" }); req.setEncoding("utf8"); @@ -5016,20 +5027,20 @@ it("http2 respondWithFD never calls options.onError and a header error never rea try { for (const { name, call, ...reported } of ignoresOnError) { - // Without onError the stream is destroyed and the client sees a reset. - const withoutOnError = { name, ...(await request(call, { withOnError: false })) }; + // Without onError the stream is destroyed with an error and the client sees a reset. + const withoutOnError = { name, ...(await request(call, {})) }; expect(withoutOnError).toMatchObject({ - serverError: expect.any(String), 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, { withOnError: true })) }; + const withOnError = { name, ...(await request(call, { onError: "must not run" })) }; expect(withOnError).toEqual({ ...withoutOnError, onError: [] }); } for (const [code, call] of Object.entries(callsOnError)) { - expect(await request(call, { withOnError: true })).toEqual({ + expect(await request(call, { onError: "answers 404" })).toEqual({ onError: [code], serverError: null, status: 404, From fa960e6e8e314f178cbf7fcdc28ef8313b5aecb3 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 19 Sep 2026 23:12:53 +0000 Subject: [PATCH 3/5] node:http2: test that onError belongs to one call, shorten comments The new test case starts respondWithFile() on a stream while the fstat of respondWithFD() is pending. A check of the per-stream kOwnsFd flag in doSendFileFD would call the onError of respondWithFD() there. The clear on the options copy of the call does not. --- src/js/node/http2.ts | 11 +++-------- test/js/node/http2/node-http2.test.js | 7 +++++++ 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/src/js/node/http2.ts b/src/js/node/http2.ts index 01c2d6d88b4b..19b4babfc9ca 100644 --- a/src/js/node/http2.ts +++ b/src/js/node/http2.ts @@ -3041,8 +3041,7 @@ function doSendFileFD(options, fd, headers, err, stat) { } catch (err) { // respond() rejected the headers (e.g. a request pseudo-header in the response): the fd opened // for the file never reaches a read stream, so close it here before the stream is destroyed. - // node destroys the stream here for both entry points and never calls onError: - // https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/core.js#L2714-L2719 + // node never calls onError: https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/core.js#L2714-L2719 if (this[kOwnsFd] === true) tryClose(fd); this.destroy(err); return; @@ -3092,8 +3091,7 @@ function doSendFileFD(options, fd, headers, err, stat) { }); fileStream.pipe(sink); } -// respond() throws when the stream is already destroyed and when it rejects the headers. A throw -// from an fs callback is an uncaught exception, so the error destroys the stream instead. +// 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); @@ -3422,10 +3420,7 @@ 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's respondWithFD() never reads options.onError, only respondWithFile() does: - // https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/core.js#L2755-L2790 - // Cleared on this call's copy of the options: kOwnsFd is per stream and can change before - // fstat returns. + // 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 diff --git a/test/js/node/http2/node-http2.test.js b/test/js/node/http2/node-http2.test.js index 44d9a87e26b1..a53e2dbd0568 100644 --- a/test/js/node/http2/node-http2.test.js +++ b/test/js/node/http2/node-http2.test.js @@ -5039,6 +5039,13 @@ it("http2 respondWithFD never calls options.onError and a header error never rea 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], From 9e2d7c87cc2004bfe2c969e6ef57a2c4604f5886 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 19 Sep 2026 23:35:49 +0000 Subject: [PATCH 4/5] ci: retrigger From f36c3e4d702f549709ea84d5644febda63364353 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 20 Sep 2026 07:33:45 +0000 Subject: [PATCH 5/5] node:http2: keep onError for a header error in respondWithFile() respond() still rejects header values that node accepts, for example undefined. A respondWithFile() caller with an onError handler gets that error today and can answer with a 500. Without the handler call the stream is destroyed, and a stream with no 'error' listener ends the process. This arm can follow node after respond() accepts what node accepts. respondWithFD() still never calls onError. --- src/js/node/http2.ts | 7 +++++-- test/js/node/http2/node-http2.test.js | 10 ++-------- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/src/js/node/http2.ts b/src/js/node/http2.ts index 19b4babfc9ca..bcd07c9f747f 100644 --- a/src/js/node/http2.ts +++ b/src/js/node/http2.ts @@ -3041,9 +3041,12 @@ function doSendFileFD(options, fd, headers, err, stat) { } catch (err) { // respond() rejected the headers (e.g. a request pseudo-header in the response): the fd opened // for the file never reaches a read stream, so close it here before the stream is destroyed. - // node never calls onError: https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/core.js#L2714-L2719 if (this[kOwnsFd] === true) tryClose(fd); - this.destroy(err); + if (typeof onError === "function") { + onError(err); + } else { + this.destroy(err); + } return; } diff --git a/test/js/node/http2/node-http2.test.js b/test/js/node/http2/node-http2.test.js index a53e2dbd0568..a1fbe59cf6cf 100644 --- a/test/js/node/http2/node-http2.test.js +++ b/test/js/node/http2/node-http2.test.js @@ -4940,9 +4940,8 @@ 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, and a header error destroys the stream for both methods (afterOpen, doSendFileFD, -// doSendFD and processRespondWithFD in lib/internal/http2/core.js, checked on node v26.3.0). -it("http2 respondWithFD never calls options.onError and a header error never reaches it, like Node.js", async () => { +// 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"); @@ -4971,11 +4970,6 @@ it("http2 respondWithFD never calls options.onError and a header error never rea call: (stream, options) => stream.respondWithFD(fileFd, rejected, options), ...headerError, }, - { - name: "respondWithFile(file), rejected headers", - call: (stream, options) => stream.respondWithFile(import.meta.path, 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",