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

Memory leak when using Reactive variables in SSR setup on the server #7274

Closed
manojVivek opened this issue Nov 2, 2020 · 3 comments
Closed

Comments

@manojVivek
Copy link

Intended outcome:

Usage of reactive vars on the server-rendering shouldn't create a memory leak.

Actual outcome:

When an app, that uses reactive vars, is rendered on the server, it holds the query results in memory forever. Even though a new client, cache, and reactiveVar instances are created freshly for every request.
Probably because the references to reactive vars are held in a global context?

How to reproduce the issue:

We make use of the following function to create a new apollo client on every request to the serve:

export default function createCache() {
  const reactiveVars = {
    var1: makeVar({}),
    var2: makeVar({}),
    var2: makeVar({}),
  };

  return new InMemoryCache({
    typePolicies: {
      Query: {
        fields: {
          field1() {
            return reactiveVars.var1();
          },
          field2() {
            return reactiveVars.var2();
          },
          field3() {
            return reactiveVars.var3();
          },
        },
      },
    },
  });
}

And we observed from heap-dumps that the cache data is held in the memory when we use the reactive-vars even after a GC.
And if we remove the usage of reactive vars, the memory recovers back to normal after the GC.

Screenshot of a sample object(one of tons of similar ones) that were in memory:
Screenshot 2020-11-02 at 1 35 53 PM

Versions

System:
    OS: macOS 10.15.7
  Binaries:
    Node: 12.13.0 - ~/.nvm/versions/node/v12.13.0/bin/node
    Yarn: 1.18.0 - /usr/local/bin/yarn
    npm: 6.12.0 - ~/.nvm/versions/node/v12.13.0/bin/npm
  Browsers:
    Chrome: 85.0.4183.121
    Edge: 81.0.416.68
    Firefox: 78.3.1
    Safari: 14.0
npmPackages:
    @apollo/client: ^3.2.5 => 3.2.5 
@benjamn
Copy link
Member

benjamn commented Nov 2, 2020

@manojVivek Thanks for opening this issue! I can see what the problem is, and how to fix it, though I suspect any possible solution to this problem will depend on you explicitly calling client.stop() after you finish using client during SSR, which ultimately cancels all cache watches, which is an event we can use to remove the cache from the caches set for any reactive variables currently holding references to the cache. We're not currently hooking into that event (hence this bug), but I think it should be straightforward to set that up.

benjamn added a commit that referenced this issue Nov 2, 2020
Each reactive variable must remember any caches previously involved in
reading its value, so those caches can be notified if/when the variable
value is next modified.

To prevent reactive variables from retaining references to caches that no
longer need to be notified, we preemptively remove the cache from the
memory of all of its associated reactive variables whenever all of its
watches are cancelled, since not having any active cache.watches means
cache.broadcastWatches has nothing to do.

Should prevent memory leaks when many caches are created and discarded by
the same process, as in the SSR use case described in #7274.
benjamn added a commit that referenced this issue Nov 3, 2020
Each reactive variable must remember any caches previously involved in
reading its value, so those caches can be notified if/when the variable
value is next modified.

To prevent reactive variables from retaining references to caches that no
longer need to be notified, we preemptively remove the cache from the
memory of all of its associated reactive variables whenever all of its
watches are cancelled, since not having any active cache.watches means
cache.broadcastWatches has nothing to do.

This change should prevent memory leaks when lots of caches are created
and discarded by the same process, as in the server-side rendering use
case described by @manojVivek in #7274.
@hwillson
Copy link
Member

Addressed by #7279 - thanks!

@choijungseon
Copy link

choijungseon commented Jun 10, 2021

@benjamn @hwillson

npmPackages:
@apollo/client: 3.3.19

we update to 3.3.19 version (prev version is ^3.0.2)
but it is still remain memory leak

is it solved this issue ?
image

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants