reactiveui / Akavache

An asynchronous, persistent key-value store created for writing desktop and mobile applications, based on SQLite3. Akavache is great for both storing important data as well as cached local data that expires.
https://evolve.xamarin.com/session/56e2044afd00c0253cae33a3
MIT License
2.45k stars 288 forks source link

What's the proper etiquette for shutting down Akavache on mobile? #342

Open PureWeen opened 7 years ago

PureWeen commented 7 years ago

Personally I've never used Shutdown but the documentation is so emphatic about it that it's always made me feel a little guilty. I've tried to use it but never quite been able to get it to work very well

Critical to the integrity of your Akavache cache is the BlobCache.Shutdown() method. You must call this when your application shuts down. Moreover, be sure to wait for the result:

Here's what I think I know about shutdown. It's always and forever. When you call the static shutdown task it sets a static bool that cannot be swapped back. This causes all the caches to always return a "ShutdownBlobCache" which makes sense because you want to stop all in flight requests. But I can't really find a reasonable place on any of the platforms to call Shutdown from

Here's my Journey so far

Windows

AFAICT There is no termination event in windows

https://blogs.windows.com/buildingapps/2016/04/28/the-lifecycle-of-a-uwp-app/

Application.Suspending event (there is no terminated event method, so this is the last point at which you can do something)

But you don't want to shutdown inside Suspending because then if the app comes back you're stuck with a shut down BlobCache.

My first experience with shutdown issues was around Background Tasks.. I figured startup Akavache then at the end of the Background Task shut it down. UWP Background Tasks run as a separate process than the main app so I knew there wouldn't be overlap. But I discovered that occasionally the same process space would be reused and I never really knew when. At this point I just opted for a flush. I think left some shutdown behavior inside the cancellation parts of Background Tasks but that was all awhile ago

Android

There's already an issue kind of around Android https://github.com/akavache/Akavache/issues/339

I never really tried that hard inside Android to figure out where to Shutdown because my app always had something running.. A startup service for doing Geofences, a GCM service that wakes up, and the main app. All of which (AFAICT) run in the same process space. I googled around about android and there seems too be no consistent place to detect an application exit

This was the best I could find http://stackoverflow.com/questions/19568315/how-to-handle-code-when-app-is-killed-by-swiping-in-android/26882533#26882533 http://stackoverflow.com/questions/24406508/how-to-detect-application-exit-on-android

Use a service and check OnTaskRemoved

But I don't know..... I still don't totally trust this

iOS

I have the least amount of experience here but there's this https://developer.apple.com/library/content/documentation/iPhone/Conceptual/iPhoneOSProgrammingGuide/TheAppLifeCycle/TheAppLifeCycle.html

App Termination

Apps must be prepared for termination to happen at any time and should not wait to save user data or perform other critical tasks. System-initiated termination is a normal part of an app’s life cycle. The system usually terminates apps so that it can reclaim memory and make room for other apps being launched by the user, but the system may also terminate apps that are misbehaving or not responding to events in a timely manner.

Suspended apps receive no notification when they are terminated; the system kills the process and reclaims the corresponding memory. If an app is currently running in the background and not suspended, the system calls the applicationWillTerminate: of its app delegate prior to termination. The system does not call this method when the device reboots.

In addition to the system terminating your app, the user can terminate your app explicitly using the multitasking UI. User-initiated termination has the same effect as terminating a suspended app. The app’s process is killed and no notification is sent to the app.

Current Conclusion

The common theme among the platforms seem to be that you are given a "suspension" event to save your data and then after that there are no guarantees about what notifications you will or will not receive. I feel like the safer bet given what's available currently is to just issue a FLUSH of the particular cache you are using

Akavache.BlobCache.LocalMachine.Flush().Wait();

Which I think is fine but I don't know enough about SqlLite to know if that's enough or if it's important to have the Connection closed guarantee everything is finished.

The other issue with Flush is that all it does is create a NOOP checkpoint in the queue, Which notifies you when the queue catches up to that point. But if other things come in afterwards those aren't going to be flushed. So there's some possibilities there for edge case issues.

Maybe something between shutdown and flush would be useful? Where it can be placed into a suspended state so no more requests can come in, everything is ensured to be persisted to disk, but it can be woken back up if needed.... If that seems excessive though maybe just a static flush on BlobCache that acts like Shutdown and just gathers up all the BlobCaches and flushes them?

