Repository navigation
fix(corebluetooth): Stop the CoreBluetooth thread once its adapter and peripherals are dropped - #508
Open
HermannBjorgvin wants to merge 1 commit into
Open
HermannBjorgvin wants to merge 1 commit into
HermannBjorgvin wants to merge 1 commit into
Conversation
…d peripherals are dropped Each Adapter started a thread that looped forever, so every Manager::adapters() call leaked a thread and its CBCentralManager (deviceplug#500). The message channel can't signal the end, because the adapter's event task and peripheral map hold senders for as long as the thread runs. The adapter, every peripheral it hands out and the event and notification streams from them now hold a shared KeepAlive (the sender of a oneshot channel). The thread stops when the last of them is dropped. The adapter's internal copies of peripherals don't hold one, so they don't keep the thread alive.
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.
Fixes #500.
Not run on macOS or iOS. I don't have a Mac, so this is only compiled. It needs a test on a Mac before merging.
Each
Adapter::new(), one perManager::adapters()call, starts a thread that loops onwait_for_message()forever, so every call leaks a thread and itsCBCentralManager. The thread can't stop when its message channel closes, because the adapter's event task and peripheral map hold senders for as long as the thread runs.KeepAlive(src/corebluetooth/adapter.rs) is the sender of a oneshot channel, shared through anArc. TheAdapterholds one. So do everyPeripheralreturned byperipherals(),peripheral()andretrieve_peripherals()(Peripheral::with_keep_aliveinsrc/corebluetooth/peripheral.rs), and the streams fromevents()andnotifications()(keep_alive_stream). The adapter's internal copies of peripherals don't hold one.run_corebluetooth_thread(src/corebluetooth/internal.rs) polls the oneshot receiver beforewait_for_message(), so a busy delegate channel can't starve it, and stops once the lastKeepAliveis dropped.CoreBluetoothInternal, with itsCBCentralManager, is dropped on that thread.Adapteris dropped, as before.Not covered:
CoreBluetoothInternal::newcreates a dispatch queue that is never released, so each adapter still leaks that queue object (not a thread).To test on a Mac: call
Manager::adapters()in a loop, drop the adapters, and watch the thread count. The integration tests against the test peripheral would also cover the keep-alive paths.Agent review
Before opening, separate AI agents (Claude Opus) reviewed the plan and the diff. Their findings are addressed here, except the dispatch queue leak above, which predates this change.
Human involvement
Agent-written (Claude), from reading the code after we found that Clawdmeter's desktop app calls
Manager::adapters()on every reconnect attempt. I haven't run it on a Mac or reviewed the diff.