From 45a5a4da09cee94e4a1c3d5a0da2344ba50cbad9 Mon Sep 17 00:00:00 2001 From: Nick Cooke <36927374+ncooke3@users.noreply.github.com> Date: Wed, 30 Oct 2024 09:55:10 -0400 Subject: [PATCH] [Core] Make storage class conform to Sendable and subsequent changes (#13939) --- .../HeartbeatController.swift | 24 +++---- .../HeartbeatLogging/HeartbeatStorage.swift | 20 +++--- .../Sources/HeartbeatLogging/Storage.swift | 29 +++++---- .../HeartbeatLogging/StorageFactory.swift | 6 +- .../_ObjC_HeartbeatController.swift | 2 +- .../Tests/Common/AdjustableDate.swift | 23 +++++++ .../HeartbeatLoggingIntegrationTests.swift | 6 +- .../Tests/Unit/HeartbeatControllerTests.swift | 37 ++++++----- .../Tests/Unit/HeartbeatStorageTests.swift | 63 +++++++++++-------- .../Internal/Tests/Unit/StorageTests.swift | 31 +++------ 10 files changed, 135 insertions(+), 106 deletions(-) create mode 100644 FirebaseCore/Internal/Tests/Common/AdjustableDate.swift diff --git a/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatController.swift b/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatController.swift index e7f4ece2781..cbc01844834 100644 --- a/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatController.swift +++ b/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatController.swift @@ -15,7 +15,7 @@ import Foundation /// An object that provides API to log and flush heartbeats from a synchronized storage container. -public final class HeartbeatController { +public final class HeartbeatController: Sendable { /// Used for standardizing dates for calendar-day comparison. private enum DateStandardizer { private static let calendar: Calendar = { @@ -31,18 +31,18 @@ public final class HeartbeatController { } /// The thread-safe storage object to log and flush heartbeats from. - private let storage: HeartbeatStorageProtocol + private let storage: any HeartbeatStorageProtocol /// The max capacity of heartbeats to store in storage. - private let heartbeatsStorageCapacity: Int = 30 + private static let heartbeatsStorageCapacity: Int = 30 /// Current date provider. It is used for testability. - private let dateProvider: () -> Date + private let dateProvider: @Sendable () -> Date /// Used for standardizing dates for calendar-day comparison. private static let dateStandardizer = DateStandardizer.self /// Public initializer. /// - Parameter id: The `id` to associate this controller's heartbeat storage with. public convenience init(id: String) { - self.init(id: id, dateProvider: Date.init) + self.init(id: id, dateProvider: { Date() }) } /// Convenience initializer. Mirrors the semantics of the public initializer with the added @@ -51,7 +51,7 @@ public final class HeartbeatController { /// - Parameters: /// - id: The id to associate this controller's heartbeat storage with. /// - dateProvider: A date provider. - convenience init(id: String, dateProvider: @escaping () -> Date) { + convenience init(id: String, dateProvider: @escaping @Sendable () -> Date) { let storage = HeartbeatStorage.getInstance(id: id) self.init(storage: storage, dateProvider: dateProvider) } @@ -61,7 +61,7 @@ public final class HeartbeatController { /// - storage: A heartbeat storage container. /// - dateProvider: A date provider. Defaults to providing the current date. init(storage: HeartbeatStorageProtocol, - dateProvider: @escaping () -> Date = Date.init) { + dateProvider: @escaping @Sendable () -> Date = { Date() }) { self.storage = storage self.dateProvider = { Self.dateStandardizer.standardize(dateProvider()) } } @@ -76,7 +76,7 @@ public final class HeartbeatController { storage.readAndWriteAsync { heartbeatsBundle in var heartbeatsBundle = heartbeatsBundle ?? - HeartbeatsBundle(capacity: self.heartbeatsStorageCapacity) + HeartbeatsBundle(capacity: Self.heartbeatsStorageCapacity) // Filter for the time periods where the last heartbeat to be logged for // that time period was logged more than one time period (i.e. day) ago. @@ -109,7 +109,7 @@ public final class HeartbeatController { // The new value that's stored will use the old's cache to prevent the // logging of duplicates after flushing. return HeartbeatsBundle( - capacity: self.heartbeatsStorageCapacity, + capacity: Self.heartbeatsStorageCapacity, cache: oldHeartbeatsBundle.lastAddedHeartbeatDates ) } @@ -126,15 +126,15 @@ public final class HeartbeatController { } } - public func flushAsync(completionHandler: @escaping (HeartbeatsPayload) -> Void) { - let resetTransform = { (heartbeatsBundle: HeartbeatsBundle?) -> HeartbeatsBundle? in + public func flushAsync(completionHandler: @escaping @Sendable (HeartbeatsPayload) -> Void) { + let resetTransform = { @Sendable (heartbeatsBundle: HeartbeatsBundle?) -> HeartbeatsBundle? in guard let oldHeartbeatsBundle = heartbeatsBundle else { return nil // Storage was empty. } // The new value that's stored will use the old's cache to prevent the // logging of duplicates after flushing. return HeartbeatsBundle( - capacity: self.heartbeatsStorageCapacity, + capacity: Self.heartbeatsStorageCapacity, cache: oldHeartbeatsBundle.lastAddedHeartbeatDates ) } diff --git a/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift b/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift index 7fd808b4457..07088a5cf68 100644 --- a/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift +++ b/FirebaseCore/Internal/Sources/HeartbeatLogging/HeartbeatStorage.swift @@ -15,21 +15,22 @@ import Foundation /// A type that can perform atomic operations using block-based transformations. -protocol HeartbeatStorageProtocol { +protocol HeartbeatStorageProtocol: Sendable { func readAndWriteSync(using transform: (HeartbeatsBundle?) -> HeartbeatsBundle?) - func readAndWriteAsync(using transform: @escaping (HeartbeatsBundle?) -> HeartbeatsBundle?) + func readAndWriteAsync(using transform: @escaping @Sendable (HeartbeatsBundle?) + -> HeartbeatsBundle?) func getAndSet(using transform: (HeartbeatsBundle?) -> HeartbeatsBundle?) throws -> HeartbeatsBundle? - func getAndSetAsync(using transform: @escaping (HeartbeatsBundle?) -> HeartbeatsBundle?, - completion: @escaping (Result) -> Void) + func getAndSetAsync(using transform: @escaping @Sendable (HeartbeatsBundle?) -> HeartbeatsBundle?, + completion: @escaping @Sendable (Result) -> Void) } /// Thread-safe storage object designed for transforming heartbeat data that is persisted to disk. -final class HeartbeatStorage: HeartbeatStorageProtocol { +final class HeartbeatStorage: Sendable, HeartbeatStorageProtocol { /// The identifier used to differentiate instances. private let id: String /// The underlying storage container to read from and write to. - private let storage: Storage + private let storage: any Storage /// The encoder used for encoding heartbeat data. private let encoder: JSONEncoder = .init() /// The decoder used for decoding heartbeat data. @@ -130,7 +131,8 @@ final class HeartbeatStorage: HeartbeatStorageProtocol { /// Asynchronously reads from and writes to storage using the given transform block. /// - Parameter transform: A block to transform the currently stored heartbeats bundle to a new /// heartbeats bundle value. - func readAndWriteAsync(using transform: @escaping (HeartbeatsBundle?) -> HeartbeatsBundle?) { + func readAndWriteAsync(using transform: @escaping @Sendable (HeartbeatsBundle?) + -> HeartbeatsBundle?) { queue.async { [self] in let oldHeartbeatsBundle = try? load(from: storage) let newHeartbeatsBundle = transform(oldHeartbeatsBundle) @@ -166,8 +168,8 @@ final class HeartbeatStorage: HeartbeatStorageProtocol { /// - completion: An escaping block used to process the heartbeat data that /// was stored (before the `transform` was applied); otherwise, the error /// that occurred. - func getAndSetAsync(using transform: @escaping (HeartbeatsBundle?) -> HeartbeatsBundle?, - completion: @escaping (Result) -> Void) { + func getAndSetAsync(using transform: @escaping @Sendable (HeartbeatsBundle?) -> HeartbeatsBundle?, + completion: @escaping @Sendable (Result) -> Void) { queue.async { do { let oldHeartbeatsBundle = try? self.load(from: self.storage) diff --git a/FirebaseCore/Internal/Sources/HeartbeatLogging/Storage.swift b/FirebaseCore/Internal/Sources/HeartbeatLogging/Storage.swift index a4cac33e27a..69bf613b690 100644 --- a/FirebaseCore/Internal/Sources/HeartbeatLogging/Storage.swift +++ b/FirebaseCore/Internal/Sources/HeartbeatLogging/Storage.swift @@ -15,7 +15,7 @@ import Foundation /// A type that reads from and writes to an underlying storage container. -protocol Storage { +protocol Storage: Sendable { /// Reads and returns the data stored by this storage type. /// - Returns: The data read from storage. /// - Throws: An error if the read failed. @@ -38,16 +38,12 @@ enum StorageError: Error { final class FileStorage: Storage { /// A file system URL to the underlying file resource. private let url: URL - /// The file manager used to perform file system operations. - private let fileManager: FileManager /// Designated initializer. /// - Parameters: /// - url: A file system URL for the underlying file resource. - /// - fileManager: A file manager. Defaults to `default` manager. - init(url: URL, fileManager: FileManager = .default) { + init(url: URL) { self.url = url - self.fileManager = fileManager } /// Reads and returns the data from this object's associated file resource. @@ -90,7 +86,7 @@ final class FileStorage: Storage { /// - Parameter url: The URL to create directories in. private func createDirectories(in url: URL) throws { do { - try fileManager.createDirectory( + try FileManager.default.createDirectory( at: url, withIntermediateDirectories: true ) @@ -104,17 +100,26 @@ final class FileStorage: Storage { /// A object that provides API for reading and writing to a user defaults resource. final class UserDefaultsStorage: Storage { - /// The underlying defaults container. - private let defaults: UserDefaults + /// The suite name for the underlying defaults container. + private let suiteName: String + /// The key mapping to the object's associated resource in `defaults`. private let key: String + /// The underlying defaults container. + private var defaults: UserDefaults { + // It's safe to force unwrap the below defaults instance because the + // initializer only returns `nil` when the bundle id or `globalDomain` + // is passed in as the `suiteName`. + UserDefaults(suiteName: suiteName)! + } + /// Designated initializer. /// - Parameters: - /// - defaults: The defaults container. + /// - suiteName: The suite name for the defaults container. /// - key: The key mapping to the value stored in the defaults container. - init(defaults: UserDefaults, key: String) { - self.defaults = defaults + init(suiteName: String, key: String) { + self.suiteName = suiteName self.key = key } diff --git a/FirebaseCore/Internal/Sources/HeartbeatLogging/StorageFactory.swift b/FirebaseCore/Internal/Sources/HeartbeatLogging/StorageFactory.swift index 6552a318158..d6d97cf78ba 100644 --- a/FirebaseCore/Internal/Sources/HeartbeatLogging/StorageFactory.swift +++ b/FirebaseCore/Internal/Sources/HeartbeatLogging/StorageFactory.swift @@ -56,11 +56,7 @@ extension FileManager { extension UserDefaultsStorage: StorageFactory { static func makeStorage(id: String) -> Storage { let suiteName = Constants.heartbeatUserDefaultsSuiteName - // It's safe to force unwrap the below defaults instance because the - // initializer only returns `nil` when the bundle id or `globalDomain` - // is passed in as the `suiteName`. - let defaults = UserDefaults(suiteName: suiteName)! let key = "heartbeats-\(id)" - return UserDefaultsStorage(defaults: defaults, key: key) + return UserDefaultsStorage(suiteName: suiteName, key: key) } } diff --git a/FirebaseCore/Internal/Sources/HeartbeatLogging/_ObjC_HeartbeatController.swift b/FirebaseCore/Internal/Sources/HeartbeatLogging/_ObjC_HeartbeatController.swift index c60a1e11cc5..520e4f96085 100644 --- a/FirebaseCore/Internal/Sources/HeartbeatLogging/_ObjC_HeartbeatController.swift +++ b/FirebaseCore/Internal/Sources/HeartbeatLogging/_ObjC_HeartbeatController.swift @@ -49,7 +49,7 @@ public class _ObjC_HeartbeatController: NSObject { /// /// - Note: This API is thread-safe. /// - Returns: A heartbeats payload for the flushed heartbeat(s). - public func flushAsync(completionHandler: @escaping (_ObjC_HeartbeatsPayload) -> Void) { + public func flushAsync(completionHandler: @escaping @Sendable (_ObjC_HeartbeatsPayload) -> Void) { // TODO: When minimum version moves to iOS 13.0, restore the async version // removed in #13952. heartbeatController.flushAsync { heartbeatsPayload in diff --git a/FirebaseCore/Internal/Tests/Common/AdjustableDate.swift b/FirebaseCore/Internal/Tests/Common/AdjustableDate.swift new file mode 100644 index 00000000000..e852d2f63f2 --- /dev/null +++ b/FirebaseCore/Internal/Tests/Common/AdjustableDate.swift @@ -0,0 +1,23 @@ +// Copyright 2024 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 + +/// Used to manipulate a date across multiple concurrent contexts for simulation purposes. +final class AdjustableDate: @unchecked Sendable { + var date: Date + init(date: Date) { + self.date = date + } +} diff --git a/FirebaseCore/Internal/Tests/Integration/HeartbeatLoggingIntegrationTests.swift b/FirebaseCore/Internal/Tests/Integration/HeartbeatLoggingIntegrationTests.swift index 3a9823aa94a..3c5cdba3950 100644 --- a/FirebaseCore/Internal/Tests/Integration/HeartbeatLoggingIntegrationTests.swift +++ b/FirebaseCore/Internal/Tests/Integration/HeartbeatLoggingIntegrationTests.swift @@ -232,8 +232,8 @@ class HeartbeatLoggingIntegrationTests: XCTestCase { func testLogRepeatedly_WithoutFlushing_LimitsOnWrite() throws { // Given - var testdate = date - let heartbeatController = HeartbeatController(id: #function, dateProvider: { testdate }) + let testDate = AdjustableDate(date: date) + let heartbeatController = HeartbeatController(id: #function, dateProvider: { testDate.date }) // When // Iterate over 35 days and log a heartbeat each day. @@ -252,7 +252,7 @@ class HeartbeatLoggingIntegrationTests: XCTestCase { heartbeatController.log("dummy_agent_3") } - testdate.addTimeInterval(60 * 60 * 24) // Advance the test date by 1 day. + testDate.date.addTimeInterval(60 * 60 * 24) // Advance the test date by 1 day. } // Then diff --git a/FirebaseCore/Internal/Tests/Unit/HeartbeatControllerTests.swift b/FirebaseCore/Internal/Tests/Unit/HeartbeatControllerTests.swift index 82691ed3f24..bac98cf4647 100644 --- a/FirebaseCore/Internal/Tests/Unit/HeartbeatControllerTests.swift +++ b/FirebaseCore/Internal/Tests/Unit/HeartbeatControllerTests.swift @@ -99,11 +99,11 @@ class HeartbeatControllerTests: XCTestCase { func testLogAtEndOfTimePeriodAndAcceptAtStartOfNextOne() throws { // Given - var testDate = date + let testDate = AdjustableDate(date: date) let controller = HeartbeatController( storage: HeartbeatStorageFake(), - dateProvider: { testDate } + dateProvider: { testDate.date } ) assertHeartbeatControllerFlushesEmptyPayload(controller) @@ -113,12 +113,12 @@ class HeartbeatControllerTests: XCTestCase { controller.log("dummy_agent") // - Advance to 2021-11-01 @ 23:59:59 (EST) - testDate.addTimeInterval(60 * 60 * 24 - 1) + testDate.date.addTimeInterval(60 * 60 * 24 - 1) controller.log("dummy_agent") // - Advance to 2021-11-02 @ 00:00:00 (EST) - testDate.addTimeInterval(1) + testDate.date.addTimeInterval(1) controller.log("dummy_agent") @@ -271,18 +271,18 @@ class HeartbeatControllerTests: XCTestCase { ).date // 2021-11-02 @ 11 PM (Tokyo time zone) ) - var testDate = newYorkDate + let testDate = AdjustableDate(date: newYorkDate) let heartbeatController = HeartbeatController( storage: HeartbeatStorageFake(), - dateProvider: { testDate } + dateProvider: { testDate.date } ) // When heartbeatController.log("dummy_agent") // Device travels from NYC to Tokyo. - testDate = tokyoDate + testDate.date = tokyoDate heartbeatController.log("dummy_agent") @@ -308,10 +308,10 @@ class HeartbeatControllerTests: XCTestCase { func testLoggingDependsOnDateNotUserAgent() throws { // Given - var testDate = date + let testDate = AdjustableDate(date: date) let heartbeatController = HeartbeatController( storage: HeartbeatStorageFake(), - dateProvider: { testDate } + dateProvider: { testDate.date } ) // When @@ -319,11 +319,11 @@ class HeartbeatControllerTests: XCTestCase { heartbeatController.log("dummy_agent") // - Day 2 - testDate.addTimeInterval(60 * 60 * 24) + testDate.date.addTimeInterval(60 * 60 * 24) heartbeatController.log("some_other_agent") // - Day 3 - testDate.addTimeInterval(60 * 60 * 24) + testDate.date.addTimeInterval(60 * 60 * 24) heartbeatController.log("dummy_agent") // Then @@ -359,20 +359,20 @@ class HeartbeatControllerTests: XCTestCase { let todaysDate = date let tomorrowsDate = date.addingTimeInterval(60 * 60 * 24) - var testDate = yesterdaysDate + let testDate = AdjustableDate(date: yesterdaysDate) let heartbeatController = HeartbeatController( storage: HeartbeatStorageFake(), - dateProvider: { testDate } + dateProvider: { testDate.date } ) // When heartbeatController.log("yesterdays_dummy_agent") - testDate = todaysDate + testDate.date = todaysDate heartbeatController.log("todays_dummy_agent") - testDate = tomorrowsDate + testDate.date = tomorrowsDate heartbeatController.log("tomorrows_dummy_agent") - testDate = todaysDate + testDate.date = todaysDate // Then let payload = heartbeatController.flushHeartbeatFromToday() @@ -426,7 +426,10 @@ class HeartbeatControllerTests: XCTestCase { // MARK: - Fakes -private class HeartbeatStorageFake: HeartbeatStorageProtocol { +private final class HeartbeatStorageFake: HeartbeatStorageProtocol, @unchecked Sendable { + // The unchecked Sendable conformance is used to prevent warnings for the below var, which + // violates the class's Sendable conformance. Ignoring this violation should be okay for + // testing purposes. private var heartbeatsBundle: HeartbeatsBundle? func readAndWriteSync(using transform: (HeartbeatsBundle?) -> HeartbeatsBundle?) { diff --git a/FirebaseCore/Internal/Tests/Unit/HeartbeatStorageTests.swift b/FirebaseCore/Internal/Tests/Unit/HeartbeatStorageTests.swift index 79f4cc5abf6..5e704a7fb89 100644 --- a/FirebaseCore/Internal/Tests/Unit/HeartbeatStorageTests.swift +++ b/FirebaseCore/Internal/Tests/Unit/HeartbeatStorageTests.swift @@ -15,6 +15,14 @@ @testable import FirebaseCoreInternal import XCTest +extension HeartbeatsBundle { + static let testHeartbeatBundle: Self = { + var heartbeatBundle = HeartbeatsBundle(capacity: 1) + heartbeatBundle.append(Heartbeat(agent: "dummy_agent", date: Date())) + return heartbeatBundle + }() +} + class HeartbeatStorageTests: XCTestCase { // MARK: - Instance Management @@ -95,15 +103,12 @@ class HeartbeatStorageTests: XCTestCase { let expectation = expectation(description: #function) let heartbeatStorage = HeartbeatStorage(id: #function, storage: StorageFake()) - var dummyHeartbeatsBundle = HeartbeatsBundle(capacity: 1) - dummyHeartbeatsBundle.append(Heartbeat(agent: "dummy_agent", date: Date())) - // When heartbeatStorage.readAndWriteAsync { heartbeatsBundle in // Assert that heartbeat storage is empty. XCTAssertNil(heartbeatsBundle) // Write new value. - return dummyHeartbeatsBundle + return HeartbeatsBundle.testHeartbeatBundle } heartbeatStorage.readAndWriteAsync { heartbeatsBundle in @@ -111,7 +116,7 @@ class HeartbeatStorageTests: XCTestCase { // Assert old value is read. XCTAssertEqual( heartbeatsBundle?.makeHeartbeatsPayload(), - dummyHeartbeatsBundle.makeHeartbeatsPayload() + HeartbeatsBundle.testHeartbeatBundle.makeHeartbeatsPayload() ) // Write some new value. return heartbeatsBundle @@ -145,9 +150,6 @@ class HeartbeatStorageTests: XCTestCase { let storageFake = StorageFake() let heartbeatStorage = HeartbeatStorage(id: #function, storage: storageFake) - var dummyHeartbeatsBundle = HeartbeatsBundle(capacity: 1) - dummyHeartbeatsBundle.append(Heartbeat(agent: "dummy_agent", date: Date())) - // When storageFake.onWrite = { _ in expectation.fulfill() // Fulfilled 2 times. @@ -156,7 +158,7 @@ class HeartbeatStorageTests: XCTestCase { heartbeatStorage.readAndWriteAsync { heartbeatsBundle in expectation.fulfill() - return dummyHeartbeatsBundle + return HeartbeatsBundle.testHeartbeatBundle } // Then @@ -164,10 +166,10 @@ class HeartbeatStorageTests: XCTestCase { expectation.fulfill() XCTAssertNotEqual( heartbeatsBundle?.makeHeartbeatsPayload(), - dummyHeartbeatsBundle.makeHeartbeatsPayload(), + HeartbeatsBundle.testHeartbeatBundle.makeHeartbeatsPayload(), "They should not be equal because the previous save failed." ) - return dummyHeartbeatsBundle + return HeartbeatsBundle.testHeartbeatBundle } wait(for: [expectation], timeout: 0.5) @@ -212,16 +214,13 @@ class HeartbeatStorageTests: XCTestCase { // Given let heartbeatStorage = HeartbeatStorage(id: #function, storage: StorageFake()) - var dummyHeartbeatsBundle = HeartbeatsBundle(capacity: 1) - dummyHeartbeatsBundle.append(Heartbeat(agent: "dummy_agent", date: Date())) - // When let expectation1 = expectation(description: #function + "_1") heartbeatStorage.getAndSetAsync { heartbeatsBundle in // Assert that heartbeat storage is empty. XCTAssertNil(heartbeatsBundle) // Write new value. - return dummyHeartbeatsBundle + return HeartbeatsBundle.testHeartbeatBundle } completion: { result in switch result { case .success: break @@ -237,7 +236,7 @@ class HeartbeatStorageTests: XCTestCase { // Assert old value is read. XCTAssertEqual( heartbeatsBundle?.makeHeartbeatsPayload(), - dummyHeartbeatsBundle.makeHeartbeatsPayload() + HeartbeatsBundle.testHeartbeatBundle.makeHeartbeatsPayload() ) // Write some new value. expectation2.fulfill() @@ -368,7 +367,7 @@ class HeartbeatStorageTests: XCTestCase { let expectations: [XCTestExpectation] = try (0 ... 1000).map { i in let expectation = expectation(description: "\(#function)_\(i)") - let transform: (HeartbeatsBundle?) -> HeartbeatsBundle? = { heartbeatsBundle in + let transform: @Sendable (HeartbeatsBundle?) -> HeartbeatsBundle? = { heartbeatsBundle in expectation.fulfill() return heartbeatsBundle } @@ -401,11 +400,24 @@ class HeartbeatStorageTests: XCTestCase { } func testForMemoryLeakInInstanceManager() { + // This unchecked Sendable class is used to avoid passing a non-sendable + // 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() + + func append(_ weakRef: WeakContainer) { + weakRefsLock.withLock { + weakRefs.append(weakRef) + } + } + } + // Given let id = "testID" - var weakRefs: [WeakContainer] = [] - // Lock is used to synchronize `weakRefs` during concurrent access. - let weakRefsLock = NSLock() + let weakRefs = WeakRefs() // When // Simulate concurrent access. This will help expose race conditions that could cause a crash. @@ -414,9 +426,7 @@ class HeartbeatStorageTests: XCTestCase { group.enter() DispatchQueue.global().async { let instance = HeartbeatStorage.getInstance(id: id) - weakRefsLock.withLock { - weakRefs.append(WeakContainer(object: instance)) - } + weakRefs.append(WeakContainer(object: instance)) group.leave() } } @@ -425,13 +435,16 @@ class HeartbeatStorageTests: XCTestCase { // Then // The `weakRefs` array's references should all be nil; otherwise, something is being // unexpectedly strongly retained. - for weakRef in weakRefs { + for weakRef in weakRefs.weakRefs { XCTAssertNil(weakRef.object, "Potential memory leak detected.") } } } -private class StorageFake: Storage { +private final class StorageFake: Storage, @unchecked Sendable { + // The unchecked Sendable conformance is used to prevent warnings for the below var, which + // violates the class's Sendable conformance. Ignoring this violation should be okay for + // testing purposes. var fakeFile: Data? var onRead: (() throws -> Data)? var onWrite: ((Data?) throws -> Void)? diff --git a/FirebaseCore/Internal/Tests/Unit/StorageTests.swift b/FirebaseCore/Internal/Tests/Unit/StorageTests.swift index 21b4fded8c1..adc47ba81d6 100644 --- a/FirebaseCore/Internal/Tests/Unit/StorageTests.swift +++ b/FirebaseCore/Internal/Tests/Unit/StorageTests.swift @@ -97,22 +97,23 @@ class FileStorageTests: XCTestCase { class UserDefaultsStorageTests: XCTestCase { var defaults: UserDefaults! - let suiteName = #file + let suiteName = "com.firebase.userdefaults.storageTests" override func setUpWithError() throws { - defaults = try XCTUnwrap(UserDefaultsFake(suiteName: suiteName)) + // Clear the user default suite before testing. + UserDefaults(suiteName: suiteName)?.removePersistentDomain(forName: suiteName) } func testRead_WhenDefaultDoesNotExist_ThrowsError() throws { // Given - let defaultsStorage = UserDefaultsStorage(defaults: defaults, key: #function) + let defaultsStorage = UserDefaultsStorage(suiteName: suiteName, key: #function) // Then XCTAssertThrowsError(try defaultsStorage.read()) } func testRead_WhenDefaultExists_ReturnsDefault() throws { // Given - let defaultsStorage = UserDefaultsStorage(defaults: defaults, key: #function) + let defaultsStorage = UserDefaultsStorage(suiteName: suiteName, key: #function) XCTAssertNoThrow(try defaultsStorage.write(Constants.testData)) // When let storedData = try defaultsStorage.read() @@ -122,7 +123,7 @@ class UserDefaultsStorageTests: XCTestCase { func testWriteData_WhenDefaultDoesNotExist_CreatesDefault() throws { // Given - let defaultsStorage = UserDefaultsStorage(defaults: defaults, key: #function) + let defaultsStorage = UserDefaultsStorage(suiteName: suiteName, key: #function) XCTAssertThrowsError(try defaultsStorage.read()) // When XCTAssertNoThrow(try defaultsStorage.write(Constants.testData)) @@ -133,7 +134,7 @@ class UserDefaultsStorageTests: XCTestCase { func testWriteData_WhenDefaultExists_ModifiesDefault() throws { // Given - let defaultsStorage = UserDefaultsStorage(defaults: defaults, key: #function) + let defaultsStorage = UserDefaultsStorage(suiteName: suiteName, key: #function) XCTAssertNoThrow(try defaultsStorage.write(Constants.testData)) // When let modifiedData = #function.data(using: .utf8) @@ -146,7 +147,7 @@ class UserDefaultsStorageTests: XCTestCase { func testWriteNil_WhenDefaultDoesNotExist_RemovesDefault() throws { // Given - let defaultsStorage = UserDefaultsStorage(defaults: defaults, key: #function) + let defaultsStorage = UserDefaultsStorage(suiteName: suiteName, key: #function) XCTAssertThrowsError(try defaultsStorage.read()) // When XCTAssertNoThrow(try defaultsStorage.write(nil)) @@ -156,7 +157,7 @@ class UserDefaultsStorageTests: XCTestCase { func testWriteNil_WhenDefaultExists_RemovesDefault() throws { // Given - let defaultsStorage = UserDefaultsStorage(defaults: defaults, key: #function) + let defaultsStorage = UserDefaultsStorage(suiteName: suiteName, key: #function) XCTAssertNoThrow(try defaultsStorage.write(Constants.testData)) // When XCTAssertNoThrow(try defaultsStorage.write(nil)) @@ -164,17 +165,3 @@ class UserDefaultsStorageTests: XCTestCase { XCTAssertThrowsError(try defaultsStorage.read()) } } - -// MARK: - Fakes - -private class UserDefaultsFake: UserDefaults { - private var defaults = [String: Any]() - - override func object(forKey defaultName: String) -> Any? { - defaults[defaultName] - } - - override func set(_ value: Any?, forKey defaultName: String) { - defaults[defaultName] = value - } -}