Apollo-client: Question about stopped queries and potential bug

[yurigenin]

I have two views in my app and each view is wrapped in Apollo component. Every time I switch between these two view a new query is being created for a component I switch to and the query for the component being switched away from is stopped. So when switching 20 times between these two views I end up with 20 queries in the apollo store but only the last one is in a running state. So the question I have is why do we keep creating a new query every time instead of reusing the old one by just reinitializing it? We could have a mechanism to find it in the store by name and then reuse it by flipping its state to running. If for some reason we need to keep all these old queries in the store then I think we have a bug in the QueryManager.collectResultBehaviorsFromUpdateQueries where the updateQueries reducer is being called repeatedly for all queries that are stopped and it results in getting exactly the same result and exactly the same state in the store is being regenerated by constantly mutating it.

If I keep my app open for a day and switch views hundreds of times I will end up with hundreds of dead queries which will, nevertheless, participate in constant state mutation without any effect.

Is there something obvious that I'm missing?

Thank you

[helfer]

@yurigenin If the queries are stopped, they should be removed from the queries part of the store. If that's not the case, then I think it's a bug and we should fix it. You could make a PR if you want!

Just to confirm: you are switching components and seeing more and more queries in your store with the Redux devtools?

edit: what I said is wrong. Stopped queries aren't removed from the store any more. That change was made to prevent the client from crashing in some race conditions (I think).

[yurigenin]

@helfer yes, the redux devtools show lots of these queries whose 'stop' property is false.

[helfer]

@yurigenin could you provide a reproduction or better yet a PR with a failing test? Then we should be able to fix it pretty easily.

[carsonxyz]

@helfer I am experiencing this as well.

Edit: I help to maintain and also have based my code off of the Apollo branch on this repo.
https://github.com/ctrlplusb/react-universally/tree/apollo

It should suffice as an example repo (although a little overkill) if a test case or a PR cannot be created.

[helfer]

Ok, I did a bit of digging and it turns out we no longer delete queries since this PR. It's normal that they're in the store, but as you correctly point out they shouldn't get updated with updateQueries.

I think we can put in a simple fix in this loop.

@yurigenin do you think you could make a PR to fix this? I'd be happy to walk you through it if you need help getting started! 🙂

[yurigenin]

@helfer Sure, I can take a stab at it. I'm finishing something for my day job today and will be happy to jump on it tomorrow. Just a little disclosure: I have never done github PRs so would need a tutorial on how to manage a subproject/forked repo on github. Thanks!

[carsonxyz]

@helfer I have a question about that, just for my own curiosity. What is the benefit of keeping them in the store? I have a rather large application and as a user navigates around, wouldn't that state just keep growing and growing with stopped queries? Is that a result of a flaw with my application design or do I need to intervene and clean up the state when a user changes to a different route? Thanks in advance.

[GreenAsJade]

What is the definition of a "stopped" query? IE when would a query be removed from the cache?

Since I am going to use updateQueries to tell components using a query that they need to update their displays, the only way that this could become "no longer needed" is when a component using the query unmounts. Is that right?

[helfer]

@GreenAsJade @carsonperrotti The QUERY_STOP action is dispatched whenever the last observer unsubscribes from a watchQuery, which will usually happens when the component unmounts. As the code is right now, queries remain in the cache in stopped state indefinitely, but they shouldn't run or update any more. We have plans to refactor the queries logic, at which point we may also be able to clear stopped queries from the store. In the meantime it should be possible to clean up stopped queries manually.

PS: I don't think having stopped queries in the store is a big issue for now, because they use relatively little storage space and the client isn't designed to be run for days without resetting the store.

[GreenAsJade]

I agree. I don't see any particular issue here, now that I understand it.

[yurigenin]

@helfer I'm ready to tackle this (set up my dev env and forked apollo-client repo). Now to the hard part. I still do not know what the solution would be. Currently, as soon as the component with a query unmounts its query is stopped (marked stopped: true in the store). When a different component (dialog usually) sends a mutation the QueryManager.collectResultBehaviorsFromUpdateQueries is called and it iterates through all keys passed to mutate in updateQueries object, retrieves their last results and passes them to the query reducers. So, in this case, all queries are marked as stopped and if we somehow 'fix' it so that these queries are ignored then no reducer will ever be called.

The problem now is that every time the component with a query is remounted a new query is created. If you switch between two components with queries 100 times you will have 100 query entries (50 for each component) in the store under queries key as the screenshot showed. And the reducers will be called 50 times per component always passing and returning the same data (mutated on every call).

