search

ginkage / wearmouse

Wear OS Air Mouse
Apache License 2.0
193 stars 39 forks source link

Non-determinism in PairedDevicesFragment #11

Closed fmeum closed 3 years ago

fmeum commented 4 years ago

During the investigation of Bluetooth connectivity issues reported against WearAuthn, I took a deeper look into the Bluetooth HID backend more or less shared between WearMouse and WearAuthn. This lead to https://github.com/FabianHenneke/WearAuthn/commit/fe96b23b9ed90860bf418f5fe8d760d2cd147358, which appears to fix at least some of the issues by removing a source of non-determinism. Since a similar change could be implemented in WearMouse and might help prevent some edge-case bugs, I briefly want to describe my findings here.

The cause of the non-determinism is the lack of synchronization between the call to HidDataSender.register here and the user-initiated calls to HidDataSender.requestConnect here and elsewhere in that file. Usually, since this requires the user to tap at least two buttons, the HidDataSender will be ready to receive connections by the time the user requests a connect, but this is not certain and failed in slightly different scenarios in WearAuthn.

Based on my experiments, requestConnect only works reliably when it is called after both onServiceStateChanged and onAppStatusChanged have fired with parameters that are non-null and true, respectively. This suggests the following fix for the non-deterministic behavior:

  1. Ensure that requestConnect is idempotent to prevent any (theoretical) races.
  2. Gate the requestConnect call on both onServiceStateChanged and onAppStatusChanged having fired positively by having each listener check whether the other has already fired and only then calling requestConnect.
  3. If both listeners have already fired, requestConnect can safely be called synchronously as before.

Please let me know if I should have missed anything or introduced a bug elsewhere.

ginkage commented 4 years ago

Wow. Thank you for the detailed investigation. I haven't touched the code for a while, but it seems that it's worth implementing these fixes. I'll get to it shortly. Thanks again!

fmeum commented 4 years ago

This made a big difference for the WearAuthn watch face complication (which can be unlocked from the phone app), which previously called register and requestConnect right after each other. The fix described above is included in version 0.9.16, so if you want to experience the difference it makes, you can give it a try.

ginkage commented 4 years ago

Gate the requestConnect call on both onServiceStateChanged and onAppStatusChanged having fired positively by having each listener check whether the other has already fired and only then calling requestConnect.

Note that onAppStatusChanged(true) can only be called after onServiceStateChanged(non-null proxy), since that's where hidDeviceApp.registerApp(proxy) is called. So, I'm going to go for a simpler fix on my side. :)

fmeum commented 4 years ago

Gate the requestConnect call on both onServiceStateChanged and onAppStatusChanged having fired positively by having each listener check whether the other has already fired and only then calling requestConnect.

Note that onAppStatusChanged(true) can only be called after onServiceStateChanged(non-null proxy), since that's where hidDeviceApp.registerApp(proxy) is called. So, I'm going to go for a simpler fix on my side. :)

Thanks for confirming this. I wanted to play it safe given that I only have a single watch to test on, but that seems reasonable. I will simplify the code in a future release.

ginkage commented 4 years ago

So, I've submitted f2ac55e4fc749860e5227810f0aa91b7d3d68850 - have a look. It should probably be enough.

fmeum commented 4 years ago

I like your approach for its improved encapsulation. I will see whether I can adapt this for WearAuthn.