`connectedAndInstalledNodes` throws into the outer scope when API is not available

[Nek-12]

Excuse me, but why is the method isAvailable(): Boolean throwing ApiNotAvailable exception?

Non-fatal Exception: com.google.android.gms.common.api.ApiException: 17: API: Wearable.API is not available on this device. Connection failed with: ConnectionResult{statusCode=API_UNAVAILABLE, resolution=null, message=null}
       at com.google.android.gms.common.internal.ApiExceptionUtil.fromStatus(ApiExceptionUtil.java:3)
       at com.google.android.gms.common.internal.zap.onComplete(com.google.android.gms:play-services-base@@18.4.0:4)
       at com.google.android.gms.common.api.internal.BasePendingResult.zab(com.google.android.gms:play-services-base@@18.4.0:7)
       at com.google.android.gms.common.api.internal.BasePendingResult.setResult(com.google.android.gms:play-services-base@@18.4.0:6)
       at com.google.android.gms.common.api.internal.BaseImplementation$ApiMethodImpl.setFailedResult(BaseImplementation.java:5)
       at com.google.android.gms.common.api.internal.zae.zad(com.google.android.gms:play-services-base@@18.4.0:1)
       at com.google.android.gms.common.api.internal.zabq.zaF(com.google.android.gms:play-services-base@@18.4.0:7)
       at com.google.android.gms.common.api.internal.zabq.zaE(com.google.android.gms:play-services-base@@18.4.0:2)
       at com.google.android.gms.common.api.internal.zabq.zar(com.google.android.gms:play-services-base@@18.4.0:23)
       at com.google.android.gms.common.api.internal.zabq.onConnectionFailed(com.google.android.gms:play-services-base@@18.4.0:1)
       at com.google.android.gms.common.internal.zai.onConnectionFailed(zai.java:1)
       at com.google.android.gms.common.internal.zzf.zzb(com.google.android.gms:play-services-basement@@18.5.0:2)
       at com.google.android.gms.common.internal.zza.zza(zza.java:7)
       at com.google.android.gms.common.internal.zzc.zze(zzc.java:3)
       at com.google.android.gms.common.internal.zzb.handleMessage(com.google.android.gms:play-services-basement@@18.5.0:31)
       at android.os.Handler.dispatchMessage(Handler.java:107)
       at android.os.Looper.loop(Looper.java:237)
       at android.os.HandlerThread.run(HandlerThread.java:67)        

The only method of Horologist we haven't wrapped in a try/catch in the entire codebase was isAvailable() and the constructors.

[Nek-12]

Devices reproducing: Galaxy s22 ultra A14, galaxy s20 ultra A13, Xiaomi 11 lite 5g NE A13, Galaxy note 9 A10, galaxy s 20 5g, galaxy S9 (A10),

[yschimke]

I'll test, that's strange.

The point of that call is to call it to check before using other APIs.

https://github.com/google/horologist/blob/main/datalayer%2Fcore%2Fsrc%2Fmain%2Fjava%2Fcom%2Fgoogle%2Fandroid%2Fhorologist%2Fdata%2Fapphelper%2FDataLayerAppHelper.kt#L250

Any GMS call can fail, but isAvailable needs to return true or false.

[Nek-12]

It could be the class constructor as well?

[Nek-12]

Also found that we called WearDataLayerClient.messageClient.addListener() without a try catch. Could it throw in addListener or removeListener?

[yschimke]

What I'll suggest, I'll add a decorator to all calls by Horologist to the Wearable APIs to catch and rethrow with a useful stacktrace attached.

That should make it easier to find the problem call.

One thing you could do is via DI guard all access to the Wearable APIs or Horologist datalayer with an isAvailable check. So you cant accidentally call these APIs.

[Nek-12]

I've identified the cause - it is the connectedAndInstalledNodes property that throws into the outer scope that is the culprit. The flow itself throws. I never expected this innocent-looking flow to throw both in the getter(?) and when emitting values.

I don't think it should stay that way. Neither a property, accessing which can throw, nor the throw in the flow are good practices, especially considering that this behavior is undocumented

[yschimke]

Yep, it definitely shouldn't. Thanks for the investigation. I'll try to fix today.

But I'm working through a migration of OSSRH before I can publish.

[yschimke]

The getter shouldn't throw. But I think the flow arguably should throw as it can't return a Set<Node> if Wearable.API are not available.

Maybe we could be tolerant and just make that an empty set.

But I think the expectation is calling isAvailable before calling that flow. It's not a field, just a getter method.

[Nek-12]

The flow being a field (the clients don't know it's computed) gives an impression of safe to call code. No throws annotation or docs on that behavior were present. An intuitive API would involve something like getConnectedNodesAsFlow() which can throw, or a property with a sealed type Set|Error.

An empty set would hide the reason that the set is empty, I'm against that.

[yschimke]

Yep, we should fix. I'll take a look.

[yschimke]

I went to fix this, but I can't see how just calling the method would make it fail?

It's a lazy property, returning a callbackFlow. None of the logic should run before the callback flow is started. It should be safe to call the field, just not start the flow.

[Nek-12]

I indeed started this flow. A collector from connectedAndInstalledNodes threw when I attempted to subscribe.

I wasn't sure about a getter initially, but I since found another place where code throws (when attempting to send a message to the event channel (but that isn't related to horologist)). I apologize for misleading you the first time, I made an assumption that the isAvailable throws because the stacktrace doesn't point anywhere, and isAvailable is the first method I call (directly next to connectedAndInstalledNodes)

[yschimke]

Yeah, I'll try to follow up to make this more obvious and easy to debug. But closing this for now.