Skip to content
This repository has been archived by the owner on Aug 30, 2022. It is now read-only.

Commit

Permalink
Change behavior on rejection to settle on Disconnected
Browse files Browse the repository at this point in the history 
Determined that `BluetoothDevice.connectGatt` can return `null` when BLE
is turned off, so settling on `Disconnected`, as caller should be able
to try to reconnect again.

Renamed `Closed` to `Cancelled` to align with function names that induce
the state (`cancel` and `cancelAndJoin`).
  • Loading branch information
@twyatt
twyatt committed May 24, 2020
1 parent 50e1889 commit 439ca46
Show file tree
Hide file tree
Showing 3 changed files with 83 additions and 50 deletions.
19 changes: 13 additions & 6 deletions keep-alive/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,14 @@ class ExampleViewModel(application: Application) : AndroidViewModel(application)
When the parent [`CoroutineScope`] (`viewModelScope` in the above example) cancels, the
`KeepAliveGatt` automatically disconnects. You may _optionally_ manually `disconnect`.

If `KeepAliveGatt` is not configured with a parent [`CoroutineContext`] (e.g. via
`parentCoroutineContext` constructor argument or `CoroutineScope.keepAliveGatt` extension function)
then it should be cancelled when no longer needed using the `cancel` or `cancelAndJoin` function.

When it a `KeepAliveGatt` is cancelled, it will end in a `Cancelled` `State`. Once a `KeepAliveGatt`
is `Cancelled` it **cannot** be reconnected (calls to `connect` will throw `IllegalStateException`);
a new `KeepAliveGatt` must be created.

## Error Handling

