Make Android write-completion delivery thread-safe - #268
Conversation
Field logs showed writes completing fast (<100ms) or never (timeouts at both 100ms and 1s deadlines, zero GATT-busy errors), sometimes latching into 100% loss until the session ended. The write-completion plumbing can lose acks between onCharacteristicWrite and Dart: 1. writeResultFutureList is an unsynchronized ArrayList mutated from the main thread (writeValue) and Bluetooth binder threads (onCharacteristicWrite / cleanUpConnection) - connectGatt registers no Handler, so callbacks arrive on binder threads. Concurrent add/removeAll can silently drop a pending completion. 2. Pigeon replies were invoked directly on the binder thread; platform channel replies must be sent from the main thread. 3. removeAll invoked completion callbacks inside its predicate: one throwing completion aborts the loop mid-iteration, leaves its entry in the list (double-completed on the next ack), and permanently blocks all later completions - the latched failure mode. Fix: guard every writeResultFutureList access with synchronized; in onCharacteristicWrite and cleanUpConnection, snapshot+remove matches under the lock, then post each completion to the main thread individually wrapped in try/catch. Write-await semantics are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![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 improves thread safety in UniversalBlePlugin.kt by synchronizing access to writeResultFutureList and dispatching future completions to the main thread. The review comments point out critical improvements, including marking mainThreadHandler as @Volatile to avoid thread visibility issues, using safe calls on characteristic.service to prevent potential null pointer exceptions, and applying similar synchronization patterns to other pending future lists.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| ) | ||
| } | ||
| for (future in matches) { | ||
| mainThreadHandler?.post { |
There was a problem hiding this comment.
The mainThreadHandler variable is mutated on the main thread (in onAttachedToEngine and onDetachedFromEngine) but read here on a Bluetooth binder thread without any synchronization or volatile guarantees. This can lead to thread visibility issues (data races) where the binder thread reads a stale null value even after initialization, causing write completions to be silently dropped and resulting in timeouts.
To ensure proper visibility across threads, please mark mainThreadHandler with the @Volatile annotation at its declaration on line 45:
@Volatile
private var mainThreadHandler: Handler? = null| } | ||
| } | ||
| } | ||
| subscriptionResultFutureList.removeAll { |
There was a problem hiding this comment.
The subscriptionResultFutureList (and similarly readResultFutureList, mtuResultFutureList, discoverServicesFutureList, and rssiResultFutureList) suffers from the exact same thread-safety and callback-delivery issues that were just fixed for writeResultFutureList:
- It is mutated concurrently from the main thread (in
setNotifiable) and Bluetooth binder threads (inupdateSubscriptionStateandcleanUpConnection) without synchronization. - Pigeon/platform-channel replies are invoked directly on the binder thread instead of the main thread.
removeAllinvokes completion callbacks inside its predicate, which can throw and abort the loop mid-iteration, leaving entries in the list and permanently blocking later completions.
To prevent similar race conditions, timeouts, and latched failures, these lists should be refactored using the same synchronized snapshot-and-remove pattern, with completions posted to the main thread.
There was a problem hiding this comment.
Good catch — readResultFutureList, subscriptionResultFutureList, mtuResultFutureList, discoverServicesFutureList, and rssiResultFutureList do share the same race, off-thread reply, and callbacks-inside-removeAll pattern. I've kept this PR scoped to the write path since that's the one we could tie to a concrete field failure with a verifiable device test.
It's probably better to create other PRs to fix other lists with some on-device tests.
|
@liyuqian thanks for the PR please check the gemini-code-assist comments. Also, are we sure this fix doesn't degrade performance? |
Addresses review feedback on the write-completion thread-safety fix: - mainThreadHandler is read on Bluetooth binder threads but assigned on the main thread; mark it @volatile so a binder thread cannot observe a stale null and silently drop a completion. - characteristic.service can be null on the binder thread; use a safe call when matching pending futures so an NPE cannot abort the matching loop and leave futures permanently hung. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@fotiDim No measurable degrade in my local tests. Before this PR:
After this PR:
|
|
A little bump because we were getting OTA updates failure (a lot of succesive write operations) and this PR fixes it completely. We hope to see it published soon. Thank you ! |
There was a problem hiding this comment.
Pull request overview
This PR hardens Android BLE write-completion delivery against concurrency hazards between Bluetooth GATT binder-thread callbacks and Flutter/Dart completion handling, aiming to prevent lost acks and latched “no completions” failure modes observed in field logs.
Changes:
- Adds synchronization around
writeResultFutureListmutations to avoid concurrent modification issues across threads. - Reworks
onCharacteristicWriteand disconnect cleanup to snapshot/remove matching write futures under a lock, then complete them on the main thread with per-callback exception isolation. - Marks
mainThreadHandleras@Volatileto improve cross-thread visibility of its assignment/clearing.
Comments suppressed due to low confidence (1)
android/src/main/kotlin/com/navideck/universal_ble/UniversalBlePlugin.kt:1142
- In
cleanUpConnection, pending write futures are removed fromwriteResultFutureListand then completed only viamainThreadHandler?.post { ... }. IfmainThreadHandleris null, those futures are never completed (and now cannot be retried because they were removed from the list). Use a main-looper handler fallback here as well so disconnect cleanup doesn't drop write completions.
for (future in pendingWrites) {
mainThreadHandler?.post {
try {
future.result(Result.failure(deviceDisconnectedError))
} catch (e: Exception) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@fotiDim how does the latest change look? |
|
It is out in |
Field logs showed writes completing fast (<100ms) or never (timeouts at both 100ms and 1s deadlines, zero GATT-busy errors), sometimes latching into 100% loss until the session ended. The write-completion plumbing can lose acks between onCharacteristicWrite and Dart:
Fix: guard every writeResultFutureList access with synchronized; in onCharacteristicWrite and cleanUpConnection, snapshot+remove matches under the lock, then post each completion to the main thread individually wrapped in try/catch. Write-await semantics are unchanged.