diff --git a/android/src/main/kotlin/com/navideck/universal_ble/UniversalBlePlugin.kt b/android/src/main/kotlin/com/navideck/universal_ble/UniversalBlePlugin.kt index f3c767ba..6f5aca71 100644 --- a/android/src/main/kotlin/com/navideck/universal_ble/UniversalBlePlugin.kt +++ b/android/src/main/kotlin/com/navideck/universal_ble/UniversalBlePlugin.kt @@ -782,26 +782,29 @@ class UniversalBlePlugin : UniversalBlePlatformChannel, BluetoothGattCallback(), characteristic: BluetoothGattCharacteristic, status: Int, ) { - // Runs on a binder thread: snapshot and remove matches under the lock, - // then complete each future on the main thread (platform-channel - // replies must be sent there). Completing outside the removal loop, - // with each completion individually contained, means one bad callback - // can neither corrupt the list nor block later completions. - val matches: List + // Runs on a binder thread: remove the matching future under the lock, + // then complete it on the main thread (platform-channel replies must + // be sent there), individually contained so a bad callback can neither + // corrupt the list nor block later completions. Android serializes + // GATT operations per device, so this callback always belongs to the + // OLDEST pending write on this characteristic — completing every match + // would acknowledge later writes that are still in flight (or not yet + // accepted natively) with this operation's status. + val future: WriteResultFuture? synchronized(writeResultFutureList) { - matches = writeResultFutureList.filter { + future = writeResultFutureList.firstOrNull { it.deviceId == gatt?.device?.address && it.characteristicId == characteristic.uuid.toString() && it.serviceId == characteristic.service?.uuid?.toString() } - writeResultFutureList.removeAll(matches) + future?.let { writeResultFutureList.remove(it) } } if (status != BluetoothGatt.GATT_SUCCESS) { UniversalBleLogger.logError( "WRITE_FAILED <- ${gatt?.device?.address} ${characteristic.uuid} status=$status" ) } - for (future in matches) { + if (future != null) { postToMainLooper { try { if (status == BluetoothGatt.GATT_SUCCESS) { diff --git a/android/src/test/kotlin/com/navideck/universal_ble/UniversalBlePluginTest.kt b/android/src/test/kotlin/com/navideck/universal_ble/UniversalBlePluginTest.kt deleted file mode 100644 index 20401439..00000000 --- a/android/src/test/kotlin/com/navideck/universal_ble/UniversalBlePluginTest.kt +++ /dev/null @@ -1,27 +0,0 @@ -package com.navideck.universal_ble - -import io.flutter.plugin.common.MethodCall -import io.flutter.plugin.common.MethodChannel -import kotlin.test.Test -import org.mockito.Mockito - -/* - * This demonstrates a simple unit test of the Kotlin portion of this plugin's implementation. - * - * Once you have built the plugin's example app, you can run these tests from the command - * line by running `./gradlew testDebugUnitTest` in the `example/android/` directory, or - * you can run them directly from IDEs that support JUnit such as Android Studio. - */ - -internal class UniversalBlePluginTest { - @Test - fun onMethodCall_getPlatformVersion_returnsExpectedValue() { - val plugin = UniversalBlePlugin() - - val call = MethodCall("getPlatformVersion", null) - val mockResult: MethodChannel.Result = Mockito.mock(MethodChannel.Result::class.java) - plugin.onMethodCall(call, mockResult) - - Mockito.verify(mockResult).success("Android " + android.os.Build.VERSION.RELEASE) - } -} diff --git a/android/src/test/kotlin/com/navideck/universal_ble/WriteCompletionTest.kt b/android/src/test/kotlin/com/navideck/universal_ble/WriteCompletionTest.kt new file mode 100644 index 00000000..760d381c --- /dev/null +++ b/android/src/test/kotlin/com/navideck/universal_ble/WriteCompletionTest.kt @@ -0,0 +1,126 @@ +package com.navideck.universal_ble + +import android.bluetooth.BluetoothDevice +import android.bluetooth.BluetoothGatt +import android.bluetooth.BluetoothGattCharacteristic +import android.bluetooth.BluetoothGattService +import android.os.Handler +import java.util.UUID +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertTrue +import org.mockito.Mockito + +/* + * Android serializes GATT operations per device, so an onCharacteristicWrite + * callback always belongs to the oldest write submitted for that + * characteristic. Completing every pending future that matches + * (device, service, characteristic) conflates distinct operations: one + * callback acknowledges writes whose bytes were never transmitted (a "false + * ACK"), and the operation whose response later arrives finds no future left, + * or worse, completes the next one - a sustained off-by-one. + * + * These tests drive the real plugin callback against seeded pending writes. + */ +internal class WriteCompletionTest { + private val deviceAddress = "AA:BB:CC:DD:EE:FF" + private val serviceUuid = UUID.fromString("0000fff0-0000-1000-8000-00805f9b34fb") + private val charUuid = UUID.fromString("0000fff1-0000-1000-8000-00805f9b34fb") + + private fun pluginWithInlineHandler(): UniversalBlePlugin { + val plugin = UniversalBlePlugin() + // Completions are posted to the main looper; execute them inline so + // the test observes them synchronously. + val handler = Mockito.mock(Handler::class.java) + Mockito.doAnswer { invocation -> + invocation.getArgument(0).run() + true + }.`when`(handler).post(Mockito.any()) + val handlerField = UniversalBlePlugin::class.java.getDeclaredField("mainThreadHandler") + handlerField.isAccessible = true + handlerField.set(plugin, handler) + return plugin + } + + @Suppress("UNCHECKED_CAST") + private fun pendingWrites(plugin: UniversalBlePlugin): MutableList { + val listField = UniversalBlePlugin::class.java.getDeclaredField("writeResultFutureList") + listField.isAccessible = true + return listField.get(plugin) as MutableList + } + + private fun mockGatt(): BluetoothGatt { + val device = Mockito.mock(BluetoothDevice::class.java) + Mockito.`when`(device.address).thenReturn(deviceAddress) + val gatt = Mockito.mock(BluetoothGatt::class.java) + Mockito.`when`(gatt.device).thenReturn(device) + return gatt + } + + private fun mockCharacteristic(): BluetoothGattCharacteristic { + val service = Mockito.mock(BluetoothGattService::class.java) + Mockito.`when`(service.uuid).thenReturn(serviceUuid) + val characteristic = Mockito.mock(BluetoothGattCharacteristic::class.java) + Mockito.`when`(characteristic.uuid).thenReturn(charUuid) + Mockito.`when`(characteristic.service).thenReturn(service) + return characteristic + } + + private fun pendingFuture(acks: MutableList>) = WriteResultFuture( + deviceAddress, + charUuid.toString(), + serviceUuid.toString(), + ) { acks.add(it) } + + @Test + fun oneWriteCompletionMustNotAckOtherPendingWrites() { + val plugin = pluginWithInlineHandler() + val firstAcks = mutableListOf>() + val secondAcks = mutableListOf>() + val pending = pendingWrites(plugin) + synchronized(pending) { + pending.add(pendingFuture(firstAcks)) + pending.add(pendingFuture(secondAcks)) + } + + // The ATT response of the FIRST write arrives. + plugin.onCharacteristicWrite(mockGatt(), mockCharacteristic(), BluetoothGatt.GATT_SUCCESS) + + assertEquals(1, firstAcks.size, "oldest pending write should be acknowledged") + assertTrue(firstAcks[0].isSuccess) + assertEquals( + 0, + secondAcks.size, + "second write is still in flight: acknowledging it with the first " + + "write's status is a false ACK", + ) + + // The second write's own response must then complete it - exactly once. + plugin.onCharacteristicWrite(mockGatt(), mockCharacteristic(), BluetoothGatt.GATT_SUCCESS) + assertEquals(1, secondAcks.size, "second write acknowledged by its own response") + assertEquals(1, firstAcks.size, "first write must not be acknowledged twice") + } + + @Test + fun failedCompletionMustFailOnlyTheOldestPendingWrite() { + val plugin = pluginWithInlineHandler() + val firstAcks = mutableListOf>() + val secondAcks = mutableListOf>() + val pending = pendingWrites(plugin) + synchronized(pending) { + pending.add(pendingFuture(firstAcks)) + pending.add(pendingFuture(secondAcks)) + } + + plugin.onCharacteristicWrite(mockGatt(), mockCharacteristic(), BluetoothGatt.GATT_FAILURE) + + assertEquals(1, firstAcks.size, "oldest pending write should be failed") + assertTrue(firstAcks[0].isFailure) + assertEquals( + 0, + secondAcks.size, + "an error of the first write must not be propagated to the second, " + + "still-in-flight write", + ) + } +}