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

resetStore: Store reset while query was in flight. #2919

Closed
joenoon opened this issue Jan 27, 2018 · 44 comments
Closed

resetStore: Store reset while query was in flight. #2919

joenoon opened this issue Jan 27, 2018 · 44 comments

Comments

@joenoon
Copy link

joenoon commented Jan 27, 2018

Intended outcome:

resetStore without errors being thrown.

https://github.com/apollographql/apollo-client/blob/b354cf07e979def7aa0219e60dca63e8a33fa822/packages/apollo-client/src/core/QueryManager.ts#L787..L796

Actual outcome:

I'm running into an issue when calling resetStore after a logout scenario. I think I understand the reasoning, but in my case I can't seem to find a workable way around it, and something else odd is happening:

fetchQueryPromises in snippet above is a Map, with values of {promise,reject,resolve}, so I look inside to see what they are:

const promises = Array.from(this.fetchQueryPromises.values()).map(x => x.promise)

My plan was to await Promise.all(promises) before calling resetStore, however, promises is [undefined]. For some reason, an entry is added to fetchQueryPromises without a promise, so I can't wait for it before resetting.

The actual error Store reset while query was in flight. looks like it might not actually cause a problem, which was my main concern. But it seems like there should be a way to avoid an error being thrown.

Waiting for existing promises to complete before reset, or exposing a function to allow the user to await them before resetting seems right to me.

Is there a case for calling resetStore where a thrown error is the right outcome?

Version

  • apollo-client@^2.2.0
@Poincare
Copy link
Contributor

The point of calling resetStore is you basically want to flush out information from the store and bring it to a clearly known state. If you have queries in flight and one of them comes back, writing the result to the cache may either cause an error (leading to a broken UI component) or lead to a state that's not expected by the application developer after calling resetStore (e.g. may leak PII).

I think throwing an error on having an inflight query is actually the intended outcome here. However, the part where you're seeing [undefined] for promises feels like a bug. Could you write a quick unit test that produces this behavior and we can then work on getting it fixed?

@ahalimkara
Copy link

Is it possible to display the query name causing this error? Error message is not helpful, it is difficult to fix this error.

@joenoon
Copy link
Author

joenoon commented Jan 31, 2018

@Poincare If no one else has noticed the undefined promise, then it's probably something specific to my app which is using vue-apollo. I'll try to think up how I could repro this in isolation, but not much comes to mind.

I'm still not clear why throwing an error is the intended outcome the way it seems to work now.

I could see if there was a way to handle the error, but aren't the promises getting rejected just floating around internally in apollo-client and detached from userland-access?

Or I could see if there was a way to first stop inflight queries purposely, and then safely call resetStore.

But I'm just not wrapping my head around calling something that may or may not throw an exception with no way to catch it. If I'm missing something obvious, then my apologies.

@Poincare
Copy link
Contributor

Poincare commented Feb 3, 2018

I believe the correct approach is the stop inflight queries at the application level and then call resetStore. The operation that resetStore represents is certainly not safe with inflight queries, so although I can imagine some cases where you might want to catch the error and proceed on with the reset anyway, it probably isn't a great practice.

I'm not sure if I completely grok the problem. I imagine that you do see the error "Store reset while query was in flight" in your application - is this error not possible to catch?

@rares-lupascu
Copy link

is this issue related to this one?
apollographql/apollo-link-state#198

@kamranayub
Copy link
Contributor

kamranayub commented Feb 6, 2018

In my case, I was running into this and needed a way to wait for in-flight queries to resolve. It worked pretty well in my case because I have wrappers around calling refetch for my observable queries, so all I really did was the following:

const inflightPromises = [];

function refetch(variables) {
  const refetchPromise = myObservable.refetch(variables).then(results => {

    // you could use native array methods here too
    // just remove the promise from the array
    _.remove(inflightPromises, p => p === refetchPromise);

    return results;
  });
  inflightPromises.push(refetchPromise);
  return refetchPromise;
}

function reset() {

  // Promise.all does not resolve with empty arrays
  const waitFor = inflightPromises.length 
    ? Promise.all(inflightPromises) 
    : Promise.resolve();

  // maybe also guard against calling this while waiting

  return waitFor.then(() => apolloClient.resetStore());
}

@wmertens
Copy link
Contributor

