remix-run / remix

Build Better Websites. Create modern, resilient user experiences with web fundamentals.
https://remix.run
MIT License
29.55k stars 2.49k forks source link

Your worker created multiple branches of a single stream (for instance, by calling `response.clone()` or `request.clone()`) #7032

Open Cool9977 opened 1 year ago

Cool9977 commented 1 year ago

What version of Remix are you using?

1.19.1

Are all your remix dependencies & dev-dependencies using the same version?

Steps to Reproduce

I am creating a Shopify app by using @remix-run/cloudflare-pages and @shopify/shopify-app-remix. Everything working fine but local log, I am getting this messages

Your worker created multiple branches of a single stream (for instance, by calling `response.clone()` or `request.clone()`) but did not read the body of both branches. This is wasteful, as it forces the system to buffer the entire stream of data in memory, rather than streaming it through. This may cause your worker to be unexpectedly terminated for going over the memory limit. If you only meant to copy the request or response headers and metadata (e.g. in order to be able to modify them), use the appropriate constructors instead (for instance, `new Response(response.body, response)`, `new Request(request)`, etc).

I am not sure how I can debug this ? When I am checking cloudflare page adapter, I see

    try {
      response = await context.env.ASSETS.fetch(
        context.request.url,
        context.request.clone()
      );
      response =
        response && response.status >= 200 && response.status < 400
          ? new Response(response.body, response)
          : undefined;
    } catch {}

This is server.ts

export const onRequest = createPagesFunctionHandler({
  build,
  getLoadContext: ({ env }) => {
    const shopify = shopifyApp({
      apiKey: env.SHOPIFY_API_KEY,
      apiSecretKey: env.SHOPIFY_API_SECRET || "",
      apiVersion: LATEST_API_VERSION,
      scopes: env.SCOPES?.split(","),
      appUrl: env.SHOPIFY_APP_URL || "",
      useOnlineTokens: env.SHOPIFY_ONLINE || false,
      authPathPrefix: "/auth",
      sessionStorage: new Storage(env.DATABASE_URL),
      distribution: AppDistribution.AppStore,
      restResources,
      webhooks: {
        APP_UNINSTALLED: {
          deliveryMethod: DeliveryMethod.Http,
          callbackUrl: "/webhooks",
        },
      },
      hooks: {
        afterAuth: async ({ session }) => {
          shopify.registerWebhooks({ session });
        },
      },
      ...(env.SHOP_CUSTOM_DOMAIN
        ? { customShopDomains: [env.SHOP_CUSTOM_DOMAIN] }
        : {}),
    });

    return { env, shopify };
  },
  mode: process.env.NODE_ENV,
});

multiple branches of a single stream is created for this ? Any Idea ?

Expected Behavior

Shoud not create multiple branches of a single stream

Actual Behavior

This is the message

Your worker created multiple branches of a single stream (for instance, by calling `response.clone()` or `request.clone()`) but did not read the body of both branches. This is wasteful, as it forces the system to buffer the entire stream of data in memory, rather than streaming it through. This may cause your worker to be unexpectedly terminated for going over the memory limit. If you only meant to copy the request or response headers and metadata (e.g. in order to be able to modify them), use the appropriate constructors instead (for instance, `new Response(response.body, response)`, `new Request(request)`, etc).
KrisBraun commented 1 year ago

I'm getting five copies of this error on 1.19.2 with a trivial, resource-only action:

export const action = async () => {
  return new Response("Not implemented", { status: 501 });
};
joaoguidev commented 1 year ago

I'm using Remix 19.3 and Cloudflare Pages. I'm also getting this message.

Phoenixmatrix commented 1 year ago

Hit this too. From reading the cloudflare issue, I assume its because of whats happening under the hood to set the arguments for actions and loaders when you don't consume them. Not sure what the fix would look like though.

Edit: Is this decision doc related? Seems like a decision that was made specifically because of scenarios like these. Yet it's still happening? The steam does get cloned here but I didn't fully dig in the code to see if it matters.

alancoppin commented 1 year ago

Any news on this? or a work around?

yankustefan commented 1 year ago

I am getting a similar error:

Your worker created multiple branches of a single stream (for instance, by calling `response.clone()` or `request.clone()`) but did not read the body of both branches. This is wasteful, as it forces the system to buffer the entire stream of data in memory, rather than streaming it through. This may cause your worker to be unexpectedly terminated for going over the memory limit. If you only meant to copy the request or response headers and metadata (e.g. in order to be able to modify them), use the appropriate constructors instead (for instance, `new Response(response.body, response)`, `new Request(request)`, etc).
workerd/jsg/jsg.c++:136: error: took recursive isolate lock; kj::getStackTrace() = 100853047 100ada22b 100af1173 100af10f7 1020e5f7b 100e52663 100e5337f 100db9f6b 100dba193 1020b91ff 100bd00bf 1020b9527 100bd0bab 1020b9527 100abc697 1020baeab 1020b9527 100abd28b 1020ba1d3 1020b91ff 100abd9bf 1020b9527 100abdaeb 1020ba1d3 1020b91ff 100abdecf 1020b9527 100abef4b 1020ba1d3

