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.

[KCraw]

@josephsavona Thanks! Any chance of letting it accept either; i.e., check if Component is a function, and if not, use Component.constructor as the key?

The reason I ask is I don't think caching any sort of query config aggregator is going to work for SSR. Otherwise you're bound to have concurrency problems. And any sort of query config aggregator is likely going to have to fake the RelayContainer static methods with instance methods, since by definition the fragments on a query aggregator rely on the children it is wrapping. Nor do the RelayContainer static methods provide any sort of argument that might allow for a query config aggregator to identify an instance in an internal registry. And turning off caching to avoid the issue is undesirable. Maybe there is some implementation of a query config aggregator that avoids these problems, but if so, it eludes me.

I'd be a little concerned with an 'accept an instance or a class' solution that there are other internal portions of Relay that rely on the Relay.Renderer Container prop being a class and not an instance. On the other hand, routing and nested routes seems pretty important, and having lots of nested Relay.Renderers sounds bad to me (but maybe I'm wrong on that). So a query aggregator is pretty much a given.

[josephsavona]

Any chance of letting it accept either; i.e., check if Component is a function, and if not, use Component.constructor as the key?

I don't think this solves the problem, since the constructor would presumably be shared amongst all QueryAggregator instances. This would mean that all aggregators would use the fragments of whatever aggregator happened to have been seen first - not good.

I believe that the issue is that a new QueryAggregator is created for each new request. Instead, it should be possible to create one aggregator per unique route combination. Fo example if /foo can have two child routes /foo/bar and /admin/baz, then each of the latter would have a cached aggregator. It isn't clear why a new aggregator would need to be recreated when re-encountering a previously seen route, as only the params on the Route object change.

I'd really recommend submitting a PR to allow manually disabling the cache. If you are using isomorphic-relay you are not getting the benefits of the component/route cache anyway, so we might as well disable it to at least avoid the memory problem. Note that this cache is originally optimized for Relay running on low-end mobile devices: having the cache is not critical on the server.

[KCraw]

Sorry to drag this convo out here, but I think it's relevant to anyone dealing with query aggregation and how to properly implement it with Relay, and not just iso-relay, react-relay-router, etc.

I don't think this solves the problem, since the constructor would presumably be shared amongst all QueryAggregator instances. This would mean that all aggregators would use the fragments of whatever aggregator happened to have been seen first - not good.

This brings up a concern with aggregation generally. QueryAggregator (as implemented in react-router-relay) appears to be generating responses to requests for fragments and queries dynamically, so when getRelayQueries is building its cache, we end up with with a single Component key in the queryCache which maps to a cache object with (eventually) a key for every possible route combination; e.g., something like this on the client (generated without SSR):

From your perspective, is this problematic with how Relay does things internally?

It isn't clear why a new aggregator would need to be recreated when re-encountering a previously seen route, as only the params on the Route object change.

My concern is whether anything async is happening between the time one request initiates use of the QueryAggregator and when it is released following the SSR. If so, another req for the same path from another user could modify the QueryAggregator while it is in use. If Relay doesn't do anything async until the actual GraphQL request, we could use one QueryAggregator cache for preparing data and another for the actual SSR, but you'd still have to guarantee nothing async would ever happen between checking out the QueryAggregator and the GraphQL request , and that's a problem if you use promises anywhere. You'd need to keep some sort of pool of QueryAggregators, you could even do it per unique route, but that would only slow down the memory leak.

Or as you suggest, just enable turning off the cache on the server. Definitely the simplest solution. Perfectly happy to work on it and submit a PR.

However, based on your reply, I'd like to verify that there isn't a problem with how QueryAggregator interacts with the query cache on the client, where we can't just turn the cache off to get around the issue. If so, then a completely different solution for query aggregation might be needed.

[denvned]

It isn't clear why a new aggregator would need to be recreated when re-encountering a previously seen route, as only the params on the Route object change.

@josephsavona The problem was that react-router-relay, on which isomorphic-relay-router is based, is designed in such a way that there supposed to be an unique instance of QueryAggregator per each Router instance, and trying to reuse a QueryAggregator concurrently would lead to problems even when the route is absolutely the same.

But I believe I finally managed to solve the issue by refactoring the container responsibility of QueryAggregator out to a new class AggregateContainer, and reusing instances of this class instead. See denvned/isomorphic-relay-router@9a98a19

[KCraw]

I think @denvned's solution fixes the issue with routing and query aggregation, and in a quick test, the cache seems to be building as intended by Relay on both the client and server.

That being said, I think we're still going to need to a way to manually disable Relay's caches on the server as @josephsavona suggested. This is completely independent of routing.

Relay caches use the params in generating cache keys as well, which would quickly grow out of hand on the server (as @josephsavona stated, the cache was never intended for server use). Just imagine how many cache keys would be generated for a Component if something like a text search string were a param, or even just the ids of nodes for large applications.

[KCraw]

Submitted a very simple PR that addresses the above issue and allows for disabling the query caches with Relay.disableQueryCache()

#1302

[denvned]

Relay caches use the params in generating cache keys as well, which would quickly grow out of hand on the server

Or alternatively to disabling cache altogether this can be solved by incorporating LRU cache. This way we would not introduce new public API, also cache still might provide some performance benefit on the server.

[KCraw]

I think an LRU would be a far more elegant solution for sure. It would also fix the issue client side--even though the likelihood of encountering the problem during normal usage is low, it still bothers me that it's possible.

[fabiosantoscode]

Unless this serverside LRU cache can be turned off, I can personally say it is still an issue for me. Serverside users of Relay may not always want an in-process cache. In fact, it can be harmful to reactive architectures in situations where constructing a fresh version of the HTML is crucial.

[denvned]

In fact, it can be harmful to reactive architectures in situations where constructing a fresh version of the HTML is crucial.

@fabiosantoscode I don't quite understand what you mean. I think a combination of a container and a query configuration, which is used as the cache key, always yields the same set of relay queries, thus should be safe for caching (in case when the cache doesn't grow unboundedly).

[fabiosantoscode]

@fabiosantoscode I don't quite understand what you mean. I think a combination of a container and a query configuration, which is used as the cache key, always yields the same set of relay queries, thus should be safe for caching (in case when the cache doesn't grow unboundedly).

Thanks for the clarification, I seemed to have misread and thought an in-process cache for query responses was being considered. I will keep quiet now :)

[KCraw]

@josephsavona Any thoughts on using LRU cache for these two caches? Would you be open to a PR that implemented something like a RelayLRUCache module that would abstract all this away, and would allow globally setting the cache size (using 0 to turn them off and setting the default as Infinity)?

If so, I'm willing to take a stab at it in the nearish future (I'd have to make time), or maybe @denvned would be willing (he's cleverer than I am anyways).

[fabiosantoscode]

After an update to 0.9.3, which contains the above commit, it seems like our memory leak is completely gone. Further investigation will occur on my side, but my guess is that this issue can be closed.

[josephsavona]

@fabiomcosta Thanks for confirming. I'm going to close as the use of this feature completely disables the caching that caused the bug - feel free to file a new issue if there are any remaining problems!

[taion]

I think the query cache should be an LRU, or otherwise customizable (perhaps with continuation local context to be request-scoped).

Without some sort of caching, it's impossible to avoid doing extra work – the queries need to be built to fetch data, but then Relay.ReadyStateRenderer also needs to build the queries to actually do the render.

[taion]

Or – would it be possible to move getQueries to the Relay environment? At least in the OSS Relay code base (and in all of my own code), I don't call getQueries without access to an actual Relay environment. The top-level static implementation could just use Relay.Store.