[Storage] Addressing Swift 6 issues with Storage's instance management

[ncooke3]

For context, the original error was:

The important takeaway is that if you synchronize something with an actor (e.g. instance management) in a method that isn't async like:

private class func storage(app: FirebaseApp, bucket: String) -> Storage

you'd have to change the signature of the public function which would be breaking.

So I avoided the formal route by decorating the property with nonisolated(unsafe) which tells the compiler that we know what we are doing (which we do with the locking mechanism in place). But using nonisolated(unsafe) (3f1ba97) on static properties does not work in Swift 5.5 (pitch revision). I'm not sure why it works on Swift 6.0, but the only way around this was to do it both ways for each property and differentiate with a #if swift(>=6.0) which seemed hacky.

So the next stop was to remove the properties from global state by making them instance properties. We can't do so on Storage though because there can be multiple instances of Storage (e.g. for multiple Firebase apps), so I made a new class that offers a singleton (so all Storage instances must access the same instance cache).

I couldn't make it an actor because of the first learning at the top (the actor would bubble up the added asynchronous-ness). I then ran into:

which makes sense. I added Sendable conformance, but the class doesn't meet all requirements. Particularly:

Contain only stored properties that are immutable and sendable

I tried working around this by changing the vars to lets. I tried using NSLock because it has reference semantics and we can therefore use the let:

-  private var instancesLock: os_unfair_lock = .init()
+  private let instancesLock: NSLock = .init()

But, this strategy has no corresponding solution for the var instances: [String: Storage]. I tried making it of type NSDictionary, but NSDictionary, does not conform to Sendable. We could box it the dictionary on a reference type, but you're just moving the problem down one abstraction.

So, the last option is to mark it with @unchecked Sendable which I believe means "I know what I'm doing in this type's implementation so don't worry": https://www.swift.org/migration/documentation/swift-6-concurrency-migration-guide/commonproblems/#Retroactive-Sendable-Conformance

Unfortunately, using @unchecked Sendable doesn't offer any improvements in terms of compile time safety. Since the instance cache management is used in a few places (here, GTMSessionFetcher mapping, heartbeat storage), it may make sense to carefully refactor the functionality in a generic type that lives in FirebaseSharedSwift.

private final class StorageInstanceCache: @unchecked Sendable {
  static let shared = StorageInstanceCache()

  /// A map of active instances, grouped by app. Keys are FirebaseApp names and values are
  /// instances of Storage associated with the given app.
  private var instances: [String: Storage] = [:]

  /// Lock to manage access to the instances array to avoid race conditions.
  private var instancesLock: os_unfair_lock = .init()

  /// ...

[google-oss-bot]

	1 Warning
⚠️	Did you forget to add a changelog entry? (Add #no-changelog to the PR description to silence this warning.)

Generated by 🚫 Danger

[github-actions]

✅  No API diff detected

Commit: 81dca91
Last updated: Fri Aug 2 06:26 PDT 2024
View workflow logs & download artifacts

---