Skip to content

Make AdapterManager::add_peripheral idempotent - #475

Closed
DaveMeade wants to merge 1 commit into
deviceplug:masterfrom
DaveMeade:fix/add-peripheral-idempotent
Closed

DaveMeade wants to merge 1 commit into
deviceplug:masterfrom
DaveMeade:fix/add-peripheral-idempotent

Conversation

@DaveMeade

Copy link
Copy Markdown

add_peripheral asserts the peripheral is absent and then inserts 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. It is easy to hit on Android because
Manager::adapters() returns a process-global singleton adapter: an application
that starts a second scan, directly or through 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, and every layer above reports a plain timeout.

The map keeps the entry it already has rather than replacing it: that one may
carry connection state a freshly constructed wrapper for the same address would
not. droidplug::Adapter::add returns the map's instance for the same reason, so
a wrapper that loses the race cannot shadow the live one.

Found against 0.12.0 in an app that scans on Android, and carried on a fork since
August. cargo check and cargo clippy --target aarch64-linux-android are clean
on master.

Two things I would take direction on:

  • whether you would rather fix the race at the report_scan_result call site than
    make add_peripheral idempotent;
  • whether clear_peripherals and DeviceDisconnected interact with a concurrent
    scan in ways this does not cover.

Happy to add a CHANGELOG entry if you want one.

🤖 Generated with Claude Code

`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) <noreply@anthropic.com>
@qdot

qdot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Ok, this came into dev via a rebase, but I had to add a follow-on commit because your claude went wild on the comments, and also because this fixed the existing android issue while adding a new race to windows.

Also, we're on 0.13.x now, you'll want to update to that for future use.

@qdot qdot closed this Sep 17, 2026
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