wmertens commented Feb 6, 2018

@Poincare When using react-apollo, the state of inflight queries is an implementation detail. Should this problem be reported there?

I think the correct solution would be to wait for all pending queries to finalize, meanwhile holding any new incoming queries, resetting the store, and then running the pending queries. This is very hard to do at any level other than the client, no?

My use case is that when the user changes the language, I want to refetch everything. The "better/more efficient" alternative is passing the current language to every query, which is a lot of work for a pretty rare event.

There are a bunch more people with this issue at #1945

@wmertens
Copy link
Contributor

wmertens commented Feb 6, 2018

That said, it seems that in my case the issue is solved by resetting the store at the same time as setting the new language state instead of after the language state was set.

@wmertens
Copy link
Contributor

wmertens commented Mar 2, 2018

@Poincare

The operation that resetStore represents is certainly not safe with inflight queries, so although I can imagine some cases where you might want to catch the error and proceed on with the reset anyway, it probably isn't a great practice.

I'm hitting the error again in another place, and I can't work around it. I'm having a hard time imagining a situation where you call client.resetStore() and then you don't want in-flight queries to be reset as well. Can you elaborate?

@stevenkaspar
Copy link

Not sure this helps anyone in this thread but I didn't think to see if resetStore was a Promise and turns out it is. For me it was an issue of the requests that were happening right after resetStore - not the already in flight ones (I think). So changing this

cookies.set('jwt', response_data.data.getCreds.token)
this.props.client.resetStore()
this.props.loggedIn(response_data)

to this

cookies.set('jwt', response_data.data.getCreds.token)
this.props.client.resetStore().then(() => {
   this.props.loggedIn(response_data)
}) 

got rid of the error

@zvictor
Copy link

zvictor commented Jun 8, 2018

@joenoon I created a reproducible to what could be the same issue, but it's not clear to me if it's actually the same issue or not. Could you take a look at #3555 and confirm if the issue is a duplicate?

@dallonf
Copy link

dallonf commented Jul 19, 2018

I agree with the sentiment that in-flight queries should be automatically dropped and refetched instead of throwing errors (or at least, that behavior should be available as an option).

Here's my situation: after completing a mutation with lots of side effects, I would like to drop the entire cache (in lieu of support for targeted cache invalidation) and then navigate to the next page.

If I call await client.resetStore(); history.push(nextPage);, the current page will visibly refetch all of its data, the app will wait for this to finish, and then navigate to the next page. This is not desirable because the app waits for data that isn't needed anymore to finish loading, and it creates visual noise as the previous page goes into its loading state, then shows its refetched data for a split second before navigating to the next page, which will also show a loading state before settling with the latest data.

If I instead reverse the process to history.push(nextPage); await client.resetStore();, I get the error shown here. I get the same result if I don't wait for the store to finish resetting before navigating (client.resetStore(); history.push(nextPage);).

@larixer
Copy link
Contributor

larixer commented Jul 19, 2018

@dallonf If you want to drop entire cache without triggering reload of current page queries you can achieve that by calling client.cache.reset() instead of client.resetStore(). E.g. call reset() on Apollo Cache directly somehow.

@dallonf
Copy link

dallonf commented Jul 19, 2018

@Vlasenko Ah, thanks, that's definitely an easier workaround than what I was going for (forcing an empty render while resetting the store), although this still definitely needs to be addressed. Relying on an undocumented internal function shouldn't be the official solution to any problem!

@larixer
Copy link
Contributor

larixer commented Jul 19, 2018

@dallonf I'm not from official team, just a user. I understand that calling client.cache.reset() is not nice, thats why I added:

E.g. call reset() on Apollo Cache directly somehow.

If you have access to Apollo Cache somehow, you can call reset() directly on cache instance, this is an official way to reset it, I believe, without relying on internals.

@mrdulin
Copy link

mrdulin commented Aug 3, 2018

same issue.

Here is my code snippet:

onError(({ graphQLErrors, networkError }) => {
        if (graphQLErrors)
          graphQLErrors.map(error => {
            // console.log(`[GraphQL error]: Message: ${message}, Location: ${locations}, Path: ${path}`)
            if (error.code === 1001) {

              auth.signout();
              //client.resetStore()
              client.cache.reset();
              window.location.replace('#/login');

              //client.resetStore()
              // .then(result => {
              //   window.location.replace('#/login');
              // })
              // .catch(err => {
              //   console.log('reset store error: ', err);
              // });
            }
          });
        if (networkError) console.log(`[Network error]: ${networkError}`);
      }),

