fix(remix-express,remix-vercel): fix request.signal (#3626)

* fix: fix request.signal for express/vercel

* fix express/vercel unit tests

* add abort signals to arc/netlify requests

* Update netlify/architect tests to assert new signal

* add changeset

.changeset/smooth-buses-approve.md

@@ -0,0 +1,8 @@
+---
+"@remix-run/architect": patch
+"@remix-run/express": patch
+"@remix-run/netlify": patch
+"@remix-run/vercel": patch
+---
+
+fix: abort requests on response close instead of request close (#3626)

integration/abort-signal-test.ts

@@ -0,0 +1,61 @@
+import { test, expect } from "@playwright/test";
+
+import { PlaywrightFixture } from "./helpers/playwright-fixture";
+import type { Fixture, AppFixture } from "./helpers/create-fixture";
+import { createAppFixture, createFixture, js } from "./helpers/create-fixture";
+
+let fixture: Fixture;
+let appFixture: AppFixture;
+
+test.beforeAll(async () => {
+  fixture = await createFixture({
+    files: {
+      "app/routes/index.jsx": js`
+        import { json } from "@remix-run/node";
+        import { useActionData, useLoaderData, Form } from "@remix-run/react";
+
+        export async function action ({ request }) {
+          console.log('action request.signal', request.signal)
+          // New event loop causes express request to close
+          await new Promise(r => setTimeout(r, 0));
+          return json({ aborted: request.signal.aborted });
+        }
+
+        export function loader({ request }) {
+          console.log('loader request.signal', request.signal)
+          return json({ aborted: request.signal.aborted });
+        }
+
+        export default function Index() {
+          let actionData = useActionData();
+          let data = useLoaderData();
+          return (
+            <div>
+              <p className="action">{actionData ? String(actionData.aborted) : "empty"}</p>
+              <p className="loader">{String(data.aborted)}</p>
+              <Form method="post">
+                <button type="submit">Submit</button>
+              </Form>
+            </div>
+          )
+        }
+      `,
+    },
+  });
+
+  // This creates an interactive app using puppeteer.
+  appFixture = await createAppFixture(fixture);
+});
+
+test.afterAll(async () => appFixture.close());
+
+test("should not abort the request in a new event loop", async ({ page }) => {
+  let app = new PlaywrightFixture(appFixture, page);
+  await app.goto("/");
+  expect(await app.getHtml(".action")).toMatch("empty");
+  expect(await app.getHtml(".loader")).toMatch("false");
+
+  await app.clickElement('button[type="submit"]');
+  expect(await app.getHtml(".action")).toMatch("false");
+  expect(await app.getHtml(".loader")).toMatch("false");
+});

packages/remix-architect/__tests__/server-test.ts

@@ -292,7 +292,13 @@ describe("architect createRemixRequest", () => {
           "method": "GET",
           "parsedURL": "https://localhost:3333/",
           "redirect": "follow",
-          "signal": null,
+          "signal": AbortSignal {
+            Symbol(kEvents): Map {},
+            Symbol(events.maxEventTargetListeners): 10,
+            Symbol(events.maxEventTargetListenersWarned): false,
+            Symbol(kAborted): false,
+            Symbol(kReason): undefined,
+          },
         },
       }
     `);
@@ -302,8 +308,7 @@ describe("architect createRemixRequest", () => {
 describe("sendRemixResponse", () => {
   it("handles regular responses", async () => {
     let response = new NodeResponse("anything");
-    let abortController = new AbortController();
-    let result = await sendRemixResponse(response, abortController);
+    let result = await sendRemixResponse(response);
     expect(result.body).toBe("anything");
   });
 
@@ -316,9 +321,7 @@ describe("sendRemixResponse", () => {
       },
     });
 
-    let abortController = new AbortController();
-
-    let result = await sendRemixResponse(response, abortController);
+    let result = await sendRemixResponse(response);
 
     expect(result.body).toMatch(json);
   });
@@ -333,9 +336,7 @@ describe("sendRemixResponse", () => {
       },
     });
 
-    let abortController = new AbortController();
-
-    let result = await sendRemixResponse(response, abortController);
+    let result = await sendRemixResponse(response);
 
     expect(result.body).toMatch(image.toString("base64"));
   });

packages/remix-architect/server.ts

@@ -64,10 +64,14 @@ export function createRemixRequest(event: APIGatewayProxyEventV2): NodeRequest {
   let isFormData = event.headers["content-type"]?.includes(
     "multipart/form-data"
   );
+  // Note: No current way to abort these for Architect, but our router expects
+  // requests to contain a signal so it can detect aborted requests
+  let controller = new AbortController();
 
   return new NodeRequest(url.href, {
     method: event.requestContext.http.method,
     headers: createRemixHeaders(event.headers, event.cookies),
+    signal: controller.signal,
     body:
       event.body && event.isBase64Encoded
         ? isFormData

packages/remix-express/__tests__/server-test.ts

@@ -1,6 +1,6 @@
 import express from "express";
 import supertest from "supertest";
-import { createRequest } from "node-mocks-http";
+import { createRequest, createResponse } from "node-mocks-http";
 import {
   createRequestHandler as createRemixRequestHandler,
   Response as NodeResponse,
@@ -234,8 +234,10 @@ describe("express createRemixRequest", () => {
         Host: "localhost:3000",
       },
     });
+    let expressResponse = createResponse();
 
-    expect(createRemixRequest(expressRequest)).toMatchInlineSnapshot(`
+    expect(createRemixRequest(expressRequest, expressResponse))
+      .toMatchInlineSnapshot(`
       NodeRequest {
         "agent": undefined,
         "compress": true,

packages/remix-express/server.ts

@@ -52,7 +52,7 @@ export function createRequestHandler({
     next: express.NextFunction
   ) => {
     try {
-      let request = createRemixRequest(req);
+      let request = createRemixRequest(req, res);
       let loadContext = getLoadContext?.(req, res);
 
       let response = (await handleRequest(
@@ -89,15 +89,16 @@ export function createRemixHeaders(
   return headers;
 }
 
-export function createRemixRequest(req: express.Request): NodeRequest {
+export function createRemixRequest(
+  req: express.Request,
+  res: express.Response
+): NodeRequest {
   let origin = `${req.protocol}://${req.get("host")}`;
   let url = new URL(req.url, origin);
 
+  // Abort action/loaders once we can no longer write a response
   let controller = new AbortController();
-
-  req.on("close", () => {
-    controller.abort();
-  });
+  res.on("close", () => controller.abort());
 
   let init: NodeRequestInit = {
     method: req.method,

packages/remix-netlify/__tests__/server-test.ts

@@ -273,7 +273,13 @@ describe("netlify createRemixRequest", () => {
           "method": "GET",
           "parsedURL": "http://localhost:3000/",
           "redirect": "follow",
-          "signal": null,
+          "signal": AbortSignal {
+            Symbol(kEvents): Map {},
+            Symbol(events.maxEventTargetListeners): 10,
+            Symbol(events.maxEventTargetListenersWarned): false,
+            Symbol(kAborted): false,
+            Symbol(kReason): undefined,
+          },
         },
       }
     `);
@@ -283,8 +289,7 @@ describe("netlify createRemixRequest", () => {
 describe("sendRemixResponse", () => {
   it("handles regular responses", async () => {
     let response = new NodeResponse("anything");
-    let abortController = new AbortController();
-    let result = await sendRemixResponse(response, abortController);
+    let result = await sendRemixResponse(response);
     expect(result.body).toBe("anything");
   });
 
@@ -297,9 +302,7 @@ describe("sendRemixResponse", () => {
       },
     });
 
-    let abortController = new AbortController();
-
-    let result = await sendRemixResponse(response, abortController);
+    let result = await sendRemixResponse(response);
 
     expect(result.body).toMatch(json);
   });
@@ -314,9 +317,7 @@ describe("sendRemixResponse", () => {
       },
     });
 
-    let abortController = new AbortController();
-
-    let result = await sendRemixResponse(response, abortController);
+    let result = await sendRemixResponse(response);
 
     expect(result.body).toMatch(image.toString("base64"));
   });

packages/remix-netlify/server.ts

@@ -65,9 +65,14 @@ export function createRemixRequest(event: HandlerEvent): NodeRequest {
     url = new URL(rawPath, `http://${origin}`);
   }
 
+  // Note: No current way to abort these for Netlify, but our router expects
+  // requests to contain a signal so it can detect aborted requests
+  let controller = new AbortController();
+
   let init: NodeRequestInit = {
     method: event.httpMethod,
     headers: createRemixHeaders(event.multiValueHeaders),
+    signal: controller.signal,
   };
 
   if (event.httpMethod !== "GET" && event.httpMethod !== "HEAD" && event.body) {

packages/remix-vercel/__tests__/server-test.ts

@@ -1,7 +1,7 @@
 import supertest from "supertest";
-import { createRequest } from "node-mocks-http";
+import { createRequest, createResponse } from "node-mocks-http";
 import { createServerWithHelpers } from "@vercel/node-bridge/helpers";
-import type { VercelRequest } from "@vercel/node";
+import type { VercelRequest, VercelResponse } from "@vercel/node";
 import {
   createRequestHandler as createRemixRequestHandler,
   Response as NodeResponse,
@@ -238,8 +238,9 @@ describe("vercel createRemixRequest", () => {
         "Cache-Control": "max-age=300, s-maxage=3600",
       },
     }) as VercelRequest;
+    let response = createResponse() as unknown as VercelResponse;
 
-    expect(createRemixRequest(request)).toMatchInlineSnapshot(`
+    expect(createRemixRequest(request, response)).toMatchInlineSnapshot(`
       NodeRequest {
         "agent": undefined,
         "compress": true,

packages/remix-vercel/server.ts

@@ -46,7 +46,7 @@ export function createRequestHandler({
   let handleRequest = createRemixRequestHandler(build, mode);
 
   return async (req, res) => {
-    let request = createRemixRequest(req);
+    let request = createRemixRequest(req, res);
     let loadContext = getLoadContext?.(req, res);
 
     let response = (await handleRequest(request, loadContext)) as NodeResponse;
@@ -75,17 +75,18 @@ export function createRemixHeaders(
   return headers;
 }
 
-export function createRemixRequest(req: VercelRequest): NodeRequest {
+export function createRemixRequest(
+  req: VercelRequest,
+  res: VercelResponse
+): NodeRequest {
   let host = req.headers["x-forwarded-host"] || req.headers["host"];
   // doesn't seem to be available on their req object!
   let protocol = req.headers["x-forwarded-proto"] || "https";
   let url = new URL(req.url!, `${protocol}://${host}`);
 
+  // Abort action/loaders once we can no longer write a response
   let controller = new AbortController();
-
-  req.on("close", () => {
-    controller.abort();
-  });
+  res.on("close", () => controller.abort());
 
   let init: NodeRequestInit = {
     method: req.method,