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.

Sign up for GitHub

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

Remove fatalError in case of persistent store backup failure and add more logging to store migration #2481

Merged
merged 3 commits into from
Jun 30, 2020
Merged

Remove fatalError in case of persistent store backup failure and add more logging to store migration #2481

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9d0961e
Remove `fatalError` in case of persistent store backup failure and ju…
jaclync Jun 27, 2020
d11e21e
Consolidate error logging in retry completion.
jaclync Jun 27, 2020
4350b45
Add more logging for store migration.
jaclync Jun 27, 2020
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
19 changes: 14 additions & 5 deletions Storage/Storage/CoreData/CoreDataIterativeMigrator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,16 @@ public struct CoreDataIterativeMigrator {
debugMessages.append(migrationAttemptMessage)
DDLogWarn(migrationAttemptMessage)

guard migrateStore(at: sourceStore, storeType: storeType, fromModel: modelFrom, toModel: modelTo, with: migrateWithModel) == true else {
let (success, migrateStoreError) = migrateStore(at: sourceStore,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this line.

I noticed that the errors we were getting are missing reasons for the failed migrations. For example:

image

It looks like we don't return any debugging messages for cases where we return false. Example:

woocommerce-ios/Storage/Storage/CoreData/CoreDataIterativeMigrator.swift

Lines 31 to 33 in 4350b45

guard let sourceMetadata = try metadataForPersistentStore(storeType: storeType, at: sourceStore) else {
return (false, [])
}

Do you think we should add messages for those?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking more closely, it looks like this will be handled by the do catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are many points of failure in this function CoreDataIterativeMigrator. iterativeMigrate, and I think the most critical one is probably where it migrates the store in this call to migrateStore. later in this file I updated to return the error for each point of failure (actual migration error, or backup error) and adding this error to debugMessages below (L113)

storeType: storeType,
fromModel: modelFrom,
toModel: modelTo,
with: migrateWithModel)
guard success else {
if let migrateStoreError = migrateStoreError {
let errorInfo = (migrateStoreError as NSError?)?.userInfo ?? [:]
debugMessages.append("Migration error: \(migrateStoreError) [\(errorInfo)]")
}
return (false, debugMessages)
}
}
Expand Down Expand Up @@ -197,7 +206,7 @@ private extension CoreDataIterativeMigrator {
storeType: String,
fromModel: NSManagedObjectModel,
toModel: NSManagedObjectModel,
with mappingModel: NSMappingModel) -> Bool {
with mappingModel: NSMappingModel) -> (success: Bool, error: Error?) {
let tempDestinationURL = createTemporaryFolder(at: url)

// Migrate from the source model to the target model using the mapping,
Expand All @@ -212,18 +221,18 @@ private extension CoreDataIterativeMigrator {
destinationType: storeType,
destinationOptions: nil)
} catch {
return false
return (false, error)
}

do {
let backupURL = try makeBackup(at: url)
try copyMigratedOverOriginal(from: tempDestinationURL, to: url)
try deleteBackupCopies(at: backupURL)
} catch {
return false
return (false, error)
}

return true
return (true, nil)
}

static func metadataForPersistentStore(storeType: String, at url: URL) throws -> [String: Any]? {
Expand Down
43 changes: 19 additions & 24 deletions Storage/Storage/CoreData/CoreDataManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,34 +48,22 @@ public class CoreDataManager: StorageManagerType {

/// Backup the old Store
///
var persistentStoreBackupError: Error?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this line.

Regarding public lazy var persistentContainer: NSPersistentContainer = {. This might be something we can look into in the future. From Apple docs:

If a property marked with the lazy modifier is accessed by multiple threads simultaneously and the property has not yet been initialized, there’s no guarantee that the property will be initialized only once.

🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ugh yea I don't feel super good about having persistentContainer: NSPersistentContainer as a public lazy var, since it could be first triggered by anywhere (like the Reviews data source before). maybe this could be part of the effort to untangle Core Data setup from the app setup? #2370

do {
let sourceURL = self.storeURL
let backupURL = sourceURL.appendingPathExtension("~")
try FileManager.default.copyItem(at: sourceURL, to: backupURL)
} catch {
let message = "☠️ [CoreDataManager] Cannot backup Store: \(error)"
self.crashLogger.logMessageAndWait(message,
properties: ["persistentStoreLoadingError": persistentStoreLoadingError,
"backupError": error,
"appState": UIApplication.shared.applicationState.rawValue,
"migrationMessages": migrationDebugMessages],
level: .fatal)
fatalError(message)
persistentStoreBackupError = error
}

/// Remove the old Store
///
var persistentStoreRemovalError: Error?
do {
try FileManager.default.removeItem(at: self.storeURL)
shiki marked this conversation as resolved.
Show resolved Hide resolved
} catch {
let message = "☠️ [CoreDataManager] Cannot remove Store: \(error)"
self.crashLogger.logMessageAndWait(message,
properties: ["persistentStoreLoadingError": persistentStoreLoadingError,
"removeStoreError": error,
"appState": UIApplication.shared.applicationState.rawValue,
"migrationMessages": migrationDebugMessages],
level: .fatal)
fatalError(message)
persistentStoreRemovalError = error
}

/// Retry!
Expand All @@ -85,20 +73,27 @@ public class CoreDataManager: StorageManagerType {
return
}

let message = "☠️ [CoreDataManager] Recovery Failed! \(error) [\(error.userInfo)]"
let message = "☠️ [CoreDataManager] Recovery Failed!"

let logProperties: [String: Any?] = ["persistentStoreLoadingError": persistentStoreLoadingError,
"persistentStoreBackupError": persistentStoreBackupError,
"persistentStoreRemovalError": persistentStoreRemovalError,
"retryError": error,
"appState": UIApplication.shared.applicationState.rawValue,
"migrationMessages": migrationDebugMessages]
self?.crashLogger.logMessageAndWait(message,
properties: ["persistentStoreLoadingError": persistentStoreLoadingError,
"retryError": error,
"appState": UIApplication.shared.applicationState.rawValue,
"migrationMessages": migrationDebugMessages],
properties: logProperties.compactMapValues { $0 },
level: .fatal)
fatalError(message)
}

let logProperties: [String: Any?] = ["persistentStoreLoadingError": persistentStoreLoadingError,
"persistentStoreBackupError": persistentStoreBackupError,
"persistentStoreRemovalError": persistentStoreRemovalError,
"appState": UIApplication.shared.applicationState.rawValue,
"migrationMessages": migrationDebugMessages]
self.crashLogger.logMessage("[CoreDataManager] Recovered from persistent store loading error",
properties: ["persistentStoreLoadingError": persistentStoreLoadingError,
"appState": UIApplication.shared.applicationState.rawValue,
"migrationMessages": migrationDebugMessages],
properties: logProperties.compactMapValues { $0 },
level: .info)
}

Expand Down