queryCache module-wide variables leaking memory on the server side

[fabiosantoscode]

Using isomorphic-relay to get relay to work on the server, and using a load testing tool to create repeated requests and find a memory leak in an application I'm building, I observed that 2 maps called queryCache were growing unbounded.

These maps are in getRelayQueries.js, and in buildRQL.js, and they grow to thousands of keys over 600 requests to a page I have with no less than 2 containers.

Longer explanation in:

#683 (comment)

I'll do my best to provide an example if it's necessary to reproduce the issue.

[denvned]

Hi @fabiosantoscode. Does your testing app recreate containers on each request using Relay.createContainer(...)? I can't see any other reason for such queryCache.size growth (well, probably except some bug in core-js polyfill for Map).

[fabiosantoscode]

Hello! Yes it does. Since the app is wrapped in react-router and
react-router itself needs a history object which changes for every URL,
we've created an isomorphic function that creates the entire thing.

On 07:42, Wed, 20 Jan 2016 Denis Nedelyaev notifications@github.com wrote:

Hi @fabiosantoscode https://github.com/fabiosantoscode. Do your testing
app recreates containers on each request using Relay.createContainer(...)?
I can't see any other reason for such queryCache.size growth (well,
probably except some bug in core-js polyfill for Map).

—
Reply to this email directly or view it on GitHub
#754 (comment).

[rodrigopr]

Hi, I'm facing the same behavior in our app.
Is it reasonable to disable the queryCache and fragmentCache on the server? I can send a PR if that's ok.

I could try change how the react-router-relay create the routers if this makes more sense, but don't seem to be a simple change after a quick look.

[josephsavona]

@rodrigopr Does your app also redefine all the containers in every http request (by calling createContainer)? The fact that the cache size is being exceeded is an indication that the application is doing unnecessary recomputation on each request; you could probably get a general performance boost by defining components/containers just once (they're meant to be reusable).

We would consider adding some way to reset these caches, but I suspect that doing so in this case would mask a larger performance problem in your app.

[rodrigopr]

I'm afraid react-router-relay and isomorphic-relay creates an instance of QueryAggregator (a kind of RelayContainer) per render (see react-router-relay/blob/master/src/QueryAggregator.js and isomorphic-relay-router/src/prepareData.js)

[josephsavona]

@rodrigopr Hmm. Those packages may be somehow redefining containers, but there isn't anything immediately obvious in the sections you linked to. I would recommend adding a breakpoint in buildRQL or getRelayQueries for cache misses to see what's causing the memory to grow so much (maybe here).

[rodrigopr]

@josephsavona thanks for the quick feedback.

I'll debug it further on that line, but if I understood correctly the Component on the line you pointed is an instance of QueryAggregator, which are created once per request (on isomorphic-relay-router/src/prepareData.js I mentioned).

I'll also investigate how we can avoid creating a QueryAggregator instance per server-side render, but probably will require a big change in react-router-relay.

[tomconroy]

We ran into this problem after putting a server-rendering relay app into production. Our app would crash every few hours. I forked relay to disable these queryCache and componentCaches and it appears to fix our problem.

tomconroy@c71e00b

Deployed at 10am, the memory appears to have stabilized:

[josephsavona]

Thanks for the feedback. Ideally we'd address the root cause - not repeatedly creating things that look like containers but aren't - but we're open to a PR that addresses this in Relay for now.

This could be a global, public function Relay.disableQueryCache() that sets a module-scoped flag in getRelayQueries and buildRQL to disable caching (where the flag would default to true). Note that the typeof window === undefined isn't a reliable check for the server, as window can also be undefined in other client environments (such as React Native). We're open to PRs along the lines of this disableQueryCache() function.

[ntharim]

@tomconroy Are you using isomorphic-relay-router? isomorphic-relay-router creates a new QueryAggregator for every request. Which is later passed to RelayReadyStateRenderer where getRelayQueries is called for every render(). Causing the queryCache to grow.

Ideally there should be a way for isomorphic-relay-router to cache QueryAggregator for every possible route combination.

[tomconroy]

Indeed, moving this over to denvned/isomorphic-relay-router#40

[josephsavona]

Ideally there should be a way for isomorphic-relay-router to cache QueryAggregator for every possible route combination.

Yes! This seems like the ideal solution to this issue.

[KCraw]

@josephsavona I've been playing with this and I'm going to comment over on denvned/isomorphic-relay-router#40, but can you confirm for me that getRelayQueries is expecting a class/constructor and not an instance of a component?

[josephsavona]

Yup, it expects a class.