Or there's just something really obvious I've missed as far as where to stick Shutdown

On a quick side note

Xamarin Forms

OnSleep seems to tie into OnSuspend on all the platforms so for now on my projects I just put a flush into there and call it good :-)

But again the doc

Critical to the integrity of your Akavache cache is the BlobCache.Shutdown() method. You must call this when your application shuts down. Moreover, be sure to wait for the result:

So I still live in fear :-x

anaisbetts commented 7 years ago

The static BlobCache.Shutdown() is Unsuitable:tm: for mobile apps that have a lifecycle (I would argue this is a bug). Call Shutdown on all of the Blobcaches themselves, then recreate them on startup via Locator.GetService. Remember that the static BlobCache.LocalMachine et al aren't actually something special, it's just a convenience thing, and that you can always just new up your own SqliteBlobCaches and manage them yourself

PureWeen commented 7 years ago

That makes sense

So I guess one could just do this new Akavache.Sqlite3.Registrations().Register(Locator.CurrentMutable); To recreate the static versions after you shut them down individually.

Without trying to make the static version too smart it seems like just adding some "Restart" method on there that sets "shutdownRequested" back to false and then re-inits those might be useful. The hard shutdown behavior strikes me as still being useful to have in cases where you want to make sure no more inbound requests show up.

I like the idea of just newing up your own to manage for the given "lifecycle"... So in my example above with a UWP background process, each run of the task could just new it up within the scope of the run and then shutdown at the end... Just skip the static ones completely.. AFAICT from looking at SQLite it seems like that's ok to do. My only fear would be having issues with multiple writes over the same file but it seems like SQLite is able to handle the locking on those files in such a way that you won't run into anything too bad. And if you're just doing reads then you don't need an EXCLUSIVE lock so it'll be fine.. Then it really just comes down to being smart about concurrency.

My only other thought would be to maybe extract the code a bit that news up those caches so one could easily just ask Akavache for a new one opposed to just having to copy and paste this

 fs.CreateRecursive(fs.GetDefaultLocalMachineCacheDirectory()).SubscribeOn(BlobCache.TaskpoolScheduler).Wait();
                return new SQLitePersistentBlobCache(Path.Combine(fs.GetDefaultLocalMachineCacheDirectory(), "blobs.db"), BlobCache.TaskpoolScheduler);

out of the code, but I guess if you're just newing it up yourself you could ignore all of that completely and just specify location yourself :-) The core idea being that Akavache is really just a nifty intelligent NOSQL wrapper over SQLite so you can just use those wrappers how you please and ignore all the static stuff that's just there to "get you started"

I'll PR this thought into the docs

The static BlobCache.Shutdown() is Unsuitable™️ for mobile apps that have a lifecycle

DennisWelu commented 7 years ago

Thanks @PureWeen for the detailed question on this topic. I struggled with this not long ago... In my case using Xamarin Forms for iOS, Android, and UWP. I resolved to what I thought was a happy solution which crosses over the ideas mentioned here:

FWIW!

Depechie commented 7 years ago

@DennisWelu you have any sample code for this? :)

DennisWelu commented 7 years ago

Sure. Abbreviated form it looks like this...

App.xaml.cs:

public partial class App
{
    ISomeUsefulRepository Repo { get; set; }

    public App()
    {
        InitializeComponent();

        ...

        Repo = DependencyLocator.Resolve<ISomeUsefulRepository>();

        ...
    }

    protected override void OnSleep()
    {
        Repo.Shudown();
    }
}

And the repository interface/implementation looks something like this:

public interface ISomeUsefulRepository
{
    Task<SomethingUseful> GetSomethingUseful(); // example of some method available on the repo

    ...

    void Shutdown(); // Ensures that everything is closed and written and happy
}

public class SomeUsefulRepository : ISomeUsefulRepository
{
    Lazy<IBlobCache> Cache { get; set; } // This is the Akavache cache where everything is loaded from and saved to.

    public SomeUsefulRepository()
    {
        Cache = new Lazy<IBlobCache>(NewBlobCache);
    }

