node:http: end HTTP/1 fallback connections after a response that must close them

src/js/internal/http1_server_fallback.ts

@@ -234,6 +234,8 @@ function createHttp1FallbackResponseHandle(socket, shouldKeepAlive, keepAliveTim
 // 'request' with http.IncomingMessage/ServerResponse, like Node's httpConnectionListener routing.
 function connectionListenerHTTP1(server, socket, options) {
   const http = require("node:http");
+  const { kMustCloseConnection } = require("node:_http_server");
+  const { kReqShouldKeepAlive } = require("node:_http_incoming");
   const { HTTPParser, prepareError, calculateLenientFlags, continueExpression } = require("node:_http_common");
   const { kHandle: kHttp1ResponseHandle } = require("internal/http");
   const { allMethods } = process.binding("http_parser");
@@ -292,6 +294,11 @@ function connectionListenerHTTP1(server, socket, options) {
     req.method = typeof methodNum === "number" ? allMethods[methodNum] : methodNum;
     req.upgrade = upgrade;
     req._addHeaderLines(rawHeaders, rawHeaders.length);
+    // Same stamp as the native dispatcher (renderNativeHeaders derives the Connection header
+    // from it): this server never keeps HTTP/1.0 alive; otherwise llhttp's verdict, which
+    // unlike req.headers saw every header field.
+    const keepAlive = versionMajor === 1 && versionMinor === 0 ? false : shouldKeepAlive;
+    req[kReqShouldKeepAlive] = keepAlive;
 
     // Node's parserOnIncoming: upgrade only sticks for CONNECT or when an 'upgrade' listener
     // exists; otherwise fall through to normal dispatch. Returning 2 makes llhttp stop after
@@ -318,20 +325,23 @@ function connectionListenerHTTP1(server, socket, options) {
     // path must carry them too or keep-alive responses lose their timeout line.
     res._keepAliveTimeout = keepAliveTimeout;
     res._maxRequestsPerSocket = server.maxRequestsPerSocket;
-    const handle = createHttp1FallbackResponseHandle(socket, shouldKeepAlive, keepAliveTimeout);
+    // Node's res._last; renderNativeHeaders and onHttp1SocketEnd set it too.
+    if (!keepAlive) res[kMustCloseConnection] = true;
+    const handle = createHttp1FallbackResponseHandle(socket, keepAlive, keepAliveTimeout);
     handle.onfinished = function () {
       socket[kHttp1ActiveRequests] = Math.max(0, (socket[kHttp1ActiveRequests] || 1) - 1);
-      if (!shouldKeepAlive && !socket.destroyed) {
-        socket.end();
-      }
     };
     res[kHttp1ResponseHandle] = handle;
     res.assignSocket(socket);
     // node's resOnFinish: release the socket once the response completes so the next
     // keep-alive request's response can attach (assignSocket throws
-    // ERR_HTTP_SOCKET_ASSIGNED while a previous response is still assigned).
+    // ERR_HTTP_SOCKET_ASSIGNED while a previous response is still assigned), and end
+    // the connection after its last response.
     res.on("finish", function onFallbackResponseFinish() {
       this.detachSocket(socket);
+      if (this[kMustCloseConnection] && !socket.destroyed) {
+        socket.end();
+      }
     });
 
     // Node's parserOnIncoming Expect routing (the native dispatcher applies the
@@ -434,7 +444,7 @@ function connectionListenerHTTP1(server, socket, options) {
     }
     const httpMessage = socket._httpMessage;
     if (httpMessage) {
-      httpMessage._last = true;
+      httpMessage[kMustCloseConnection] = true;
     } else if (socket.writable) {
       socket.end();
     }

src/js/node/_http_incoming.ts

@@ -28,9 +28,9 @@ const ObjectDefineProperty = Object.defineProperty;
 const ArrayPrototypeSlice = Array.prototype.slice;
 
 const kHeaders = Symbol("kHeaders");
-// Cache slot for the server dispatcher's keep-alive decision (stamped once
-// per request in _http_server.ts); declared in the constructor so the stamp
-// never shape-transitions the request.
+// Cache slot for the server dispatchers' keep-alive decision (stamped once
+// per request in _http_server.ts / http1_server_fallback.ts); declared in the
+// constructor so the stamp never shape-transitions the request.
 const kReqShouldKeepAlive = Symbol("kReqShouldKeepAlive");
 const kHeadersDistinct = Symbol("kHeadersDistinct");
 const kHeadersCount = Symbol("kHeadersCount");

src/js/node/_http_server.ts

@@ -4024,4 +4024,5 @@ export default {
   Server,
   ServerResponse,
   kConnectionsCheckingInterval,
+  kMustCloseConnection,
 };

test/js/node/http/node-http.test.ts

@@ -4139,3 +4139,171 @@ it("connectionListener hands off Upgrade and CONNECT like Node", async () => {
     expect(serverSide.destroyed).toBe(true);
   }
 });
+
+describe("connectionListener closes the connection like Node's resOnFinish", () => {
+  // A connection is ended after a response when the request forbade reuse, the
+  // response itself did, or (httpAllowHalfOpen) the peer half-closed while the
+  // response was in flight: Node's res._last. The server.emit("connection") path
+  // used to go by the parser's keep-alive flag alone, so an HTTP/1.0 request
+  // asking for keep-alive kept being served on a connection whose every response
+  // said Connection: close, and none of the response-level reasons closed anything.
+  type Handler = (req: IncomingMessage, res: ServerResponse) => void;
+  type Outcome = { connection: string | undefined; responses: number; ended: boolean; served: string[] };
+  type Case = {
+    name: string;
+    version?: string;
+    requestHeaders?: string;
+    handler?: Handler;
+    // httpAllowHalfOpen server; the client half-closes right after its request.
+    peerEndsFirst?: boolean;
+    expected: Outcome;
+  };
+
+  async function serve({
+    version = "1.1",
+    requestHeaders = "",
+    handler = endBody,
+    peerEndsFirst = false,
+  }: Case): Promise<Outcome> {
+    const requestFor = (path: string) => `GET ${path} HTTP/${version}\r\nHost: x\r\n${requestHeaders}\r\n`;
+    const served: string[] = [];
+    const server = createServer((req, res) => {
+      served.push(req.url!);
+      handler(req, res);
+    });
+    if (peerEndsFirst) server.httpAllowHalfOpen = true;
+    const [clientSide, serverSide] = duplexPair();
+    server.emit("connection", serverSide);
+    try {
+      const { promise, resolve, reject } = Promise.withResolvers<void>();
+      let wire = "";
+      let responses = 0;
+      let ended = false;
+      let firstResponseSeen = false;
+      clientSide.on("error", reject);
+      clientSide.on("end", () => {
+        ended = true;
+        resolve();
+      });
+      clientSide.on("data", chunk => {
+        wire += chunk;
+        responses = wire.match(/HTTP\/1\.1 \d{3} /g)?.length ?? 0;
+        if (responses >= 2) {
+          resolve();
+        } else if (responses === 1 && !firstResponseSeen) {
+          firstResponseSeen = true;
+          // A server that ends the connection does so as the first response
+          // finishes, so its 'end' settles this before the immediate runs. One
+          // that keeps it open is sent a second request and answers it (or, once
+          // the client has half-closed, is simply observed to still be open).
+          setImmediate(() => {
+            if (ended) return;
+            if (peerEndsFirst) resolve();
+            else clientSide.write(requestFor("/2"));
+          });
+        }
+      });
+      if (peerEndsFirst) clientSide.end(requestFor("/1"));
+      else clientSide.write(requestFor("/1"));
+      await promise;
+      return { connection: wire.match(/^connection: ([^\r\n]*)/im)?.[1], responses, ended, served };
+    } finally {
+      clientSide.destroy();
+      serverSide.destroy();
+    }
+  }
+
+  function endBody(_req: IncomingMessage, res: ServerResponse) {
+    res.end("served");
+  }
+  const closes: Outcome = { connection: "close", responses: 1, ended: true, served: ["/1"] };
+  const closesAdvertisingKeepAlive: Outcome = { ...closes, connection: "keep-alive" };
+  let manyOtherHeaders = "";
+  for (let i = 0; i < 40; i++) manyOtherHeaders += `X-Filler-${i}: ${i}\r\n`;
+
+  const cases: Case[] = [
+    {
+      name: "ends it after answering an HTTP/1.0 request that asked for keep-alive",
+      version: "1.0",
+      requestHeaders: "Connection: keep-alive\r\n",
+      expected: closes,
+    },
+    { name: "ends it after answering an HTTP/1.0 request", version: "1.0", expected: closes },
+    {
+      name: "ends it after answering an HTTP/1.1 request that sent Connection: close",
+      requestHeaders: "Connection: close\r\n",
+      expected: closes,
+    },
+    {
+      // The parser's verdict must drive both the header and the close: req.headers
+      // does not necessarily retain every header field of a large request.
+      name: "ends it after answering a Connection: close request that carried many other headers",
+      requestHeaders: "Connection: close\r\n" + manyOtherHeaders,
+      expected: closes,
+    },
+    {
+      name: "ends it after a response the handler gave a Connection: close header",
+      handler: (_req, res) => {
+        res.setHeader("Connection", "close");
+        res.end("served");
+      },
+      expected: closes,
+    },
+    {
+      name: "ends it after a response the handler cleared shouldKeepAlive on",
+      handler: (_req, res) => {
+        res.shouldKeepAlive = false;
+        res.end("served");
+      },
+      expected: closes,
+    },
+    {
+      // Node's _removedConnection branch: nothing advertised, still the last response.
+      name: "ends it after a response with shouldKeepAlive cleared and the Connection header removed",
+      handler: (_req, res) => {
+        res.removeHeader("Connection");
+        res.shouldKeepAlive = false;
+        res.end("served");
+      },
+      expected: { ...closes, connection: undefined },
+    },
+    {
+      name: "ends it after a 204 response carrying a Transfer-Encoding header",
+      handler: (_req, res) => {
+        res.writeHead(204, { "Transfer-Encoding": "chunked" });
+        res.end();
+      },
+      expected: closes,
+    },
+    {
+      // This server never reuses an HTTP/1.0 connection (same as its native
+      // listener), whatever Connection header the handler writes itself.
+      name: "ends an HTTP/1.0 connection even when the handler advertised keep-alive",
+      version: "1.0",
+      requestHeaders: "Connection: keep-alive\r\n",
+      handler: (_req, res) => {
+        res.setHeader("Connection", "keep-alive");
+        res.end("served");
+      },
+      expected: closesAdvertisingKeepAlive,
+    },
+    {
+      name: "ends it after the response when the peer half-closed before the handler answered (httpAllowHalfOpen)",
+      peerEndsFirst: true,
+      handler: (req, res) => req.socket.once("end", () => res.end("served")),
+      expected: closesAdvertisingKeepAlive,
+    },
+    {
+      name: "ends it after a synchronous response to a request that arrived together with the peer's FIN (httpAllowHalfOpen)",
+      peerEndsFirst: true,
+      expected: closesAdvertisingKeepAlive,
+    },
+    {
+      name: "keeps a kept-alive HTTP/1.1 connection open for the next request",
+      expected: { connection: "keep-alive", responses: 2, ended: false, served: ["/1", "/2"] },
+    },
+  ];
+  it.each(cases)("$name", async testCase => {
+    expect(await serve(testCase)).toEqual(testCase.expected);
+  });
+});

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

@@ -4365,6 +4365,35 @@ it("http2 allowHTTP1 fallback omits the Connection header on a close-delimited r
   }
 });
 
+it("http2 allowHTTP1 fallback ends the connection after answering an HTTP/1.0 keep-alive request with Connection: close", async () => {
+  // HTTP/1.0 responses are always answered with Connection: close (like
+  // node:http's own server); the fallback used to leave the connection open
+  // anyway when the request had asked for keep-alive.
+  const server = http2.createSecureServer({ ...TLS_CERT, allowHTTP1: true }, (req, res) => {
+    res.end(`served ${req.httpVersion} ${req.url}`);
+  });
+  await new Promise(resolve => server.listen(0, resolve));
+  const { promise, resolve, reject } = Promise.withResolvers();
+  const socket = tls.connect(
+    { host: "localhost", port: server.address().port, ca: TLS_CERT.cert, ALPNProtocols: ["http/1.1"] },
+    () => socket.write("GET /first HTTP/1.0\r\nHost: localhost\r\nConnection: keep-alive\r\n\r\n"),
+  );
+  try {
+    const chunks = [];
+    socket.on("error", reject);
+    socket.on("data", chunk => chunks.push(chunk));
+    // Never arrives while the server keeps the connection open.
+    socket.on("end", () => resolve(Buffer.concat(chunks).toString()));
+    const raw = await promise;
+    expect(raw).toStartWith("HTTP/1.1 200 OK\r\n");
+    expect(raw.slice(0, raw.indexOf("\r\n\r\n") + 4).toLowerCase()).toContain("\r\nconnection: close\r\n");
+    expect(raw.slice(raw.indexOf("\r\n\r\n") + 4)).toBe("served 1.0 /first");
+  } finally {
+    socket.destroy();
+    server.close();
+  }
+});
+
 // close() must not depend on the peer sending a SETTINGS ACK — Node's kMaybeDestroy
 // waits on nghttp2_session_want_write()/want_read(), which does not track outstanding
 // ACKs. A server that never ACKs a client-sent SETTINGS must not stall close().