if I ignore handling the promise client.resetStore() method returned. It will throw Store reset while query was in flight. error.

And, I have another issue. If graphQLErrors happened, client.resetStore().then().catch() will always trigger catch and dead loop.

@hwillson
Copy link
Member

hwillson commented Sep 5, 2018

This issue is related to #3555, #3766 and #3792. We'll address them all in one shot.

@hwillson hwillson self-assigned this Sep 5, 2018
@hwillson hwillson added the to do label Sep 5, 2018
x5engine pushed a commit to x5engine/Asteroid that referenced this issue Sep 15, 2018
resetStore: Store reset while query was in flight.


apollographql/apollo-client#2919 (comment)
@settings settings bot removed reset labels Dec 12, 2018
@joshdcuneo
Copy link

Not sure if this is relevant to all the use cases here but this flow is working ok for me:

login = async ({ token, user, authenticated }) => {
    const {
      props: { client }
    } = this;
    if (authenticated) {
      localStorage.setItem("token", token);
      await client.resetStore();
      this.setState({ user });
    }
    return null;
  };

  logout = async () => {
    const {
      props: { client }
    } = this;
    localStorage.setItem("token", "");
    this.setState({ user: null, token: null });
    await client.resetStore();
  };

@lukejagodzinski
Copy link

lukejagodzinski commented Jan 7, 2019

@joshdcuneo your solution works as you're storing "login" state in the React component state which is not a case in most examples presented here. The problem occurs when you try to store "login" state in the apollo cache which is the most obvious place to store it. I want to have access to it from any place in the app.

It's more and more frustrating to use Apollo Client. With Redux I didn't have such kind of problems. I'm really considering getting back to Redux for local state management.
I will prepare reproduction repository tomorrow to show that this is still an issue

@lukejagodzinski
Copy link

lukejagodzinski commented Jan 8, 2019

I've created simplest possible reproduction repository here: https://github.com/lukejagodzinski/apollo-client-reset-store-bug

Error is thrown on each sign out (client.resetStore()). I'm not doing any fancy things here just one server query (when signed in) and one mutation of the client cache for storing sign in status. I think that here

error shouldn't be thrown if promises were rejected because of the store being reset.

To my surprise it take really long time to resolve those promises. Even when result of the query was already displayed the promise related with this query is still being processed? Maybe it some other bug in the code.

I've also recorded short screencast showing a bug https://youtu.be/hY_JiLApXzk

@WilliamConnatser
Copy link

WilliamConnatser commented Feb 1, 2019

I was just having the same issue. After some experimentation, including some fixes listed above (to no avail,) I ended up making my logout function synchronous and it got rid of the error.

No Error:

            signoutUser: () => {
                //Remove token in localStorage
                localStorage.setItem("token", "");
                //End Apollo Client Session
                apolloClient.resetStore();
            }

Error:

            signoutUser: async () => {
                //Remove token in localStorage
                localStorage.setItem("token", "");
                //End Apollo Client Session
                await apolloClient.resetStore();
            }

I don't get why I was getting this error because I have the onError property for my ApolloClient instance set which seems to work in catching all other Apollo errors???

