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

State of inMemoryCache is not always persisted/restored correctly #423

Closed
Sewdn opened this issue Sep 26, 2019 · 6 comments
Closed

State of inMemoryCache is not always persisted/restored correctly #423

Sewdn opened this issue Sep 26, 2019 · 6 comments
Labels
🐛 bug Something isn't working 📦 cache Relates to caching functionality

Comments

@Sewdn
Copy link

Sewdn commented Sep 26, 2019

Describe the bug
When the cache grows, writing bigger cache to a file cant finish before the app was teared down. As a result persisted cache is incomplete and can result in unexpected behavior when this incomplete cache was restored (references not being found).

To Reproduce
Steps to reproduce the behavior:

  1. Setup a Query resolving a rather large result set (>1000 records of complex Types referencing other Types that will be normalized)
  2. Close the app after the result set was loaded (don't just move to background, but suspend the app) (cache will only partially been written to file)
  3. Reopen the app (partially written cache will be restored)
  4. Check your cached result set to w newly fetch Query result. It will not be of the same length

Expected behavior
Writing inmemorycache to persistence layer and restoring it to memory from persistence layer should always be idempotent.

Additional context
CacheProvider can't await cache writes when cache is only being written to file on app lifecycle methods. https://github.com/zino-app/graphql-flutter/blob/128b937d2ef5fbc6225d46de3da1a5cf1832dfca/packages/graphql_flutter/lib/src/widgets/cache_provider.dart#L49. didChangeAppLifecycleState can be written as an async method, but in our experience, when awaiting the client.cache.save method will not stop the app from closing.

It would seem less fragile to us, writing to cache on several occasions when the app is not about to be suspended.

For instance, debounce a client.cache.save method call everytime the inmemory cache was being written to.

@micimize micimize added the 🐛 bug Something isn't working label Sep 26, 2019
@Sewdn
Copy link
Author

Sewdn commented Sep 26, 2019

We managed to setup an InMemoryCache implementation with a file based persistence backend not relying on app lifecycles, but rather persist cache when new cache writes have completed (debounced).

However, when writing big caches to file, the application's UI is jittered, since Dart is single threaded. Correct me if I'm wrong, but since file writing (IO in Dart) is happening in another thread, it must be the JSON encoding of the current inmemorycache's data to string to be able to write them to file, that is hogging all the resources.

We need to offload this encoding to another thread, when we want to persist cache during the app's lifetime.

Compute (https://api.flutter.dev/flutter/foundation/compute.html) and IsolateRunner (https://pub.dev/documentation/isolate/latest/isolate.isolate_runner/IsolateRunner-class.html) are 2 possible abstraction layers on top of Darts Isolates hat seems fit.

However, you can only pass primitives (or lists of them) as argument to the function that was offloaded, since they need to be serialized I guess. So the cache, consisting of Typed objects, is not fit to be passed as argument to the Isolate to perform some background processing on it.

Since our Dart knowledge is not sufficient, we don't know how to JSON encode the cache state in a background process (if we don't want to jitter the rendering in the main thread or if we don't want to wait to persist cache until app is about to be destroyed).

With the right advice, we're happy to contribute an alternative InMemoryCache with a different approach on persistence (still using file as a backend) avoiding incomplete cache restores.

Probably, another persistence backend like SQLite or Moor (cfr #402 ) is probably more suited, but that will be a much bigger effort. This alternative could be a better temporary solution...

@Sewdn
Copy link
Author

Sewdn commented Sep 27, 2019

Update on this issue.

We weren't able to offload the cache write to another thread. We then reverted all of our efforts and decided to keep things as they originally were, except for:

  • we don't write to cache.txt directly, because the file will be truncated as soon as the first line is being written. When the process is killed before the cache.save method can finish, you end up with an incomplete cache. Instead, we first write the cache to a temp file and when all has been written rename the temp file to cache.txt to overwrite existing cache.
  • we await cache restoration before building the app (during startup screen), to make sure all of our queries hit cache

This way, cache is still being persisted when the app moves to background or is being suspended, but it can never corrupt cache and we always end up with cache results if we have ever cached before.

Worst case now, is that cache was not persisted in edge cases, but this can not break user experience.

@micimize
Copy link
Collaborator

micimize commented Oct 6, 2019

I wonder if we should could just depend on https://pub.dev/packages/sembast. It's api is async, which might be an issue.

@smkhalsa
Copy link
Contributor

@micimize there's also https://github.com/hivedb/hive

@rcunning
Copy link

rcunning commented Nov 8, 2019

  • we await cache restoration before building the app (during startup screen), to make sure all of our queries hit cache

@sewd Specifically how do you await cache restoration? cache.restore() appears to be synchronous, but does not restore the cache before returning.

@klavs klavs added the 📦 cache Relates to caching functionality label Feb 18, 2020
@micimize micimize mentioned this issue Apr 19, 2020
5 tasks
@micimize
Copy link
Collaborator

micimize commented Oct 7, 2020

Closed by the addition of HiveStore in v4 (4.0.0-beta.1) #648

@micimize micimize closed this as completed Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working 📦 cache Relates to caching functionality
Projects
None yet
Development

No branches or pull requests

5 participants