ResourcesCachePathChangeCallback is sporadically not returning

[arnekaiser]

Sometimes the ResourcesCachePathChangeCallback is not returning. After some debugging I found out, that the FileUtils.OnCheckFileWritePermissionListener is gone (null) in the FileUtils.CheckFileWritePermissionTask. Looks like a strange JNI behavior.

The problem occurs more often when the resources cache path should be changed directly after the initialization of Mapbox and also on older devices.

Steps to reproduce

1. Initialize Mapbox
2. Set custom resource cache path

Expected behavior

The cache path should be set every time.

Actual behavior

Sometimes the cache path isn't set and the callback doesn't return.

Configuration

Android versions: all
Device models: all
Mapbox SDK versions: 7.3.0

[tobrun]

@arnekaiser tracked this down to the callback being GC'ed in some edge cases as it was wrapped internally by a weak reference. Solution for now is to avoid using an anonymous inner class but instead keeping a reference scoped to the outer class instead (this will prevent GC'ing it).

That said, we should revisit how we implement callbacks to avoid behaviors as shown in OP. Some notes on this in #14515 (comment).

[Guardiola31337]

The problem here is that the GC is non-deterministic and how we're storing WeakReferences in FileUtils tasks

mapbox-gl-native/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/utils/FileUtils.java

Line 20 in d25ae1c

private final WeakReference<OnCheckFileReadPermissionListener> listenerWeakReference;

mapbox-gl-native/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/utils/FileUtils.java

Line 77 in d25ae1c

private final WeakReference<OnCheckFileWritePermissionListener> listenerWeakReference;

but not retaining them in the public APIs from OfflineManager e.g.

mapbox-gl-native/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/offline/OfflineManager.java

Lines 226 to 254 in 0fcab65

	public void mergeOfflineRegions(@NonNull String path, @NonNull final MergeOfflineRegionsCallback callback) {
	final File src = new File(path);
	new FileUtils.CheckFileReadPermissionTask(new FileUtils.OnCheckFileReadPermissionListener() {
	@Override
	public void onReadPermissionGranted() {
	new FileUtils.CheckFileWritePermissionTask(new FileUtils.OnCheckFileWritePermissionListener() {
	@Override
	public void onWritePermissionGranted() {
	// path writable, merge and update schema in place if necessary
	mergeOfflineDatabaseFiles(src, callback, false);
	}
	@Override
	public void onError() {
	// path not writable, copy the the file to temp directory, then merge and update schema on a copy if
	// necessary
	File dst = new File(FileSource.getInternalCachePath(context), src.getName());
	new CopyTempDatabaseFileTask(OfflineManager.this, callback).execute(src, dst);
	}
	}).execute(src);
	}
	@Override
	public void onError() {
	// path not readable, abort
	callback.onError("Secondary database needs to be located in a readable path.");
	}
	}).execute(src);
	}

i.e. there's no way to retain the callback from the client side because callbacks are implemented as inner classes, storing the content and not the reference.

In order to solve this we could:

• Refactor all these callbacks into retained properties (as you mentioned) + add docs so clients are aware of that - forcing the caller to retain callbacks it's not a common practice so IMO this is not ideal
• Remove the WeakReferences and reset the callbacks when onCancelled and onPostExecute so they're properly GC'ed

Both seem that could be implemented without adding any breaking changes and could be included in a patch release.

👀 https://www.pacoworks.com/2016/07/26/dissecting-callbacks-a-murder-mystery/

[tobrun]

your suggestions are valid @Guardiola31337 but they don't take in account that FileSource#setResourcesCachePath is static so you don't have a hook into instance properties. Feel free to take it for a spin but haven't found a way to not leak it. We might be able to manage this statically but have been trying to put that off.

[Guardiola31337]

We took this for a spin in #14589

When you have a chance @tobrun let us know what you think