    IBlobCache NewBlobCache() // Factory method to create an IBlobCache
    {
        // This snippet based on https://github.com/akavache/Akavache/blob/501b397d8c071366c3b6783aae3e98695b3d7442/src/Akavache.Sqlite3/Registrations.cs
        // In this case, using an encrypted provider but that is not relevant to the Shutdown process below...
        var fs = Locator.Current.GetService<IFilesystemProvider>();
        var cacheFolder = fs.GetDefaultSecretCacheDirectory();
        fs.CreateRecursive(cacheFolder).SubscribeOn(BlobCache.TaskpoolScheduler).Wait();
        return new SQLiteEncryptedBlobCache(Path.Combine(cacheFolder, "useful.db"),
            new MyEncryptionProvider(), BlobCache.TaskpoolScheduler);
    }

    public async Task<SomethingUseful> GetSomethingUseful()
    {
        ... 
    }

    public void Shutdown()
    {
        // If the cache has not been accessed then no need to flush it
        if (Cache.IsValueCreated)
        {
            Cache.Value.Dispose();
            Cache.Value.Shutdown.Wait();
            Cache = new Lazy<IBlobCache>(NewBlobCache);
        }
    }
}
PureWeen commented 7 years ago

That's awesome @DennisWelu

That's along the lines of what I was thinking as well and why I was thinking adding some helpers into Akavache for that super fun "NewBlobCache" code would be really useful.

DennisWelu commented 7 years ago

Cool - glad it can help! And I agree that NewBlobCache snippet has a quite a few things going on so it makes sense to make things a little easier there for everyone.

EmilAlipiev commented 6 years ago

@DennisWelu

I would like to try your code but I am confused about something, you create database file useful.db, what is the default database file created when we just use Akavache.BlobCache.ApplicationName = "myApp";

is it myApp.db? I am asking because I dont want to use already existing cache. So i need to continue using the same name.

Other question i was using UserAccount as for example await BlobCache.UserAccount.InsertObject(key, data);

with your implementation will it be UserAccount or LocalMachine? what would be the difference?

DennisWelu commented 6 years ago

Hi @EmilAlipiev, in my example I was creating a specific db by name and not using the built in convenience methods, as you've seen. However BlobCache.UserAccount should just return an IBlobCache and so as it relates to shutdown I think the same ideas apply. Same goes for LocalMachine. The choice is up to you and what you are trying to accomplish (per user or per machine storage). If you need to know what the actual default db filename is behind the scenes for those convenience methods I forget what they are off hand...but if you follow through the code in the Akavache repository you can find it. :-)

EmilAlipiev commented 6 years ago

@DennisWelu so based on your sample. you create a new instance of SomeUsefulRepository every time you need in any class. Does it any advantage over using a static instance regards to performance, I wonder? I am asking this because in my xamarin forms app, I experience some memory leaks and every where akavache prints. I am not sure if that might help. I will definetly try and test but just asking if you have seen improvements. thank you.

DennisWelu commented 6 years ago

@EmilAlipiev In my case the SomeUsefulRepository is actually the implementation for ISomeUsefulRepository, which is available in an IoC container...so it is actually only created once for the life of the container and every time someone asks for the ISomeUsefulRepository they get that instance. If you were to create SomeUsefulRepository instances for each usage then yes you would get the behavior you are describing. If you are using the default convenience static methods (BlobCache.UserAccount and friends) then you won't have that problem either.

I have not noticed Akavache specific leaks. Xamarin memory leaks tend to be due to cycles in object references not allowing the garbage collector to clean up things you think are no longer needed (especially in iOS). The profiler is your friend for finding those. But that's a whole different thread of discussion...

EmilAlipiev commented 6 years ago

