From 3823de9e77135b4cfa2c374159e75a60aa88586d Mon Sep 17 00:00:00 2001 From: Morgan Chen Date: Tue, 6 May 2025 12:25:23 -0400 Subject: [PATCH 1/5] Morgan's core internal work from mc/messaging --- .../Sources/FIRAllocatedUnfairLock.swift | 66 +++++++++++++++++++ .../HeartbeatLogging/HeartbeatStorage.swift | 10 ++- .../Sources/Utilities/AtomicBox.swift | 45 ------------- .../Tests/Unit/HeartbeatStorageTests.swift | 14 ++-- 4 files changed, 82 insertions(+), 53 deletions(-) create mode 100644 FirebaseCore/Internal/Sources/FIRAllocatedUnfairLock.swift delete mode 100644 FirebaseCore/Internal/Sources/Utilities/AtomicBox.swift diff --git a/FirebaseCore/Internal/Sources/FIRAllocatedUnfairLock.swift b/FirebaseCore/Internal/Sources/FIRAllocatedUnfairLock.swift new file mode 100644 index 00000000000..ae52faefce6 --- /dev/null +++ b/FirebaseCore/Internal/Sources/FIRAllocatedUnfairLock.swift @@ -0,0 +1,66 @@ +// Copyright 2025 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import Foundation +import os.lock + +/// A reference wrapper around `os_unfair_lock`. Replace this class with +/// `OSAllocatedUnfairLock` once we support only iOS 16+. For an explanation +/// on why this is necessary, see the docs: +/// https://developer.apple.com/documentation/os/osallocatedunfairlock +public final class FIRAllocatedUnfairLock: @unchecked Sendable { + private var lockPointer: UnsafeMutablePointer + private var state: State + + public init(initialState: sending State) { + lockPointer = UnsafeMutablePointer + .allocate(capacity: 1) + lockPointer.initialize(to: os_unfair_lock()) + state = initialState + } + + public convenience init() where State == Void { + self.init(initialState: ()) + } + + public func lock() { + os_unfair_lock_lock(lockPointer) + } + + public func unlock() { + os_unfair_lock_unlock(lockPointer) + } + + @discardableResult + public func withLock(_ body: (inout State) throws -> R) rethrows -> R { + let value: R + lock() + defer { unlock() } + value = try body(&state) + return value + } + + @discardableResult + public func withLock(_ body: () throws -> R) rethrows -> R { + let value: R + lock() + defer { unlock() } + value = try body() + return value + } + + deinit { + lockPointer.deallocate() + } +} diff --git a/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift b/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift index f28c2038582..57a0b571e6c 100644 --- a/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift +++ b/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift @@ -52,9 +52,15 @@ final class HeartbeatStorage: Sendable, HeartbeatStorageProtocol { // MARK: - Instance Management /// Statically allocated cache of `HeartbeatStorage` instances keyed by string IDs. - private nonisolated(unsafe) static var cachedInstances: AtomicBox< + // In Swift 6, this property is not concurrency-safe because it is + // nonisolated global shared mutable state. Because this target's + // deployment version does not support Swift concurrency, it is marked as + // `nonisolated(unsafe)` to disable concurrency-safety checks. The + // property's access is protected by an external synchronization mechanism + // (see `FIRAllocatedUnfairLock` type). + private static let cachedInstances: FIRAllocatedUnfairLock< [String: WeakContainer] - > = AtomicBox([:]) + > = FIRAllocatedUnfairLock(initialState: [:]) /// Gets an existing `HeartbeatStorage` instance with the given `id` if one exists. Otherwise, /// makes a new instance with the given `id`. diff --git a/FirebaseCore/Internal/Sources/Utilities/AtomicBox.swift b/FirebaseCore/Internal/Sources/Utilities/AtomicBox.swift deleted file mode 100644 index 6346d05701c..00000000000 --- a/FirebaseCore/Internal/Sources/Utilities/AtomicBox.swift +++ /dev/null @@ -1,45 +0,0 @@ -// Copyright 2025 Google LLC -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -import Foundation - -final class AtomicBox { - private var _value: T - private let lock = NSLock() - - public init(_ value: T) { - _value = value - } - - public func value() -> T { - lock.withLock { - _value - } - } - - @discardableResult - public func withLock(_ mutatingBody: (_ value: inout T) -> Void) -> T { - lock.withLock { - mutatingBody(&_value) - return _value - } - } - - @discardableResult - public func withLock(_ mutatingBody: (_ value: inout T) throws -> R) rethrows -> R { - try lock.withLock { - try mutatingBody(&_value) - } - } -} diff --git a/FirebaseCore/Internal/Tests/Unit/HeartbeatStorageTests.swift b/FirebaseCore/Internal/Tests/Unit/HeartbeatStorageTests.swift index 8d5a03f942c..826590b219c 100644 --- a/FirebaseCore/Internal/Tests/Unit/HeartbeatStorageTests.swift +++ b/FirebaseCore/Internal/Tests/Unit/HeartbeatStorageTests.swift @@ -405,13 +405,13 @@ class HeartbeatStorageTests: XCTestCase { // type '[WeakContainer]' to a `@Sendable` closure // (`DispatchQueue.global().async { ... }`). final class WeakRefs: @unchecked Sendable { - private(set) var weakRefs: [WeakContainer] = [] // Lock is used to synchronize `weakRefs` during concurrent access. - private let weakRefsLock = NSLock() + private(set) var weakRefs = + FIRAllocatedUnfairLock<[WeakContainer]>(initialState: []) func append(_ weakRef: WeakContainer) { - weakRefsLock.withLock { - weakRefs.append(weakRef) + weakRefs.withLock { + $0.append(weakRef) } } } @@ -436,8 +436,10 @@ class HeartbeatStorageTests: XCTestCase { // Then // The `weakRefs` array's references should all be nil; otherwise, something is being // unexpectedly strongly retained. - for weakRef in weakRefs.weakRefs { - XCTAssertNil(weakRef.object, "Potential memory leak detected.") + weakRefs.weakRefs.withLock { refs in + for weakRef in refs { + XCTAssertNil(weakRef.object, "Potential memory leak detected.") + } } } } From 86b5071198c29d2e4c9d7275a24c0f9908a622e2 Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Wed, 7 May 2025 11:22:39 -0400 Subject: [PATCH 2/5] Don't delete AtomicBox yet --- .../Sources/Utilities/AtomicBox.swift | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 FirebaseCore/Internal/Sources/Utilities/AtomicBox.swift diff --git a/FirebaseCore/Internal/Sources/Utilities/AtomicBox.swift b/FirebaseCore/Internal/Sources/Utilities/AtomicBox.swift new file mode 100644 index 00000000000..6346d05701c --- /dev/null +++ b/FirebaseCore/Internal/Sources/Utilities/AtomicBox.swift @@ -0,0 +1,45 @@ +// Copyright 2025 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import Foundation + +final class AtomicBox { + private var _value: T + private let lock = NSLock() + + public init(_ value: T) { + _value = value + } + + public func value() -> T { + lock.withLock { + _value + } + } + + @discardableResult + public func withLock(_ mutatingBody: (_ value: inout T) -> Void) -> T { + lock.withLock { + mutatingBody(&_value) + return _value + } + } + + @discardableResult + public func withLock(_ mutatingBody: (_ value: inout T) throws -> R) rethrows -> R { + try lock.withLock { + try mutatingBody(&_value) + } + } +} From 17b163733c34bb11d014c5163a98b6d0007bbf88 Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Wed, 7 May 2025 11:23:11 -0400 Subject: [PATCH 3/5] Move into utilities dir --- .../Internal/Sources/{ => Utilities}/FIRAllocatedUnfairLock.swift | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename FirebaseCore/Internal/Sources/{ => Utilities}/FIRAllocatedUnfairLock.swift (100%) diff --git a/FirebaseCore/Internal/Sources/FIRAllocatedUnfairLock.swift b/FirebaseCore/Internal/Sources/Utilities/FIRAllocatedUnfairLock.swift similarity index 100% rename from FirebaseCore/Internal/Sources/FIRAllocatedUnfairLock.swift rename to FirebaseCore/Internal/Sources/Utilities/FIRAllocatedUnfairLock.swift From cf1432cf4b161231cafbab490a759a80ba9e0e72 Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Wed, 7 May 2025 11:26:17 -0400 Subject: [PATCH 4/5] remove comment --- .../Sources/HeartbeatLogging/HeartbeatStorage.swift | 6 ------ 1 file changed, 6 deletions(-) diff --git a/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift b/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift index 57a0b571e6c..224426e086a 100644 --- a/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift +++ b/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift @@ -52,12 +52,6 @@ final class HeartbeatStorage: Sendable, HeartbeatStorageProtocol { // MARK: - Instance Management /// Statically allocated cache of `HeartbeatStorage` instances keyed by string IDs. - // In Swift 6, this property is not concurrency-safe because it is - // nonisolated global shared mutable state. Because this target's - // deployment version does not support Swift concurrency, it is marked as - // `nonisolated(unsafe)` to disable concurrency-safety checks. The - // property's access is protected by an external synchronization mechanism - // (see `FIRAllocatedUnfairLock` type). private static let cachedInstances: FIRAllocatedUnfairLock< [String: WeakContainer] > = FIRAllocatedUnfairLock(initialState: [:]) From ca145274c1635490756889e89aa950ebd0f99642 Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Wed, 7 May 2025 11:32:38 -0400 Subject: [PATCH 5/5] Style --- FirebaseCore/Internal/Tests/Unit/HeartbeatStorageTests.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/FirebaseCore/Internal/Tests/Unit/HeartbeatStorageTests.swift b/FirebaseCore/Internal/Tests/Unit/HeartbeatStorageTests.swift index 826590b219c..c48cea653f7 100644 --- a/FirebaseCore/Internal/Tests/Unit/HeartbeatStorageTests.swift +++ b/FirebaseCore/Internal/Tests/Unit/HeartbeatStorageTests.swift @@ -407,7 +407,7 @@ class HeartbeatStorageTests: XCTestCase { final class WeakRefs: @unchecked Sendable { // Lock is used to synchronize `weakRefs` during concurrent access. private(set) var weakRefs = - FIRAllocatedUnfairLock<[WeakContainer]>(initialState: []) + FIRAllocatedUnfairLock<[WeakContainer]>(initialState: []) func append(_ weakRef: WeakContainer) { weakRefs.withLock {