my setup package json:

  "dependencies": {
    "@remix-run/cloudflare": "^1.16.1",
    "@remix-run/cloudflare-pages": "^1.19.3",
    "@remix-run/css-bundle": "^1.19.3",
    "@remix-run/react": "^1.19.3",
    "classnames": "^2.3.2",
    "cross-env": "^7.0.3",
    "isbot": "^3.6.13",
    "react": "^18.2.0",
    "react-dom": "^18.2.0",
    "react-dropzone": "^14.2.3"
  },
  "devDependencies": {
    "@cloudflare/workers-types": "^3.19.0",
    "@remix-run/dev": "^1.19.3",
    "@remix-run/eslint-config": "^1.19.3",
    "@types/react": "^18.2.21",
    "@types/react-dom": "^18.2.7",
    "eslint": "^8.48.0",
    "npm-run-all": "^4.1.5",
    "prettier": "^3.0.3",
    "prettier-plugin-tailwindcss": "^0.5.4",
    "tailwindcss": "^3.3.3",
    "typescript": "^5.2.2",
    "wrangler": "^3.6.0"
  },
  "engines": {
    "node": ">=16.13"
  }

remix config:

  devServerBroadcastDelay: 1000,
  ignoredRouteFiles: ["**/.*"],
  server: "./server.ts",
  serverBuildPath: "functions/[[path]].js",
  serverConditions: ["worker"],
  serverDependenciesToBundle: "all",
  serverMainFields: ["browser", "module", "main"],
  serverMinify: true,
  serverModuleFormat: "esm",
  serverPlatform: "neutral",
  // appDirectory: "app",
  // assetsBuildDirectory: "public/build",
  // publicPath: "/build/",
  future: {
    v2_dev: true,
    v2_errorBoundary: true,
    v2_headers: true,
    v2_meta: true,
    v2_normalizeFormMethod: true,
    v2_routeConvention: true,
  },
  tailwind: true,
};
ACPixel commented 1 year ago

Also running into this issue. Not a huge deal but gets pretty annoying when it's printed every time any action(even a completely empty one) run.

RenderCoder commented 1 year ago

Any update?

AaryanAdil commented 11 months ago

Having this issue using hono.dev

tj-noor commented 9 months ago

I ran into the same issue for all actions/post with @remix-run/*@2.4.1 & Cloudflare Pages. Patching wrangler@3.22.3 where it creates a new request with the clone of the original request in templates/pages-template-worker.ts seems to alleviate the issue locally. I plan on deploying it later today but I'll report back if there are any issues.

Also, found a closed issue/unmerged PR with the same fix

pnpm patch wrangler@3.22.3

diff --git a/templates/pages-template-worker.ts b/templates/pages-template-worker.ts
index 67e6cee9ac7cbac5bf88f8dc75f70be3a010c4fb..a67a6e3f6d19a370a5037141d50828f8269ad505 100644
--- a/templates/pages-template-worker.ts
+++ b/templates/pages-template-worker.ts
@@ -137,7 +137,7 @@ export default {
            if (result.done === false) {
                const { handler, params, path } = result.value;
                const context = {
-                   request: new Request(request.clone()),
+                   request: new Request(request),
                    functionPath: path,
                    next,
                    params,
saidelimam commented 9 months ago

Same error for me, looks like it's related to Wrangler

TheBuilderJR commented 8 months ago

Same error for me too

Your worker created multiple branches of a single stream (for instance, by calling `response.clone()` or `request.clone()`) but did not read the body of both branches. This is wasteful, as it forces the system to buffer the entire stream of data in memory, rather than streaming it through. This may cause your worker to be unexpectedly terminated for going over the memory limit. If you only meant to copy the request or response headers and metadata (e.g. in order to be able to modify them), use the appropriate constructors instead (for instance, `new Response(response.body, response)`, `new Request(request)`, etc).
nicoschriever commented 8 months ago

I've got the same error

kentonv commented 7 months ago

Just want to emphasize here that this problem is not specific to Cloudflare Workers, it's just that only Cloudflare Workers actually warns about it. On all other platforms, this pattern will simply silently burn RAM for no benefit. Workers is happy to do that too -- the message is only a warning but everything still works (as long as you don't exceed your memory limit).

kentonv commented 7 months ago

Oh, except I see the bug is present in code that is only used with Cloudflare Pages. So I suppose that does make the problem here specific to Cloudflare in that sense.

But in short, request.clone() is almost never what you want. It's honestly a bit of a mistake in the standard API.