if I’m not mistaken (and I haven’t looked into the code beyond the initDirectories), that would change the DB Path for the 99% of people who don’t have the issue and already have a database instance set up in the sandbox (since you are passing in the external dir to use for Both paths if it exists).
It only changes the temp directory path used by SQLite.
Are you saying both of these are only temp paths? (and that changing the folder passed in that dbPath gets set to won’t affect an existing database that had dbPath set to something else before?)
private static void initDirectories(@Nullable String rootDirPath) {
final String dbPath = makeDbPath(rootDirPath);
final String tmpPath = makeTmpPath(rootDirPath);
I just saw this crash again on another app, OnePlus8Pro - Android 11, CBL 3.0.2.
I solved it in my Kotlin Multiplatform bindings like this:
fun init(ctxt: Context, debug: Boolean) {
init(
ctxt,
debug,
ctxt.filesDir,
ctxt.getExternalFilesDir(CouchbaseLiteInternal.SCRATCH_DIR_NAME)
?: File(ctxt.cacheDir, CouchbaseLiteInternal.SCRATCH_DIR_NAME)
)
}
@blake.meike I really think a solution like this would make sense in the Android SDK, rather than assuming ctxt.getExternalFilesDir(CouchbaseLiteInternal.SCRATCH_DIR_NAME) is non-null, which the API states is nullable, and requiring developers to handle when they experience the crash. Using Context.getCacheDir() ensures the user’s sandbox is not filled, as it will be cleaned up by the OS if it uses too much space.
It may even make sense to use getExternalCacheDir() as the default, instead of the current getExternalFilesDir(), which could just as easily cause the user’s external storage to fill up with these SQLite scratch files, as it could internal storage using getFilesDir(). And on most modern Android devices, there’s no difference between the external and internal storage volumes. This can be checked with Environment.isExternalStorageEmulated() and just use the internal storage if true (since it’s the same volume anyway).
This could look something like this:
fun init(ctxt: Context, debug: Boolean) {
val cacheDir = ctxt.externalCacheDir?.let { extDir ->
if (!Environment.isExternalStorageEmulated(extDir)) extDir else null
} ?: ctxt.cacheDir
init(
ctxt,
debug,
ctxt.filesDir,
File(cacheDir, CouchbaseLiteInternal.SCRATCH_DIR_NAME)
)
}
This is just a hairy problem. Here’s my analysis:
First, let me describe what these directories are:
- root dir: this is where databases go, by default. It is just the default. If you wanna put a db elsewhere, it is trivial to do so.
- scratch dir: I’m not even sure what this is for. It is not used by any Couchbase code, at all. SQLite needs it for something…
- cbl scratch: this is a directory over which client code has no control (as of 3.0). It is a canonical directory inside <root dir>. It must be so, because LiteCore depends on hard linking to files in it, which cannot cross volumes. Up until v3.0, the scratch dir and the cbl scratch were often confused, in platform code.
Most Android devices, up to v30, are handled correctly by the current code. It uses the package sandbox for database default location and uses the applications owned, external files directory, for scratch files. The latter is appropriate (especially since I am not certain how SQLite uses the directory) because access to it is limited (not leaking sensitive information), the files are deleted along with the owning application, and the contents are not subject to pruning.
This seems to me to be the right choice for most Android devices.
The problem, as you aptly point out, is that this does not cover all Android devices. Your suggestion, which, I believe, boils down to:
cacheDir = ctxt.cxternalCacheDir
if ((cacheDir == null) || (!Environment.isExternalStorageEmulated(cacheDir))
cacheDir = ctxt.cacheDir
is pretty slick… If it works for you, that’s awesome. I have a couple of reasons for not making it the standard:
- It uses the cacheDir, which is pruned. I can’t be confident that that works.
- Why so complicated? The docs say:
If a shared storage device is emulated (as determined by
Environment.isExternalStorageEmulated(File)), its contents are backed by a private user data
partition, which means there is little benefit to storing data here instead of the private directory
returned by getCacheDir().
… so just use cacheDir
The whole reason I exposed all of this stuff, in 3.0, is so that, if you know what you are doing, and you have to accommodate special devices, you can do whatever you need to do, to handle that device. For me to try to guess what that is just seems like a bad idea.
I will consider using the cache dir, instead of the external files dir. I need to do a little research on how SQLite uses the directory, but will use it instead, if it seems safe.
Thanks for those details. My effort is to help identify a proper fix for the library code, such that it works properly for all Android devices, all the time. The issue boils down to handling the nullability of the getExternalFilesDir() API, which is used for the (SQLite) scratch dir, with a safe default that makes sense for the majority of users.
This is the stack trace in CBL 3.0.2:
Fatal Exception: java.lang.IllegalArgumentException: directory must not be null
at com.couchbase.lite.internal.utils.Preconditions.assertNotNull(Preconditions.java:80)
at com.couchbase.lite.internal.utils.FileUtils.verifyDir(FileUtils.java:42)
at com.couchbase.lite.internal.CouchbaseLiteInternal.init(CouchbaseLiteInternal.java:96)
at com.couchbase.lite.CouchbaseLite.init(CouchbaseLite.java:53)
at com.couchbase.lite.CouchbaseLite.init(CouchbaseLite.java:39)
at com.couchbase.lite.CouchbaseLite.init(CouchbaseLite.java:33)
at com.couchbase.lite.kmp.CouchbaseLite.init(CouchbaseLite.kt:12)
at org.timpfest.android.TimpFestApplication.onCreate(TimpFestApplication.kt:26)
at android.app.Instrumentation.callApplicationOnCreate(Instrumentation.java:1194)
at android.app.ActivityThread.handleBindApplication(ActivityThread.java:6719)
at android.app.ActivityThread.access$1300(ActivityThread.java:237)
at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1913)
at android.os.Handler.dispatchMessage(Handler.java:106)
at android.os.Looper.loop(Looper.java:223)
at android.app.ActivityThread.main(ActivityThread.java:7664)
at java.lang.reflect.Method.invoke(Method.java)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:592)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:947)
I suggested using the cache directory because you seemed to initially have the concern that using the app’s files directory would “present the threat of filling the sandbox space”.
But the needs you just described:
- access to it is limited (not leaking sensitive information)
- the files are deleted along with the owning application
- and the contents are not subject to pruning
seem to better describe the app’s files directory, rather than the cache directory.
The external files directory actually doesn’t fit the first requirement as well as internal storage does. The app’s external files directory path is /<external/storage/root>/Android/data/<app.package>/files/CouchbaseLiteTemp, which is accessible to the user and any application with permission to read external storage. Only the app’s internal files directory is truly inaccessible to other apps and non-rooted users.
Based on the SQLite documentation, it doesn’t seem like these scratch files should be long-lived and certainly shouldn’t grow indefinitely. These are the default locations SQLite uses.
If you prefer the scratch directory be safe from pruning and limit access, I’d suggest just using the internal files directory to best fit those requirements you described. It’s also guaranteed to be non-null, so it wouldn’t need a safe alternative as a backup.
public static void init(@NonNull Context ctxt, boolean debug) {
init(ctxt, debug, ctxt.getFilesDir(), ctxt.getDir(CouchbaseLiteInternal.SCRATCH_DIR_NAME, Context.MODE_PRIVATE));
}
or alternatively, if external storage is preferred as a default when available, despite access not being completely limited:
public static void init(@NonNull Context ctxt, boolean debug) {
File scratchDir = ctxt.getExternalFilesDir(CouchbaseLiteInternal.SCRATCH_DIR_NAME);
if (scratchDir == null || Environment.isExternalStorageEmulated(scratchDir)) {
scratchDir = ctxt.getDir(CouchbaseLiteInternal.SCRATCH_DIR_NAME, Context.MODE_PRIVATE);
}
init(ctxt, debug, ctxt.getFilesDir(), scratchDir);
}
This is similar to my Kotlin code above, only using the files directory instead of cache directory. Your Java conversion had the isExternalStorageEmulated() check backwards. The intention is to use the external directory, if it’s not null and not emulated, with the internal directory used if either is true, either as a safe fallback or because it’s the same volume anyway.