Conversation
`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>
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
add_peripheralasserts the peripheral is absent and then inserts it. That is acheck-then-act pair on a concurrent
DashMap, and the caller that reaches it isinherently racy: droidplug's
Adapter::report_scan_resultlooks a peripheral up,takes the
Nonebranch, and only then callsadd, holding nothing across the twosteps.
Two scan results for the same device can therefore both observe
Noneand bothadd, and the second aborts the process. It is easy to hit on Android because
Manager::adapters()returns a process-global singleton adapter: an applicationthat 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
JoinHandlenobody joins, and Android's defaultpanic 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::addreturns the map's instance for the same reason, soa 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 checkandcargo clippy --target aarch64-linux-androidare cleanon master.
Two things I would take direction on:
report_scan_resultcall site thanmake
add_peripheralidempotent;clear_peripheralsandDeviceDisconnectedinteract with a concurrentscan in ways this does not cover.
Happy to add a CHANGELOG entry if you want one.
🤖 Generated with Claude Code