From be6d36346d00c73b79f670df12ded9829f7f8c36 Mon Sep 17 00:00:00 2001 From: Andreas Bauer Date: Mon, 4 Nov 2024 17:28:16 +0100 Subject: [PATCH] Maintain Shadow state for observable state on MainActor --- Sources/SpeziBluetooth/Bluetooth.swift | 4 +- .../Model/BluetoothManagerStorage.swift | 35 ++++---- .../Model/MainActorBuffered.swift | 85 +++++++++++++++++++ .../ManagedAtomicMainActorBuffered.swift | 74 ++++++++++++++++ .../Model/PeripheralStorage.swift | 61 ++++++------- .../Model/Properties/Characteristic.swift | 14 ++- .../Model/Properties/Service.swift | 6 +- .../DeviceStateTestInjections.swift | 12 +-- .../ConnectedDevicesEnvironmentModifier.swift | 4 +- .../Utils/ConnectedDevices.swift | 3 +- 10 files changed, 223 insertions(+), 75 deletions(-) create mode 100644 Sources/SpeziBluetooth/CoreBluetooth/Model/MainActorBuffered.swift create mode 100644 Sources/SpeziBluetooth/CoreBluetooth/Model/ManagedAtomicMainActorBuffered.swift diff --git a/Sources/SpeziBluetooth/Bluetooth.swift b/Sources/SpeziBluetooth/Bluetooth.swift index d965ac56..b1b544e6 100644 --- a/Sources/SpeziBluetooth/Bluetooth.swift +++ b/Sources/SpeziBluetooth/Bluetooth.swift @@ -234,7 +234,9 @@ import Spezi public final class Bluetooth: Module, EnvironmentAccessible, Sendable { @Observable class Storage { - var nearbyDevices: OrderedDictionary = [:] + @MainActor var nearbyDevices: OrderedDictionary = [:] + + nonisolated init() {} } nonisolated static let logger = Logger(subsystem: "edu.stanford.spezi.bluetooth", category: "Bluetooth") diff --git a/Sources/SpeziBluetooth/CoreBluetooth/Model/BluetoothManagerStorage.swift b/Sources/SpeziBluetooth/CoreBluetooth/Model/BluetoothManagerStorage.swift index a677990b..e44fc7b1 100644 --- a/Sources/SpeziBluetooth/CoreBluetooth/Model/BluetoothManagerStorage.swift +++ b/Sources/SpeziBluetooth/CoreBluetooth/Model/BluetoothManagerStorage.swift @@ -13,10 +13,10 @@ import OrderedCollections @Observable final class BluetoothManagerStorage: ValueObservable, Sendable { - private let _isScanning = ManagedAtomic(false) - private let _state = ManagedAtomic(.unknown) + private let _isScanning = ManagedAtomicMainActorBuffered(false) + private let _state = ManagedAtomicMainActorBuffered(.unknown) - @ObservationIgnored private nonisolated(unsafe) var _discoveredPeripherals: OrderedDictionary = [:] + private let _discoveredPeripherals: MainActorBuffered> = .init([:]) private let rwLock = RWLock() @SpeziBluetooth var retrievedPeripherals: OrderedDictionary> = [:] { @@ -46,9 +46,7 @@ final class BluetoothManagerStorage: ValueObservable, Sendable { @inlinable var readOnlyDiscoveredPeripherals: OrderedDictionary { access(keyPath: \._discoveredPeripherals) - return rwLock.withReadLock { - _discoveredPeripherals - } + return _discoveredPeripherals.load(using: rwLock) } @SpeziBluetooth var state: BluetoothState { @@ -56,10 +54,13 @@ final class BluetoothManagerStorage: ValueObservable, Sendable { readOnlyState } set { - withMutation(keyPath: \._state) { - _state.store(newValue, ordering: .relaxed) + let didChange = _state.storeAndCompare(newValue) { @Sendable mutation in + self.withMutation(keyPath: \._state, mutation) + } + + if didChange { + _$simpleRegistrar.triggerDidChange(for: \.state, on: self) } - _$simpleRegistrar.triggerDidChange(for: \.state, on: self) for continuation in subscribedContinuations.values { continuation.yield(state) @@ -72,10 +73,13 @@ final class BluetoothManagerStorage: ValueObservable, Sendable { readOnlyIsScanning } set { - withMutation(keyPath: \._isScanning) { - _isScanning.store(newValue, ordering: .relaxed) + let didChange = _isScanning.storeAndCompare(newValue) { @Sendable mutation in + self.withMutation(keyPath: \._isScanning, mutation) + } + + if didChange { + _$simpleRegistrar.triggerDidChange(for: \.isScanning, on: self) // didSet } - _$simpleRegistrar.triggerDidChange(for: \.isScanning, on: self) // didSet } } @@ -84,11 +88,10 @@ final class BluetoothManagerStorage: ValueObservable, Sendable { readOnlyDiscoveredPeripherals } set { - withMutation(keyPath: \._discoveredPeripherals) { - rwLock.withWriteLock { - _discoveredPeripherals = newValue - } + _discoveredPeripherals.store(newValue, using: rwLock) { @Sendable mutation in + self.withMutation(keyPath: \._discoveredPeripherals, mutation) } + _$simpleRegistrar.triggerDidChange(for: \.discoveredPeripherals, on: self) // didSet } } diff --git a/Sources/SpeziBluetooth/CoreBluetooth/Model/MainActorBuffered.swift b/Sources/SpeziBluetooth/CoreBluetooth/Model/MainActorBuffered.swift new file mode 100644 index 00000000..4c4f6d89 --- /dev/null +++ b/Sources/SpeziBluetooth/CoreBluetooth/Model/MainActorBuffered.swift @@ -0,0 +1,85 @@ +// +// This source file is part of the Stanford Spezi open-source project +// +// SPDX-FileCopyrightText: 2024 Stanford University and the project authors (see CONTRIBUTORS.md) +// +// SPDX-License-Identifier: MIT +// + +import Foundation +import SpeziFoundation + + +final class MainActorBuffered: Sendable { + private nonisolated(unsafe) var unsafeValue: Value + @MainActor private(set) var mainActorValue: Value? + + init(_ value: Value) { + self.unsafeValue = value + self.mainActorValue = value + } + + func loadUnsafe() -> Value { + loadIfMainActor() ?? unsafeValue + } + + func load(using lock: NSLock) -> Value { + loadIfMainActor() ?? lock.withLock { + unsafeValue + } + } + + func load(using lock: RWLock) -> Value { + loadIfMainActor() ?? lock.withReadLock { + unsafeValue + } + } + + private func loadIfMainActor() -> Value? { + if Thread.isMainThread { + MainActor.assumeIsolated { + mainActorValue + } + } else { + nil + } + } + + private func _store(_ newValue: Value, mutation: sending @MainActor @escaping (@MainActor () -> Void) -> Void) { + Task { @MainActor in + let valueMutation = { @MainActor in + self.mainActorValue = newValue + } + + mutation(valueMutation) + } + } + + func store(_ newValue: Value, using lock: NSLock, mutation: sending @MainActor @escaping (@MainActor () -> Void) -> Void) { + lock.withLock { + unsafeValue = newValue + } + _store(newValue, mutation: mutation) + } + + func store(_ newValue: Value, using lock: RWLock, mutation: sending @MainActor @escaping (@MainActor () -> Void) -> Void) { + lock.withWriteLock { + unsafeValue = newValue + } + _store(newValue, mutation: mutation) + } +} + + +extension MainActorBuffered where Value: Equatable { + func storeAndCompare(_ newValue: Value, using lock: RWLock, mutation: sending @MainActor @escaping (@MainActor () -> Void) -> Void) -> Bool { + let didChange = lock.withWriteLock { + let didChange = unsafeValue != newValue + unsafeValue = newValue + return didChange + } + _store(newValue, mutation: mutation) + + return didChange + } +} diff --git a/Sources/SpeziBluetooth/CoreBluetooth/Model/ManagedAtomicMainActorBuffered.swift b/Sources/SpeziBluetooth/CoreBluetooth/Model/ManagedAtomicMainActorBuffered.swift new file mode 100644 index 00000000..6ce665dd --- /dev/null +++ b/Sources/SpeziBluetooth/CoreBluetooth/Model/ManagedAtomicMainActorBuffered.swift @@ -0,0 +1,74 @@ +// +// This source file is part of the Stanford Spezi open-source project +// +// SPDX-FileCopyrightText: 2024 Stanford University and the project authors (see CONTRIBUTORS.md) +// +// SPDX-License-Identifier: MIT +// + +import Atomics +import Foundation + + +final class ManagedAtomicMainActorBuffered: Sendable where Value.AtomicRepresentation.Value == Value { + private let managedValue: ManagedAtomic + @MainActor private var mainActorValue: Value? + + init(_ value: Value) { + self.managedValue = ManagedAtomic(value) + self.mainActorValue = value + } + + @_semantics("atomics.requires_constant_orderings") + @inlinable + func load(ordering: AtomicLoadOrdering = .relaxed) -> Value { + if Thread.isMainThread { + MainActor.assumeIsolated { + mainActorValue + } ?? managedValue.load(ordering: ordering) + } else { + managedValue.load(ordering: ordering) + } + } + + @_semantics("atomics.requires_constant_orderings") + private func mutateMainActorBuffer( + _ newValue: Value, + mutation: sending @MainActor @escaping (@MainActor () -> Void) -> Void + ) { + Task { @MainActor in + let valueMutation = { @MainActor in + self.mainActorValue = newValue + } + + mutation(valueMutation) + } + } + + @_semantics("atomics.requires_constant_orderings") + @inlinable + func store( + _ newValue: Value, + ordering: AtomicStoreOrdering = .relaxed, + mutation: sending @MainActor @escaping (@MainActor () -> Void) -> Void + ) { + managedValue.store(newValue, ordering: ordering) + mutateMainActorBuffer(newValue, mutation: mutation) + } +} + + +extension ManagedAtomicMainActorBuffered where Value: Equatable { + @_semantics("atomics.requires_constant_orderings") + @inlinable + func storeAndCompare( + _ newValue: Value, + ordering: AtomicUpdateOrdering = .relaxed, + mutation: sending @MainActor @escaping (@MainActor () -> Void) -> Void + ) -> Bool { + let previousValue = managedValue.exchange(newValue, ordering: ordering) + mutateMainActorBuffer(newValue, mutation: mutation) + + return previousValue != newValue + } +} diff --git a/Sources/SpeziBluetooth/CoreBluetooth/Model/PeripheralStorage.swift b/Sources/SpeziBluetooth/CoreBluetooth/Model/PeripheralStorage.swift index d96d8192..95b28da4 100644 --- a/Sources/SpeziBluetooth/CoreBluetooth/Model/PeripheralStorage.swift +++ b/Sources/SpeziBluetooth/CoreBluetooth/Model/PeripheralStorage.swift @@ -16,14 +16,14 @@ import Foundation /// into a separate state container that is `@Observable`. @Observable final class PeripheralStorage: ValueObservable, Sendable { - private let _state: ManagedAtomic - private let _rssi: ManagedAtomic - private let _nearby: ManagedAtomic + private let _state: ManagedAtomicMainActorBuffered + private let _rssi: ManagedAtomicMainActorBuffered + private let _nearby: ManagedAtomicMainActorBuffered private let _lastActivityTimeIntervalSince1970BitPattern: ManagedAtomic // workaround to store store Date atomically // swiftlint:disable:previous identifier_name - @ObservationIgnored private nonisolated(unsafe) var _peripheralName: String? - @ObservationIgnored private nonisolated(unsafe) var _advertisementData: AdvertisementData + private let _peripheralName: MainActorBuffered + private let _advertisementData: MainActorBuffered // Its fine to have a single lock. Readers will be isolated anyways to the SpeziBluetooth global actor. // The only side-effect is, that readers will wait for any write to complete, which is fine as peripheralName is rarely updated. private let lock = RWLock() @@ -45,8 +45,9 @@ final class PeripheralStorage: ValueObservable, Sendable { @inlinable var name: String? { access(keyPath: \._peripheralName) access(keyPath: \._advertisementData) + return lock.withReadLock { - _peripheralName ?? _advertisementData.localName + _peripheralName.loadUnsafe() ?? _advertisementData.loadUnsafe().localName } } @@ -67,9 +68,7 @@ final class PeripheralStorage: ValueObservable, Sendable { @inlinable var readOnlyAdvertisementData: AdvertisementData { access(keyPath: \._advertisementData) - return lock.withReadLock { - _advertisementData - } + return _advertisementData.load(using: lock) } var readOnlyLastActivity: Date { @@ -80,16 +79,11 @@ final class PeripheralStorage: ValueObservable, Sendable { @SpeziBluetooth var peripheralName: String? { get { access(keyPath: \._peripheralName) - return lock.withReadLock { - _peripheralName - } + return _peripheralName.load(using: lock) } set { - let didChange = newValue != _peripheralName - withMutation(keyPath: \._peripheralName) { - lock.withWriteLock { - _peripheralName = newValue - } + let didChange = _peripheralName.storeAndCompare(newValue, using: lock) { @Sendable mutation in + self.withMutation(keyPath: \._peripheralName, mutation) } if didChange { @@ -103,9 +97,8 @@ final class PeripheralStorage: ValueObservable, Sendable { readOnlyRssi } set { - let didChange = newValue != readOnlyRssi - withMutation(keyPath: \._rssi) { - _rssi.store(newValue, ordering: .relaxed) + let didChange = _rssi.storeAndCompare(newValue) { @Sendable mutation in + self.withMutation(keyPath: \._rssi, mutation) } if didChange { _$simpleRegistrar.triggerDidChange(for: \.rssi, on: self) @@ -118,11 +111,8 @@ final class PeripheralStorage: ValueObservable, Sendable { readOnlyAdvertisementData } set { - let didChange = newValue != _advertisementData - withMutation(keyPath: \._advertisementData) { - lock.withWriteLock { - _advertisementData = newValue - } + let didChange = _advertisementData.storeAndCompare(newValue, using: lock) { @Sendable mutation in + self.withMutation(keyPath: \._advertisementData, mutation) } if didChange { @@ -136,10 +126,10 @@ final class PeripheralStorage: ValueObservable, Sendable { readOnlyState } set { - let didChange = newValue != readOnlyState - withMutation(keyPath: \._state) { - _state.store(newValue, ordering: .relaxed) + let didChange = _state.storeAndCompare(newValue) { @Sendable mutation in + self.withMutation(keyPath: \._state, mutation) } + if didChange { _$simpleRegistrar.triggerDidChange(for: \.state, on: self) } @@ -151,9 +141,8 @@ final class PeripheralStorage: ValueObservable, Sendable { readOnlyNearby } set { - let didChange = newValue != readOnlyNearby - withMutation(keyPath: \._nearby) { - _nearby.store(newValue, ordering: .relaxed) + let didChange = _nearby.storeAndCompare(newValue) { @Sendable mutation in + self.withMutation(keyPath: \._nearby, mutation) } if didChange { @@ -166,11 +155,11 @@ final class PeripheralStorage: ValueObservable, Sendable { @ObservationIgnored let _$simpleRegistrar = ValueObservationRegistrar() init(peripheralName: String?, rssi: Int, advertisementData: AdvertisementData, state: PeripheralState, lastActivity: Date = .now) { - self._peripheralName = peripheralName - self._advertisementData = advertisementData - self._rssi = ManagedAtomic(rssi) - self._state = ManagedAtomic(state) - self._nearby = ManagedAtomic(false) + self._peripheralName = MainActorBuffered(peripheralName) + self._advertisementData = MainActorBuffered(advertisementData) + self._rssi = ManagedAtomicMainActorBuffered(rssi) + self._state = ManagedAtomicMainActorBuffered(state) + self._nearby = ManagedAtomicMainActorBuffered(false) self._lastActivity = lastActivity self._lastActivityTimeIntervalSince1970BitPattern = ManagedAtomic(lastActivity.timeIntervalSince1970.bitPattern) } diff --git a/Sources/SpeziBluetooth/Model/Properties/Characteristic.swift b/Sources/SpeziBluetooth/Model/Properties/Characteristic.swift index f1de5a05..2ec2d183 100644 --- a/Sources/SpeziBluetooth/Model/Properties/Characteristic.swift +++ b/Sources/SpeziBluetooth/Model/Properties/Characteristic.swift @@ -226,7 +226,7 @@ public struct Characteristic: Sendable { } } - @ObservationIgnored private nonisolated(unsafe) var _value: Value? + private let _value: MainActorBuffered @ObservationIgnored private nonisolated(unsafe) var _capture: CharacteristicCaptureRetrieval? // protects both properties above private let lock = RWLock() @@ -241,9 +241,7 @@ public struct Characteristic: Sendable { @inlinable var readOnlyValue: Value? { access(keyPath: \._value) - return lock.withReadLock { - _value - } + return _value.load(using: lock) } var capture: GATTCharacteristicCapture? { @@ -263,15 +261,13 @@ public struct Characteristic: Sendable { } init(initialValue: Value?) { - self._value = initialValue + self._value = MainActorBuffered(initialValue) } @inlinable func inject(_ value: Value?) { - withMutation(keyPath: \._value) { - lock.withWriteLock { - _value = value - } + _value.store(value, using: lock) { @Sendable mutation in + self.withMutation(keyPath: \._value, mutation) } } } diff --git a/Sources/SpeziBluetooth/Model/Properties/Service.swift b/Sources/SpeziBluetooth/Model/Properties/Service.swift index 67397a47..e73b716c 100644 --- a/Sources/SpeziBluetooth/Model/Properties/Service.swift +++ b/Sources/SpeziBluetooth/Model/Properties/Service.swift @@ -66,7 +66,7 @@ public struct Service { } } - private let _serviceState = ManagedAtomic(.notPresent) + private let _serviceState = ManagedAtomicMainActorBuffered(.notPresent) var serviceState: ServiceState { get { @@ -74,8 +74,8 @@ public struct Service { return _serviceState.load(ordering: .relaxed) } set { - withMutation(keyPath: \.serviceState) { - _serviceState.store(newValue, ordering: .relaxed) + _serviceState.store(newValue) { @Sendable mutation in + self.withMutation(keyPath: \.serviceState, mutation) } } } diff --git a/Sources/SpeziBluetooth/Model/PropertySupport/DeviceStateTestInjections.swift b/Sources/SpeziBluetooth/Model/PropertySupport/DeviceStateTestInjections.swift index 447e89ae..905c6704 100644 --- a/Sources/SpeziBluetooth/Model/PropertySupport/DeviceStateTestInjections.swift +++ b/Sources/SpeziBluetooth/Model/PropertySupport/DeviceStateTestInjections.swift @@ -12,7 +12,7 @@ import Foundation @Observable final class DeviceStateTestInjections: Sendable { @ObservationIgnored private nonisolated(unsafe) var _subscriptions: ChangeSubscriptions? - @ObservationIgnored private nonisolated(unsafe) var _injectedValue: Value? + private let _injectedValue: MainActorBuffered = .init(nil) private let lock = NSLock() // protects both properties above var subscriptions: ChangeSubscriptions? { @@ -31,15 +31,11 @@ final class DeviceStateTestInjections: Sendable { var injectedValue: Value? { get { access(keyPath: \.injectedValue) - return lock.withLock { - _injectedValue - } + return _injectedValue.load(using: lock) } set { - withMutation(keyPath: \.injectedValue) { - lock.withLock { - _injectedValue = newValue - } + _injectedValue.store(newValue, using: lock) { @Sendable mutation in + self.withMutation(keyPath: \.injectedValue, mutation) } } } diff --git a/Sources/SpeziBluetooth/Modifier/ConnectedDevicesEnvironmentModifier.swift b/Sources/SpeziBluetooth/Modifier/ConnectedDevicesEnvironmentModifier.swift index f4405ac8..af07a48e 100644 --- a/Sources/SpeziBluetooth/Modifier/ConnectedDevicesEnvironmentModifier.swift +++ b/Sources/SpeziBluetooth/Modifier/ConnectedDevicesEnvironmentModifier.swift @@ -13,6 +13,8 @@ private struct ConnectedDeviceEnvironmentModifier: View @Environment(ConnectedDevicesModel.self) var connectedDevices + @State private var devicesList = ConnectedDevices() + init() {} @@ -23,7 +25,7 @@ private struct ConnectedDeviceEnvironmentModifier: View device as? Device } - let devicesList = ConnectedDevices(connectedDevicesList) + let _ = devicesList.devices = connectedDevicesList // swiftlint:disable:this redundant_discardable_let content .environment(firstConnectedDevice) diff --git a/Sources/SpeziBluetooth/Utils/ConnectedDevices.swift b/Sources/SpeziBluetooth/Utils/ConnectedDevices.swift index 578e82cf..5d950637 100644 --- a/Sources/SpeziBluetooth/Utils/ConnectedDevices.swift +++ b/Sources/SpeziBluetooth/Utils/ConnectedDevices.swift @@ -32,8 +32,9 @@ import SwiftUI /// ``` @Observable public final class ConnectedDevices { - private let devices: [Device] + var devices: [Device] + @MainActor init(_ devices: [Device] = []) { self.devices = devices }