@DennisWelu thanks for your answer. Just last stupid question from me. I was doing in MainActivity of android as follows and in OnStart of App.xaml.cs. Is there something wrong with this implementation over yours? I am even thinking instead of doing in MainActivity, if I do it OnResume of App.xaml.cs?

   protected override void OnDestroy()
        {
               base.OnDestroy();
            Akavache.BlobCache.Shutdown().Wait();  // this calls the shutdown code
            Akavache.BlobCache.LocalMachine = null; // clear out the static cached variables
            Akavache.BlobCache.Secure = null;
            Akavache.BlobCache.UserAccount = null;
            FieldInfo fi = typeof(Akavache.BlobCache).GetField("shutdownRequested", BindingFlags.Static | BindingFlags.NonPublic);
            fi.SetValue(null, false); // reset the private flag
            Splat.Locator.Current = new Splat.ModernDependencyResolver(); // create a new dependency resolver so akavache has to create new objects when the app is resurrected.

        }

      protected async override void OnStart()
        {             
            Akavache.BlobCache.ApplicationName = "myApp";
DennisWelu commented 6 years ago

I wouldn't be in the Android project at all to do this kind of thing for a Xamarin Forms app - you can control it all in the shared/cross platform parts of the code (such as the App.xaml.cs you mention if not in an injected service like I've done). You should only ever have to set the ApplicationName once so you can do that in something like the App constructor. Don't forget to both call Dispose on the cache and THEN do the Shutdown().Wait(). If you are only using the UserAccount cache you should only have to null that handle out and not worry about Secure or LocalMachine. Override OnSleep, OnStart and OnResume in the App to control the lifetime of your blob cache object (create and/cleanup) as needed. That's my thoughts!

EmilAlipiev commented 6 years ago

@DennisWelu I just tried to code below in my Onsleep method of app.xaml.cs and initializing BlobCache in OnStart but it throw exception saying "System.ArgumentNullException" when I try to query GetAll function. Is it because of I am Disposing it? thats why you are doing Cache = new Lazy<IBlobCache>(NewBlobCache); ?

        protected async override void OnStart()
        {

            Akavache.BlobCache.ApplicationName = "myApp";

}
 protected override void OnSleep()
        {
            // Handle when your app sleeps       

            try
            {

                if (Akavache.BlobCache.UserAccount != null)
                {
                    Akavache.BlobCache.UserAccount.Dispose();
                    // Akavache.BlobCache.UserAccount.Flush();
                    Akavache.BlobCache.Shutdown().Wait();  // this calls the shutdown code              
                    Splat.Locator.Current = new Splat.ModernDependencyResolver(); // create a new dependency resolver so akavache has to create new objects when the app is resurrected.

                }

            }
            catch (Exception ex)
            {

            }
        }
DennisWelu commented 6 years ago

Yes you can't do anything with it after Dispose that is true for any kind of C# object. I would set it to null and if needed you can re-initialize something in OnResume. I can't help you much further there though I usually don't use the default convenience methods / blobcache instances. Not sure if just setting it to null will trigger it to re-initialize...you'll have to look through the repo source.

EmilAlipiev commented 6 years ago

Setting it Null also didnt help. it was the same as dispose I believe but I converted my app using your code and all works fine. thank you :)

PureWeen commented 6 years ago

I feel like this issue is creating some negative ideas about Akavache.

Akavache when asked to save something places the item into a Queue to be persisted to SQL (which is awesome cause that's one reason it's so efficient).

Calling Flush() or Shutdown() causes that queue to all be pushed to SQL and then you are alerted once it's done. Personally I just call Flush() in the "OnSleep" hook for Xamarin Forms and also inside OnDestroy for Android and I haven't had any issues.

But all we're talking about is dealing with a small window where things are in the "Queue" and then while in the queue something happens that unloads the process from memory and thus the Queue.

So you'd have to tell something to persist and then barely anytime after that the App Domain gets unloaded before the Queue can persist. I feel like that's a pretty small window of time where the data is "volatile". A small window where just placing "Flush" at some life cycle points takes care of

I didn't write a lot of the core components to Akavache so if I'm mistaken someone else can weigh in but AFAIK all we're really talking about here is an edge condition that's for now is easily taken care of by using Flush at some life cycle points.

DennisWelu commented 6 years ago

You're probably right about Flush being suitable. I think my approach originally was just incorporating Paul's recommendation for calling Shutdown on the blob caches themselves like he mentions earlier in this thread. So I went ahead and incorporated that into the lifecycle. Wasn't much work to do that. But yeah I get what you are saying and am not sure why Flush wouldn't be sufficient on its own.

PureWeen commented 6 years ago

Yea either should work.. I just keep seeing people referring back to this thread now with trepidation about using Akavache in a mobile scenario :-/ which wasn't my intent of creating this in the first place. Like I bet 99.9 percent of the time you could not call either and it'd be fine (me for the first year or so I used Akavache).

