fix(DCMAW-21804): Advertise after opening gatt server (#522) - #1
fix(DCMAW-21804): Advertise after opening gatt server (#522)#1covolution wants to merge 1 commit into
Conversation
|
/gemini help |
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Bluetooth GATT server and advertising flow to defer advertising until the GATT service is successfully added, and introduces a state check to reject incoming connections if the service is not yet ready. Feedback on these changes highlights a potential race condition where the advertising coroutine is launched on a scope that outlives the transport's lifecycle, thread-safety concerns regarding the connectedDevice property which should be marked @Volatile, and a missing state update to set isServiceReady to false when advertising fails.
| is GattServerEvent.ServiceAdded -> { | ||
| coroutineScope.launch( | ||
| ioDispatcher + "$logTag.StartAdvertising".asCoroutineName() | ||
| ) { | ||
| startAdvertising(event.service.uuid) | ||
| } | ||
| } |
There was a problem hiding this comment.
In handleGattEvent, when GattServerEvent.ServiceAdded is received, a coroutine is launched on coroutineScope (which is the ApplicationScope). Since coroutineScope is not cancelled when stop() is called, this coroutine can continue running even after the transport has been stopped. If stop() is called while startAdvertising is starting, it can lead to a race condition where advertising is started after the GATT server has been closed.
To fix this, we should launch the coroutine using a scope that is tied to the lifecycle of monitoringJob. We can create a CoroutineScope using coroutineScope.coroutineContext + monitoringJob so that the coroutine is automatically cancelled when monitoringJob is cancelled during stop().
| is GattServerEvent.ServiceAdded -> { | |
| coroutineScope.launch( | |
| ioDispatcher + "$logTag.StartAdvertising".asCoroutineName() | |
| ) { | |
| startAdvertising(event.service.uuid) | |
| } | |
| } | |
| is GattServerEvent.ServiceAdded -> { | |
| CoroutineScope(coroutineScope.coroutineContext + monitoringJob).launch( | |
| ioDispatcher + "$logTag.StartAdvertising".asCoroutineName() | |
| ) { | |
| startAdvertising(event.service.uuid) | |
| } | |
| } |
| override val events: SharedFlow<GattServerEvent> = _events | ||
| private var gattServer: BluetoothGattServer? = null | ||
| private var connectedDevice: BluetoothDevice? = null | ||
| internal var connectedDevice: BluetoothDevice? = null |
There was a problem hiding this comment.
The connectedDevice property is read and written across multiple threads (e.g., the Android Bluetooth binder threads in handleConnectionStateChange and the application's ioDispatcher threads in cancelCurrentConnection and sendMessage). To ensure thread safety and proper visibility of updates across threads, connectedDevice should be marked with @Volatile.
| internal var connectedDevice: BluetoothDevice? = null | |
| @Volatile | |
| internal var connectedDevice: BluetoothDevice? = null |
| is AdvertiserState.Failed -> { | ||
| _state.value = | ||
| PeripheralBluetoothState.Error( | ||
| PeripheralBluetoothTransportError.ADVERTISING_FAILED | ||
| ) | ||
| } |
There was a problem hiding this comment.
When the advertiser state transitions to AdvertiserState.Failed, isServiceReady is not updated. If advertising fails, the service is no longer ready to accept connections, so isServiceReady should be explicitly set to false to prevent accepting any incoming connections in an invalid state.
| is AdvertiserState.Failed -> { | |
| _state.value = | |
| PeripheralBluetoothState.Error( | |
| PeripheralBluetoothTransportError.ADVERTISING_FAILED | |
| ) | |
| } | |
| is AdvertiserState.Failed -> { | |
| isServiceReady = false | |
| _state.value = | |
| PeripheralBluetoothState.Error( | |
| PeripheralBluetoothTransportError.ADVERTISING_FAILED | |
| ) | |
| } |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the BLE peripheral transport layer to ensure connections are only accepted when the GATT service is fully ready, introducing a state-tracking mechanism and the ability to cancel connections. However, the reviewer identified critical issues with the changes: changing BluetoothGattService to non-nullable is risky as the Android framework can pass null on registration failure, potentially causing runtime crashes. Additionally, the reviewer pointed out a state leak during connection rejection due to a missing early return, and a potential race condition where advertising coroutines are launched on an unsupervised scope instead of being tied to the lifecycle of the monitoring job.
| } | ||
|
|
||
| override fun onServiceAdded(status: Int, service: BluetoothGattService?) { | ||
| override fun onServiceAdded(status: Int, service: BluetoothGattService) { |
There was a problem hiding this comment.
Reverting BluetoothGattService? to a non-nullable BluetoothGattService in onServiceAdded is risky. In the Android BLE framework, if service registration fails (i.e., status != GATT_SUCCESS), the framework can pass null for the service parameter.
Since Kotlin enforces strict null-safety checks on non-nullable parameters for overridden platform methods, receiving a null value at runtime will trigger an immediate NullPointerException before the method body even executes, causing the application to crash.
Please revert this parameter to be nullable.
| override fun onServiceAdded(status: Int, service: BluetoothGattService) { | |
| override fun onServiceAdded(status: Int, service: BluetoothGattService?) { |
| } | ||
|
|
||
| data class ServiceAdded(val status: Int, val service: BluetoothGattService?) : | ||
| data class ServiceAdded(val status: Int, val service: BluetoothGattService) : |
There was a problem hiding this comment.
Revert the service property to be nullable to match the nullability of the parameter in GattServerCallback.onServiceAdded and prevent potential runtime crashes when service registration fails.
| data class ServiceAdded(val status: Int, val service: BluetoothGattService) : | |
| data class ServiceAdded(val status: Int, val service: BluetoothGattService?) : |
| data class Connected(val address: String) : GattServerEvent | ||
| data class Disconnected(val address: String, val isSessionEnd: Boolean) : GattServerEvent | ||
| data class ServiceAdded(val service: BluetoothGattService?) : GattServerEvent | ||
| data class ServiceAdded(val service: BluetoothGattService) : GattServerEvent |
There was a problem hiding this comment.
Revert the service property to be nullable to safely handle cases where the GATT service registration fails and the Android framework passes null.
| data class ServiceAdded(val service: BluetoothGattService) : GattServerEvent | |
| data class ServiceAdded(val service: BluetoothGattService?) : GattServerEvent |
|
|
||
| is GattServerEvent.ServiceAdded -> { | ||
| logger.debug(logTag, "Service Added: ${source.service?.uuid}") | ||
| logger.debug(logTag, "Service Added: ${source.service.uuid}") |
There was a problem hiding this comment.
Revert to using a safe call (?.) since service should remain nullable to prevent potential null pointer exceptions under error conditions.
| logger.debug(logTag, "Service Added: ${source.service.uuid}") | |
| logger.debug(logTag, "Service Added: ${source.service?.uuid}") |
| is GattServerEvent.Connected -> { | ||
| if (!isServiceReady) { | ||
| logger.debug( | ||
| logTag, | ||
| "Rejecting connection from ${event.address} - service not ready" | ||
| ) | ||
| gattServerManager.cancelCurrentConnection() | ||
| } | ||
| } | ||
|
|
||
| is GattServerEvent.ServiceAdded -> { | ||
| coroutineScope.launch( | ||
| ioDispatcher + "$logTag.StartAdvertising".asCoroutineName() | ||
| ) { | ||
| startAdvertising(event.service.uuid) | ||
| } | ||
| } |
There was a problem hiding this comment.
This block contains two significant issues:
- State Leak on Rejected Connection: When
!isServiceReadyis true, we cancel the connection but do not prevent the event from being transformed and updating_state.valuetoPeripheralBluetoothState.Connected. This causes a brief, incorrect state transition toConnectedbefore the subsequent disconnection event is processed. Returning early when rejecting the connection prevents this. - Race Condition / Unsupervised Coroutine: Launching the
startAdvertisingcoroutine on the application-levelcoroutineScopemeans it is not cancelled whenstop()is called (which only cancelsmonitoringJob). Ifstop()is called whileStartAdvertisingis pending, the advertiser could be started after the transport has stopped, leaving it running indefinitely. Launching it on a scope tied tomonitoringJobensures it is automatically cancelled whenstop()is called.
Additionally, we use a safe call (?.let) on event.service to handle the reverted nullable type safely.
| is GattServerEvent.Connected -> { | |
| if (!isServiceReady) { | |
| logger.debug( | |
| logTag, | |
| "Rejecting connection from ${event.address} - service not ready" | |
| ) | |
| gattServerManager.cancelCurrentConnection() | |
| } | |
| } | |
| is GattServerEvent.ServiceAdded -> { | |
| coroutineScope.launch( | |
| ioDispatcher + "$logTag.StartAdvertising".asCoroutineName() | |
| ) { | |
| startAdvertising(event.service.uuid) | |
| } | |
| } | |
| is GattServerEvent.Connected -> { | |
| if (!isServiceReady) { | |
| logger.debug( | |
| logTag, | |
| "Rejecting connection from ${event.address} - service not ready" | |
| ) | |
| gattServerManager.cancelCurrentConnection() | |
| return | |
| } | |
| } | |
| is GattServerEvent.ServiceAdded -> { | |
| event.service?.let { service -> | |
| CoroutineScope(ioDispatcher + monitoringJob + "$logTag.StartAdvertising".asCoroutineName()).launch { | |
| startAdvertising(service.uuid) | |
| } | |
| } | |
| } |
3 participants
DCMAW-00000: title of change
Tutorial for writing good descriptions.
Evidence of the change
Checklist