Check for network connectivity before requesting resources

[zugaldia]

First attempt at adding an isConnected() method to MapboxAccountManager to avoid unnecessary HTTPRequests. We're using MapboxAccountManager to host this method because 1) it contains a Context instance, 2) it's easy to access statically via getInstance() and 3) is available before actual requests from the map are trigged.

Strangely, this works when an Internet connection is available but when a connection isn't available, the SDK crashes with the following exception:

08-23 14:32:05.104 17200-17273/com.mapbox.mapboxsdk.testapp W/com.mapbox.mapboxsdk.http.HTTPRequest: [HTTP] Request could not be executed: No Internet connection available.
08-23 14:32:05.105 17200-17273/com.mapbox.mapboxsdk.testapp A/libc: /Volumes/Android/buildbot/src/android/ndk-r12-release/ndk/sources/cxx-stl/llvm-libc++abi/libcxxabi/src/abort_message.cpp:74: void abort_message(const char *, ...): assertion "terminating with uncaught exception of type jni::PendingJavaException" failed
08-23 14:32:05.105 17200-17273/com.mapbox.mapboxsdk.testapp A/libc: Fatal signal 6 (SIGABRT), code -6 in tid 17273 (DefaultFileSour)
08-23 14:32:05.207 194-194/? A/DEBUG: *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
08-23 14:32:05.207 194-194/? A/DEBUG: Build fingerprint: 'google/hammerhead/hammerhead:6.0.1/MOB30Y/3067468:user/release-keys'
08-23 14:32:05.207 194-194/? A/DEBUG: Revision: '11'
08-23 14:32:05.207 194-194/? A/DEBUG: ABI: 'arm'
08-23 14:32:05.207 194-194/? A/DEBUG: pid: 17200, tid: 17273, name: DefaultFileSour  >>> com.mapbox.mapboxsdk.testapp <<<
08-23 14:32:05.207 194-194/? A/DEBUG: signal 6 (SIGABRT), code -6 (SI_TKILL), fault addr --------
08-23 14:32:05.242 194-194/? A/DEBUG: Abort message: '/Volumes/Android/buildbot/src/android/ndk-r12-release/ndk/sources/cxx-stl/llvm-libc++abi/libcxxabi/src/abort_message.cpp:74: void abort_message(const char *, ...): assertion "terminating with uncaught exception of type jni::PendingJavaException" failed'
08-23 14:32:05.242 194-194/? A/DEBUG:     r0 00000000  r1 00004379  r2 00000006  r3 9d453978
08-23 14:32:05.243 194-194/? A/DEBUG:     r4 9d453980  r5 9d453930  r6 0000000b  r7 0000010c
08-23 14:32:05.243 194-194/? A/DEBUG:     r8 9d453620  r9 7fffffff  sl ffffffff  fp aed6b2c0
08-23 14:32:05.243 194-194/? A/DEBUG:     ip 00000006  sp 9d453118  lr b6d2cb61  pc b6d2ef50  cpsr 400e0010
08-23 14:32:05.255 194-194/? A/DEBUG: backtrace:
08-23 14:32:05.255 194-194/? A/DEBUG:     #00 pc 00041f50  /system/lib/libc.so (tgkill+12)
08-23 14:32:05.255 194-194/? A/DEBUG:     #01 pc 0003fb5d  /system/lib/libc.so (pthread_kill+32)
08-23 14:32:05.255 194-194/? A/DEBUG:     #02 pc 0001c30f  /system/lib/libc.so (raise+10)
08-23 14:32:05.256 194-194/? A/DEBUG:     #03 pc 000194c1  /system/lib/libc.so (__libc_android_abort+34)
08-23 14:32:05.256 194-194/? A/DEBUG:     #04 pc 000174ac  /system/lib/libc.so (abort+4)
08-23 14:32:05.256 194-194/? A/DEBUG:     #05 pc 0001af23  /system/lib/libc.so (__libc_fatal+16)
08-23 14:32:05.256 194-194/? A/DEBUG:     #06 pc 00019549  /system/lib/libc.so (__assert2+20)
08-23 14:32:05.256 194-194/? A/DEBUG:     #07 pc 00395b7f  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so
08-23 14:32:05.256 194-194/? A/DEBUG:     #08 pc 00395c67  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so
08-23 14:32:05.256 194-194/? A/DEBUG:     #09 pc 00394251  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so
08-23 14:32:05.256 194-194/? A/DEBUG:     #10 pc 003942b1  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so (_ZSt9terminatev+44)
08-23 14:32:05.256 194-194/? A/DEBUG:     #11 pc 0004c020  /data/app/com.mapbox.mapboxsdk.testapp-1/lib/arm/libmapbox-gl.so

TODO

• Once the new logic is working, bring it to MapboxEventManager too.

This fixes #6042.

@ivovandongen @bleege I'd love your 👀 here.

/cc: @danswick

[mention-bot]

@zugaldia, thanks for your PR! By analyzing this pull request, we identified @jfirebaugh, @bleege and @tmpsantos to be potential reviewers.

[bleege]

I refactored things to use an IOUtils class instead of putting isConnected() in MapboxAccountManager for better separation of concerns. It's started at the same time as MapboxEventsManager is first started up in MapboxAccountManager so it'll be ready to go when HTTPRequest needs it. I've also refactored MapboxEventsManager to use the isConnected() method in IOUtils so the code base is using the same logic all around.

As for the stacktrace that's in the OP, I'm also seeing that too when I test the map tile requests but not when testing it in the MapboxEventsManager. The error message implies an issue with some Core GL code. From what I can see it's being triggered when HTTPRequest.onFailure() is called after the NoRouteToHostException is thrown during the isConnected() check. Specifically it looks like nativeOnFailure(type, errorMessage) is where things start going off the rails.

[ivovandongen]

@zugaldia Took a little look and it's a NullPointerException in HttpRequest#cancel().

Since the constructor gets aborted here the mCall is never set. When the destructor calls cancel here, a NullPointerException is thrown here.

Add in a null check on mCall in cancel() and you should be good.

On a side note, I'm not a big fan of things called *Util and try to avoid too many singletons. Can we think of a better name or just leave it in MapboxAccountManager for now as it is not a public api?

[zugaldia]

@bleege Thanks for bringing the implementation to the Telemetry.

@ivovandongen Thanks for finding the cause for the NullPointerException, I added the check you recommended in 773776d. Also, good point about calling it IOUtils, this might give the impression that it's a supported API while it isn't. I brought it back to MapboxAccountManager for now for the lack of a better place and added a note to make its private use more explicit.

Waiting for CI and for your final 👀 before merging.

[bleege]

On a side note, I'm not a big fan of things called *Util and try to avoid too many singletons.

Agreed. The thinking was to be consistent with the existing com.mapbox.mapboxsdk.utils API that contains AnimatorUtils, ColorUtils, and MathUtils though. That and while it's definitely convenient to locate it there, isConnected() isn't functionality that is logically related to what MapboxAccountManager is concerned about.

[zugaldia]

isConnected() isn't functionality that is logically related to what MapboxAccountManager is concerned about.

Right, maybe we could generalize MapboxAccountManager's definition (as MapboxManager?) to include this functionality, but that'd bring a breaking change to devs that we want to avoid in 4.2. Though the current situation isn't ideal, it seems better than potentially exposing a new API. For now, let's merge as this PR already tackles the necessary HTTPRequest change.