Skip to content

Android: fix lost MTU completions and reconnect isolation - #307

Open
featzima wants to merge 1 commit into
Navideck:mainfrom
featzima:fix/android-mtu-completion-upstream
Open

featzima wants to merge 1 commit into
Navideck:mainfrom
featzima:fix/android-mtu-completion-upstream

Conversation

@featzima

Copy link
Copy Markdown
Contributor

Android reconnects can report onMtuChanged(247, GATT_SUCCESS) while the Dart requestMtu future still times out. In the existing implementation, the waiter is registered after the native request, the pending list is accessed from both the platform and Binder threads without synchronization, and an MTU reported before the request is discarded.

This registers the waiter before calling Android, synchronizes pending-request completion, and delivers replies on the main looper consistently with the other BLE completion paths. It also handles native rejection immediately and coalesces requests already pending for the same GATT client.

Negotiated MTUs are retained per GATT client, so a reconnect cannot reuse an old connection's value or consume its late callback. The actual observed MTU is returned, including values smaller than requested. Android 13 and earlier can still request an increase; Android 14+ reuses the observed value because subsequent MTU requests are ignored. Disconnect clears the cached value and fails pending requests.

Validation:

  • Android native tests: 23 passed, including 11 MTU cases covering early callbacks, duplicate delivery, already negotiated MTU, native rejection, disconnect and stale reconnect callbacks. Run with Java 17 / Gradle 8.14.3 using the example project.
  • Flutter tests: 109 passed.
  • Chrome tests: 105 passed.
  • Package and HIL analysis: passed with fatal infos enabled.
  • Physical regression in a consuming app on a Pixel 6a (Android API 37): three background reconnect/read/disconnect cycles completed at MTU 247 without timeouts, followed by successful foreground recovery. This hardware check used the 2.3.0-based patch before porting it to current main; the API 37 MTU path is unchanged.

The native API and generated Pigeon files are unchanged.

@fotiDim fotiDim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the four files in this PR. The core fix is sound: the waiter is registered before the native call, mtuResultFutureList/negotiatedMtus access is consistently under one monitor, completions are posted to the main looper, stale-gatt callbacks are filtered by identity, and the failure paths now complete instead of hanging. I did not find a blocking bug. The inline notes below are edge-case/behavior observations; the test coverage for the orderings (early callback, duplicate delivery, rejection, reconnect, disconnect) is good.

val alreadyPending = mtuResultFutureList.any { it.gatt === gatt }
// Register before requestMtu: its callback can arrive immediately.
mtuResultFutureList.add(MtuResultFuture(deviceId, gatt, callback))
if (alreadyPending) return

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coalescing drops the later request's expectedMtu. When a request is already pending for this GATT, the new waiter is registered and requestMtu returns without asking for the new size, so both callers resolve to the first negotiation's result. On Android ≤13 a requestMtu(517) issued while requestMtu(247) is pending therefore gets 247 even though the platform could still negotiate an increase once the first exchange completes.

This matches the previous outcome (the old removeAll also completed every device future with the first callback) and the default per-device queue serializes calls in practice, so the impact is low. Since the coalescing is now explicit and locked in by simultaneousRequestsShareOneNegotiation, it may be worth documenting on UniversalBle.requestMtu that concurrent requests share one negotiation.

synchronized(mtuResultFutureList) {
pendingMtus = mtuResultFutureList.filter { it.deviceId == deviceId }
mtuResultFutureList.removeAll(pendingMtus)
negotiatedMtus.keys.removeAll { it.device.address == deviceId }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

negotiatedMtus is keyed by BluetoothGatt for reconnect isolation, but cleanUpConnection evicts by device address. For a disconnect the app did not request, onConnectionStateChange calls cleanUpConnection(deviceId) even if a newer client for the same address is already registered — the superseded guard only covers the app-initiated disconnect() path (closingByRequest).

If the new client has already observed an MTU, this clears its cache. On Android 14+ the following requestMtu is ignored by the framework and may never produce onMtuChanged, leaving that future to the command-queue timeout. The window is narrow (the old client's state usually flips before connect() can create a replacement), so low severity — but passing the disconnecting BluetoothGatt into cleanUpConnection would let it evict only that entry and fully honor the per-client isolation.

private fun completeMtu(gatt: BluetoothGatt, result: Result<Long>) {
val pending: List<MtuResultFuture>
synchronized(mtuResultFutureList) {
result.getOrNull()?.let { negotiatedMtus[gatt] = it.toInt() }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onMtuChanged lets the callback through when findGatt() returns null (client already removed), so completeMtu can cache a value for a gatt that is no longer current. Harmless here because the map is weak and keyed by instance, so a later connection cannot read it — just noting that the current != null guard in onMtuChanged is what actually keeps a stale value out of a new connection.

// Android 14+ ignores subsequent negotiations. Earlier versions
// can still request an increase when the observed MTU is smaller.
if (canReuseNegotiatedMtu(mtu, expectedMtu.toInt(), Build.VERSION.SDK_INT)) {
postMtuResult(callback, Result.success(mtu.toLong()))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

postMtuResult only enqueues on the main looper, so this is not a deadlock. But every other completion path removes pending futures under the lock and delivers outside it; keeping the reuse reply the same shape would make the locking discipline uniform.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants