yschimke
commented
2 years ago Apologies if I'm missing some simple way of deferring the SimpleCache being built until off the main thread?
Open yschimke opened 2 years ago
yschimke
commented
2 years ago Apologies if I'm missing some simple way of deferring the SimpleCache being built until off the main thread?
marcbaechinger
commented
2 years ago Thanks for reporting!
I think we are aware of that. :) Guess we need to discuss whether we want to fix this with a Supplier or similar. I'm not sure how bad or urgent that actually is though. Obviously, strict mode doesn't like this but I'm unsure what the actual impact of performance is. I for instance don't think this case would cause an ANR for apps. The file access that triggered this exception is a file.exists().
Strictly speaking strict mode is right of course. :)
From 'StrictMode' API doc: But don't feel compelled to fix everything that StrictMode finds. In particular, many cases of disk access are often necessary during the normal activity lifecycle.
yschimke
commented
2 years ago My workaround is
override val cacheDir: File by lazy {
StrictMode.allowThreadDiskWrites().resetAfter {
application.cacheDir
}
}
// Confusingly the result of allowThreadDiskWrites is the old policy,
// while allow* methods immediately apply the change.
// So `this` is the policy before we overrode it.
fun <R> StrictMode.ThreadPolicy.resetAfter(block: () -> R) = try {
block()
} finally {
StrictMode.setThreadPolicy(this)
}I don't think it would be too hard to update the samples to avoid strict mode violations, and would demonstrate better app higiene. But there were other things in the samples that triggered it also.
I think solving just the SimpleCache API would be useful.
The samples may not warrant it, but I expect every high volume media app should really be doing this themselves and SimpleCache is just another hurdle to workaround.
yschimke
commented
2 years ago Also for release.
downloadCache.release()Not sure it can be avoided.
ziem
commented
9 months ago I would like to bump this issue. The topic was recently mentioned by an engineer from Reddit in the article below:
https://www.reddit.com/r/RedditEng/comments/1af2d8d/improving_video_playback_with_exoplayer/
We hit the same problem they mentioned. It would be great to have some sort of built-in solution for this problem. It's somewhat hidden and hard to discover.
marcbaechinger
commented
9 months ago https://www.reddit.com/r/RedditEng/comments/1af2d8d/improving_video_playback_with_exoplayer/
Sorry, I can't see that page because they don't want to show me because of "Your request has been blocked due to a network policy."
It would be great to have some sort of built-in solution for this problem
What is the solution they are suggesting in the article?
marcbaechinger
commented
9 months ago Found someone that sent me the article. Thanks!
The proposed solution to this problem is instantiating SimpleCache in a background thread.
As far as I can tell, the code that causes the problem is calling cacheDir.getAbsoluteFile() that is called to add the path of the chosen cache directory to a set of used directory paths to make sure that the same directory isn't used multiple times by multiple SimpleCaches. If it finds the directory is already used, then the app is crashed with an IllegalArgumentException.
The reason for this procedure is to make sure that SimpleCache doesn't intervenes with another cache using the same directory. If this would slip though, then the cache directory of both caches may be corrupted or simple cleaned unexpectedly.
It would be great to have some sort of built-in solution for this problem.
Yeah, totally agreed. It's not obvious though how to do that without compromising developer experience.
Thinking out load about a long list of potential solutions:
A builder that does the creation on a different thread would do that, but then the the app would possibly have to wait until the cache is created before a source (or the player) that wants to use the cache can be created. The reddit article says this is acceptable though.
We could just drop this check and hope that apps don't do it wrong. We would probably have other issues popping up here when doing that. So I don't think this is an option.
Another idea is certainly that (only) this check could be made on a different thread and crash the app from there if needed. However, this would mean that the SimpleCache could already have started corrupting the cache directory when we find there is a problem. Given, this would potentially only happen at dev time (which seems the only case in which an app-crashing check is sensible), that maybe acceptable.
@ziem What did you do in your app and would this be an additional option to my reasoning above?
ziem
commented
8 months ago We created a simple cache wrapper that fall-backs to default values as long as the simpleCache variable is not initialized. It's by no means a perfect solution, but in our case, it's good enough as the interaction with the cache happens later in the application flow.
@Singleton
class CacheWrapper @Inject constructor(
@ApplicationContext private val context: Context,
applicationScopeProvider: ApplicationScopeProvider,
private val databaseProvider: DatabaseProvider,
) : Cache {
private var simpleCache: Cache? = null
init {
applicationScopeProvider.applicationScope.launch(Dispatchers.IO) {
simpleCache = createSimpleCache()
}
}
private fun createSimpleCache(): Cache {
var downloadDirectory = context.getExternalFilesDir(null)
if (downloadDirectory == null) {
downloadDirectory = context.filesDir
}
val downloadContentDirectory = File(downloadDirectory, DOWNLOAD_PODCAST_DIRECTORY)
return SimpleCache(downloadContentDirectory, NoOpCacheEvictor(), databaseProvider)
}
override fun getUid(): Long {
return simpleCache?.uid ?: 0L
}
override fun release() {
simpleCache?.release()
}
...
}
The reason for this procedure is to make sure that SimpleCache doesn't intervenes with another cache using the same directory. If this would slip though, then the cache directory of both caches may be corrupted or simple cleaned unexpectedly.
We (devs) control this directory as we pass a cacheDir when creating a SimpleCache instance. It's our responsibility to make sure it's correct, so for me the last option works fine. I also think that in this case any valid solution is better than making users experience ANRs due to the current (slightly hidden) behavior of the SimpleCache.
A commit with test example
https://github.com/yschimke/media/commit/c9cbe959173b23c92075c7fdc4238fd01c90e019
It seems that SimpleCache will always require a file from the application context, like context.filesDir. This triggers a strictmode violation.
This happens in the demo app in an activity, and would in the service for demo-session if it used a cache.
Apart from this it seems like most of ExoPlayer is setup to correctly defer any network or I/O work. Could the SimpleCache be changed to support a deferred File, e.g. a Provider param, before it's actually used?