Enhance Thread Safety in Core Data Management

RELEASE-NOTES.txt

@@ -3,8 +3,9 @@
 18.8
 -----
 - [*] Menu > Payments > Collect Payment: fix the the "card reader" & "Tap To Pay" payment methods where it was no-op before. [https://github.com/woocommerce/woocommerce-ios/pull/12801]
-- [internal] Optimize API calls sent for Dashboard screen. [https://github.com/woocommerce/woocommerce-ios/pull/12775]
 - [**] Adds edit support for the Min/Max Quantities extension in product details. [https://github.com/woocommerce/woocommerce-ios/pull/12758]
+- [internal] Optimize API calls sent for Dashboard screen. [https://github.com/woocommerce/woocommerce-ios/pull/12775]
+- [internal] Enhanced Thread Safety in Core Data Management. [https://github.com/woocommerce/woocommerce-ios/pull/12805]
 
 18.7
 -----

Storage/Storage/CoreData/CoreDataManager.swift

@@ -15,6 +15,9 @@ public final class CoreDataManager: StorageManagerType {
 
     private let modelsInventory: ManagedObjectModelsInventory
 
+    // A dispatch queue for synchronizing access to shared attributes
+    private let syncQueue = DispatchQueue(label: "com.automattic.woocommerce.CoreDataManager.syncQueue")
+
     /// Module-private designated Initializer.
     ///
     /// - Parameter name: Identifier to be used for: [database, data model, container].
@@ -59,71 +62,76 @@ public final class CoreDataManager: StorageManagerType {
     /// Returns the Storage associated with the View Thread.
     ///
     public var viewStorage: StorageType {
-        return persistentContainer.viewContext
+        return syncQueue.sync {
+            return persistentContainer.viewContext
+        }
     }
 
     /// Returns a shared derived storage instance dedicated for write operations.
     ///
     public lazy var writerDerivedStorage: StorageType = {
-        let childManagedObjectContext = NSManagedObjectContext(concurrencyType: .privateQueueConcurrencyType)
-        childManagedObjectContext.parent = persistentContainer.viewContext
-        childManagedObjectContext.mergePolicy = NSMergeByPropertyObjectTrumpMergePolicy
-        return childManagedObjectContext
+        return syncQueue.sync {
+            let childManagedObjectContext = NSManagedObjectContext(concurrencyType: .privateQueueConcurrencyType)
+            childManagedObjectContext.parent = persistentContainer.viewContext
+            childManagedObjectContext.mergePolicy = NSMergeByPropertyObjectTrumpMergePolicy
+            return childManagedObjectContext
+        }
     }()
 
     /// 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)
+            let container = NSPersistentContainer(name: name, managedObjectModel: modelsInventory.currentModel)
+            container.persistentStoreDescriptions = [storeDescription]
 
-        container.loadPersistentStores { [weak self] (storeDescription, error) in
-            guard let `self` = self, let persistentStoreLoadingError = error else {
-                return
-            }
+            let migrationDebugMessages = migrateDataModelIfNecessary(using: container.persistentStoreCoordinator)
 
-            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)
-            } catch {
-                persistentStoreRemovalError = error
-            }
-
-            /// Retry!
-            ///
-            container.loadPersistentStores { [weak self] (storeDescription, underlyingError) in
-                guard let underlyingError = underlyingError as NSError? else {
+            container.loadPersistentStores { [weak self] (storeDescription, error) in
+                guard let `self` = self, let persistentStoreLoadingError = error else {
                     return
                 }
 
-                let error = CoreDataManagerError.recoveryFailed
+                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)
+                } 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,
-                                                     "retryError": underlyingError,
                                                      "appState": UIApplication.shared.applicationState.rawValue,
                                                      "migrationMessages": migrationDebugMessages]
-                self?.crashLogger.logFatalErrorAndExit(error,
-                                                  userInfo: logProperties.compactMapValues { $0 })
+                self.crashLogger.logMessage("[CoreDataManager] Recovered from persistent store loading error",
+                                            properties: logProperties.compactMapValues { $0 },
+                                            level: .info)
             }
 
-            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
+            return container
     }()
 
     /// Performs the received closure in Background. Note that you should use the received Storage instance (BG friendly!).
@@ -150,14 +158,16 @@ public final class CoreDataManager: StorageManagerType {
     /// This method effectively destroys all of the stored data, and generates a blank Persistent Store from scratch.
     ///
     public func reset() {
-        let viewContext = persistentContainer.viewContext
+        syncQueue.sync {
+            let viewContext = persistentContainer.viewContext
 
-        viewContext.performAndWait {
-            viewContext.reset()
-            self.deleteAllStoredObjects()
+            viewContext.performAndWait {
+                viewContext.reset()
+                self.deleteAllStoredObjects()
 
-            DDLogVerbose("💣 [CoreDataManager] Stack Destroyed!")
-            NotificationCenter.default.post(name: .StorageManagerDidResetStorage, object: self)
+                DDLogVerbose("💣 [CoreDataManager] Stack Destroyed!")
+                NotificationCenter.default.post(name: .StorageManagerDidResetStorage, object: self)
+            }
         }
     }
 

Storage/StorageTests/CoreData/CoreDataManagerTests.swift

@@ -223,6 +223,79 @@ final class CoreDataManagerTests: XCTestCase {
         insertAccount(to: manager.viewStorage)
         XCTAssertEqual(manager.viewStorage.countObjects(ofType: Account.self), 3)
     }
+
+    func test_viewStorage_is_thread_safe() {
+        // Given
+        let manager = CoreDataManager(name: storageIdentifier, crashLogger: MockCrashLogger())
+        let expectation = self.expectation(description: "Thread Safety")
+
+        // When
+        DispatchQueue.global(qos: .background).async {
+            let viewContext = manager.viewStorage as? NSManagedObjectContext
+            // Then
+            XCTAssertNotNil(viewContext)
+            expectation.fulfill()
+        }
+
+        wait(for: [expectation], timeout: Constants.expectationTimeout)
+    }
+
+    func test_reset_is_thread_safe() {
+        // Given
+        let manager = CoreDataManager(name: storageIdentifier, crashLogger: MockCrashLogger())
+        let expectation = self.expectation(description: "Thread Safety")
+
+        // When
+        DispatchQueue.global(qos: .background).async {
+            manager.reset()
+            // Then
+            expectation.fulfill()
+        }
+
+        wait(for: [expectation], timeout: Constants.expectationTimeout)
+    }
+
+    func test_concurrent_access_to_core_data_manager() {
+        // Given
+        let manager = CoreDataManager(name: storageIdentifier, crashLogger: MockCrashLogger())
+        let expectation1 = self.expectation(description: "Thread 1")
+        let expectation2 = self.expectation(description: "Thread 2")
+
+        // When
+        DispatchQueue.global(qos: .background).async {
+            manager.reset()
+            // Then
+            expectation1.fulfill()
+        }
+
+        DispatchQueue.global(qos: .background).async {
+            let viewContext = manager.viewStorage as? NSManagedObjectContext
+            // Then
+            XCTAssertNotNil(viewContext)
+            expectation2.fulfill()
+        }
+
+        wait(for: [expectation1, expectation2], timeout: Constants.expectationTimeout)
+    }
+
+    func test_saving_operations_are_thread_safe() {
+        // Given
+        let manager = CoreDataManager(name: storageIdentifier, crashLogger: MockCrashLogger())
+        let expectation = self.expectation(description: "Thread Safety")
+
+        // When
+        DispatchQueue.global(qos: .background).async {
+            let derivedContext = manager.writerDerivedStorage as? NSManagedObjectContext
+            derivedContext?.performAndWait {
+                _ = derivedContext?.insertNewObject(ofType: ShippingLine.self)
+                derivedContext?.saveIfNeeded()
+            }
+            // Then
+            expectation.fulfill()
+        }
+
+        wait(for: [expectation], timeout: Constants.expectationTimeout)
+    }
 }
 
 // MARK: - Helpers

Yosemite/Yosemite/Tools/EntityListener.swift

@@ -28,6 +28,8 @@ public class EntityListener<T: ReadOnlyType> {
     ///
     public var onDelete: (() -> Void)?
 
+    /// Serial queue for thread safety.
+    private let queue = DispatchQueue(label: "com.automattic.woocommerce.CoreDataManager.syncQueue")
 
     /// Designated Initializer.
     ///
@@ -66,22 +68,24 @@ private extension EntityListener {
     /// Handles the ContextObjectDidChange Notification triggered by the ViewContext.
     ///
     func viewContextDidChange(notification: Notification) {
-        guard let note = ManagedObjectsDidChangeNotification(notification: notification) else {
-            return
-        }
+        queue.sync {
+            guard let note = ManagedObjectsDidChangeNotification(notification: notification) else {
+                return
+            }
 
-        /// Scenario: Upsert (Insert + Update + Refresh)
-        ///
-        if let storageEntity = readOnlyConvertible(from: note.upsertedObjects, representing: readOnlyEntity),
-            let updatedEntity = storageEntity.toTypeErasedReadOnly() as? T {
-            readOnlyEntity = updatedEntity
-            onUpsert?(readOnlyEntity)
-        }
+            /// Scenario: Upsert (Insert + Update + Refresh)
+            ///
+            if let storageEntity = readOnlyConvertible(from: note.upsertedObjects, representing: readOnlyEntity),
+               let updatedEntity = storageEntity.toTypeErasedReadOnly() as? T {
+                readOnlyEntity = updatedEntity
+                onUpsert?(readOnlyEntity)
+            }
 
-        /// Scenario: Nuked
-        ///
-        if let _ = readOnlyConvertible(from: note.deletedObjects, representing: readOnlyEntity) {
-            onDelete?()
+            /// Scenario: Nuked
+            ///
+            if let _ = readOnlyConvertible(from: note.deletedObjects, representing: readOnlyEntity) {
+                onDelete?()
+            }
         }
     }
 

Yosemite/YosemiteTests/Tools/EntityListenerTests.swift

@@ -145,4 +145,47 @@ class EntityListenerTests: XCTestCase {
 
         wait(for: [expectation], timeout: Constants.expectationTimeout)
     }
+
+
+    /// Tests the thread safety of the readOnlyEntity property.
+    ///
+    func test_thread_safety_for_readOnlyEntity() {
+        // Given: A sample storage account is inserted and saved.
+        let storageAccount = storageManager.insertSampleAccount()
+        viewContext.saveIfNeeded()
+
+        // And: An EntityListener is set up with the readOnly entity being the inserted storage account.
+        let listener = EntityListener(viewContext: viewContext, readOnlyEntity: storageAccount.toReadOnly())
+
+        // And: Expectations for multiple threads.
+        let expectation1 = self.expectation(description: "Thread 1")
+        let expectation2 = self.expectation(description: "Thread 2")
+        let expectation3 = self.expectation(description: "Thread 3")
+
+        // When: Multiple threads access the readOnlyEntity property concurrently.
+        DispatchQueue.global().async {
+            for _ in 0..<1000 {
+                _ = listener.readOnlyEntity
+            }
+            expectation1.fulfill()
+        }
+
+        DispatchQueue.global().async {
+            for _ in 0..<1000 {
+                _ = listener.readOnlyEntity
+            }
+            expectation2.fulfill()
+        }
+
+        DispatchQueue.global().async {
+            for _ in 0..<1000 {
+                _ = listener.readOnlyEntity
+            }
+            expectation3.fulfill()
+        }
+
+        // Then
+        wait(for: [expectation1, expectation2, expectation3], timeout: 0.1)
+    }
+
 }