From 9522d9e22e5cde56f159ed1e60eca81ab2a6d28e Mon Sep 17 00:00:00 2001 From: Dave Meade Date: Wed, 12 Aug 2026 13:17:42 -0400 Subject: [PATCH] Make AdapterManager::add_peripheral idempotent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `add_peripheral` asserted that the peripheral was not already present and then inserted it. That is a check-then-act pair on a concurrent DashMap, and the caller that reaches it is inherently racy: droidplug's `Adapter::report_scan_result` looks a peripheral up, takes the `None` branch, and only then calls `add`, holding nothing across the two steps. Two scan results for the same device can therefore both observe `None` and both add, and the second aborts the process. This is easy to hit on Android because `Manager::adapters()` returns a process-global singleton adapter: an application that starts a second scan — directly, or via a library that scans internally to find a peripheral — re-reports devices already in the shared map. The abort is unusually hard to diagnose. These callers are spawned tasks, so Tokio stores the panic payload in a JoinHandle nobody joins, and Android's default panic hook writes to stderr, which is not in logcat. The observable symptom is a task that silently stops existing. Keeps the existing entry rather than replacing it: it may already carry connection state a freshly constructed wrapper for the same address would not. droidplug's `add` returns the map's instance for the same reason, so a wrapper that loses the race cannot shadow the live one. Co-Authored-By: Claude Opus 5 (1M context) --- src/common/adapter_manager.rs | 25 ++++++++++++++++++++----- src/droidplug/adapter.rs | 16 +++++++++++++++- 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/src/common/adapter_manager.rs b/src/common/adapter_manager.rs index 7da91d66..cac07030 100644 --- a/src/common/adapter_manager.rs +++ b/src/common/adapter_manager.rs @@ -58,12 +58,27 @@ where Box::pin(BroadcastStream::new(receiver).filter_map(|x| async move { x.ok() })) } + /// Idempotent: keeps the peripheral already in the map, if any. + /// + /// Was an `assert!` + `insert`. That is a check-then-act pair on a + /// concurrent `DashMap`, and the callers that reach it are inherently + /// racy — droidplug's `Adapter::report_scan_result` looks a peripheral + /// up, finds nothing, and then adds, with no lock held across the two + /// steps. Two scan results for the same device (which is what starting a + /// second scan on the process-global adapter produces) could both take + /// the `None` branch and the second `add` would abort the process. + /// + /// A panic here is especially hard to diagnose because the callers are + /// spawned tasks: Tokio stores the payload in the `JoinHandle` nobody + /// joins, and on Android the default hook writes to stderr, which is not + /// in logcat — so the symptom is a task that silently stops existing. + /// + /// Keeping the existing entry (rather than replacing it) is deliberate: + /// it may already carry connection state and characteristics that a + /// freshly constructed wrapper for the same address would not. pub fn add_peripheral(&self, peripheral: PeripheralType) { - assert!( - !self.peripherals.contains_key(&peripheral.id()), - "Adding a peripheral that's already in the map." - ); - self.peripherals.insert(peripheral.id(), peripheral); + let id = peripheral.id(); + self.peripherals.entry(id).or_insert(peripheral); } pub fn clear_peripherals(&self) { diff --git a/src/droidplug/adapter.rs b/src/droidplug/adapter.rs index 36d49e61..2a82d6a6 100644 --- a/src/droidplug/adapter.rs +++ b/src/droidplug/adapter.rs @@ -87,11 +87,25 @@ impl Adapter { } fn add(&self, address: BDAddr) -> Result { + // Fast path: another scan result for this address may have added it + // between our caller's lookup and here. Returning the instance the + // map already holds — rather than a second wrapper for the same + // address — keeps `report_properties` writing to the peripheral + // everyone else will later read. + if let Some(existing) = self.manager.peripheral(&PeripheralId(address)) { + return Ok(existing); + } jvm()?.attach_current_thread(|env| { let local_adapter = env.new_local_ref(self.internal.as_obj())?; let peripheral = Peripheral::new(env, local_adapter, address)?; self.manager.add_peripheral(peripheral.clone()); - Ok(peripheral) + // `add_peripheral` is idempotent, so if we lost the race the map + // kept the winner; hand that back rather than our now-orphaned + // wrapper. + Ok(self + .manager + .peripheral(&PeripheralId(address)) + .unwrap_or(peripheral)) }) }