Open ghost opened 7 months ago
I've been playing with something like this, but it won't work in React Native as there is no support for WeakRef
nor the FinalizationRegistry
(https://github.com/facebook/react-native/issues/42742):
https://github.com/nbarrow-inspire-labs/bug-demo/blob/main/src/IntervalTimer.ts
I just wanted to bump this and see if there's a potential work-around while ReactNative works on supporting WeakRef
, etc? My example is a good starting point but clearly not production ready (until WeakRef
finds support).
A pub/sub method might be more desireable:
The static class can act as a publisher, using setInterval
once at the static level to emit tick
events on a regular pattern. All instantiated instances could then subscribe to the events that the static class publisher is emitting; when the static class emits a tick
event, the instances could 'do' the tick function.
This would solve the garbage collection issue, as the subscribers exist independently of the one main (static) publisher; if an instance is due for garbage collection, so too would its event subscriber, allowing instances to be garbage collected while the static class publisher is just always running in the background.
Perhaps a library like https://docs.evt.land/ while supports (perportedly) all JS runtimes.
Is there any updates on this?
Is this only for React Native?
As far as I can tell, it effects non-Deno and non-Node.js environments, because the solution attempted previously only fixes this leak for Deno and Node: https://github.com/supabase/gotrue-js/blob/15c7c8258b2d42d3378be4f7738c728a07523579/src/GoTrueClient.ts#L2043
For the time being, I think this will work (still testing):
import {useEffect, useState} from "react";
import {createClient} from "@supabase/supabase-js";
import AsyncStorage from "@react-native-async-storage/async-storage";
export const useSupabaseClient = (supabase: {
url: string;
keys: {
anon: string;
}
}) => {
// this is important too, to use an initializer function in setState
const [client] = useState(() => createClient(supabase.url, supabase.keys.anon, {
auth: {
storage: AsyncStorage
}
}));
useEffect(() => {
return () => {
client.auth.stopAutoRefresh().then(); // this should dump the `Timeout`, eliminating all refs, and allow garbage collection
}
}, []);
return client;
};
The operative line is:
client.auth.stopAutoRefresh().then(); // this should dump the `Timeout`, eliminating all refs, and allow garbage collection
From what I can see, ^ seems to work until Hermes has support for WeakRef
... once that happens, I can do a PR for a better fix
I'm pretty sure I'm dealing with something similar in Node.JS land. I just migrated my app to Supabase and my memory usage now climbs steadily over time until i get an OOM
Around March 9th, i updated my app to only call supabase.auth.getUser();
when necessary instead of on each request. As you can see the slope of memory usage decreased (but since i still need to call this method for authenticated endpoints, the memory usage still grows...just slower). The steep drops are from app restarts.
I see spikes when there are large batches of supabase.auth.getUser()
called. Running a memory heap from a load test shows that getItemAsync()
and _acquireLock()
are the biggest offenders. I can provide this heap if necessary. Anyone else running in to this problem serverside?
Not sure why it's a "memory leak," probably the React Native runtime is a bit bad at tracking this. But, as far as I know, you don't have to use this in React Native. You can call supabase.auth.startAutoRefresh()
and supabase.auth.stopAutoRefresh()
.
Take a look at these docs: https://supabase.com/docs/reference/javascript/auth-startautorefresh
@hf the only memory leak will occur in react runtimes that are not (a) Node.js and (b) Deno because of the way setInterval
works: if you hold a reference, this is a "strong" reference that will prevent garbage collection. The problem does not exist because, in Node.js, they have a specific dereferencing feature (https://nodejs.org/api/timers.html#timeoutunref) and same with Deno (https://deno.land/api@v1.41.3?s=Deno.unrefTimer) which Supabase already implements automatically:
Basically, in non-Node.js and non-Deno runtimes (like hermes
in ReactNative) calls to setInterval
that reference objects are enough to prevent garbage collection; even if you lose all references to the object, as in this bug report, the setInterval
prevents garbage collection as it counts as a reference (even though you've lost all references to the underlying object itself).
If you see my example above (https://github.com/supabase/gotrue-js/issues/856#issuecomment-1989401929), my temporary fix was already using the stopAutoRefresh
as you mention for non-compatible runetimes (utilizing the callback/return feature when useEffect
unmounts).
I also have a solution using the WeakRef
api to completely fix this on most runtimes, but React Natives hermes
engine is not releasing support for WeakRef
until Q2/Q3 this year: https://github.com/facebook/react-native/issues/42742#issuecomment-2003869205
@nbarrow-inspire-labs i guess I'm a bit confused ...I'm also dealing with a memory leak but I use Supabase in a Node.JS context so it's still affecting me as well, not just in React Native (etc.). Calling .getUser() causes my memory in a Fastify server to steadily increase over time 🤔
@uncvrd can you try the debug
option in your createClient
(I believe it is as follows):
const [client] = useState(() => createClient(supabase.url, supabase.keys.anon, {
auth: {
debug: true
}
}));
Perhaps the logs will give some insight? Also, if you see the number of clients increasing (each log statement indicates which number client it is) then you might be having the exact same issue, but for a different reason.
@nbarrow-inspire-labs oh thank you for leading me to the debug option! Yikes, yes, i ran a load test for 45 seconds and it has created 4000+ clients it seems like
@uncvrd yeah you definitely have a memory leak problem (youre correct that 4267
clients were created in your short testing period)... can I see the load test you ran (if it was a simple test repo) or perhaps can you post snipets of how you are creating your supabase clients?
For sure! I'll post snippets for now and if needed, can break out a test repo if needed
I have a simple k6 load test hitting my endpoint. I use tRPC
(if you are familiar?) to create my endpoints https://trpc.io/. It allows you to batch multiple requests in to one HTTP request. It might be a bit confusing if you aren't familiar, let me know if you have questions. So in this case I'm hitting several authenticated endpoints in one batched network request.
// load-test.js
export default function () {
const res = http.get(
"https://trpc-web.foundry-dev.ac/trpc/authed.orgContact.workspaces,authed.orgContact.permissionsV2,authed.workspaceContact.byOrgContactId,authed.workspace.permissionsV2,orgContact.byId,authed.organization.byId?batch=1&input=%7B%220%22%3A%7B%22json%22%3A%7B%22orgContactId%22%3A%221abc123%22%7D%7D%2C%221%22%3A%7B%22json%22%3A%7B%22orgContactId%22%3A%221abc123%22%7D%7D%2C%222%22%3A%7B%22json%22%3A%7B%22orgContactId%22%3A%221abc123%22%2C%22workspaceId%22%3A1%7D%7D%2C%223%22%3A%7B%22json%22%3A%7B%22orgContactId%22%3A%221abc123%22%2C%22workspaceId%22%3A1%7D%7D%2C%224%22%3A%7B%22json%22%3A%7B%22id%22%3A%221abc123%22%7D%7D%2C%225%22%3A%7B%22json%22%3A%7B%22id%22%3A1%7D%7D%7D",
{
cookies: {
"sb-127-auth-token": "[REMOVED]",
},
}
);
check(res, { "status was 200": (r) => r.status == 200 });
}
Once this HTTP request makes it's way to my server, the requests are sent off in parallel to the respective endpoints, the network request first passes through a context function to define the properties I'll have access to within the business logic of the backend endpoint. This reduced context returns an object that has the user (set to null by default) and a function that uses getUser
to pull the information from the cookie in the header (using getCookie
)
// context.ts
export interface CreateInnerContextOptions {
headers: IncomingHttpHeaders;
setCookie?: (name: string, value: string, options?: CookieSerializeOptions) => void;
getCookie?: (name: string) => string | undefined;
removeCookie?: (name: string, options: CookieSerializeOptions) => void;
}
export type Context = {
supabase: SupaClient; // this is a typed SupabaseClient<Database>
user: User | null;
getUser: () => Promise<User | null>;
};
export const createContextInner = async (opts?: CreateInnerContextOptions): Promise<Context> => {
let user: User | null = null;
return {
supabase: createServiceRoleClient(),
user: null,
// used for public routers so that auth doesn't have to run on each request
getUser: async () => {
if (!user) {
const supabase = createAnonClient(opts);
const { data } = await supabase.auth.getUser();
user = data.user;
}
return user;
},
};
};
The endpoints that started with authed.
must pass through a middleware that will populate the user
property on the above object. I do that by creating the following middleware:
// trpc.ts
const isAuthed = t.middleware(async ({ next, ctx, meta }) => {
const user = await ctx.getUser();
if (!user) {
throw new TRPCError({ code: "UNAUTHORIZED" });
}
return next({
ctx: {
user,
},
});
});
As you can see, this calls the getUser
method from the context object to get the supabase user upon request. If the user exists, it updates the context with the user object from supabase and can be accessed in a API endpoint like this
// trpc/routes/currentUser.ts
byCtx: authedProcedure.query(async ({ ctx }) => {
const { data } = await ctx.supabase.from("User").select("*").eq("id", ctx.user.id).maybeSingle();
return data;
}),
I had previously used an old context.ts
that called the .getUser
method on each request which caused the memory to spike way faster, calling it on demand in the middleware reduced the memory usage:
// OLD BAD WAY
export const createContextInner = async (opts?: CreateInnerContextOptions): Promise<Context> => {
const supabase = createAnonClient(opts);
const { data } = await supabase.auth.getUser();
let user: User | null = data.user;
return {
supabase: createServiceRoleClient(),
user,
};
};
Does this help a bit? I know it's a lot. I can make a video demo stepping through all of these hoops
@uncvrd given your usecase, it sounds like the ctx
object that gets passed along (which you use to call, for example, ctx.supabase.from...
) is destroyed after each request is handled?
If so, I would modify all of your createClient
calls, to create the actual supabase clients, to apply the step that I use above (calling await supabase.auth.stopAutoRefresh()
).
If you are not destroying the ctx after each request, you will still likely want to implement some way of cleaning-up every single supabase instance you make, by calling .auth.stopAutoRefresh
at some point. I.e., in React, right before the component/page exists/unmounts, I call .auth.stopAutoRefresh
on the supabase client. In your scenario, whenever you are 'done' with a particular supabase instance (created from calling createClient
) try calling .auth.stopAutoRefresh
and see if that helps.
Note that garbage collection, in my tests, took upto 15-20 seconds, so it might take around that amount of time before you stop seeing logs.
Try this and let me know if that decreases memory usage.
I created a PR that would at least notify users if their platform will experience a memory leak: https://github.com/supabase/gotrue-js/pull/867
@nbarrow-inspire-labs really appreciate your time and insight. I'm going to try the stopAutoRefresh
in a middleware after the request has completed. My first thing I am trying is to use a serverClient and add the user JWT manually within my getUser()
method
const token = opts?.getCookie?.(`sb-${env.SUPABASE_PROJECT_ID}-auth-token`);
if (!!token) {
const supabase = createServiceRoleClient();
const { data } = await supabase.auth.getUser(JSON.parse(token)["access_token"]);
return data.user;
}
return null;
Not quite sure why the above would help but just wanted to try setting the auth instead of having supabase try to parse the cookie for me to knock out that hypothesis. I'm attempting to disable a couple properties, first just the persistSession: false
let supabase: SupaClient | undefined;
export function createServiceRoleClient(): SupaClient {
if (!supabase) {
supabase = createServerClient<Database>(env.SUPABASE_PROJECT_URL, env.SUPABASE_SERVICE_ROLE_KEY, {
cookies: {},
auth: {
// autoRefreshToken: false,
persistSession: false,
},
});
}
return supabase;
}
But I would like to try autoRefreshToken: false
afterwards. I saw in documentation they have both of these disabled for a serverside client that I think may help?
https://supabase.com/docs/reference/javascript/auth-admin-getuserbyid
@uncvrd yeah I would definitely call .auth.stopAutoRefresh()
on any client you create in your backend. The persistSession
I believe just stores session info., but since you reconstruct a new client every time it should also be fine to disable.
@nbarrow-inspire-labs so i set autoRefreshToken: false
and it's odd that even that doesn't prevent the leak...I'll give stopAutoRefresh()
during the next deployment hopefully next week
@uncvrd I havne't tried the autoRefreshToken: false
option (and its possible that that setting is being ingored/not handled properly 100% of the time), but I have tried it with stopAutoRefresh
and had success, so yeah give that a try and see how it goes.
@nbarrow-inspire-labs After inspecting the source code, I guess I don't understand how calling stopAutoRefresh
would help in my scenario as the only time this is called is if autoRefreshTicker
is set to the setInterval()
which is only set if autoRefreshToken
is set to true
With this disabled, I don't think this would fix things :/
My next guess was that the supabase client is being initialized with an inmemory storage adapter when persistSession
is false
Where I thought that js object might be slowly getting filled with k/v pairs but when debug: true
is set, it shows that "getting" the value from this inmem storage returns null every time so I don't think there's anything in this object either
GoTrueClient@0 (0.0.0) 2024-04-01T07:55:58.296Z #getSession() session from storage null
ugh, I'll keep searching
Just wanted to report back but my hunch led me down to a memory issue with unidici, the fetch library used by node in newer versions. There were some mentions of memory issues with undici in node v18. I've updated to v20 on April 2nd and while my memory has increased a little bit initially, it seems to have somewhat stabilized, so maybe that was my issue...
TLDR
This line is causing a memory leak: https://github.com/supabase/gotrue-js/blob/15c7c8258b2d42d3378be4f7738c728a07523579/src/GoTrueClient.ts#L2040
setInterval
will persist even if the object calling it (in this case,GoTrueClient
) has been dereferenced. This prevents garbage collection (assetInterval
is still holding a reference, even if the user has lost all references) and leads to the memory leak.A solution seems to have been attempted, but it is not working in (at the very least) React Native, where I encountered this issue: https://github.com/supabase/gotrue-js/blob/15c7c8258b2d42d3378be4f7738c728a07523579/src/GoTrueClient.ts#L2043
See Also
History
I originally opened this here (https://github.com/supabase/supabase-js/issues/983) but moved once I discovered the
setInterval
issue.