fix(bluetooth-sdk/ios): make ObservableStore thread-safe#3104
Open
cedricshan wants to merge 1 commit into
Open
fix(bluetooth-sdk/ios): make ObservableStore thread-safe#3104cedricshan wants to merge 1 commit into
cedricshan wants to merge 1 commit into
Conversation
ObservableStore is annotated @mainactor, but it is reached from BLE background callbacks through the Obj-C/React Native bridge, which bypasses Swift actor isolation. The unsynchronized access races on the backing `values` dictionary and intermittently crashes the app with: -[__NSTaggedDate countByEnumeratingWithState:objects:count:]: unrecognized selector sent to instance 0x8000000000000000 The crash surfaces inside getCategory()'s `for (key, value) in values` enumeration (via DeviceStore.shared.store.getCategory in the bluetoothStatus getter, during dispatchStoreUpdate) once the dictionary reference has been corrupted by the race. Guard all reads/writes of `values` and `listeners` with an NSRecursiveLock (recursive so a listener firing synchronously on the same thread can safely re-enter). In set(), listeners are snapshotted under the lock and the lock is released before emitting so a cross-thread re-entrant listener cannot deadlock. Co-authored-by: Cursor <cursoragent@cursor.com>
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.
Summary
ObservableStore(iOS) is annotated@MainActor, but it is reached from BLE background callbacks through the Obj-C / React Native bridge, which bypasses Swift actor isolation. The unsynchronized access races on the backingvaluesdictionary and intermittently crashes the app with:Crash backtrace (observed in Xcode, main thread aborts)
The crash surfaces inside
getCategory()'sfor (key, value) in valuesenumeration once the dictionary reference has been corrupted by the data race (the corrupted pointer0x8000000000000000is a taggedNSDate). Because it is a race, it is intermittent — it typically fires after the BLE link has been up for a while and status updates are flowing.Fix
Guard all reads/writes of
valuesandlistenerswith anNSRecursiveLock:set(), listeners are snapshotted under the lock and the lock is released before emitting, so a cross-thread re-entrant listener cannot deadlock.set,get,getCategory,wouldSkipSet,addListener,removeListener.This makes the dictionary access safe regardless of the calling thread without changing the public API or the
@MainActorannotation.Notes / scope
ObservableStore.ktis out of scope for this PR.Test plan
__NSTaggedDateabort no longer reproduces over extended BLE status streaming where it previously crashed intermittently.Made with Cursor
Note
Medium Risk
Touches shared BLE state used on every status update; locking changes concurrency semantics but is narrowly scoped and preserves existing emit behavior.
Overview
Fixes intermittent iOS crashes from unsynchronized access to
ObservableStore's backing dictionaries when BLE / RN bridge callbacks hit the store off the main thread despite@MainActor.The change adds an
NSRecursiveLockaround all reads and writes tovaluesandlisteners(get,set,getCategory,wouldSkipSet,addListener,removeListener). Inset(), listeners are snapshotted under the lock and notifications run after unlock so cross-thread re-entry cannot deadlock. Public API and emit / skip-if-unchanged behavior stay the same.Reviewed by Cursor Bugbot for commit 0c0fbb5. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Make the iOS
ObservableStorethread-safe to stop intermittent crashes caused by BLE background callbacks bypassing@MainActor. Fixes the-[__NSTaggedDate …]: unrecognized selectorcrash during category enumeration.valuesandlistenerswithNSRecursiveLock.set, snapshot listeners under the lock and unlock before emitting to avoid re-entrant deadlocks.set,get,getCategory,wouldSkipSet,addListener,removeListener; no API/behavior changes; iOS only.Written for commit 0c0fbb5. Summary will update on new commits.