urql-graphql / urql

The highly customizable and versatile GraphQL client with which you add on features like normalized caching as you grow.
https://urql.dev/goto/docs
MIT License
8.54k stars 444 forks source link

Possible memory leak when using multiple queries on the same page #3507

Open juusopiikkila opened 4 months ago

juusopiikkila commented 4 months ago

Describe the bug

When using two queries on the same page and the other one is awaited and the other one is not and that one is using data from the awaited query, there is a memory leak.

Using the reproduction memory usage climbs from ~40 Mb up to ~800 Mb (autocannon with 60 sec duration and 10 connections) and it never comes down.

Reproduction

https://github.com/juusopiikkila/nuxt-urql-memory-leak

Urql version

@urql/vue v1.1.2

Validations

kitten commented 4 months ago

Just to clarify before looking into this because that's an important distinction: Does it never come down and stay stuck at 800MB (I assume MB not Mb?) or does it crash?

If your server doesn't actually rise in usage and/or crash it's not a leak, so just clarifying here.

Also just to point this out, there isn't anything special going on here. If you have just two operations (ie two document + variables combos) then it's more of a question how many concurrent queries are actually run as no memory is retained in the core client, bindings, or exchanges 🤔

juusopiikkila commented 4 months ago

Yeah sorry I meant MB. I tested it again and when the test was completed memory usage was at ~800MB and then it slightly decreased to ~700MB. No crashes, still works fine at that point when manually checking with browser. Memory usage just stays at ~700MB while I waited for 10 minutes for it to come down.

kitten commented 4 months ago

Hm, ok gotcha 👍 That doesn't necessarily sound like a memory leak, but a Chrome debugger memory snapshot could show you what's being retained and what's using that memory.

Generally, it's possible that a combination of GraphQL result data and Vue app tree data is being retained concurrently. But it's hard to tell how much memory that adds up to. Generally, 800MB could be plausible given that the GC may choose to also not activate an aggressive sweep phase when it still has enough heap space left

juusopiikkila commented 4 months ago

Ok I've now updated the reproduction repo with clearer tests. It wasn't even about using the data from the first query in the second, it leaks with just two awaited queries. With just one query there isn't any leak. Also I added a mock API endpoint under the api routes so that the external service isn't affecting the results.

I also set max_old_space_size to 512MB to see if it changed anything.

Below are the results from the autocannon tests. I'm not that good at decyphering the snapshots yet so that doesn't help me much. Those memory readings are taken from the Chrome debuggers memory view after each test.

# /no-leak

11k requests in 60.04s, 17 MB read
Memory usage: 23.8 MB

# /leak

9k requests in 60.04s, 13.2 MB read
Memory usage: 1201 MB
kitten commented 4 months ago

Gotcha! That's awesome. I'll have a look at the memory debugger myself this evening. Typically, I'd assume we've created some kind of promise chain or cycle here 🤔

juusopiikkila commented 4 months ago

Have you had time to check this one out yet?

Bcavez commented 3 months ago

I can confirm we have the same issue on our project. Using @urql/vue v1.1.2 with nuxt 3.

Any page/component which has two queries will retain all the setup context of the component in memory and it is never released. image image image

For a while I thought it was not an issue with urql as it is not just urql objects like graphcache, queries and data that are retained but also other objects like router object, unhead object etc... Basically anything in the setup function.

But if I remove the second query, it always fixes the problem, on all of the pages/components.

@kitten Have you had a chance to look at this issue yet ?

negezor commented 2 weeks ago

It seems that I'm in the same boat. My SSR is failing and getting killed by OOM. I've been profiling for a long time, but the only clues I have point to the Store object from graphcache. I'm not entirely sure how to verify this, as I don't see any obvious references where it could be stored. image

I had to increase the amount of memory to reduce the crash rate. image

JoviDeCroock commented 2 weeks ago

@negezor are you by any chance initializing your graphCache globally? If so you might want to move that to your boostrapping code so that it can be garbage collected when your SSR completes.

negezor commented 2 weeks ago

@JoviDeCroock no. I have a new instance of the application initialized for each request. Including graphcache, otherwise the same data would be sent for each user and it would be a data access violation.

negezor commented 2 weeks ago

After investigating further, I seem to have some ideas about why the memory leak is happening. For some reason, the request object, which is not being mutated on my end, has a large number of ReactiveEffect. Screenshot 2024-06-18 040625

Apparently, this is happening here. https://github.com/urql-graphql/urql/blob/118d74b238007264cacfb91fc12de74370d5766e/packages/vue-urql/src/useQuery.ts#L248

UPD: By applying markRaw(QueryDocument) from vue the memory leak disappeared. But I don't think this should be the solution, we should get rid of reactive() for args.

yurks commented 2 weeks ago

@negezor, https://github.com/urql-graphql/urql/pull/3612 completely breaks reactivity support for variables, like { variables: { somevar: ref('value') }}, as reactive() unwraps all nested reactive props

yurks commented 2 weeks ago

@JoviDeCroock @negezor this issue should be resolved in https://github.com/urql-graphql/urql/pull/3619 without side-effects