kable icon indicating copy to clipboard operation
kable copied to clipboard

Make disconnect timeout configurable instead of requiring use of `withTimeout`

Open twyatt opened this issue 3 years ago • 2 comments

Per documentation, when disconnect is called, it should be wrapped with withTimeout. This makes the API error-prone, as forgetting to do so can create very rare and hard to track down bugs.

It would be better to configure the disconnect timeout via the peripheral builder lambda (with a sane default).

twyatt avatar Jul 15 '21 19:07 twyatt

Should scanner also be considered? val advertisement = Scanner() .advertisements .first { it.name?.startsWith("Example") } Can potententially suspend indefinitelly and on the SensorTag project is actually also wrapped with withTimeoutOrNull(SCAN_DURATION_MILLIS)

Devinete avatar Jul 16 '21 08:07 Devinete

Should scanner also be considered?

Interesting idea, although with advertisements being a Flow it would be a bit more difficult to internally provide a timeout (as apposed to the Peripheral.disconnect method which is a suspend fun and therefore very easy to internally wrap its actions in withTimeoutOrNull).

Scanning (being powered by a Flow) also carries a familiar API so I feel is a bit less error prone? Being that it is familiar for developers to have to ultimately collect or use a terminal operator on a Flow, I think the potential pitfalls and ways to avoid them are well rehearsed. Whereas the disconnect() function conveys its behavior by its naming, so developers will read it as "the operation to disconnect a peripheral" but it isn't obvious that it could wait an indefinite amount of time — as I believe other similar connection-based paradigms (such as sockets) would not have a similar "potentially indefinite" behavior. On top of that, disconnect really shouldn't ever stall, but Android BLE is (as many are aware) a strange and unpredictable beast, so the timeout is a safeguard/defensive measure against Android not informing Kable that the disconnect was successful (i.e. in the event that Android never delivers the STATE_DISCONNECTED event). If Android's BLE API always behaved correctly, then it would not be an issue at all (Kable would always get the signal that disconnect succeeded and would never stall). But since number of BLE connections on Android is very limited, the timeout prevents connection leaks in the event Android BLE API misbehaves.

Additionally, there may eventually be an official Flow timeout operator (https://github.com/Kotlin/kotlinx.coroutines/issues/2624)?

Sorry for the long-winded response, that is all to say, I'm not apposed to the idea of offering a built in timeout for scanning, but it'll likely be something to revisit further down the road.

twyatt avatar Jul 16 '21 08:07 twyatt