search

cloudflare / next-on-pages

CLI to build and develop Next.js apps for Cloudflare Pages
https://www.npmjs.com/package/@cloudflare/next-on-pages
MIT License
1.28k stars 125 forks source link

[🐛 Bug]: Your worker created multiple branches of a single stream #685

Open Mecanik opened 8 months ago

Mecanik commented 8 months ago

next-on-pages environment related information

I was sent here after the guys from Clerk checked this out, apparently this is an issue with CF/Next.

Description

This is happening when using Cloudflare Pages after logging in: 

✨ Compiled Worker successfully
 ⛅️ wrangler 3.28.1
-------------------
Using vars defined in .dev.vars
Your worker has access to the following bindings:
- Vars:
  - NEXT_PUBLIC_CLERK_PUBLISHABLE_KEY: "(hidden)"
  - CLERK_SECRET_KEY: "(hidden)"
⎔ Starting local server...
Parsed 1 valid header rule.
[wrangler:inf] Ready on http://localhost:8788
✘ [ERROR] INFO: Clerk: The request to / is being redirected because there is no signed-in user, and the path is not included in `ignoredRoutes` or `publicRoutes`. To prevent this behavior, choose one of:

  1. To make the route accessible to both signed in and signed out users, pass `publicRoutes: ["/"]`
  to authMiddleware
  2. To prevent Clerk authentication from running at all, pass `ignoredRoutes:
  ["/((?!api|trpc))(_next.*|.+\.[\w]+$)", "/"]` to authMiddleware
  3. Pass a custom `afterAuth` to authMiddleware, and replace Clerk's default behavior of
  redirecting unless a route is included in publicRoutes

  For additional information about middleware, please visit https://clerk.com/docs/nextjs/middleware
  (This log only appears in development mode, or if `debug: true` is passed to authMiddleware)

[wrangler:inf] GET / 307 Temporary Redirect (293ms)
[wrangler:inf] GET / 401 Unauthorized (3ms)
[wrangler:inf] GET /favicon.ico 304 Not Modified (8ms)
[wrangler:inf] GET / 200 OK (5276ms)
✘ [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).

[wrangler:inf] GET /vercel.svg 304 Not Modified (6ms)
[wrangler:inf] GET /next.svg 304 Not Modified (7ms)
[wrangler:inf] GET /_next/static/chunks/983-c740277b6d3702da.js 200 OK (8ms)
[wrangler:inf] GET /_next/static/chunks/app/page-cf19a580b1879b0e.js 200 OK (6ms)

Reproduction

You can download this example project and test: https://github.com/Mecanik/clerk-nextjs-cloudflare

Pages Deployment Method

None

Pages Deployment ID

No response

Additional Information

Thanks

Would you like to help?

Mecanik commented 8 months ago

@dario-piotrowicz Where are you... :smiley:

Maronato commented 8 months ago

This is a bug in wrangler, not next-on-pages: https://github.com/cloudflare/workers-sdk/issues/3259

dario-piotrowicz commented 8 months ago

@Mecanik sorry I'm lately been pulled on some other projects and haven't had time to take care of issues here as much as I would like 🙇 😓

I'll try to find the time and give this a look soon 🙇

By the way, I am not completely sure why but this "error" from our runtime is actually a warning since when it happens nothing is really broken 😕 (but resources are likely being wasted), so even if you see it, everything should still work as you'd expect it to, no?

dario-piotrowicz commented 8 months ago

This is a bug in wrangler, not next-on-pages: cloudflare/workers-sdk#3259

The issue there is specific for when you use the functions/ directory (as the logic that takes that code and bundles it into a worker makes it so that requests get cloned more than they should).

Next-on-pages does not use the functions/ directory but uses the Pages advance mode instead, so I do not believe this to be an issue in wrangler, I think that the issue either resides in either:

dario-piotrowicz commented 8 months ago

quick update, I've started looking into this and by debugging it seems like the error is triggered when you call clerk's currentUser, the issue however seems to come form Next.js code, specifically this

what I think it's happening (I not 100% sure yet) is that currentUser performs a fetch and Next.js eagerly clones the response so that the app can read its body as many times as it wants, thus performing this cloning even when not needed, causing the error to show up

(this is actually quite similar to what happens in wrangler with the functions/ directory! the only difference being that we do that for the request and not the response)

dario-piotrowicz commented 8 months ago

Ok, I think my above assessment was correct, please have a look at this repository: https://github.com/dario-piotrowicz/nextjs-cachedfetch-clone-issue-repro

It shows that the issue ultimately lies in Next.js' cachedFetch implementation.

I'll open an issue in the Next.js repo regarding this, but honestly I am not sure if they'll be interested in fixing this... we'll have to see 😓

Basically what's happening here is that Clerk is most likely fetching from the same endpoint more than once thus kicking off the cached fetch behavior offered by Next.js. This behavior however triggers the issue.

