diff --git a/.changeset/smooth-buses-approve.md b/.changeset/smooth-buses-approve.md new file mode 100644 index 00000000000..ae7f798c101 --- /dev/null +++ b/.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) diff --git a/integration/abort-signal-test.ts b/integration/abort-signal-test.ts new file mode 100644 index 00000000000..cca95a88a85 --- /dev/null +++ b/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 ( +
+

{actionData ? String(actionData.aborted) : "empty"}

+

{String(data.aborted)}

+
+ +
+
+ ) + } + `, + }, + }); + + // 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"); +}); diff --git a/packages/remix-architect/__tests__/server-test.ts b/packages/remix-architect/__tests__/server-test.ts index 0dc6bca7a42..5ac8bb24de6 100644 --- a/packages/remix-architect/__tests__/server-test.ts +++ b/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")); }); diff --git a/packages/remix-architect/server.ts b/packages/remix-architect/server.ts index c71c7c56242..0fe02c45d46 100644 --- a/packages/remix-architect/server.ts +++ b/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 diff --git a/packages/remix-express/__tests__/server-test.ts b/packages/remix-express/__tests__/server-test.ts index 3d3b02b9c37..e4b6a6afce8 100644 --- a/packages/remix-express/__tests__/server-test.ts +++ b/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, diff --git a/packages/remix-express/server.ts b/packages/remix-express/server.ts index 576be2c0d33..edbbac623c8 100644 --- a/packages/remix-express/server.ts +++ b/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, diff --git a/packages/remix-netlify/__tests__/server-test.ts b/packages/remix-netlify/__tests__/server-test.ts index 2f344a77a0a..783723d249b 100644 --- a/packages/remix-netlify/__tests__/server-test.ts +++ b/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")); }); diff --git a/packages/remix-netlify/server.ts b/packages/remix-netlify/server.ts index bb3afe1cd19..f73b02a46f4 100644 --- a/packages/remix-netlify/server.ts +++ b/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) { diff --git a/packages/remix-vercel/__tests__/server-test.ts b/packages/remix-vercel/__tests__/server-test.ts index 4e4e72c246c..a6fdd6c427c 100644 --- a/packages/remix-vercel/__tests__/server-test.ts +++ b/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, diff --git a/packages/remix-vercel/server.ts b/packages/remix-vercel/server.ts index cd5ccdd9bd8..44a4f88f4d4 100644 --- a/packages/remix-vercel/server.ts +++ b/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,