My thoughts on the solution here would be some additional documentation (I've been meaning to add) and I think Akavache should move to using an "INIT" step. For example right now the Akavache.Sqllite3 dll will get removed because of xamarin's static linker analysis without that extra file. Having an INIT removes the need for that file. I think having an INIT is a bit more clear plus it allows for say Initializing some cross platform life cycle thing. I have one pull request up there for an Init variation of things...

But yea

Moral of the story just stick Flush in there when the app sleeps/destroyed and be happy about Akavache don't fret :-)

EmilAlipiev commented 6 years ago

I am not sure if it is related but after implementing this solution. I started seeing some exception logs as below even though not many and looks only for Android 4.x versions. Could it be related to disposing in OnSleep? I have implemented it almost one to one. I have never had this exception using BlobCache default static values.

 System.TypeInitializationExceptionThe type initializer for 'Akavache.Sqlite3.Internal.SQLiteConnection' threw an exception.
Raw
  at (wrapper managed-to-native) System.Object:__icall_wrapper_mono_generic_class_init (intptr)
  at Akavache.Sqlite3.SQLitePersistentBlobCache..ctor (System.String databaseFile, System.Reactive.Concurrency.IScheduler scheduler) [0x00026] in <67aced6c5c1a4c15b03e120d7300429d>:0 
  at myApp.Database.Akavache.CacheRepository.NewBlobCache () <0x6e5f5af4 + 0x00127> in <07bdd14ef78c452783c11438e70282d3>:0 
  at System.Lazy`1[T].CreateValue () <0x6d8514d4 + 0x0032c> in <4981a9d6498a40b1a60bc9b92c959fea>:0 
  at System.Lazy`1[T].LazyInitValue () [0x00080] in <4981a9d6498a40b1a60bc9b92c959fea>:0 
  at System.Lazy`1[T].get_Value () <0x6d851408 + 0x00087> in <4981a9d6498a40b1a60bc9b92c959fea>:0 
  at myApp.Database.Akavache.CacheRepository+<Save>d__7`1[T].MoveNext () [0x0001a] in <07bdd14ef78c452783c11438e70282d3>:0 
PureWeen commented 6 years ago

I think that's a linker issue. The IWantsToRegisterStuff pattern uses reflection which then causes some things to get linked out

I just linkskip the akavache binaries and it makes everything fine

or you can have this in a linker xml file

    <assembly fullname="Akavache">
        <type fullname="*" />
    </assembly>
    <assembly fullname="Akavache.Sqlite3">
        <type fullname="*" />
    </assembly>
EmilAlipiev commented 6 years ago

@PureWeen I have already defined them as shown in the image and I have never seen these exceptions earlier when I was using static blobcache methods. I am thinking to go back to that solution. so your suggestion is use onDestroy method of Android like below and plus do flush in OnSleep method of PCL. Do I understand correct? where do you handle this in UWP and IOS?

   protected override void OnDestroy()
        {
               base.OnDestroy();
            Akavache.BlobCache.Shutdown().Wait();  // this calls the shutdown code
            Akavache.BlobCache.LocalMachine = null; // clear out the static cached variables
            Akavache.BlobCache.Secure = null;
            Akavache.BlobCache.UserAccount = null;
            FieldInfo fi = typeof(Akavache.BlobCache).GetField("shutdownRequested", BindingFlags.Static | BindingFlags.NonPublic);
            fi.SetValue(null, false); // reset the private flag
            Splat.Locator.Current = new Splat.ModernDependencyResolver(); // create a new dependency resolver so akavache has to create new objects when the app is resurrected.

        }

image

EmilAlipiev commented 6 years ago

@PureWeen so is it just enough to do in PCL

   protected override void OnSleep()
        {
            // Handle when your app sleeps       
            Akavache.BlobCache.UserAccount.Flush();
        }

or should I additionally call in MainActivity on Android?


   protected override void OnDestroy()
        {          
            Akavache.BlobCache.Shutdown().Wait();  // this calls the shutdown code         
            Akavache.BlobCache.UserAccount = null;
            FieldInfo fi = typeof(Akavache.BlobCache).GetField("shutdownRequested", BindingFlags.Static | BindingFlags.NonPublic);
            fi.SetValue(null, false); // reset the private flag
            Splat.Locator.Current = new Splat.ModernDependencyResolver(); // create a new dependency resolver so akavache has to create new objects when the app is resurrected.

        }
Meekohi commented 1 year ago

This should be part of the docs!

glennawatson commented 1 year ago

This should be part of the docs!

We take pull requests.