The bottom line: if you ignore stopped queries, no reducer will be called; if you keep doing what we are doing now you end up with multiple redundant calls doing exactly nothing.

I'm ready for any ideas...

[yurigenin]

@helfer Thinking loud. I think one solution would be on creating a new query to always check if there is another query with the same name and variables already in the store under queries key. If there is then dispatch a new action APOLLO_QUERY_REMOVE and only then create a new query. This, I think' would work: even components not currently mounted would be able to 'reduce' and only one query at a time would exist in the store under queries key.
What do you think?

[yurigenin]

No, AFAIK. One query entry per component is enough. Every time a component mounts it creates a new query entry. So duplicate ones serve no purpose.

On Nov 18, 2016, at 4:13 PM, GreenAsJade [email protected] wrote:

Are the duplicate queries "connected" (in some way) to the specific components that they feed? Just wildly speculating :) If they are, then maybe each duplicate has to remain there to cause the re-render of the wrapped component that it is responsible for?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub https://github.com/apollostack/apollo-client/issues/902#issuecomment-261666667, or mute the thread https://github.com/notifications/unsubscribe-auth/AKrxkHKJ_yfMXXJudP7AdrcqEg6imNJOks5q_jEggaJpZM4Kwmr3.

[yurigenin]

Copied from #903 as some of it is related.

Once a component mounts its query is added to a map (QueryManager.addObservableQuery) and never, never gets removed from it. There is a method that is supposed to be called to remove it (QueryManager.removeObservableQuery) but it is not called from any path in the app. So every time you unmount and remount any component with a query it adds a new query structure to that map (QueryManager.observableQueries and QueryManager.queryIdsByName). I used the GitHunt-React app and was able to reproduce it easily. Just switch between the Feed view and Submit view. Every time you click on the Feed view a new query is added to the map. I clicked 100 times and got 100 queries. If you register a mutation with updateQueries for the Feed query it will be called exactly 100 times, no kidding. Imagine 20 HOCs with queries and 20 HOCs with mutations. Could get ugly fast. Resetting the store does not solve this problem as observableQueries and queryIdsByName are never cleaned up.

I think the reason the queries are not removed is because they are tied to how updateQueries and reducer logic works for mutations. If you switch to another view but still want the updateQueries and reducers to be called, then you cannot drop these queries so they hang around forever.

This must be redesigned to decouple queries and query reducers. Those should ideally live independently and be registered once as an app starts up and shared among multiple queries if need be. But based on where the code is now it would require major overhaul.

The problem with parameterized queries is that they are repurposed every time you change a variable value (the opposite of the problem above). Let's say you have a view that has a dropdown with 3 choices (Open, InProgress, Closed) and you want to filter data in the view based on one of these values. So you create a query with one variable (status). Every time user picks a different choice from a dropdown you send a new query. Let's say you chose all of them and now you have three different results in the cache. So far so good. Now you want to add several items to the list via a mutation. But since it is the same query only the last one is actually registered with observableQueries and queryIdsByName in the QueryManager. And it means updateQueries and reducer will only be called once with the variable value set to the last one used. For example, you last viewed items with an Open state. In mutation you added some items with a Closed state. updateQueries/reducer is only called for the query results with an Open status. Now if you go to see your list filtered by a Closed state you will not find the newly added ones there since a reducer was not called for these query results. Interesting that GitHunt-React 'solves' this problem by setting forceFetch flag on the query so it always ignores cache which defeats the whole purpose, right?

One more major issue I found (probably need to submit it separately but want to mention it here since it is related) is that it looks like the current design results in memory leaks. See, you can add a reducer function on any query HOC. It is a function that is attached to query options object. But when you switch between different HOCs they are unmounted and remounted again resulting in new objects being constructed. But unmounted HOCs that have reducers attached cannot be garbage collected since they are kept being referenced by observableQueries that never get cleaned up for the reason I described above. That is why reducers should probably be registered globally so that they do not prevent unmounted components from being garbage collected.

Another minor (?) issue is that the queries in the queries sub store in the apollo store never get deleted but just marked as stopped: true. I could not find a good reason fro them to be there once they are stopped but I might have overlooked something. So, as components get remounted this sub store grows and grows and you can only trim it by resetting the entire store.

So dilemma now is that the the ideas behind the apollo-client are great (normalized cache, optimistic responses, paging, etc.) but the implementation is still not up to enterprise level where you can have lots of HOCs with queries and mutations and you need to make sure that the integrity of data in cache is not compromised and there is no unbound growth of internal objects and memory leaks are not an issue.