-
Notifications
You must be signed in to change notification settings - Fork 298
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
@clerk/nextjs
performance regression/hitting rate limits in next@15
due to fetch
default behavior change
#4894
Comments
Hello and thank you for the report. We're going to discuss tomorrow whether it's appropriate to adjust our Next.js SDK to cache by default, or to expose an option. I did want to quickly address this concern:
We just verified that our rate limits over this API are per-application rather than per IP, so you will not be impacted by others accessing from within Vercel. This is an error in our documentation that we will update. |
Appreciate the detailed writeup! Additionally as far as we're aware, Next.js still has request-level fetch de-duping by way of "request memoization". Passing the explicit |
Yes, I quite like the explicit cache semantics of the Astro library, so having the ability to cache and evict users more aggressively in clerk nextjs module userland code would be great. |
Thank you for the quick response/corrections! next/vercel has a lot of caching nowadays and it's not clear what is cached and where sometimes, so happy to be corrected where wrong! |
ok, that makes much more sense, we have a cron job (that I have been meaning to rewrite using webhooks) on a separate backend microservice with the same API key is probably eating up all the requests. i would still be interested in more explicit cache semantics, as we will probably have to start caching within our codebase otherwise as we continue to scale |
I'd like to know more about how you are utilizing |
@panteliselef I'm working with @sigmachirality. We were calling |
In some cases we're running into race conditions with However in those cases we're probably going to move those values into our backend/db to avoid rate limits. I think it might be good for someone to revisit the custom onboarding docs. I followed the general approach outlined in the article, but ran into the aforementioned race condition (sessionClaims being stale for a few seconds) resulting in redirect loops and general bugs |
Preliminary Checks
I have reviewed the documentation: https://clerk.com/docs
I have searched for existing issues: https://github.com/clerk/javascript/issues
I have not already reached out to Clerk support via email or Discord (if you have, no need to open an issue here)
This issue is not a question, general help request, or anything other than a bug report directly related to Clerk. Please ask questions in our Discord community: https://clerk.com/discord.
Reproduction
https://github.com/sigmachirality/nextjs-auth-starter-template
Publishable key
pk_test_c3VyZS1kYW5lLTM5LmNsZXJrLmFjY291bnRzLmRldiQ
Description
NextJS monkey-patches the global
fetch
to overload it with acache
parameter that allows users to configure caching of network calls. Additionally, in Next 13 and 14, this cache was aggressively enabled by default, such that allGET
requests were automatically cached.For some, this resulted in performance improvements. For others, it resulted in regressions in functionality where libraries and codebases assumed
fetch
would return fresh data each time. Eventually, in Next 15 these default caching semantics were rolled back such thatfetch
would be uncached by default.However since this was the default for two major semvers, some libraries (including this one) now assume
fetch
cachesGET
requests aggressively by default. See:Note: calls fetch(), so it is automatically deduped per request
Clerk is rate-limited to the tune of 100 requests per 10 seconds (~10 requests per second) for a specific IP address, but because
fetch
was cached by default, the implementation ofcurrentUser
does not cache in memory with explicit caching semantics/eviction, unlike, say, the nanostores-based implementation of similar library features in the Astro clerk package..In cases where
currentUser()
is called in the RSCs for multiple components in the same DOM during render (for example, in a few nested layouts containing a navbar, a sidebar, and in a few one-off components like a CTA login that contains the user's pfp), this rate limit can be easily hit. In cases where the user is deploying NextJS by themselves on, say, Fly or AWS, or in their own bare-metal server, this can be fixed in user land by wrappingcurrentUser
inReact.cache
,unstable_cache
oruse cache;
.However, a large percentage of people host NextJS apps on Vercel., or any other public cloud. When people host NextJS apps on Vercel (or most other public clouds), they share Vercel's public IP ranges. I'm sure the rate limit for Vercel to Clerk is specifically set to be higher under the table, but the result is still that if a few people inside a Vercel IP range update to Next 15 without exhaustively auditing their caching semantics to the point of thinking about the caching semantics of their external modules, their codebase can clobber everyone else hosted within that IP range, resulting in 422 rate limits the others have no control over.
Here is a sketch of a few possible solutions:
Solution 1: Update the Docs
The docs above should NOT tell users that
currentUsers
dedupes requests. Instead, they should encourage users to wrapcurrentUsers
inReact.cache
orunstable_cache
oruse cache;
. This is by far the easiest solution, but also the most irresponsible from a library maintainer ethics standpoint. I do think the docs should be updated eventually, but the vast majority of users will not see that these docs updated and simply bump to Next 15, which does nothing to stop them from clobbering others in public clouds.Solution 2: Export a new
cachedCurrentUser
helper from@nextjs/clerk/server
Export a
cachedCurrentUser
which is equal toReact.cache(currentUser)
. React.cache is not for use outside of server components, andcurrentUser
is intended to be used outside of server components in any server context such as in server actions or route handlers. To my knowledge callingReact.cache
outside of a server component is undefined behavior, or may even cause an error. This also has similar issues to the above, since people are unlikely to migrate to usingcachedCurrentUser
fromcurrentUser
unless they read the docs.Solution 3: Monkey patch
runtime.fetch
in the@clerk/backend
module@clerk/backend
callsfetch
from aruntime
object exported internally due to differences in howfetch
is defined in different JS server environments (for example, Cloudflare workers). Treat this module as a closure, and export an internal function inside
@clerk/backend
to call inside@clerk/nextjs/server
to monkey-patch/override the params offetch
to include caching semantics that were implicitely default in Next 13/14. I imagine it would look something like this:Alternatively, if it is determined that the additional param is unlikely to cause functional regressions in other pages downstream of
@clerk/backend
, you could just always pass it in by default inside@clerk/backend
. Making the caching semantics of Next 13/14 explicit in calls tofetch()
would not cause a regression for those semvers, but would improve the performance and behavior of the@clerk/nextjs
module in Next 15 significantly.Solution 4: Tell users to re-enable aggressive fetch GET caching by default in the docs
I think this solution is unacceptable due to the aforementioned performance/functionally regressions in libraries which use
fetch
, but I suppose it is a solution.Happy to make a PR following any of the above approaches. We have already lost some clients through our onboarding process due to unpredictable/sporadic rate limit errors.
Environment
The text was updated successfully, but these errors were encountered: