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)) }) }