manolofdez / AsyncBluetooth

A small library that adds concurrency to CoreBluetooth APIs.
MIT License
168 stars 31 forks source link

“waitUntilReady” may have a bug. #31

Closed HannibalRoyal closed 1 year ago

HannibalRoyal commented 1 year ago

“waitUntilReady” may have a bug. Scenario: If CentralManager is an inherent property of an Actor, when calling waitUntilReady of CentralManager, the state may be incorrect, causing it to wait indefinitely. My solution is to add a 1s delay before the wait to give CentralManager enough initialization time.

Also, if possible, can timeout logic be added to waitUntilReady or can Executor be added with timeout settings?

manolofdez commented 1 year ago

HI! I'm not sure I follow. The CentralManager is not an Actor, only the waitUntilReadyExecutor. I think it might be more correct to pass in a conditional block to the enqueue function, so that after it awaits it checks the given conditional block before enqueuing. I haven't encountered this issue yet, but I'll put it in the backlog.

For now timeouts are not handled by the library. But yes, I hope to eventually add convenience overloads.

VincentBeltman commented 1 year ago

Hi! We've encountered this bug now too. Unfortunately we don't see this on all devices and it doesn't happen every time. We couldn't find the common denominator. I created a timeout mechanism around the waitUntilReady that retries it it times out. If the bug presents itself and the retry happens, the waitUntilReady always returns directly. I did walk through this repo trying to find out why this happens, but unfortunately your "magic" is too difficult for me :). Please let me know if you could use more information.

VincentBeltman commented 1 year ago

Did some extra digging. I guess there is a very small window inside waitUntilReady where first you check the state and afterwards you enqueue to the waitUntilReadyExecutor. If centralManagerDidUpdateState is called in between, then to my knowledge the executor will never be called (until the state changes, but that'd only happen when bluetooth is manually disabled/enabled). @manolofdez What do you think?

manolofdez commented 1 year ago

Yeah. Actually, it should only hold up that first call to waitUntilReady. If it changed before enqueuing it, subsequent calls should be able to continue because the waitUntilReady function will first check immediately before enqueuing.

That said, we can check whether Bluetooth is ready in the enqueued block to avoid waiting indefinitely on that first call. I'll merge this. If you can, please let me know if this fixes it.