Core Data: Make persistent container thread-safe

RELEASE-NOTES.txt

@@ -5,6 +5,7 @@
 - [*] Jetpack Setup: Fixed an issue where the WordPress.com authentication fails when using a passwordless account that's already connected to Jetpack [https://github.com/woocommerce/woocommerce-ios/pull/14501]
 - [*] Jetpack Setup: Fixed an issue with magic link handling when the app is cold started. [https://github.com/woocommerce/woocommerce-ios/pull/14502]
 - [**] Media Library: On sites logged in with application password, when picking image from WordPress Media Library, all images will now load correctly. [https://github.com/woocommerce/woocommerce-ios/pull/14444]
+- [Internal] Updated CoreDataManager to be thread-safe [https://github.com/woocommerce/woocommerce-ios/pull/14534]
 
 21.2
 -----

Storage/Storage/CoreData/CoreDataManager.swift

@@ -7,48 +7,69 @@ import WooFoundation
 ///
 public final class CoreDataManager: StorageManagerType {
 
-    /// Storage Identifier.
-    ///
-    public let name: String
-
     private let crashLogger: CrashLogger
 
-    private let modelsInventory: ManagedObjectModelsInventory
-
     /// A serial queue used to ensure there is only one writing operation at a time.
     private let writerQueue: OperationQueue
 
+    let storeURL: URL
+
+    let storeDescription: NSPersistentStoreDescription
+
     /// Module-private designated Initializer.
     ///
     /// - Parameter name: Identifier to be used for: [database, data model, container].
     /// - Parameter crashLogger: allows logging a message of any severity level
-    /// - Parameter modelsInventory: The models to load when spinning up the Core Data stack.
-    ///     This is automatically generated if `nil`. You would probably only specify this for
-    ///     unit tests to test migration and/or recovery scenarios.
     ///
     /// - Important: This should *match* with your actual Data Model file!.
     ///
     init(name: String,
          crashLogger: CrashLogger,
          modelsInventory: ManagedObjectModelsInventory?) {
-        self.name = name
         self.crashLogger = crashLogger
         self.writerQueue = OperationQueue()
         self.writerQueue.name = "com.automattic.woocommerce.CoreDataManager.writer"
         self.writerQueue.maxConcurrentOperationCount = 1
 
+        let inventory: ManagedObjectModelsInventory
         do {
-            if let modelsInventory = modelsInventory {
-                self.modelsInventory = modelsInventory
+            if let modelsInventory {
+                inventory = modelsInventory
             } else {
-                self.modelsInventory = try .from(packageName: name, bundle: Bundle(for: type(of: self)))
+                inventory = try .from(packageName: name, bundle: Bundle(for: type(of: self)))
             }
         } catch {
             // We'll throw a fatalError() because we can't really proceed without a
             // ManagedObjectModel.
             let error = CoreDataManagerError.modelInventoryLoadingFailed(name, error)
             crashLogger.logFatalErrorAndExit(error, userInfo: nil)
         }
+
+        let storeURL = Self.storeURL(with: name)
+        let storeDescription = Self.storeDescription(with: storeURL)
+        let persistentContainer = Self.createPersistentContainer(with: name,
+                                                                 storeURL: storeURL,
+                                                                 storeDescription: storeDescription,
+                                                                 crashLogger: crashLogger,
+                                                                 modelsInventory: inventory)
+        self.persistentContainer = persistentContainer
+        self.storeURL = storeURL
+        self.storeDescription = storeDescription
+
+        self.viewStorage = {
+            let context = persistentContainer.viewContext
+            /// This simplifies the process of merging updates from persistent container to view context.
+            /// When disable auto merge, we need to handle merging manually using `NSManagedObjectContextDidSave` notifications.
+            context.automaticallyMergesChangesFromParent = true
+            context.mergePolicy = NSMergeByPropertyObjectTrumpMergePolicy
+            return context
+        }()
+
+        self.writerDerivedStorage = {
+            let backgroundContext = persistentContainer.newBackgroundContext()
+            backgroundContext.mergePolicy = NSMergeByPropertyObjectTrumpMergePolicy
+            return backgroundContext
+        }()
     }
 
     /// Public designated initializer.
@@ -64,79 +85,15 @@ public final class CoreDataManager: StorageManagerType {
 
     /// Returns the Storage associated with the View Thread.
     ///
-    public var viewStorage: StorageType {
-        let context = persistentContainer.viewContext
-        /// This simplifies the process of merging updates from persistent container to view context.
-        /// When disable auto merge, we need to handle merging manually using `NSManagedObjectContextDidSave` notifications.
-        context.automaticallyMergesChangesFromParent = true
-        context.mergePolicy = NSMergeByPropertyObjectTrumpMergePolicy
-        return context
-    }
+    public let viewStorage: StorageType
 
     /// Returns a shared derived storage instance dedicated for write operations.
     ///
-    public lazy var writerDerivedStorage: StorageType = {
-        let backgroundContext = persistentContainer.newBackgroundContext()
-        backgroundContext.mergePolicy = NSMergeByPropertyObjectTrumpMergePolicy
-        return backgroundContext
-    }()
+    public let writerDerivedStorage: StorageType
 
     /// Persistent Container: Holds the full CoreData Stack
     ///
-    public lazy var persistentContainer: NSPersistentContainer = {
-        let container = NSPersistentContainer(name: name, managedObjectModel: modelsInventory.currentModel)
-        container.persistentStoreDescriptions = [storeDescription]
-
-        let migrationDebugMessages = migrateDataModelIfNecessary(using: container.persistentStoreCoordinator)
-
-        container.loadPersistentStores { [weak self] (storeDescription, error) in
-            guard let `self` = self, let persistentStoreLoadingError = error else {
-                return
-            }
-
-            DDLogError("⛔️ [CoreDataManager] loadPersistentStore failed. Attempting to recover... \(persistentStoreLoadingError)")
-
-            /// Remove the old Store which is either corrupted or has an invalid model we can't migrate from
-            ///
-            var persistentStoreRemovalError: Error?
-            do {
-                try container.persistentStoreCoordinator.destroyPersistentStore(at: self.storeURL,
-                                                                                ofType: storeDescription.type,
-                                                                                options: nil)
-                NotificationCenter.default.post(name: .StorageManagerDidResetStorage, object: self)
-
-            } catch {
-                persistentStoreRemovalError = error
-            }
-
-            /// Retry!
-            ///
-            container.loadPersistentStores { [weak self] (storeDescription, underlyingError) in
-                guard let underlyingError = underlyingError as NSError? else {
-                    return
-                }
-
-                let error = CoreDataManagerError.recoveryFailed
-                let logProperties: [String: Any?] = ["persistentStoreLoadingError": persistentStoreLoadingError,
-                                                     "persistentStoreRemovalError": persistentStoreRemovalError,
-                                                     "retryError": underlyingError,
-                                                     "appState": UIApplication.shared.applicationState.rawValue,
-                                                     "migrationMessages": migrationDebugMessages]
-                self?.crashLogger.logFatalErrorAndExit(error,
-                                                       userInfo: logProperties.compactMapValues { $0 })
-            }
-
-            let logProperties: [String: Any?] = ["persistentStoreLoadingError": persistentStoreLoadingError,
-                                                 "persistentStoreRemovalError": persistentStoreRemovalError,
-                                                 "appState": UIApplication.shared.applicationState.rawValue,
-                                                 "migrationMessages": migrationDebugMessages]
-            self.crashLogger.logMessage("[CoreDataManager] Recovered from persistent store loading error",
-                                        properties: logProperties.compactMapValues { $0 },
-                                        level: .info)
-        }
-
-        return container
-    }()
+    public let persistentContainer: NSPersistentContainer
 
     /// Saves the derived storage. Note: the closure may be called on a different thread
     ///
@@ -224,6 +181,66 @@ public final class CoreDataManager: StorageManagerType {
         }, on: .main)
     }
 
+    private static func createPersistentContainer(with storageName: String,
+                                                  storeURL: URL,
+                                                  storeDescription: NSPersistentStoreDescription,
+                                                  crashLogger: CrashLogger,
+                                                  modelsInventory: ManagedObjectModelsInventory) -> NSPersistentContainer {
+        let container = NSPersistentContainer(name: storageName, managedObjectModel: modelsInventory.currentModel)
+        container.persistentStoreDescriptions = [storeDescription]
+
+        let migrationDebugMessages = migrateDataModelIfNecessary(using: container.persistentStoreCoordinator,
+                                                                 storeURL: storeURL,
+                                                                 modelsInventory: modelsInventory)
+
+        container.loadPersistentStores { (storeDescription, error) in
+            guard let persistentStoreLoadingError = error else {
+                return
+            }
+
+            DDLogError("⛔️ [CoreDataManager] loadPersistentStore failed. Attempting to recover... \(persistentStoreLoadingError)")
+
+            /// Remove the old Store which is either corrupted or has an invalid model we can't migrate from
+            ///
+            var persistentStoreRemovalError: Error?
+            do {
+                try container.persistentStoreCoordinator.destroyPersistentStore(at: storeURL,
+                                                                                ofType: storeDescription.type,
+                                                                                options: nil)
+                NotificationCenter.default.post(name: .StorageManagerDidResetStorage, object: self)
+
+            } catch {
+                persistentStoreRemovalError = error
+            }
+
+            /// Retry!
+            ///
+            container.loadPersistentStores { (storeDescription, underlyingError) in
+                guard let underlyingError = underlyingError as NSError? else {
+                    return
+                }
+
+                let error = CoreDataManagerError.recoveryFailed
+                let logProperties: [String: Any?] = ["persistentStoreLoadingError": persistentStoreLoadingError,
+                                                     "persistentStoreRemovalError": persistentStoreRemovalError,
+                                                     "retryError": underlyingError,
+                                                     "appState": UIApplication.shared.applicationState.rawValue,
+                                                     "migrationMessages": migrationDebugMessages]
+                crashLogger.logFatalErrorAndExit(error, userInfo: logProperties.compactMapValues { $0 })
+            }
+
+            let logProperties: [String: Any?] = ["persistentStoreLoadingError": persistentStoreLoadingError,
+                                                 "persistentStoreRemovalError": persistentStoreRemovalError,
+                                                 "appState": UIApplication.shared.applicationState.rawValue,
+                                                 "migrationMessages": migrationDebugMessages]
+            crashLogger.logMessage("[CoreDataManager] Recovered from persistent store loading error",
+                                   properties: logProperties.compactMapValues { $0 },
+                                   level: .info)
+        }
+
+        return container
+    }
+
     private func deleteAllStoredObjects(in context: NSManagedObjectContext) {
         let storeCoordinator = persistentContainer.persistentStoreCoordinator
         do {
@@ -245,7 +262,9 @@ public final class CoreDataManager: StorageManagerType {
 
     /// Migrates the current persistent store to the latest data model if needed.
     /// - Returns: an array of debug messages for logging. Please feel free to remove when #2371 is resolved.
-    private func migrateDataModelIfNecessary(using coordinator: NSPersistentStoreCoordinator) -> [String] {
+    private static func migrateDataModelIfNecessary(using coordinator: NSPersistentStoreCoordinator,
+                                                    storeURL: URL,
+                                                    modelsInventory: ManagedObjectModelsInventory) -> [String] {
         var debugMessages = [String]()
 
         let migrationCheckMessage = "ℹ️ [CoreDataManager] Checking if migration is necessary."
@@ -277,10 +296,10 @@ public final class CoreDataManager: StorageManagerType {
 
 // MARK: - Descriptors
 //
-extension CoreDataManager {
+private extension CoreDataManager {
     /// Returns the PersistentStore Descriptor
     ///
-    var storeDescription: NSPersistentStoreDescription {
+    static func storeDescription(with storeURL: URL) -> NSPersistentStoreDescription {
         let description = NSPersistentStoreDescription(url: storeURL)
         description.shouldAddStoreAsynchronously = false
         description.shouldMigrateStoreAutomatically = false
@@ -294,12 +313,12 @@ extension CoreDataManager {
 extension CoreDataManager {
     /// Returns the Store URL (the actual sqlite file!)
     ///
-    var storeURL: URL {
+    static func storeURL(with storageName: String) -> URL {
         guard let url = FileManager.default.urls(for: .documentDirectory, in: .userDomainMask).first else {
             logErrorAndExit("Okay: Missing Documents Folder?")
         }
 
-        return url.appendingPathComponent(name + ".sqlite")
+        return url.appendingPathComponent(storageName + ".sqlite")
     }
 }
 

Storage/StorageTests/CoreData/CoreDataManagerTests.swift

@@ -280,13 +280,14 @@ private extension CoreDataManagerTests {
     }
 
     func makeManager(using modelsInventory: ManagedObjectModelsInventory,
-                     deletingExistingStoreFiles deleteStoreFiles: Bool) throws -> CoreDataManager {
+                     deletingExistingStoreFiles: Bool) throws -> CoreDataManager {
+        let storeURL = CoreDataManager.storeURL(with: storageIdentifier)
+        if deletingExistingStoreFiles {
+            try deleteStoreFiles(at: storeURL)
+        }
         let manager = CoreDataManager(name: storageIdentifier,
                                       crashLogger: MockCrashLogger(),
                                       modelsInventory: modelsInventory)
-        if deleteStoreFiles {
-            try self.deleteStoreFiles(at: manager.storeURL)
-        }
         return manager
     }