diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt index eb440d0bddc..7ce50e8f447 100644 --- a/RELEASE-NOTES.txt +++ b/RELEASE-NOTES.txt @@ -6,6 +6,7 @@ - [*] 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] - [*] Media: Media upload will work for sites without XML-RPC. [https://github.com/woocommerce/woocommerce-ios/pull/14537] +- [Internal] Updated CoreDataManager to be thread-safe [https://github.com/woocommerce/woocommerce-ios/pull/14534] - [Internal] Updated storage usage in CouponStore [https://github.com/woocommerce/woocommerce-ios/pull/14530] - [Internal] Updated storage usage in ProductShippingClassStore [https://github.com/woocommerce/woocommerce-ios/pull/14520] diff --git a/Storage/Storage/CoreData/CoreDataManager.swift b/Storage/Storage/CoreData/CoreDataManager.swift index 35eec88a0ca..1829f953c14 100644 --- a/Storage/Storage/CoreData/CoreDataManager.swift +++ b/Storage/Storage/CoreData/CoreDataManager.swift @@ -7,41 +7,36 @@ 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 @@ -49,6 +44,32 @@ public final class CoreDataManager: StorageManagerType { 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") } } diff --git a/Storage/StorageTests/CoreData/CoreDataManagerTests.swift b/Storage/StorageTests/CoreData/CoreDataManagerTests.swift index 93ef75f84d6..bbd981709bf 100644 --- a/Storage/StorageTests/CoreData/CoreDataManagerTests.swift +++ b/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 }