Leverage structured concurrency for connection handling #749

twyatt · 2024-09-12T23:01:36Z

Summary

Major changes include:

Updated JavaScript to use SharedRepeatableAction
- Now all platforms utilize SharedRepeatableAction (for triggering connect/disconnect)
Peripherals are no longer children of a CoroutineScope
- Their lifecycle now must be explicitly ended via cancel
Peripherals are themselves CoroutineScopes
- Tasks may be launched from Peripherals when the lifecycle of the task is intended to match the Peripherals lifecycle
Peripheral.connect now returns a CoroutineScope
- Tasks that are intended to match the current connection's lifecycle may be run from this CoroutineScope
Simplified exception hierarchy
- On the path to extending either IOException or IllegalStateException for most exceptions (Restructure exception hierarchy #724)

Scoped connections

Connection and I/O management, as well as disconnect process was moved to Connection class (for every platform). The Connection class manages any context switching necessary for I/O and ensuring proper teardown on disconnect. This simplifies connection lifecycle management by leveraging coroutine's structured concurrency for connection lifecycle (proper teardown on either disconnect or peripheral scope completing).

We no longer have to do a Deferred dance (state management) when performing I/O (when cancelled by caller). When I/O goes through Connection.execute (which is scoped to the Connection's CoroutineScope), if caller cancels I/O request, then we can continue (asynchronously) waiting for the response (rather than trying to drop the response on the next I/O operation).

The following chart shows the approximate relationship between the various CoroutineScopes within a Peripheral:

flowchart TD
    
    peripheralScope:::scope --> connectionScope:::sscope
    connectionScope --> taskScope
    connectionScope --> action:::sscope

    subgraph Peripheral
    peripheralScope:::scope
    
    subgraph Connection
    connectionScope:::sscope
    taskScope:::sscope
    end
    
    disconnect -.-x connectionScope
    action -.-o Connection
    connect -..-> action
    
        
    end

    peripheralScope("peripheralScope (SupervisorJob)")
    action("action (Deferred)")
    taskScope("taskScope (SupervisorJob)")
    connectionScope("connectionScope (Job)")

    Connection:::klazz
    
    classDef scope fill:#33f
    classDef sscope fill:#a3c
    classDef klazz fill:#333

    linkStyle 0 stroke-width:2px,fill:none,stroke:white;
    linkStyle 1 stroke-width:2px,fill:none,stroke:white;
    linkStyle 2 stroke-width:2px,fill:none,stroke:white;
    linkStyle 3 stroke-width:2px,fill:none,stroke:red;
    linkStyle 4 stroke-width:2px,fill:none,stroke:green;
    linkStyle 5 stroke-width:2px,fill:none,stroke:white;

When Peripheral.connect is called, the CoroutineScope tree (shown in pink) is spun up (via SharedRepeatableAction). The SharedRepeatableAction creates a CoroutineScope (which becomes the Connection's connectionScope) and a child Deferred (which is executed as an action within the Peripheral.connect function). In the action, the Connection is created (using the connectionScope) and immediately a child CoroutineScope (which is shown as taskScope) is created.

In summary:

connectionScope is used for asynchronously executing I/O operations
taskScope is provided to library consumers to launch "tasks" that should match the lifecycle of the connection
action runs the connection process (e.g. connect, discover services, etc)

Some characteristics of this CoroutineScope tree that are pertinent:

A failure of connectionScope will shutdown the connection CoroutineScope tree (pink elements)
Tasks (coroutines launched from taskScope) are allowed to fail w/o shutting anything else down
A failure in action (during connect process) will shutdown the connection CoroutineScope tree (pink elements)

When Connection is created, it registers a Job completion listener on the connectionScope which properly disposes of the underlying peripheral (e.g. on Android, the BluetoothGatt is closed) — this ensures that after the connection CoroutineScope tree has completed that the underlying BLE objects are always properly disposed.

Partially addresses #724
Partially addresses #721
Closes #622
Closes #567
Closes #378
Closes #177
Closes #144
Closes #497
Closes #286
Closes #168

twyatt · 2024-09-18T09:24:11Z

kable-core/src/androidMain/kotlin/BluetoothDeviceAndroidPeripheral.kt

        } catch (e: Exception) {
-            closeConnection()


We no longer have to explicitly close the connection, because it will be closed implicitly when the connection coroutine scope tree completes. Since the connection process runs within a Deferred that is a child of the connection coroutine scope tree, any failure during connect will implicit close down the Connection.

twyatt · 2024-09-18T09:24:32Z

kable-core/src/androidMain/kotlin/BluetoothDeviceAndroidPeripheral.kt

            throw failure
        }

        logger.info { message = "Connected" }
        _state.value = State.Connected

-        state.watchForConnectionLossIn(scope)


This is performed in the Connection now.

kable-core/src/androidMain/kotlin/gatt/Callback.kt

kable-core/src/appleMain/kotlin/CentralManager.kt

Phoenix7351 · 2024-09-18T18:50:08Z

kable-core/src/appleMain/kotlin/CentralManagerDelegate.kt

Thought there were going to be changes to didUpdateState here?

I don't follow. Can you elaborate?

Was referring to the flow that tells the consumer the BLE state. Doesn't sound like it's happening.

Since forwarding state-updates is longer living than each connection, those events are forwarded from the central singleton (to each peripheral) here (within the Peripheral's coroutine scope), with the state being available here.

Perhaps there was confusion re: removal of the connectionState flow?
There was previously connectionEvents and connectionState, I decided that having a dedicated flow (connectionState) for the connection events was unnecessary, and now they go through connectionEvents (along with the other connection events).

Phoenix7351 · 2024-09-18T18:57:15Z

kable-core/src/appleMain/kotlin/bluetooth/CheckBluetoothIsOn.kt

+/**
+ * @throws UnmetRequirementException If [CentralManager] state is not [CBManagerStatePoweredOn].
+ */
+internal fun CentralManager.checkBluetoothIsOn() {


Not sure if this file would be the right place, but it would be really useful to have a flow of the CBManagerState itself. Is that factored into your plan somewhere?

Sadly, I've decided that Kable should not provide real-time (i.e. flow based) bluetooth state for consumers, at least not in the core module (perhaps a separate module or library 🤷) — it is too difficult to provide a unified API.

I'm trying to drive Kable in a direction where the entire API can be used from common source sets (write once, use everywhere paradigm).

In this particular instance: CBManager* states encompass:

is BLE supported or not

is BLE powered on or not

does user have permission to use BLE

Whereas Android has the three above bullets available independently (and there is no live-updating permission info available on Android — it is on-demand only).

If CBManager* states didn't include the permission aspect, then this would be trivial to provide a unified API (of a flow of "is BLE powered on or not"), but since Apple doesn't separate those states, means that I just can't provide such an API.

So, we're left with only being able to provide an on-demand approach. Which sadly, I may even need to abandon this; as I've found that my previous findings of "being able to suppress the dialog when using BLE" isn't actually possible on Apple (I was previously mistaken on my findings: I am able to suppress for BLE on/off dialog, but permissions always comes up — see also: What causes this iOS permission prompt for "use Bluetooth for new connections"?). See also: #752

The Apple BLE dialog makes this API super intrusive...ultimately, it is hot garbage that Core Bluetooth doesn't let you query any information about BLE availability without first showing the user a dialog. 😢 You can't even check if BLE is supported w/o showing a dialog!! 🤦

I haven't completely abandoned the idea, but a unified API for these things may be impossible; really just trying to find the best compromise (and still have a reasonably friendly API).

Additional details can also be found here: #737

davertay-j

This is pretty awesome, much easier to reason about the lifecycle of the various moving parts now. 🚀

davertay-j · 2024-09-23T21:10:42Z

kable-core/src/androidMain/kotlin/Connection.kt

+        // todo: Move this `require` to the PeripheralBuilder.
+        require(disconnectTimeout > ZERO) { "Disconnect timeout must be >0, was $disconnectTimeout" }


Is this TODO dependent on some other work or can this be moved to PeripheralBuilder as part of this PR?

I forget why I didn't do it in this PR. I'll try again. If I can't move it quite yet, then I'll merge w/ it as-is (with the todo) and try to get to it soon.

Looked into it, I could move this, but it'd be duplicated for each platform (which is expected), but I realized that I plan to do some refactoring of PeripheralBuilder (so that Config objects are created, to make passing of those values around easier). I figured I'd just wait for that small refactor to pull this in. 🤷

kable-core/src/androidMain/kotlin/Connection.kt

twyatt added the major Changes that should bump the MAJOR version number label Sep 12, 2024

twyatt force-pushed the twyatt/connection branch 8 times, most recently from 2115401 to f77ffa7 Compare September 17, 2024 21:40

twyatt mentioned this pull request Sep 17, 2024

Rewrite sample app to use Compose Multiplatform JuulLabs/sensortag#380

Merged

twyatt force-pushed the twyatt/connection branch 4 times, most recently from bf4d2a5 to 565f307 Compare September 17, 2024 22:08

twyatt marked this pull request as ready for review September 17, 2024 22:23

twyatt requested review from davertay-j and a team as code owners September 17, 2024 22:23

twyatt requested a review from Phoenix7351 September 17, 2024 22:23

twyatt force-pushed the twyatt/connection branch 2 times, most recently from c5dde43 to 602390a Compare September 18, 2024 07:24

Major connection handling refactor

cab1cab

twyatt force-pushed the twyatt/connection branch from 602390a to cab1cab Compare September 18, 2024 07:25

twyatt commented Sep 18, 2024

View reviewed changes

Phoenix7351 reviewed Sep 18, 2024

View reviewed changes

PR feedback

dc0d94f

twyatt requested a review from Phoenix7351 September 18, 2024 21:21

Phoenix7351 approved these changes Sep 19, 2024

View reviewed changes

twyatt changed the title ~~Major connection handling refactor~~ Leverage structured concurrency for connection handling Sep 19, 2024

twyatt mentioned this pull request Sep 19, 2024

OutOfOrderGattCallbackException: Unexpected response type OnCharacteristicWrite received #497

Closed

davertay-j approved these changes Sep 23, 2024

View reviewed changes

twyatt enabled auto-merge (squash) September 25, 2024 22:20

Merge branch 'main' into twyatt/connection

43769f3

twyatt merged commit bff8d14 into main Sep 25, 2024
2 checks passed

twyatt deleted the twyatt/connection branch September 25, 2024 22:30

This was referenced Oct 2, 2024

Consider leveraging JetBrains/kotlin-wrappers library #721

Closed

Should remove listeners in Observations.clear on JavaScript #170

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Leverage structured concurrency for connection handling #749

Leverage structured concurrency for connection handling #749

twyatt commented Sep 12, 2024 •

edited

Loading

twyatt Sep 18, 2024

twyatt Sep 18, 2024

Phoenix7351 Sep 18, 2024

twyatt Sep 18, 2024 •

edited

Loading

Phoenix7351 Sep 19, 2024

twyatt Sep 19, 2024 •

edited

Loading

twyatt Sep 19, 2024

Phoenix7351 Sep 18, 2024

twyatt Sep 18, 2024 •

edited

Loading

twyatt Sep 19, 2024

davertay-j left a comment

davertay-j Sep 23, 2024

twyatt Sep 23, 2024

twyatt Sep 25, 2024 •

edited

Loading

		// todo: Move this `require` to the PeripheralBuilder.
		require(disconnectTimeout > ZERO) { "Disconnect timeout must be >0, was $disconnectTimeout" }

Leverage structured concurrency for connection handling #749

Leverage structured concurrency for connection handling #749

Conversation

twyatt commented Sep 12, 2024 • edited Loading

Summary

Scoped connections

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twyatt Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twyatt Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twyatt Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davertay-j left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twyatt Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

twyatt commented Sep 12, 2024 •

edited

Loading

twyatt Sep 18, 2024 •

edited

Loading

twyatt Sep 19, 2024 •

edited

Loading

twyatt Sep 18, 2024 •

edited

Loading

twyatt Sep 25, 2024 •

edited

Loading