//Catches all errors
    onError: ({
        operation,
        graphQLErrors,
        networkError
    }) => {
        //operations object not currently in use, but stores query/mutation data the error occurred on
        if (networkError) Vue.toasted.show(networkError.message);
        if (graphQLErrors) {
            for (let err of graphQLErrors) {
                Vue.toasted.show(err.message);
            }
        }
    }```

@eric-burel
Copy link

eric-burel commented Apr 5, 2019

Hi, not sure if it is worth opening a new issue for this, but sometimes resetStore() neither resolve or fails. This is very hard to reproduce, it only happens when I run my app in a popup window, and I log out. Then, when I sign in again, the promise eventually throws a "store reset when query in flight" error.

Any idea what could cause resetStore() to wait indefinitely ? Could it be related to some page reload, an invalid URL or a query that fails silently ?
Edit: the reset store error also seems to happen for no reason, even when I don't call resetStore(). Could SSR provoke this error too ?

@doomsower
Copy link
Contributor

@eric-burel I think I ran into something similar, but was too lazy to open issue or create minimal reproduction. Here is what happened for me:

  • I have two nested Query components A (outer) and B (inner)
  • When user is logged in, both A and B are rendered
  • When user is not logged in, A is rendered, but B is not.
  • When user logs out, as a part of this process I call resetStore
  • What I think happens is:
    • First, resetStore calls refetch on all mounted queries . Both A and B are still mounted, so refetch is called on both.
    • The order in which A and B are resolved is arbitrary
    • If B gets resolved first, everything is fine
    • If A gets resolved first, B gets unmounted, and it's query promise gets stuck somewhere in limbo of QueryManager
  • I just rearranged my code so that B gets unmounted before resetStore during logout.

@eric-burel
Copy link

eric-burel commented Apr 5, 2019

@doomsower very interesting, I will dig that later but thank you very much for the insight. This might be related indeed, as the view changes on logout even if the resetStore() is not done. It also happens on login (without logout), which also triggers some redirect.

Edit: this has been solved in Vulcan by @ErikDakoda : VulcanJS/Vulcan#2313. Instead of relying on a Promise, the solution for us was to use the new onResetStore callback of the Apollo client.
This way, the callback is not registered in the logout React component, which may be erratically dismounted depended on your workflow (eg with an optimistic UI), but at the Apollo client level. So it is preserved and run even after an amount. This issue has disappeared so far and the code feels more robust.

@hwillson hwillson removed their assignment May 16, 2019
@jbaxleyiii jbaxleyiii added the 🚧 in-triage Issue currently being triaged label Jul 9, 2019
@jbaxleyiii
Copy link
Contributor

Thanks for reporting this. There hasn't been any activity here in quite some time, so we'll close this issue for now. If this is still a problem (using a modern version of Apollo Client), please let us know. Thanks!

@wmertens
Copy link
Contributor

wmertens commented Jul 9, 2019

Still happening on stable as of two weeks ago

@jedwards1211
Copy link
Contributor

jedwards1211 commented Aug 15, 2019

The way I see it, resetStore should:

  • Ignore the results of all in-flight queries, mutations, and subscriptions
  • Queue up any new queries, mutations, and subscriptions initiated after resetStore call until the reset is finished

And do the following in order:

  • Internally stop all subscriptions
  • Wait for in-flight mutations to complete
  • Clear the cache
  • Then restart all pre-existing queries and subscriptions without causing them to error
  • Then start all new queued-up queries, mutations, and subscriptions

Is there any case where this wouldn't be appropriate?

In-progress Query components don't need to know that the store is resetting; from their perspective it can just look like the query took from before the reset to after the reset to finish.

Queries that were finished before the reset, and subsequently need to be refetched, can just jump from ready state back to loading state -- or does this violate any critical assumptions?

Subscriptions don't need to see a status change due to the reset either. From their perspective it can look like they're still subscribed and there just haven't been any events.

@jedwards1211
Copy link
Contributor

@Vlasenko resetting only the cache seems risky unless you're stopping all of your in-flight queries at application level. Queries that were in-flight before the reset (e.g. queries for the previous logged-in user) could write invalid results to the reset cache when finished.

@MarcoScabbiolo
Copy link

Then restart all pre-existing queries and subscriptions without causing them to error

@jedwards1211 a very common use case is to reset the store on logout, if the pre-reset queries are restarted, they will either fail or resolve with invalidated parameters (Authorization header of the not-logged-anymore user).

@jedwards1211
Copy link
Contributor

I don't know how you're putting in the auth header but in my case when the queries are restarted, the middleware to add headers gets called anew and injects the new user's auth header

@khaphannm
Copy link

Running into this issue now. Try all the solutions above, but seems not work. Anyone have some news about this issue ?

@azuxx
Copy link

azuxx commented Apr 27, 2020

Same here, and I am using @apollo/client 3.0.0-beta.43

@anajavi
Copy link

anajavi commented May 8, 2020

I can't reproduce this anymore with @apollo/client 3.0.0-beta.46.

@hinok
Copy link

hinok commented May 29, 2020

It's happening in my project for modern apollo

├── @apollo/react-hoc@3.1.3
├── @apollo/react-hooks@3.1.3
├── @apollo/react-ssr@3.1.3
├── apollo-cache@1.3.4
├── apollo-cache-inmemory@1.6.5
├── apollo-client@2.6.8
├── apollo-link@1.2.13
├── apollo-link-batch-http@1.2.13
├── apollo-link-error@1.1.12
├── apollo-upload-client@12.1.0
├── apollo-utilities@1.3.3

Reported error

Error: Network error: Store reset while query was in flight (not completed in link chain)
    at new ApolloError (bundle.esm.js:63)
    at ObservableQuery.push.../../node_modules/apollo-client/bundle.esm.js.ObservableQuery.getCurrentResult (bundle.esm.js:159)
    at QueryData.push.../../node_modules/@apollo/react-hooks/lib/react-hooks.esm.js.QueryData.getQueryResult (react-hooks.esm.js:265)
    at QueryData._this.getExecuteResult (react-hooks.esm.js:73)
    at QueryData.push.../../node_modules/@apollo/react-hooks/lib/react-hooks.esm.js.QueryData.execute (react-hooks.esm.js:106)
    at react-hooks.esm.js:380
    at useDeepMemo (react-hooks.esm.js:354)
    at useBaseQuery (react-hooks.esm.js:380)
    at useQuery (react-hooks.esm.js:397)
    at Query (react-components.esm.js:8) "
    at Meta (http://localhost:8080/common.chunk.js:104011:3) // <------------ This is my component
    at Query (http://localhost:8080/app.chunk.js:167:26)

So basically what I'm trying to achieve is to reset apollo in-memory cache on redux store change when user logged out, it may be because user clicked "Logout" link intentionally or token expired etc.

After "logout" user is automatically redirected to another page that run some graphql queries about the page (head meta data like title, description etc.).

I'm not sure how should I call client.clearStore() from store.subscribe or redux middleware in a way that it won't conflict with any new or pending queries. Is there a way to safely schedule clear of apollo's cache?

Is something like below (#3766 (comment)) the way to fix it?

client.stop()
client.clearStore()

@Fi1osof
Copy link

Fi1osof commented Oct 31, 2020

I can't reproduce this anymore with @apollo/client 3.0.0-beta.46.

Just open react-development-tools, find ApolloProvider and exec

for (let i = 0; i< 3; i++) {
    console.log('i', i);
    setTimeout($r.props.client.resetStore, 10)
}

I still got error

Unhandled Runtime Error
Invariant Violation: Store reset while query was in flight (not completed in link chain)

Before, in pure js i just checking by

if(!client.queryManager.fetchCancelFns.size) {
   await client.resetStore()
}

but i white in typescript now and client.queryManager is private https://github.com/apollographql/apollo-client/blob/main/src/core/ApolloClient.ts#L74

It's so sad! I can't check this any more and apollo does not provide any checking(((

UPD. But i can check like this

client["queryManager"].fetchCancelFns.size

and no typescript errors...

@khushahal-sharma
Copy link

Calling client.stop() method before clearStore solved my problem.

client.stop()
client.clearStore();

Accroding the official doc:
Call this method to terminate any active client processes, making it safe to dispose of this ApolloClient instance.
https://www.apollographql.com/docs/react/api/core/ApolloClient/#ApolloClient.stop

@Fi1osof
Copy link

Fi1osof commented Nov 24, 2020

Calling client.stop() method before clearStore solved my problem.

client.stop()
client.clearStore();

Accroding the official doc:
Call this method to terminate any active client processes, making it safe to dispose of this ApolloClient instance.
https://www.apollographql.com/docs/react/api/core/ApolloClient/#ApolloClient.stop

@khushahal-sharma it's bot optimal. For example, i have many subscriptions and got many events for updating at the moment. In this case starts many cicles "Send query, stop, send another...". Checking global sending status more useful.

@terryatgithub
Copy link

Calling client.stop() method before clearStore solved my problem.

client.stop() client.clearStore();

Accroding the official doc: Call this method to terminate any active client processes, making it safe to dispose of this ApolloClient instance. https://www.apollographql.com/docs/react/api/core/ApolloClient/#ApolloClient.stop

still encounter this error with "@apollo/client": "~3.4.17".
so we will adopt this solution for now.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 14, 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