From 6ee69c13dd3e71df2950eba0d33c547f4538ff9c Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 2 Oct 2024 17:58:26 -0400 Subject: [PATCH] fix abort behavior for renderToReadableStream (#10047) --- .changeset/short-maps-reflect.md | 10 +++++++++ .../defaults/entry.server.cloudflare.tsx | 4 +++- .../config/defaults/entry.server.deno.tsx | 4 +++- .../cloudflare-pages/app/entry.server.tsx | 21 ++++++++++++++---- .../cloudflare-workers/app/entry.server.tsx | 21 ++++++++++++++---- .../deno/app/entry.server.tsx | 22 ++++++++++++++----- .../cloudflare-workers/app/entry.server.tsx | 21 ++++++++++++++---- templates/cloudflare/app/entry.server.tsx | 21 ++++++++++++++---- 8 files changed, 101 insertions(+), 23 deletions(-) create mode 100644 .changeset/short-maps-reflect.md diff --git a/.changeset/short-maps-reflect.md b/.changeset/short-maps-reflect.md new file mode 100644 index 00000000000..4bb701e6a97 --- /dev/null +++ b/.changeset/short-maps-reflect.md @@ -0,0 +1,10 @@ +--- +"@remix-run/dev": patch +--- + +Stop passing `request.signal` as the `renderToReadableStream` `signal` to abort server rendering for cloudflare/deno runtimes because by the time that `request` is aborted, aborting the rendering is useless because there's no way for React to flush down the unresolved boundaries + +- This has been incorrect for some time, but only recently exposed due to a bug in how we were aborting requests when running via `remix vite:dev` because we were incorrectly aborting requests after successful renders - which was causing us to abort a completed React render, and try to close an already closed `ReadableStream`. +- This has likely not shown up in any production scenarios because cloudflare/deno production runtimes are (correctly) not aborting the `request.signal` on successful renders +- The built-in `entry.server` files no longer pass a `signal` to `renderToReadableStream` because adding a timeout-based abort signal to the default behavior would constitute a breaking change +- Users can configure this abort behavior via their own `entry.server` via `remix reveal entry.server`, and the template entry.server files have been updated with an example approach for newly created Remix apps diff --git a/packages/remix-dev/config/defaults/entry.server.cloudflare.tsx b/packages/remix-dev/config/defaults/entry.server.cloudflare.tsx index e7bb7a4f4f8..a47be3fc285 100644 --- a/packages/remix-dev/config/defaults/entry.server.cloudflare.tsx +++ b/packages/remix-dev/config/defaults/entry.server.cloudflare.tsx @@ -13,7 +13,9 @@ export default async function handleRequest( const body = await renderToReadableStream( , { - signal: request.signal, + // If you wish to abort the rendering process, you can pass a signal here. + // Please refer to the templates for example son how to configure this. + // signal: controller.signal, onError(error: unknown) { // Log streaming rendering errors from inside the shell console.error(error); diff --git a/packages/remix-dev/config/defaults/entry.server.deno.tsx b/packages/remix-dev/config/defaults/entry.server.deno.tsx index cef1e72d970..0680671278e 100644 --- a/packages/remix-dev/config/defaults/entry.server.deno.tsx +++ b/packages/remix-dev/config/defaults/entry.server.deno.tsx @@ -13,7 +13,9 @@ export default async function handleRequest( const body = await renderToReadableStream( , { - signal: request.signal, + // If you wish to abort the rendering process, you can pass a signal here. + // Please refer to the templates for example son how to configure this. + // signal: controller.signal, onError(error: unknown) { // Log streaming rendering errors from inside the shell console.error(error); diff --git a/templates/classic-remix-compiler/cloudflare-pages/app/entry.server.tsx b/templates/classic-remix-compiler/cloudflare-pages/app/entry.server.tsx index 0d5c40a755e..9bbdcfaa350 100644 --- a/templates/classic-remix-compiler/cloudflare-pages/app/entry.server.tsx +++ b/templates/classic-remix-compiler/cloudflare-pages/app/entry.server.tsx @@ -9,6 +9,8 @@ import { RemixServer } from "@remix-run/react"; import { isbot } from "isbot"; import { renderToReadableStream } from "react-dom/server"; +const ABORT_DELAY = 5000; + export default async function handleRequest( request: Request, responseStatusCode: number, @@ -19,18 +21,29 @@ export default async function handleRequest( // eslint-disable-next-line @typescript-eslint/no-unused-vars loadContext: AppLoadContext ) { + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), ABORT_DELAY); + const body = await renderToReadableStream( - , + , { - signal: request.signal, + signal: controller.signal, onError(error: unknown) { - // Log streaming rendering errors from inside the shell - console.error(error); + if (!controller.signal.aborted) { + // Log streaming rendering errors from inside the shell + console.error(error); + } responseStatusCode = 500; }, } ); + body.allReady.then(() => clearTimeout(timeoutId)); + if (isbot(request.headers.get("user-agent") || "")) { await body.allReady; } diff --git a/templates/classic-remix-compiler/cloudflare-workers/app/entry.server.tsx b/templates/classic-remix-compiler/cloudflare-workers/app/entry.server.tsx index 0d5c40a755e..9bbdcfaa350 100644 --- a/templates/classic-remix-compiler/cloudflare-workers/app/entry.server.tsx +++ b/templates/classic-remix-compiler/cloudflare-workers/app/entry.server.tsx @@ -9,6 +9,8 @@ import { RemixServer } from "@remix-run/react"; import { isbot } from "isbot"; import { renderToReadableStream } from "react-dom/server"; +const ABORT_DELAY = 5000; + export default async function handleRequest( request: Request, responseStatusCode: number, @@ -19,18 +21,29 @@ export default async function handleRequest( // eslint-disable-next-line @typescript-eslint/no-unused-vars loadContext: AppLoadContext ) { + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), ABORT_DELAY); + const body = await renderToReadableStream( - , + , { - signal: request.signal, + signal: controller.signal, onError(error: unknown) { - // Log streaming rendering errors from inside the shell - console.error(error); + if (!controller.signal.aborted) { + // Log streaming rendering errors from inside the shell + console.error(error); + } responseStatusCode = 500; }, } ); + body.allReady.then(() => clearTimeout(timeoutId)); + if (isbot(request.headers.get("user-agent") || "")) { await body.allReady; } diff --git a/templates/classic-remix-compiler/deno/app/entry.server.tsx b/templates/classic-remix-compiler/deno/app/entry.server.tsx index 27f656dd448..b0af1d5cbd0 100644 --- a/templates/classic-remix-compiler/deno/app/entry.server.tsx +++ b/templates/classic-remix-compiler/deno/app/entry.server.tsx @@ -9,6 +9,8 @@ import { RemixServer } from "@remix-run/react"; import { isbot } from "isbot"; import { renderToReadableStream } from "react-dom/server"; +const ABORT_DELAY = 5000; + export default async function handleRequest( request: Request, responseStatusCode: number, @@ -16,21 +18,31 @@ export default async function handleRequest( remixContext: EntryContext, // This is ignored so we can keep it in the template for visibility. Feel // free to delete this parameter in your app if you're not using it! - loadContext: AppLoadContext ) { + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), ABORT_DELAY); + const body = await renderToReadableStream( - , + , { - signal: request.signal, + signal: controller.signal, onError(error: unknown) { - // Log streaming rendering errors from inside the shell - console.error(error); + if (!controller.signal.aborted) { + // Log streaming rendering errors from inside the shell + console.error(error); + } responseStatusCode = 500; }, } ); + body.allReady.then(() => clearTimeout(timeoutId)); + if (isbot(request.headers.get("user-agent") || "")) { await body.allReady; } diff --git a/templates/cloudflare-workers/app/entry.server.tsx b/templates/cloudflare-workers/app/entry.server.tsx index 0d5c40a755e..9bbdcfaa350 100644 --- a/templates/cloudflare-workers/app/entry.server.tsx +++ b/templates/cloudflare-workers/app/entry.server.tsx @@ -9,6 +9,8 @@ import { RemixServer } from "@remix-run/react"; import { isbot } from "isbot"; import { renderToReadableStream } from "react-dom/server"; +const ABORT_DELAY = 5000; + export default async function handleRequest( request: Request, responseStatusCode: number, @@ -19,18 +21,29 @@ export default async function handleRequest( // eslint-disable-next-line @typescript-eslint/no-unused-vars loadContext: AppLoadContext ) { + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), ABORT_DELAY); + const body = await renderToReadableStream( - , + , { - signal: request.signal, + signal: controller.signal, onError(error: unknown) { - // Log streaming rendering errors from inside the shell - console.error(error); + if (!controller.signal.aborted) { + // Log streaming rendering errors from inside the shell + console.error(error); + } responseStatusCode = 500; }, } ); + body.allReady.then(() => clearTimeout(timeoutId)); + if (isbot(request.headers.get("user-agent") || "")) { await body.allReady; } diff --git a/templates/cloudflare/app/entry.server.tsx b/templates/cloudflare/app/entry.server.tsx index 0d5c40a755e..9bbdcfaa350 100644 --- a/templates/cloudflare/app/entry.server.tsx +++ b/templates/cloudflare/app/entry.server.tsx @@ -9,6 +9,8 @@ import { RemixServer } from "@remix-run/react"; import { isbot } from "isbot"; import { renderToReadableStream } from "react-dom/server"; +const ABORT_DELAY = 5000; + export default async function handleRequest( request: Request, responseStatusCode: number, @@ -19,18 +21,29 @@ export default async function handleRequest( // eslint-disable-next-line @typescript-eslint/no-unused-vars loadContext: AppLoadContext ) { + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), ABORT_DELAY); + const body = await renderToReadableStream( - , + , { - signal: request.signal, + signal: controller.signal, onError(error: unknown) { - // Log streaming rendering errors from inside the shell - console.error(error); + if (!controller.signal.aborted) { + // Log streaming rendering errors from inside the shell + console.error(error); + } responseStatusCode = 500; }, } ); + body.allReady.then(() => clearTimeout(timeoutId)); + if (isbot(request.headers.get("user-agent") || "")) { await body.allReady; }