`OfflineManager.loadStylePack` does nothing when invoked from worker thread

[mhelder]

Environment

• Android OS version: Any
• Devices affected: All
• Maps SDK Version: 10.12.0

Observed behavior and steps to reproduce

1. Invoke OfflineManager.loadStylePack on any other thread than the main/UI thread.
2. Observe nothing happens: none of the provided callbacks are ever hit and no error is raised anywhere either.

In its most simple form:

Thread {
    offlineManager.loadStylePack(
        styleURI,
        loadOptions,
        { progress -> ... },
        { stylePack -> ... }
    )
}

In reality, our setup uses Rx to combine downloading a style pack and tile region. The resulting stream is subscribed on using the IO scheduler. We noticed that the tile region download would start just fine, but nothing ever happened with the stream propagating the style pack updates.

Expected behavior

Downloading the style pack should start and the callbacks should be hit. Alternatively, an error should be raised indicating this function should be called from the main thread.

Notes / preliminary analysis

In my opinion, if OfflineManager's functions are required to be invoked from the main thread, this requirement should also be made very clear, in docs and using appropriate annotations. I actually noticed that the legacy OfflineManager at least did the latter:

https://github.com/mapbox/mapbox-gl-native-android/blob/6db4db9fffb7a696feb92438fa4bb6b21b65d117/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/offline/OfflineManager.java#L35-L36

The new OfflineManager and OfflineManagerInterface do not. So presumably a regression in the update from v9 -> v10?

Because docs and annotations only get you so far, I'd really advocate to fail hard and fast if developers don't consume the SDK as intended. It looks like ThreadChecker could be useful for this?

[kiryldz]

@mhelder thanks for detailed description and the valid point. The problem with OfflineManager is that this class is owned by our internal library and thus we could not easily add @MainThread annotation there (or any other logic). I'll drop the request to support it (without strict timeline though). Also you correctly mentioned ThreadChecker - enabling this in manifest will help you catch WorkerThreadException() in majority of other places in the SDK (but better disable that for release build). Overall rule - all methods need to be invoked from main thread, unless docs clearly say otherwise.

[mhelder]

Overall rule - all methods need to be invoked from main thread, unless docs clearly say otherwise.

Cool, so can we have this explicitly and clearly mentioned in the docs somewhere? That would already be a useful step in the right direction (though I still think enforcing it is what you should aim for).

[richardpineo]

This is also an issue with mapbox-maps-iOS. The behavior is very confusing because the completion function is never called with no explanation. It would not be hard to check the calling thread and throw an error if called from the wrong thread which would at least provide some information.

I found this issue from this stack overflow: https://stackoverflow.com/questions/70558858/mapbox-migrating-to-v10-1-offlinemanager-loadstylepack-does-not-complete-or-retu