Complete only the oldest matching pending write on onCharacteristicWrite - #273
Open
tanguy-penfen wants to merge 1 commit into
Open
Complete only the oldest matching pending write on onCharacteristicWrite#273tanguy-penfen wants to merge 1 commit into
tanguy-penfen wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
On Android,
onCharacteristicWritecompletes every pending write future matching(device, service, characteristic) with the single callback status, and
writeValueaddsthe future to
writeResultFutureListbefore the nativegatt.writeCharacteristic()call returns.
Android serializes GATT operations per device, so a write callback always belongs to the
oldest accepted write. When a callback for a previous operation lands between "future
listed" and "native call returned" (a window the size of a binder round-trip), the newer
write is completed with the previous operation's status:
writeValue()resolves successfully although the write's bytes werenever transmitted. With
withoutResponseandwithResponsewrites sharing acharacteristic, a local no-response completion is indistinguishable from a peer ATT
response, so the caller's protocol state silently diverges from the peripheral's.
call then returns non-success, the failure branch invokes the already-invoked
platform-channel reply.
later completes the next pending future. Under continuous traffic every write is
acknowledged by its predecessor's outcome; a quiet gap heals it.
When can two pending futures for one characteristic coexist? Three verified paths:
on the Dart side while the Kotlin future stays in
writeResultFutureList, and the queuethen releases the next command — a late peer response completes both the stale and the
fresh future;
QueueType.noneis a documented API — concurrent commands are supported usage;matching, one stray callback acknowledges a write that was never transmitted.
We traced a field incident in our app to exactly this signature: a stop command reported as acknowledged that provably never executed on the peripheral, followed by a stretch of misattributed acknowledgements that ended after a connection gap.
Fix
Complete only the oldest matching future. This is exact for the Android backend —
per-device GATT serialization guarantees the callback belongs to the oldest accepted
operation — and it defuses all three failure modes above: a future that is listed but not
yet accepted natively can no longer be completed by an earlier operation's callback.
cleanUpConnectionintentionally keeps complete-all semantics: on disconnect, everypending write has genuinely failed. The #268 delivery properties are preserved: removal
under the lock, completion posted to the main looper, each completion individually
contained.
One deliberate trade worth naming: if an accepted operation's callback were permanently
lost while the link stayed up, the stale queue head would shift subsequent attributions
(the previous behavior "healed" this by flushing everything on the next callback — as
false ACKs). In practice the ATT transaction timeout tears the link down within ~30 s and
cleanUpConnectionfails every pending write, which bounds that case.Tests
The Android unit tests do not compile on
main— the templateUniversalBlePluginTest.ktstill referencesonMethodCall, gone since the pigeonmigration — so no Android unit test can currently run at all, including the ones this
PR adds. The PR therefore replaces that dead template with
WriteCompletionTest.kt(happy to restore a minimal compiling template test instead, if you'd rather keep the
file). The new test drives the real
onCharacteristicWriteagainst seeded pendingwrites:
one successful completion must ack only the oldest write, and the second write's own
later response must complete it exactly once — fails on current
main:one failed completion must fail only the oldest write, not propagate to the second,
still-in-flight one — also fails on current
main.With the fix both pass and the existing
UniversalBleHelperTestsuite stays green(
gradlew :universal_ble:testDebugUnitTest, JDK 17, Gradle 8.14.3).We also validated this commit in our production app on Android hardware (pinned via
dependency_overrides) with an HCI snoop running: several rapid series of BLErequest/response cycles, deliberate peer-rejected writes, and a full OTA transfer
(~1500 write-without-response packets with write-requests interleaved — the exact mixed
pattern above). Every write request was answered on the air (126/126, p50 36 ms), the
rejections surfaced as genuine ATT error responses, and no response ever arrived without
a pending request.
Scope: Android backend only — the Darwin queue has its own bookkeeping (recently reworked
in #272) and is not touched here.
Notes for review: the test reaches the private
writeResultFutureListandmainThreadHandlervia reflection to avoid widening any API — happy to switch to@VisibleForTestinginternals or a small extracted registry if you prefer. Also happy toadd a CHANGELOG entry to this PR.
Related: #268 fixed the thread-safety of this same list; this PR addresses the
completion-matching granularity, which #268 deliberately left unchanged ("write-await
semantics are unchanged").