Conversation
fotiDim
left a comment
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 } |
There was a problem hiding this comment.
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() } |
There was a problem hiding this comment.
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())) |
There was a problem hiding this comment.
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.
Android reconnects can report
onMtuChanged(247, GATT_SUCCESS)while the DartrequestMtufuture 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:
main; the API 37 MTU path is unchanged.The native API and generated Pigeon files are unchanged.