google / ExoPlayer

This project is deprecated and stale. The latest ExoPlayer code is available in https://github.com/androidx/media
https://developer.android.com/media/media3/exoplayer
Apache License 2.0
21.69k stars 6.01k forks source link

SimpleCache: Try and retain UID if platform deletes from cache directory #8619

Open halaei opened 3 years ago

halaei commented 3 years ago

I see that SimpleCache creates a uid file in its cache dir. Also it has its own database to keep the list of files that are actually cached. That makes me wonder if SimpleCache is really compatible with context.getCacheDir(). What will happen when the OS removes all/parts of the files in cache directory? Does android delete files all togheter or it may delete some of them, probably that uid file, while keeping the reset.

File cacheDir = new File(context.getCacheDir(), "TrackPlayer");
DatabaseProvider db = new ExoDatabaseProvider(context);
cache = new SimpleCache(cacheDir, new LeastRecentlyUsedCacheEvictor(cacheMaxSize), db);
halaei commented 3 years ago

It seems we shouldn't do this, because at the very least it leads to storage leak by lost SQLite tables prefixed with uid. However, I see many people has done that. Take a look at your closed issues for instance: https://github.com/google/ExoPlayer/issues?q=is%3Aissue+context.getCacheDir+is%3Aclosed Also look at this popular React Native module: https://github.com/react-native-kit/react-native-track-player/blob/7ce3d08c9915a2d129a65c5f9427f16b264b7691/android/src/main/java/com/guichaguri/trackplayer/service/player/LocalPlayback.java#L47

Could you please either fix this issue or add a warning about it in the documentation?

ojw28 commented 3 years ago

It seems we shouldn't do this, because at the very least it leads to storage leak by lost SQLite tables prefixed with uid.

I would think this is pretty much negligible, in the grand scheme of things. It doesn't seem like a reason to not use the cache directory. It's actually fairly difficult to do something that's optimal for all cases here. If we were to co-locate the database/index inside the cache, then it would be much slower to modify it in some cases (e.g., when the cache directory is located on certain types of external storage). As soon as we don't co-locate the database/index, it's hard to know when to clean it up. Note that the cache no longer existing (i.e., UID file is missing) is not a great signal, because storage can be temporarily mounted, unmounted, and swapped in and out, for a variety of reasons. In particular for users who swap between SD cards, each of their SD cards may have a different non-empty cache directory, so even the UID actively changing is not a good signal for cleaning up the database/index.

For the cache files themselves, which are much larger, I think consistency will always be restored automatically, one way or another, regardless of which subset of the cache files are deleted by the underlying platform. So I don't think there's a significant problem here. That said, I think we can improve on what we do now by using tombstone behavior to prevent the UID file from being deleted (we may need to move the UID file into a sub-directory within the cache to do this, or just use an empty directory to represent the UID). This would avoid the leak you've identified in all cases other than SD card swapping, for which I think retaining the database/index content is the right thing to do, all things considered. Preventing UID deletion this way would also avoid the cache from being completely emptied when the platform only wanted to delete a small part of it, which I think may happen currently in the case that the small part that the platform deletes includes the UID file.

We will look into improving this. As noted above, I don't think it is a significant problem with the current implementation, so it should be fine to continue using the cache directory in the meantime.

halaei commented 3 years ago

Can storing uid in a sqlite table help? Maybe by dedicating a table that maps directories to uids? This way uid will never be deleted.

ojw28 commented 3 years ago

No. Storing the UID in an internal SQLite table doesn't achieve what the UID is there to do. The point of the UID is to uniquely identify the actual cache directory and its contents. For example, if the cache directory is on external storage and the user swaps the SD card, then we want the UID to change (note that the directory path is unchanged when this happens). Furthermore, if the original SD card is re-inserted later, we want to be able to recognise that. For this to work, the UID needs to be stored within the cache directory.