From d2f2dd273ae3cac63d508607b14897c5a8668166 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 13 Mar 2018 10:51:57 -0300 Subject: [PATCH] Fix edge cases in migrations. --- .../ViewControllers/DebugUI/DebugUIBackup.m | 21 ++- Signal/src/util/OWSBackupExportJob.m | 3 +- Signal/src/util/OWSBackupImportJob.m | 177 ++++++++---------- Signal/src/util/OWSBackupJob.m | 7 + .../OWS106EnsureProfileComplete.swift | 5 - .../migrations/OWSDatabaseMigrationRunner.m | 54 +++--- SignalServiceKit/src/Storage/OWSStorage.m | 2 +- 7 files changed, 132 insertions(+), 137 deletions(-) diff --git a/Signal/src/ViewControllers/DebugUI/DebugUIBackup.m b/Signal/src/ViewControllers/DebugUI/DebugUIBackup.m index e55826d4e..bad3604d4 100644 --- a/Signal/src/ViewControllers/DebugUI/DebugUIBackup.m +++ b/Signal/src/ViewControllers/DebugUI/DebugUIBackup.m @@ -90,7 +90,26 @@ NS_ASSUME_NONNULL_BEGIN { DDLogInfo(@"%@ tryToImportBackup.", self.logTag); - [OWSBackup.sharedManager tryToImportBackup]; + UIAlertController *controller = + [UIAlertController alertControllerWithTitle:@"Restore CloudKit Backup" + message:@"This will delete all of your database contents." + preferredStyle:UIAlertControllerStyleAlert]; + + [controller addAction:[UIAlertAction + actionWithTitle:@"Restore" + style:UIAlertActionStyleDefault + handler:^(UIAlertAction *_Nonnull action) { + [[OWSPrimaryStorage.sharedManager newDatabaseConnection] + readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { + [transaction removeAllObjectsInCollection:[TSThread collection]]; + [transaction removeAllObjectsInCollection:[TSInteraction collection]]; + [transaction removeAllObjectsInCollection:[TSAttachment collection]]; + }]; + [OWSBackup.sharedManager tryToImportBackup]; + }]]; + [controller addAction:[OWSAlerts cancelAction]]; + UIViewController *fromViewController = [[UIApplication sharedApplication] frontmostViewController]; + [fromViewController presentViewController:controller animated:YES completion:nil]; } @end diff --git a/Signal/src/util/OWSBackupExportJob.m b/Signal/src/util/OWSBackupExportJob.m index e38959158..898f7daab 100644 --- a/Signal/src/util/OWSBackupExportJob.m +++ b/Signal/src/util/OWSBackupExportJob.m @@ -11,6 +11,7 @@ #import #import #import +#import #import #import #import @@ -268,7 +269,7 @@ NSString *const kOWSBackup_ExportDatabaseKeySpec = @"kOWSBackup_ExportDatabaseKe // Copy attachments. [srcTransaction - enumerateKeysAndObjectsInCollection:[TSAttachmentStream collection] + enumerateKeysAndObjectsInCollection:[TSAttachment collection] usingBlock:^(NSString *key, id object, BOOL *stop) { if (self.isComplete) { *stop = YES; diff --git a/Signal/src/util/OWSBackupImportJob.m b/Signal/src/util/OWSBackupImportJob.m index 387e26f64..8e8be2524 100644 --- a/Signal/src/util/OWSBackupImportJob.m +++ b/Signal/src/util/OWSBackupImportJob.m @@ -359,7 +359,6 @@ NSString *const kOWSBackup_ImportDatabaseKeySpec = @"kOWSBackup_ImportDatabaseKe [backupStorage runAsyncRegistrationsWithCompletion:^{ dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ [weakSelf restoreDatabaseContents:backupStorage completion:completion]; - completion(YES); }); }]; }); @@ -387,113 +386,85 @@ NSString *const kOWSBackup_ImportDatabaseKeySpec = @"kOWSBackup_ImportDatabaseKe return completion(NO); } - __block unsigned long long copiedThreads = 0; - __block unsigned long long copiedInteractions = 0; + NSDictionary *collectionTypeMap = @{ + [TSThread collection] : [TSThread class], + [TSAttachment collection] : [TSAttachment class], + [TSInteraction collection] : [TSInteraction class], + [OWSDatabaseMigration collection] : [OWSDatabaseMigration class], + }; + // Order matters here. + NSArray *collectionsToRestore = @[ + [TSThread collection], + [TSAttachment collection], + // Interactions refer to threads and attachments, + // so copy them afterward. + [TSInteraction collection], + [OWSDatabaseMigration collection], + ]; + NSMutableDictionary *restoredEntityCounts = [NSMutableDictionary new]; __block unsigned long long copiedEntities = 0; - __block unsigned long long copiedAttachments = 0; - __block unsigned long long copiedMigrations = 0; - + __block BOOL aborted = NO; [tempDBConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *srcTransaction) { [primaryDBConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *dstTransaction) { - // Copy threads. - [srcTransaction - enumerateKeysAndObjectsInCollection:[TSThread collection] - usingBlock:^(NSString *key, id object, BOOL *stop) { - if (self.isComplete) { - *stop = YES; - return; - } - if (![object isKindOfClass:[TSThread class]]) { - OWSProdLogAndFail( - @"%@ unexpected class: %@", self.logTag, [object class]); - return; - } - TSThread *thread = object; - [thread saveWithTransaction:dstTransaction]; - copiedThreads++; - copiedEntities++; - }]; - - // Copy attachments. - [srcTransaction - enumerateKeysAndObjectsInCollection:[TSAttachmentStream collection] - usingBlock:^(NSString *key, id object, BOOL *stop) { - if (self.isComplete) { - *stop = YES; - return; - } - if (![object isKindOfClass:[TSAttachment class]]) { - OWSProdLogAndFail( - @"%@ unexpected class: %@", self.logTag, [object class]); - return; - } - TSAttachment *attachment = object; - [attachment saveWithTransaction:dstTransaction]; - copiedAttachments++; - copiedEntities++; - }]; - - // Copy interactions. - // - // Interactions refer to threads and attachments, so copy the last. - [srcTransaction - enumerateKeysAndObjectsInCollection:[TSInteraction collection] - usingBlock:^(NSString *key, id object, BOOL *stop) { - if (self.isComplete) { - *stop = YES; - return; - } - if (![object isKindOfClass:[TSInteraction class]]) { - OWSProdLogAndFail( - @"%@ unexpected class: %@", self.logTag, [object class]); - return; - } - // Ignore disappearing messages. - if ([object isKindOfClass:[TSMessage class]]) { - TSMessage *message = object; - if (message.isExpiringMessage) { - return; - } - } - TSInteraction *interaction = object; - // Ignore dynamic interactions. - if (interaction.isDynamicInteraction) { - return; - } - [interaction saveWithTransaction:dstTransaction]; - copiedInteractions++; - copiedEntities++; - }]; + for (NSString *collection in collectionsToRestore) { + if ([collection isEqualToString:[OWSDatabaseMigration collection]]) { + // It's okay if there are existing migrations; we'll clear those + // before restoring. + continue; + } + if ([dstTransaction numberOfKeysInCollection:collection] > 0) { + DDLogError(@"%@ cannot restore into non-empty database (%@).", self.logTag, collection); + aborted = YES; + return completion(NO); + } + } // Clear existing migrations. + // + // This is safe since we only ever import into an empty database. + // Non-database migrations should be idempotent. [dstTransaction removeAllObjectsInCollection:[OWSDatabaseMigration collection]]; - // Copy migrations. - [srcTransaction - enumerateKeysAndObjectsInCollection:[OWSDatabaseMigration collection] - usingBlock:^(NSString *key, id object, BOOL *stop) { - if (self.isComplete) { - *stop = YES; - return; - } - if (![object isKindOfClass:[OWSDatabaseMigration class]]) { - OWSProdLogAndFail( - @"%@ unexpected class: %@", self.logTag, [object class]); - return; - } - OWSDatabaseMigration *migration = object; - [migration saveWithTransaction:dstTransaction]; - copiedMigrations++; - copiedEntities++; - }]; + // Copy database entities. + for (NSString *collection in collectionsToRestore) { + [srcTransaction enumerateKeysAndObjectsInCollection:collection + usingBlock:^(NSString *key, id object, BOOL *stop) { + if (self.isComplete) { + *stop = YES; + aborted = YES; + return; + } + Class expectedType = collectionTypeMap[collection]; + OWSAssert(expectedType); + if (![object isKindOfClass:expectedType]) { + OWSProdLogAndFail(@"%@ unexpected class: %@ != %@", + self.logTag, + [object class], + expectedType); + return; + } + TSYapDatabaseObject *databaseObject = object; + [databaseObject saveWithTransaction:dstTransaction]; + + NSUInteger count + = restoredEntityCounts[collection].unsignedIntValue; + restoredEntityCounts[collection] = @(count + 1); + copiedEntities++; + }]; + } }]; }]; - DDLogInfo(@"%@ copiedThreads: %llu", self.logTag, copiedThreads); - DDLogInfo(@"%@ copiedMessages: %llu", self.logTag, copiedInteractions); + if (aborted) { + return; + } + + for (NSString *collection in collectionsToRestore) { + Class expectedType = collectionTypeMap[collection]; + OWSAssert(expectedType); + DDLogInfo(@"%@ copied %@ (%@): %@", self.logTag, expectedType, collection, restoredEntityCounts[collection]); + } DDLogInfo(@"%@ copiedEntities: %llu", self.logTag, copiedEntities); - DDLogInfo(@"%@ copiedAttachments: %llu", self.logTag, copiedAttachments); - DDLogInfo(@"%@ copiedMigrations: %llu", self.logTag, copiedMigrations); [backupStorage logFileSizes]; @@ -510,9 +481,15 @@ NSString *const kOWSBackup_ImportDatabaseKeySpec = @"kOWSBackup_ImportDatabaseKe DDLogVerbose(@"%@ %s", self.logTag, __PRETTY_FUNCTION__); - [[[OWSDatabaseMigrationRunner alloc] initWithPrimaryStorage:self.primaryStorage] runAllOutstandingWithCompletion:^{ - completion(YES); - }]; + // It's okay that we do this in a separate transaction from the + // restoration of backup contents. If some of migrations don't + // complete, they'll be run the next time the app launches. + dispatch_async(dispatch_get_main_queue(), ^{ + [[[OWSDatabaseMigrationRunner alloc] initWithPrimaryStorage:self.primaryStorage] + runAllOutstandingWithCompletion:^{ + completion(YES); + }]; + }); } - (BOOL)restoreFileWithRecordName:(NSString *)recordName diff --git a/Signal/src/util/OWSBackupJob.m b/Signal/src/util/OWSBackupJob.m index 258e3db49..6c1d7697f 100644 --- a/Signal/src/util/OWSBackupJob.m +++ b/Signal/src/util/OWSBackupJob.m @@ -21,6 +21,7 @@ NSString *const kOWSBackup_KeychainService = @"kOWSBackup_KeychainService"; @property (nonatomic, weak) id delegate; @property (atomic) BOOL isComplete; +@property (atomic) BOOL hasSucceeded; @property (nonatomic) OWSPrimaryStorage *primaryStorage; @@ -96,6 +97,12 @@ NSString *const kOWSBackup_KeychainService = @"kOWSBackup_KeychainService"; return; } self.isComplete = YES; + + // There's a lot of asynchrony in these backup jobs; + // ensure we only end up finishing these jobs once. + OWSAssert(!self.hasSucceeded); + self.hasSucceeded = YES; + [self.delegate backupJobDidSucceed:self]; }); } diff --git a/SignalMessaging/environment/migrations/OWS106EnsureProfileComplete.swift b/SignalMessaging/environment/migrations/OWS106EnsureProfileComplete.swift index dbeac5ae7..d01092f12 100644 --- a/SignalMessaging/environment/migrations/OWS106EnsureProfileComplete.swift +++ b/SignalMessaging/environment/migrations/OWS106EnsureProfileComplete.swift @@ -21,11 +21,6 @@ public class OWS106EnsureProfileComplete: OWSDatabaseMigration { // Overriding runUp since we have some specific completion criteria which // is more likely to fail since it involves network requests. override public func runUp(completion:@escaping ((Void)) -> Void) { - guard type(of: self).sharedCompleteRegistrationFixerJob == nil else { - owsFail("\(self.TAG) should only be called once.") - return - } - let job = CompleteRegistrationFixerJob(completionHandler: { Logger.info("\(self.TAG) Completed. Saving.") self.save() diff --git a/SignalMessaging/environment/migrations/OWSDatabaseMigrationRunner.m b/SignalMessaging/environment/migrations/OWSDatabaseMigrationRunner.m index 1456c0d2e..4a0d3e7a9 100644 --- a/SignalMessaging/environment/migrations/OWSDatabaseMigrationRunner.m +++ b/SignalMessaging/environment/migrations/OWSDatabaseMigrationRunner.m @@ -56,52 +56,48 @@ NS_ASSUME_NONNULL_BEGIN - (void)runAllOutstandingWithCompletion:(OWSDatabaseMigrationCompletion)completion { - [self runMigrations:self.allMigrations completion:completion]; + [self runMigrations:[self.allMigrations mutableCopy] completion:completion]; } -- (void)runMigrations:(NSArray *)migrations +// Run migrations serially to: +// +// * Ensure predictable ordering. +// * Prevent them from interfering with each other (e.g. deadlock). +- (void)runMigrations:(NSMutableArray *)migrations completion:(OWSDatabaseMigrationCompletion)completion { OWSAssert(migrations); OWSAssert(completion); - NSMutableArray *migrationsToRun = [NSMutableArray new]; + // TODO: Remove. + DDLogInfo(@"%@ Considering migrations: %zd", self.logTag, migrations.count); for (OWSDatabaseMigration *migration in migrations) { - if ([OWSDatabaseMigration fetchObjectWithUniqueID:migration.uniqueId] == nil) { - [migrationsToRun addObject:migration]; - } + DDLogInfo(@"%@ Considering migrations: %@", self.logTag, migration.class); } - if (migrationsToRun.count < 1) { + // If there are no more migrations to run, complete. + if (migrations.count < 1) { dispatch_async(dispatch_get_main_queue(), ^{ completion(); }); return; } - NSUInteger totalMigrationCount = migrationsToRun.count; - __block NSUInteger completedMigrationCount = 0; - // Call the completion exactly once, when the last migration completes. - void (^checkMigrationCompletion)(void) = ^{ - @synchronized(self) - { - completedMigrationCount++; - if (completedMigrationCount == totalMigrationCount) { - dispatch_async(dispatch_get_main_queue(), ^{ - completion(); - }); - } - } - }; - - for (OWSDatabaseMigration *migration in migrationsToRun) { - if ([OWSDatabaseMigration fetchObjectWithUniqueID:migration.uniqueId]) { - DDLogDebug(@"%@ Skipping previously run migration: %@", self.logTag, migration); - } else { - DDLogWarn(@"%@ Running migration: %@", self.logTag, migration); - [migration runUpWithCompletion:checkMigrationCompletion]; - } + // Pop next migration from front of queue. + OWSDatabaseMigration *migration = migrations.firstObject; + [migrations removeObjectAtIndex:0]; + + // If migration has already been run, skip it. + if ([OWSDatabaseMigration fetchObjectWithUniqueID:migration.uniqueId] != nil) { + [self runMigrations:migrations completion:completion]; + return; } + + DDLogInfo(@"%@ Running migration: %@", self.logTag, migration); + [migration runUpWithCompletion:^{ + DDLogInfo(@"%@ Migration complete: %@", self.logTag, migration); + [self runMigrations:migrations completion:completion]; + }]; } @end diff --git a/SignalServiceKit/src/Storage/OWSStorage.m b/SignalServiceKit/src/Storage/OWSStorage.m index 0b84a0e78..de7ae50c3 100644 --- a/SignalServiceKit/src/Storage/OWSStorage.m +++ b/SignalServiceKit/src/Storage/OWSStorage.m @@ -74,7 +74,7 @@ typedef NSData *_Nullable (^CreateDatabaseMetadataBlock)(void); { id delegate = self.delegate; OWSAssert(delegate); - OWSAssert(delegate.areAllRegistrationsComplete || self.canWriteBeforeStorageReady); + // OWSAssert(delegate.areAllRegistrationsComplete || self.canWriteBeforeStorageReady); OWSBackgroundTask *_Nullable backgroundTask = nil; if (CurrentAppContext().isMainApp) {