Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Core] Make instance manager conform to Swift 6 principles #13933

Merged
merged 8 commits into from
Oct 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -51,20 +51,41 @@ final class HeartbeatStorage: HeartbeatStorageProtocol {
// MARK: - Instance Management

/// Statically allocated cache of `HeartbeatStorage` instances keyed by string IDs.
private static var cachedInstances: [String: WeakContainer<HeartbeatStorage>] = [:]
#if compiler(>=6)
// 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
ncooke3 marked this conversation as resolved.
Show resolved Hide resolved
// property's access is protected by an external synchronization mechanism
// (see `instancesLock` property).
private nonisolated(unsafe) static var cachedInstances: [
String: WeakContainer<HeartbeatStorage>
] = [:]
#else
// TODO(Xcode 16): Delete this block when minimum supported Xcode is
// Xcode 16.
private static var cachedInstances: [
String: WeakContainer<HeartbeatStorage>
] = [:]
#endif // compiler(>=6)

/// Used to synchronize concurrent access to the `cachedInstances` property.
private static let instancesLock = NSLock()
ncooke3 marked this conversation as resolved.
Show resolved Hide resolved

/// Gets an existing `HeartbeatStorage` instance with the given `id` if one exists. Otherwise,
/// makes a new instance with the given `id`.
///
/// - Parameter id: A string identifier.
/// - Returns: A `HeartbeatStorage` instance.
static func getInstance(id: String) -> HeartbeatStorage {
if let cachedInstance = cachedInstances[id]?.object {
return cachedInstance
} else {
let newInstance = HeartbeatStorage.makeHeartbeatStorage(id: id)
cachedInstances[id] = WeakContainer(object: newInstance)
return newInstance
instancesLock.withLock {
if let cachedInstance = cachedInstances[id]?.object {
return cachedInstance
} else {
let newInstance = HeartbeatStorage.makeHeartbeatStorage(id: id)
cachedInstances[id] = WeakContainer(object: newInstance)
return newInstance
}
}
}

Expand All @@ -88,7 +109,9 @@ final class HeartbeatStorage: HeartbeatStorageProtocol {

deinit {
// Removes the instance if it was cached.
Self.cachedInstances.removeValue(forKey: id)
_ = Self.instancesLock.withLock {
Self.cachedInstances.removeValue(forKey: id)
}
}

// MARK: - HeartbeatStorageProtocol
Expand Down
30 changes: 30 additions & 0 deletions FirebaseCore/Internal/Tests/Unit/HeartbeatStorageTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,36 @@ class HeartbeatStorageTests: XCTestCase {
// Then
wait(for: expectations, timeout: 1.0, enforceOrder: true)
}

func testForMemoryLeakInInstanceManager() {
// Given
let id = "testID"
var weakRefs: [WeakContainer<HeartbeatStorage>] = []
// Lock is used to synchronize `weakRefs` during concurrent access.
let weakRefsLock = NSLock()

// When
// Simulate concurrent access. This will help expose race conditions that could cause a crash.
let group = DispatchGroup()
for _ in 0 ..< 100 {
group.enter()
DispatchQueue.global().async {
let instance = HeartbeatStorage.getInstance(id: id)
weakRefsLock.withLock {
weakRefs.append(WeakContainer(object: instance))
}
group.leave()
}
}
group.wait()

// Then
// The `weakRefs` array's references should all be nil; otherwise, something is being
// unexpectedly strongly retained.
for weakRef in weakRefs {
XCTAssertNil(weakRef.object, "Potential memory leak detected.")
}
}
}

private class StorageFake: Storage {
Expand Down
Loading