If Clerk could avoid fetching from the same endpoint more than once that would solve this issue, but I don't think they really should, as the root issue is really a Next.js one.

Needless to say, there isn't much that can be done regarding this on our side 🥲

Mecanik commented 8 months ago

Thanks @dario-piotrowicz . Things just get better and better...

So this issue occurs only in currentUser or every fetch they do? How does this affect the billing if you were to deploy a project like this?...

dario-piotrowicz commented 8 months ago

Hi @Mecanik, sorry above I was slightly mistaken, the issue is not that clerk fetches from the same endpoint but that they fetch at all! (see my revised reproduction: https://github.com/dario-piotrowicz/nextjs-cachedfetch-clone-issue-repro/commit/08261623e2046bfd0edae84be3d32406efdd4f5d)

So yeah it's fully a Next.js issue that clerk can do nothing about

So this issue occurs only in currentUser or every fetch they do?

it occurs in every single fetch that happens on the server

I just checked with the team and I am bringing the possibility to remove this warning as, I believe is unnecessary and doesn't really help much (on the contrary it creates confusions and worries), I'll keep you posted

Also I checked with the wrangler team and this, comes from the runtime as a warning not a real error: https://github.com/cloudflare/workerd/blob/5c9519e2faaeaf0f0c7ad1e184af9c29d060b459/src/workerd/api/streams/internal.c%2B%2B#L380 but it is displayed as an error by wrangler... I am yet to understand if this is intentional of it should be fixed 😕

How does this affect the billing if you were to deploy a project like this?...

I'll have to check with the team, I think that the effect on billing should be negligible at most, again since I think that this is not a real issue, but I'll try to get more infos for you 👍

Mecanik commented 8 months ago

Hi @Mecanik, sorry above I was slightly mistaken, the issue is not that clerk fetches from the same endpoint but that they fetch at all! (see my revised reproduction: dario-piotrowicz/nextjs-cachedfetch-clone-issue-repro@0826162)

So yeah it's fully a Next.js issue that clerk can do nothing about

So this issue occurs only in currentUser or every fetch they do?

it occurs in every single fetch that happens on the server

I just checked with the team and I am bringing the possibility to remove this warning as, I believe is unnecessary and doesn't really help much (on the contrary it creates confusions and worries), I'll keep you posted

Also I checked with the wrangler team and this, comes from the runtime as a warning not a real error: https://github.com/cloudflare/workerd/blob/5c9519e2faaeaf0f0c7ad1e184af9c29d060b459/src/workerd/api/streams/internal.c%2B%2B#L380 but it is displayed as an error by wrangler... I am yet to understand if this is intentional of it should be fixed 😕

How does this affect the billing if you were to deploy a project like this?...

I'll have to check with the team, I think that the effect on billing should be negligible at most, again since I think that this is not a real issue, but I'll try to get more infos for you 👍

Thank you for looking into this, really appreciated. I do not really understand why "caching" necessary on a fetch... but I tried export const forceCache = 'force-no-store' without success since it won't compile.

I do not have this issue with Auth0, however I am not using Auth0 because it has many limitations in terms of branding, etc; and it's quite expensive when scaling. So that's why I am here trying to use Clerk, as an alternative for registration/authentication for NextJS.

But the absolute real reason I am doing this is because I created my own user system with next-on-pages, but I have a ton of "hydration" errors (which make no sense to me). I would have preferred to use my own system, with D1, where everything is under control and works properly...

If this cannot be resolved, I will have to let both Auth0 and Clerk and go with mine. Probably going to publish the repo so others can benefit and why not contribute.

Thanks again.

anonymouscatcher commented 8 months ago

I have same issue here as well, I'm also getting worker limit exceeded error after deployment and after refreshing the page for couple of times. Not sure if this is related to this worker thing

Mecanik commented 8 months ago

I have same issue here as well, I'm also getting worker limit exceeded error after deployment and after refreshing the page for couple of times. Not sure if this is related to this worker thing

Most likely because of the bug cloning responses...

anonymouscatcher commented 7 months ago

Hi @dario-piotrowicz Do you think this is something that will be fixed any time soon? We are on paid plan on CF and this is blocking us from deploying to CF.

dario-piotrowicz commented 7 months ago

Hi @anonymouscatcher, I'm sorry that you're being blocked

From your comment

I have same issue here as well, I'm also getting worker limit exceeded error after deployment and after refreshing the page for couple of times. Not sure if this is related to this worker thing

It seems like you're getting a worker limit exceeded error? that is, I think, likely something unrelated, could you maybe open a new issue sharing the error message you get and as much info as you can? (and I'll look into it right away)

(Regarding the multiple branches issue, I'll try to think of a solution, but I am afraid that that is likely not achievable and that it would require a breaking change in Next.js itself 😕)