When connection failures occur, the corresponding `Exception`s are propagated to `KeepAliveGatt`'s
Expand All @@ -152,13 +160,12 @@ val scope = CoroutineScope(Job() + exceptionHandler)
val gatt = scope.keepAliveGatt(...)
```

When a failure occurs during the connection sequence, `KeepAliveGatt` will disconnect/close the
in-flight connection and reconnect. If a connection attempt results in `GattConnectResult.Rejected`,
then the failure is considered unrecoverable and `KeepAliveGatt` will finish in a `Closed` `State`.
Additionally, a `ConnectionRejected` Exception is propagated to the parent [`CoroutineContext`].
When a failure occurs during the connection sequence, `KeepAliveGatt` will disconnect the in-flight
connection and reconnect. If a connection attempt results in `GattConnectResult.Rejected`, then the
failure is considered unrecoverable (e.g. BLE is off) and `KeepAliveGatt` will settle on
`Disconnected` `State`. Additionally, a `ConnectionRejected` Exception is propagated to the parent
[`CoroutineContext`]. The `connect` function may be used to attempt to establish connection again.

Once a `KeepAliveGatt` is `Closed` it **cannot** be reconnected (calls to `connect` will throw
`IllegalStateException`); a new `KeepAliveGatt` must be created.

# Setup

Expand Down
34 changes: 20 additions & 14 deletions keep-alive/src/main/java/KeepAliveGatt.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import com.juul.able.gatt.OnDescriptorWrite
import com.juul.able.gatt.OnMtuChanged
import com.juul.able.gatt.OnReadRemoteRssi
import com.juul.able.gatt.WriteType
import com.juul.able.keepalive.State.Closed
import com.juul.able.keepalive.State.Cancelled
import com.juul.able.keepalive.State.Connected
import com.juul.able.keepalive.State.Connecting
import com.juul.able.keepalive.State.Disconnected
Expand All @@ -37,6 +37,7 @@ import kotlinx.coroutines.CoroutineStart.UNDISPATCHED
import kotlinx.coroutines.FlowPreview
import kotlinx.coroutines.Job
import kotlinx.coroutines.NonCancellable
import kotlinx.coroutines.SupervisorJob
import kotlinx.coroutines.cancelAndJoin
import kotlinx.coroutines.channels.BroadcastChannel
import kotlinx.coroutines.channels.Channel.Factory.BUFFERED
Expand All @@ -61,8 +62,12 @@ sealed class State {
object Connecting : State()
object Connected : State()
object Disconnecting : State()
object Disconnected : State()
data class Closed(val cause: Throwable?) : State()
data class Disconnected(val cause: Throwable? = null) : State() {
override fun toString() = super.toString()
}
data class Cancelled(val cause: Throwable?) : State() {
override fun toString() = super.toString()
}

override fun toString(): String = javaClass.simpleName
}
Expand Down Expand Up @@ -90,19 +95,19 @@ class KeepAliveGatt(

private val applicationContext = androidContext.applicationContext

private val job = Job(parentCoroutineContext[Job]).apply {
invokeOnCompletion { cause -> _state.value = Closed(cause) }
private val job = SupervisorJob(parentCoroutineContext[Job]).apply {
invokeOnCompletion { cause -> _state.value = Cancelled(cause) }
}
private val scope = CoroutineScope(parentCoroutineContext + job)

private val isRunning = AtomicBoolean()
internal val isRunning = AtomicBoolean()

@Volatile
private var _gatt: GattIo? = null
private val gatt: GattIo
inline get() = _gatt ?: throw NotReady(toString())

private val _state = MutableStateFlow<State>(Disconnected)
private val _state = MutableStateFlow<State>(Disconnected())

/**
* Provides a [Flow] of the [KeepAliveGatt]'s [State].
Expand Down Expand Up @@ -135,13 +140,12 @@ class KeepAliveGatt(
while (isActive) {
spawnConnection()
}
}
}.invokeOnCompletion { isRunning.set(false) }
return true
}

suspend fun disconnect() {
job.children.forEach { it.cancelAndJoin() }
isRunning.set(false)
}

fun cancel() {
Expand All @@ -156,7 +160,7 @@ class KeepAliveGatt(

private suspend fun spawnConnection() {
try {
_state.value = Connecting
_state.value = Connecting.also(::println)

val gatt = when (val result = bluetoothDevice.connectGatt(applicationContext)) {
is Success -> result.gatt
Expand All @@ -176,11 +180,11 @@ class KeepAliveGatt(
.launchIn(this, start = UNDISPATCHED)
onConnectAction?.invoke(gatt)
_gatt = gatt
_state.value = Connected
_state.value = Connected.also(::println)
}
} finally {
_gatt = null
_state.value = State.Disconnecting
_state.value = State.Disconnecting.also(::println)

withContext(NonCancellable) {
withTimeoutOrNull(disconnectTimeoutMillis) {
Expand All @@ -192,8 +196,10 @@ class KeepAliveGatt(
}
}
}
} finally {
_state.value = Disconnected
_state.value = Disconnected().also(::println)
} catch (failure: Exception) {
_state.value = Disconnected(failure).also(::println)
throw failure
}
}

Expand Down
80 changes: 50 additions & 30 deletions keep-alive/src/test/java/KeepAliveGattTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import com.juul.able.gatt.OnReadRemoteRssi
import com.juul.able.keepalive.ConnectionRejected
import com.juul.able.keepalive.KeepAliveGatt
import com.juul.able.keepalive.NotReady
import com.juul.able.keepalive.State.Closed
import com.juul.able.keepalive.State.Cancelled
import com.juul.able.keepalive.State.Connected
import com.juul.able.keepalive.State.Connecting
import com.juul.able.keepalive.State.Disconnected
Expand Down Expand Up @@ -135,7 +135,7 @@ class KeepAliveGattTest {
disconnectTimeoutMillis = DISCONNECT_TIMEOUT
)
assertEquals(
expected = Disconnected,
expected = Disconnected(),
actual = keepAlive.state.first()
)

Expand Down Expand Up @@ -227,7 +227,7 @@ class KeepAliveGattTest {
}

@Test
fun `KeepAliveGatt is Closed when connection is rejected`() = runBlocking {
fun `KeepAliveGatt settles on Disconnected when connection is rejected`() = runBlocking {
val bluetoothDevice = mockBluetoothDevice()

val scope = CoroutineScope(Job())
Expand All @@ -238,44 +238,64 @@ class KeepAliveGattTest {
)

assertTrue(keepAlive.connect())
val closed = keepAlive.state.first { it is Closed } as Closed
val closed = keepAlive.state.first { it is Disconnected } as Disconnected

assertThrowable<ConnectionRejected>(closed.cause)
assertThrowable<RemoteException>(closed.cause?.cause)

assertFalse(scope.isActive, "Failure should propagate to parent")
assertFalse(keepAlive.isRunning.get(), "Rejection should mark KeepAliveGatt as not running")
coVerify(exactly = 1) { bluetoothDevice.connectGatt(any(), false, any()) }
}

@Test
fun `KeepAliveGatt with SupervisorJob parent does not fail siblings on connection rejection`() {
fun `KeepAliveGatt does not fail CoroutineContext on connection rejection`() = runBlocking {
val bluetoothDevice = mockBluetoothDevice()
val done = Channel<Unit>()

runBlocking {
launch(SupervisorJob()) {
val keepAlive = keepAliveGatt(
androidContext = mockk(relaxed = true),
bluetoothDevice = bluetoothDevice,
disconnectTimeoutMillis = DISCONNECT_TIMEOUT
)
val job = launch {
val keepAlive = keepAliveGatt(
androidContext = mockk(relaxed = true),
bluetoothDevice = bluetoothDevice,
disconnectTimeoutMillis = DISCONNECT_TIMEOUT
)

assertTrue(keepAlive.connect())
keepAlive.state.first { it is Closed }
}.apply {
invokeOnCompletion { done.offer(Unit) }
}
assertTrue(keepAlive.connect())
keepAlive.state.first { it is Disconnected && it.cause is ConnectionRejected }
done.offer(Unit)
}

val job = launch { delay(Long.MAX_VALUE) }
done.receive() // Wait for KeepAliveGatt to be in a (rejected) Disconnected state.
coVerify(exactly = 1) { bluetoothDevice.connectGatt(any(), false, any()) }

done.receive() // Wait for first job to complete (fail).
coVerify(exactly = 1) { bluetoothDevice.connectGatt(any(), false, any()) }
assertTrue(job.isActive, "Coroutine did not remain active")
job.cancelAndJoin()
}

// Verify sibling `launch` is still active.
assertTrue(job.isActive, "Launch with sibling SupervisorJob did not remain active")
@Test
fun `Can connect after connection is rejected`() = runBlocking {
val bluetoothDevice = mockBluetoothDevice()
val done = Channel<Unit>()

job.cancelAndJoin()
val job = launch {
val keepAlive = keepAliveGatt(
androidContext = mockk(relaxed = true),
bluetoothDevice = bluetoothDevice,
disconnectTimeoutMillis = DISCONNECT_TIMEOUT
)

assertTrue(keepAlive.connect())
keepAlive.state.first { it is Disconnected && it.cause is ConnectionRejected }

assertTrue(keepAlive.connect())
done.offer(Unit)
}

done.receive() // Wait for KeepAliveGatt to be in a (rejected) Disconnected state.

coVerify(exactly = 2) { bluetoothDevice.connectGatt(any(), false, any()) }
assertTrue(job.isActive, "Coroutine did not remain active")

job.cancelAndJoin()
}

@Test
Expand Down Expand Up @@ -321,7 +341,7 @@ class KeepAliveGattTest {
connected.await()

val disconnected = async(start = UNDISPATCHED) {
keepAlive.state.first { it == Disconnected }
keepAlive.state.first { it is Disconnected }
}
keepAlive.disconnect()
disconnected.await()
Expand Down Expand Up @@ -361,7 +381,7 @@ class KeepAliveGattTest {
ready.await()

val disconnected = async(start = UNDISPATCHED) {
keepAlive.state.first { it == Disconnected }
keepAlive.state.first { it is Disconnected }
}
keepAlive.disconnect()
disconnected.await()
Expand Down Expand Up @@ -500,7 +520,7 @@ class KeepAliveGattTest {
}
}
assertEquals(
expected = Disconnected,
expected = Disconnected(),
actual = keepAlive.state.first()
)

Expand All @@ -520,9 +540,9 @@ class KeepAliveGattTest {
private fun mockBluetoothDevice(): BluetoothDevice = mockk {
every { this@mockk.toString() } returns MAC_ADDRESS

// Mocked as returning `null` to indicate BLE request rejected (i.e. BLE unavailable).
// Most tests usually use `mockkStatic` to mock `BluetoothDevice.connectGatt(Context)` extension
// function, so this mocked method usually isn't used.
// Mocked as returning `null` to indicate BLE request rejected (i.e. BLE turned off).
// Most tests use `mockkStatic` to mock `BluetoothDevice.connectGatt(Context)` extension
// function (which would normally call this function), so this mocked method usually isn't used.
every { connectGatt(any(), any(), any()) } returns null
}

Expand Down

0 comments on commit 439ca46

Please sign in to comment.