Huge blocking time as soon as persistQueryClient/createWebStoragePersistor clears the old entries from cache #3034

Activity

1. TkDodo commented on Nov 29, 2021

   

   TkDodo

   on Nov 29, 2021

   Collaborator

   More actions

   Can you create a codesandbox example with that amount of data please? The auto removal feature is rather new so it could be that its not efficient enough. On the other hand, I’m not sure what we can do apart from removing the oldest query and then try to persist again 🤔

   @NewFuture FYI, maybe you’d have an idea here?
2. TkDodo commented on Nov 29, 2021

   

   TkDodo

   on Nov 29, 2021

   Collaborator

   More actions

   maybe we can also agree that always doing what we are trying to do (remove the oldest item and try to persist again) is not really applicable for all situations. Maybe we could let the user provide an overflowStrategy, which could be either firstInFirstOut (what we have currently), or something else that works better? Maybe even a custom function where the user can return true to keep persisting the query and false to throw it away?

   overflowStrategy?: 'firstInFirstOut' | (query: Query) => boolean
   

   what do you think?
3. NewFuture commented on Nov 30, 2021

   

   NewFuture

   on Nov 30, 2021

   Contributor

   More actions

   we use try-serialize-catch to detect the maximum space to store now.
   It's a performance-sensitive way,
   It will cost lots of resources when frequently called with continually increasing queries.

   @tilman can try to increase the throttle time, or add dehydrateOptions.shouldDehydrateQuery to filter the persistQueries.

   @TkDodo it's a good solution that users can provide a function handling this situation.
   But it's hard to define the API.
   overflowStrategy?: (query: persistedClient ) => persistedClient|false ?
   or
   saveStrategy?: (query: persistedClient ,storage:Storage,retryTimes) => boolean ?
4. chronark commented on Feb 8, 2022

   

   chronark

   on Feb 8, 2022

   More actions

   I was working with tilman together on this and we eventually settled on using this custom Storage implementation.
   It's basically the same as the default storage from react-query, just way more aggressive when purging data

   class LRUStorage implements Storage {
     private storage: Storage;
   
     private queryClient: QueryClient;
   
     constructor(queryClient: QueryClient) {
       if (typeof window === "undefined") {
         throw new Error("window is undefined");
       }
   
       this.storage = window.sessionStorage;
       this.queryClient = queryClient;
     }
   
     public get length() {
       return this.storage.length;
     }
   
     public getItem(key: string) {
       return this.storage.getItem(key);
     }
   
     public removeItem(key: string) {
       return this.storage.removeItem(key);
     }
   
     public clear() {
       return this.storage.clear();
     }
   
     public key(index: number): string | null {
       return this.storage.key(index);
     }
   
     public setItem(key: string, value: string): void {
       let err: Error | undefined;
       const MAX_ATTEMPTS = 10;
       for (let attempt = 0; attempt <= MAX_ATTEMPTS; attempt++) {
         try {
           return this.storage.setItem(key, value);
         } catch (error) {
           err = error as Error;
   
           const cache = this.queryClient.getQueryCache();
           const oldest = [...cache.getAll()].sort(
             (a, b) => a.state.dataUpdatedAt - b.state.dataUpdatedAt,
           );
           oldest.slice(0, 2 ** attempt).forEach((entry) => cache.remove(entry));
         }
       }
       console.error(err);
     }
   }

   And then calling it

   const queryClient = new QueryClient({ ... })
   persistQueryClient({
         queryClient,
         persistor: createWebStoragePersistor({
           storage: new LRUStorage(queryClient),
         }),
   });
       

   we didn't notice any performance issues after using this.
   I think being able to provide an overflowStrategy or similar would be nice, since our solution requires quite a bit of boilerplate code.

   👍React with 👍1Reacted by Vijay Pemmaraju
5. TkDodo commented on Mar 24, 2022

   

   TkDodo

   on Mar 24, 2022

   Collaborator

   More actions

   now that the plugin will become stable in v4, I think it's time to decide what we want to do here. The current approach seems to "work", but it's also computationally expensive. Also, we've only added it to the webStoragePersister, but not the AyncStorage one. But in react-native, there is also a limit on how much you can store, and it would be good to also support different overflowStrategies there.

   So if someone wants to contribute this to v4 (currently alpha branch), I'd be happy to see your proposals :)
6. TkDodo commented on Apr 24, 2022

   

   TkDodo

   on Apr 24, 2022

   Collaborator

   More actions

   Another idea: instead of providing overflowStrategy, we could simply provide an

   handlePersistError(persistedClient: PersistedClient, error: Error, errorCount: number): Promisable<PersistedClient>

   callback function where everyone can react however they want to any kinds of errors while persisting, including overflow errors. We would expect to get a new persistedClient back that might have fewer entries with which we would re-try the save.

   additionally, we could export some default strategies to handle these errors that conform to this interface. One would be the current try-remove-old-things handler, but given it has perf issues, I wouldn't use this one per default anymore. We can think about providing more errr handling strategy functions (should be treeshakable if unused), and they should work for both the async persister and the webstorage persister.

   not doing LRU anymore per default is breaking, but all of this should be for v4 anyways, and the fix would be simply to import and use the error handler.

   thoughts?

   👍React with 👍1Reacted by New Future
7. TkDodo

   added

   v4

   on Apr 24, 2022
8. TkDodo commented on Apr 24, 2022

   

   TkDodo

   on Apr 24, 2022

   Collaborator

   More actions

   I do have a draft PR of the aforementioned proposal, but I would really like some feedback and someone else to take this over. It's missing tests and docs, especially to see if this also works for the async persister:

   #3556
9. TkDodo

   added

   help wantedExtra attention is neededExtra attention is needed

   on Apr 24, 2022
10. TkDodo

    closed this as completedon May 21, 2022