Skip to content
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

perf: server side fetch requests made to various endpoints #742

Closed
warflash opened this issue Apr 20, 2024 · 13 comments · Fixed by #750
Closed

perf: server side fetch requests made to various endpoints #742

warflash opened this issue Apr 20, 2024 · 13 comments · Fixed by #750
Labels
bug A bug that needs to be resolved p4 Important Issue

Comments

@warflash
Copy link

Environment


  • Operating System: Windows_NT
  • Node Version: v18.18.2
  • Nuxt Version: 3.11.2
  • CLI Version: 3.11.1
  • Nitro Version: 2.9.6
  • Package Manager: pnpm@8.7.0
  • Builder: -
  • User Config: -
  • Runtime Modules: -
  • Build Modules: -

Reproduction

https://github.com/warflash/nuxt-auth-debug

Using the module's playground-authjs dir.

Describe the bug

In our production env we noticed a ton of /api/auth/session?callbackUrl=http:%2F%2Flocalhost:3333%2Falways-unprotected requests coming through our proxy layer, all with user agent set to "node".

To debug this I spun up a local version of the playground of the repo and attached a proxy, -> see repro above.

I am not logged in on the local playground, depite that:

In the logs below you can see me calling for /always-unprotected with my browsers user agent in the first line.
Then 3 requests are fired for /api/auth/session?callbackUrl=http:%2F%2Flocalhost:3333%2Falways-unprotected (which is what we've been seeing in our prod proxy logs as well, followed by /api/auth/providers and /api/auth/csrf.

Additional context

Wanted to perhaps spark a discussion or question on whether these calls can be avoided or made internally using $fetch or at least limited to users that are actually logged in or pages that are protected. Sending out a full http request for all of these adds a considerable amount of overhead and delay to the rendered SSR page.

Logs

Received request for /always-unprotected
"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/123.0.0.0 Safari/537.36"
Received request for /api/auth/session?callbackUrl=http:%2F%2Flocalhost:3333%2Falways-unprotected
"node"
Received request for /api/auth/providers
"node"
Received request for /api/auth/csrf
"node"
Received request for /_nuxt/DdW6MrNW.js
"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/123.0.0.0 Safari/537.36"
Received request for /_nuxt/CiQvDwpd.js
"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/123.0.0.0 Safari/537.36"
Received request for /_nuxt/DZ4PCLmw.js
"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/123.0.0.0 Safari/537.36"
Received request for /_nuxt/ChRk-71c.js
"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/123.0.0.0 Safari/537.36"
Received request for /_nuxt/DlAUqK2U.js
"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/123.0.0.0 Safari/537.36"
Received request for /_nuxt/CuxXRkfr.js
"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/123.0.0.0 Safari/537.36"
Received request for /_nuxt/builds/meta/28820aa0-4610-4b7e-b225-6a545c117d71.json
"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/123.0.0.0 Safari/537.36"
Received request for /favicon.ico
"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/123.0.0.0 Safari/537.36"
Received request for /api/auth/providers
"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/123.0.0.0 Safari/537.36"
Received request for /api/auth/csrf
"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/123.0.0.0 Safari/537.36"
Received request for /api/token
"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/123.0.0.0 Safari/537.36"
@warflash warflash added bug A bug that needs to be resolved pending An issue waiting for triage labels Apr 20, 2024
@phoenix-ru
Copy link
Collaborator

I absolutely agree with you, it is quite wasteful to make full-blown http requests in Node side. What I don't understand, however, is that we are already using $fetch to avoid that:

export const _fetch = async <T>(
nuxt: ReturnType<typeof useNuxtApp>,
path: string,
fetchOptions?: Parameters<typeof $fetch>[1]
): Promise<T> => {
try {
const joinedPath = await callWithNuxt(nuxt, () => joinPathToApiURL(path))
return $fetch(joinedPath, fetchOptions)
} catch (error) {
// TODO: Adapt this error to be more generic
console.error(
'Error in `nuxt-auth`-app-side data fetching: Have you added the authentication handler server-endpoint `[...].ts`? Have you added the authentication handler in a non-default location (default is `~/server/api/auth/[...].ts`) and not updated the module-setting `auth.basePath`? Error is:'
)
console.error(error)
throw new Error(
'Runtime error, checkout the console logs to debug, open an issue at https://github.com/sidebase/nuxt-auth/issues/new/choose if you continue to have this problem'
)
}
}

I guess we'd have to refer to the $fetch docs and probably check our passed options. Will look into it later (possibly this week)

@warflash
Copy link
Author

warflash commented Apr 22, 2024

Awesome, thank you @phoenix-ru!

While looking into this I came up with two possible reasons.
Please do note however that that's just possible explanations I had, not based on much. But figured sharing can't hurt.

  1. Those calls are made by authjs itself somewhere internally and are not using $fetch

  2. Prepending the auth.baseURL prop to fetch calls makes $fetch unable to resolve the fact that the calls are local. With a full featured domain it has no way of knowing those are supposed to be internal calls. If that's the case then the easiest solution might be to have a auth.baseUrl as well as auth.serverBaseUrl that's used in import.meta.server contexts.

@warflash
Copy link
Author

warflash commented Apr 24, 2024

So after looking into this it turns out the fetch implementation does indeed prepend the entire host section of the URL. This causes $fetch to not resolve calls internally.

I've played with it for a bit and technically all that's needed is a change in the fetch constructor. -> warflash/nuxt-auth-debug@746cfd8

To show the differences and debug I went ahead and created cpu flamegraphs for both versions.

Default/Current version:

image

Notice the fetch call and http dispatch.

Full cpuprofile:
no-local-fetch.cpuprofile

Patched version based on repo linked:

image

Notice the localFetch working and only calling callHandle

Full cpuprofile:
local-fetch-ufo.cpuprofile

Obviously on localhost those changes only shave off .2ms, however in a real env they save an entire roundtrip that would otherwise have been routed out to the public internet and back to the nuxt app.

@phoenix-ru phoenix-ru added p4 Important Issue and removed pending An issue waiting for triage labels May 2, 2024
@phoenix-ru
Copy link
Collaborator

Hi @warflash, thank you a lot for a thorough investigation, I can indeed confirm the issue exists. Moreover, it is caused by this particular line in Nuxt's $fetch implementation (tldr request must start with /):
https://github.com/nuxt/nuxt/blob/572c36745597ee45e9b57f8f32a24dd99a5b9d42/packages/nuxt/src/app/composables/fetch.ts#L172

I checked your linked repo, however, I don't think I can use it, since the issue is really in _fetch implementation I already linked here.

It uses joinedPath, which is a concatenation of baseURL + path:

export const joinPathToApiURL = (path: string) => joinURL(useAuthState()._internal.baseURL, path)

baseURL comes from computed state:

const { origin, pathname, fullBaseUrl } = useRuntimeConfig().public.auth.computed
if (origin) {
// Case 1: An origin was supplied by the developer in the runtime-config. Use it by returning the already assembled full base url that contains it
baseURL = fullBaseUrl

and, finally, computed state fullBaseUrl comes from user's baseURL:

nuxt-auth/src/module.ts

Lines 117 to 119 in 949e556

const { origin, pathname = '/api/auth' } = getOriginAndPathnameFromURL(
userOptions.baseURL ?? ''
)

fullBaseUrl: joinURL(origin ?? '', pathname)

So, going back to the $fetch, it is always being called with http://localhost:3000/api/auth/..., which triggers the usual HTTP fetch rather than a server procedure call.
The correct way would be to determine if the request is actually local or not. I would assume using baseURL from ofetch and selectively providing/omitting it.

But there may be much better solutions, so all the ideas are welcome.


I have seen quite a lot of confusion when it comes to origin, baseURL, pathname and other related things:

Even though there was origin prior to v0.6, I think the whole design needs to be adjusted

@warflash
Copy link
Author

warflash commented May 2, 2024

Oh I apologize, for some reason my linked commit was faulty and didn't include the actual "fix"/workaround.
Added that here: warflash/nuxt-auth-debug@746cfd8

But yeah, $fetch indeed only works if it's a relative path that's passed into it. Prepending the origin breaks that behavior.

And I highly agree with your concerns regarding baseURL and origin and pathname and whatnot. That combined with the fact that there's also a bunch of env vars floating around can get very tricky.
In this context and in the context of the workaround I linked I'd also like to point out that not leveraging nuxt's app.baseURL seems like a weird design decision to me.

I feel like it should for sure be included somewhere in the path building logic -> #340

Just from an minimal implementation perspective I feel like this logic: warflash/nuxt-auth-debug@746cfd8 should already work for most things. In fact we've been using a patched version with that logic for 2 weeks now on our clusters and everything is smooth so far.

It still requires the possibility the overwrite the hardcoded /api/auth path if users set their api in a different path + a way to set the auth domain, not for fetching but for next auth to validate redirects iirc.

@phoenix-ru
Copy link
Collaborator

Okay, I see it now:

  1. Next Auth requires a fully-specified auth URL, such as NEXTAUTH_URL=https://example.com/custom-route/api/auth;
  2. $fetch can work with fully-specified URLs, but should do this only for external requests;
  3. $fetch for internal functionality should always receive pathnames instead of full URLs;
  4. _fetch is used in both authjs and local providers;

@warflash Regarding app.baseURL: I agree that it needs to be taken into account, however, the patch is not fully sufficient. I will try to come up with a patch which fits all 4 points above.

@phoenix-ru
Copy link
Collaborator

@warflash wow, thanks for reporting this issue! I can already see 50% improvement on req/sec when stress-tested on a local machine (and similar flamegraph results as yours).
I am committing the patch, would you be able to test it on your loads and whether it satisfies your usecases?

Before:
image

After:
image

@warflash
Copy link
Author

warflash commented May 6, 2024

Great you're seeing the same sort of improvements, nice!

I am committing the patch, would you be able to test it on your loads and whether it satisfies your usecases?

Would there be a way for me to install a prerelease of that PR or should I go ahead and create a pnpm patch myself?

@phoenix-ru
Copy link
Collaborator

Would there be a way for me to install a prerelease of that PR

I will ask @zoey-kaiser to do a prerelease as a part of her functional review.

@zoey-kaiser
Copy link
Member

I will ask @zoey-kaiser to do a prerelease as a part of her functional review.

I will do it right now! ❤️

@zoey-kaiser
Copy link
Member

Just released the changes in a next release tag: https://www.npmjs.com/package/@sidebase/nuxt-auth/v/0.8.0-alpha.1

We will also begin testing the improvements internally and include it in the next full 0.8.0 release! Thank you so much for bringing this issue to our attention and the amazing investigation! We really appreciate it ❤️

@spras
Copy link

spras commented Jul 1, 2024

@zoey-kaiser I'm currently testing upgrade from 0.7.2 to 0.8.0-rc

This break my config : local provider and auth.baseURL defined to a custom base url ex : https://example.com/

Login is posted to the nuxt base URL , not to my custom base url

I have to redefine all the endpoints and removing the / prefix .
Perhaps it has to become the default values ?

endpoints: { 
       signIn: { path: 'login', method: 'post' },        
       signOut: { path: 'logout', method: 'post' },        
       signUp: { path: 'register', method: 'post' },        
       getSession: { path: 'session', method: 'get' }    
}

@zoey-kaiser
Copy link
Member

Hi @spras 👋

Could you please open a new issue for this! I will look into it tomorrow! 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug that needs to be resolved p4 Important Issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants