From 96da091e9b706984a959249e926cf621fa4aff5d Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 4 Jul 2017 12:42:33 -0400 Subject: [PATCH 1/4] Run orphan cleanup on startup. // FREEBIE --- src/Storage/OWSOrphanedDataCleaner.h | 13 +- src/Storage/OWSOrphanedDataCleaner.m | 245 ++++++++++++++++++++------- 2 files changed, 194 insertions(+), 64 deletions(-) diff --git a/src/Storage/OWSOrphanedDataCleaner.h b/src/Storage/OWSOrphanedDataCleaner.h index ad06a02e8..b84698a5d 100644 --- a/src/Storage/OWSOrphanedDataCleaner.h +++ b/src/Storage/OWSOrphanedDataCleaner.h @@ -1,10 +1,13 @@ -// Copyright (c) 2016 Open Whisper Systems. All rights reserved. +// +// Copyright (c) 2017 Open Whisper Systems. All rights reserved. +// @interface OWSOrphanedDataCleaner : NSObject -/** - * Remove any inaccessible data left behind due to application bugs. - */ -- (void)removeOrphanedData; +- (instancetype)init NS_UNAVAILABLE; + ++ (void)auditAsync; + ++ (void)auditAndCleanupAsync; @end diff --git a/src/Storage/OWSOrphanedDataCleaner.m b/src/Storage/OWSOrphanedDataCleaner.m index d0072969c..d436dfa4b 100644 --- a/src/Storage/OWSOrphanedDataCleaner.m +++ b/src/Storage/OWSOrphanedDataCleaner.m @@ -11,82 +11,209 @@ @implementation OWSOrphanedDataCleaner -- (void)removeOrphanedData ++ (void)auditAsync { - // Remove interactions whose threads have been deleted - for (NSString *interactionId in [self orphanedInteractionIds]) { - DDLogWarn(@"Removing orphaned interaction with id: %@", interactionId); - TSInteraction *interaction = [TSInteraction fetchObjectWithUniqueID:interactionId]; - [interaction remove]; - } + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ + [OWSOrphanedDataCleaner auditAndCleanup:NO]; + }); +} + ++ (void)auditAndCleanupAsync +{ + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ + [OWSOrphanedDataCleaner auditAndCleanup:YES]; + }); +} - // Remove any lingering attachments - for (NSString *path in [self orphanedFilePaths]) { - DDLogWarn(@"Removing orphaned file attachment at path: %@", path); +// This method finds and optionally cleans up: +// +// * Orphan messages (with no thread). +// * Orphan attachments (with no message). +// * Orphan attachment files (with no attachment). +// * Missing attachment files (cannot be cleaned up). ++ (void)auditAndCleanup:(BOOL)shouldCleanup +{ + NSString *attachmentsFolder = [TSAttachmentStream attachmentsFolder]; + DDLogDebug(@"attachmentsFolder: %@", attachmentsFolder); + + __block int fileCount = 0; + __block long long totalFileSize = 0; + NSMutableSet *diskFilePaths = [NSMutableSet new]; + __unsafe_unretained __block void (^visitAttachmentFilesRecursable)(NSString *); + void (^visitAttachmentFiles)(NSString *); + visitAttachmentFiles = ^(NSString *dirPath) { NSError *error; - [[NSFileManager defaultManager] removeItemAtPath:path error:&error]; + NSArray *fileNames = + [[NSFileManager defaultManager] contentsOfDirectoryAtPath:dirPath error:&error]; if (error) { - DDLogError(@"Unable to remove orphaned file attachment at path:%@", path); + OWSFail(@"contentsOfDirectoryAtPath error: %@", error); + return; } - } -} + for (NSString *fileName in fileNames) { + NSString *filePath = [dirPath stringByAppendingPathComponent:fileName]; + BOOL isDirectory; + [[NSFileManager defaultManager] fileExistsAtPath:filePath isDirectory:&isDirectory]; + if (isDirectory) { + visitAttachmentFilesRecursable(filePath); + } else { + NSNumber *fileSize = + [[NSFileManager defaultManager] attributesOfItemAtPath:filePath error:&error][NSFileSize]; + if (error) { + OWSFail(@"attributesOfItemAtPath: %@ error: %@", filePath, error); + continue; + } + totalFileSize += fileSize.longLongValue; + fileCount++; + [diskFilePaths addObject:filePath]; + } + } + }; + visitAttachmentFilesRecursable = visitAttachmentFiles; + visitAttachmentFiles(attachmentsFolder); -- (NSArray *)orphanedInteractionIds -{ - NSMutableArray *interactionIds = [NSMutableArray new]; - [[TSInteraction dbReadConnection] readWithBlock:^(YapDatabaseReadTransaction *_Nonnull transaction) { - [TSInteraction enumerateCollectionObjectsWithTransaction:transaction - usingBlock:^(TSInteraction *interaction, BOOL *stop) { - TSThread *thread = [TSThread - fetchObjectWithUniqueID:interaction.uniqueThreadId - transaction:transaction]; - if (!thread) { - [interactionIds addObject:interaction.uniqueId]; - } - }]; + TSStorageManager *storageManager = [TSStorageManager sharedManager]; + YapDatabaseConnection *databaseConnection = storageManager.newDatabaseConnection; + + __block int attachmentStreamCount = 0; + NSMutableSet *attachmentFilePaths = [NSMutableSet new]; + NSMutableSet *attachmentIds = [NSMutableSet new]; + [databaseConnection readWithBlock:^(YapDatabaseReadTransaction *_Nonnull transaction) { + [transaction enumerateKeysAndObjectsInCollection:TSAttachmentStream.collection + usingBlock:^(NSString *key, TSAttachment *attachment, BOOL *stop) { + [attachmentIds addObject:attachment.uniqueId]; + if (![attachment isKindOfClass:[TSAttachmentStream class]]) { + return; + } + TSAttachmentStream *attachmentStream + = (TSAttachmentStream *)attachment; + attachmentStreamCount++; + NSString *_Nullable filePath = [attachmentStream filePath]; + OWSAssert(filePath); + [attachmentFilePaths addObject:filePath]; + }]; }]; + DDLogDebug(@"fileCount: %d", fileCount); + DDLogDebug(@"totalFileSize: %lld", totalFileSize); + DDLogDebug(@"attachmentStreams: %d", attachmentStreamCount); + DDLogDebug(@"attachmentStreams with file paths: %zd", attachmentFilePaths.count); - return [interactionIds copy]; -} + NSMutableSet *orphanDiskFilePaths = [diskFilePaths mutableCopy]; + [orphanDiskFilePaths minusSet:attachmentFilePaths]; + NSMutableSet *missingAttachmentFilePaths = [attachmentFilePaths mutableCopy]; + [missingAttachmentFilePaths minusSet:diskFilePaths]; -- (NSArray *)orphanedFilePaths -{ - NSError *error; - NSMutableArray *filenames = - [[[NSFileManager defaultManager] contentsOfDirectoryAtPath:[TSAttachmentStream attachmentsFolder] error:&error] - mutableCopy]; - if (error) { - DDLogError(@"error getting orphanedFilePaths:%@", error); - return @[]; - } + DDLogDebug(@"orphan disk file paths: %zd", orphanDiskFilePaths.count); + DDLogDebug(@"missing attachment file paths: %zd", missingAttachmentFilePaths.count); - NSMutableDictionary *attachmentIdFilenames = [NSMutableDictionary new]; - for (NSString *filename in filenames) { - // Remove extension from (e.g.) 1234.png to get the attachmentId "1234" - NSString *attachmentId = [filename stringByDeletingPathExtension]; - attachmentIdFilenames[attachmentId] = filename; - } + [self printPaths:orphanDiskFilePaths.allObjects label:@"orphan disk file paths"]; + [self printPaths:missingAttachmentFilePaths.allObjects label:@"missing attachment file paths"]; + + NSMutableSet *threadIds = [NSMutableSet new]; + [databaseConnection readWithBlock:^(YapDatabaseReadTransaction *_Nonnull transaction) { + [transaction enumerateKeysInCollection:TSThread.collection + usingBlock:^(NSString *_Nonnull key, BOOL *_Nonnull stop) { + [threadIds addObject:key]; + }]; + }]; - [TSInteraction enumerateCollectionObjectsUsingBlock:^(TSInteraction *interaction, BOOL *stop) { - if ([interaction isKindOfClass:[TSMessage class]]) { - TSMessage *message = (TSMessage *)interaction; - if ([message hasAttachments]) { - for (NSString *attachmentId in message.attachmentIds) { - [attachmentIdFilenames removeObjectForKey:attachmentId]; + NSMutableSet *orphanInteractionIds = [NSMutableSet new]; + NSMutableSet *messageAttachmentIds = [NSMutableSet new]; + [databaseConnection readWithBlock:^(YapDatabaseReadTransaction *_Nonnull transaction) { + [transaction enumerateKeysAndObjectsInCollection:TSMessage.collection + usingBlock:^(NSString *key, TSInteraction *interaction, BOOL *stop) { + if (![threadIds containsObject:interaction.uniqueThreadId]) { + [orphanInteractionIds addObject:interaction.uniqueId]; + } + + if (![interaction isKindOfClass:[TSMessage class]]) { + return; + } + TSMessage *message = (TSMessage *)interaction; + if (message.attachmentIds.count > 0) { + [messageAttachmentIds addObjectsFromArray:message.attachmentIds]; + } + }]; + }]; + + DDLogDebug(@"attachmentIds: %zd", attachmentIds.count); + DDLogDebug(@"messageAttachmentIds: %zd", messageAttachmentIds.count); + + NSMutableSet *orphanAttachmentIds = [attachmentIds mutableCopy]; + [orphanAttachmentIds minusSet:messageAttachmentIds]; + NSMutableSet *missingAttachmentIds = [messageAttachmentIds mutableCopy]; + [missingAttachmentIds minusSet:attachmentIds]; + + DDLogDebug(@"orphan attachmentIds: %zd", orphanAttachmentIds.count); + DDLogDebug(@"missing attachmentIds: %zd", missingAttachmentIds.count); + DDLogDebug(@"orphan interactions: %zd", orphanInteractionIds.count); + + // We need to avoid cleaning up new attachments and files that are still in the process of + // being created/written, so we don't clean up anything recent. + const NSTimeInterval kMinimumOrphanAge = 15 * 60.f; + + if (shouldCleanup) { + [databaseConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *_Nonnull transaction) { + for (NSString *interactionId in orphanInteractionIds) { + TSInteraction *interaction = [TSInteraction fetchObjectWithUniqueID:interactionId]; + if (!interaction) { + // This could just be a race condition, but it should be very unlikely. + OWSFail(@"Could not load interaction: %@", interactionId); + continue; } + DDLogInfo(@"Removing orphan message: %@", interaction.uniqueId); + [interaction removeWithTransaction:transaction]; + } + for (NSString *attachmentId in orphanAttachmentIds) { + TSAttachment *attachment = [TSAttachment fetchObjectWithUniqueID:attachmentId]; + if (!attachment) { + // This could just be a race condition, but it should be very unlikely. + OWSFail(@"Could not load attachment: %@", attachmentId); + continue; + } + if (![attachment isKindOfClass:[TSAttachmentStream class]]) { + continue; + } + TSAttachmentStream *attachmentStream = (TSAttachmentStream *)attachment; + // Don't delete attachments which were created in the last N minutes. + if (fabs([attachmentStream.creationTimestamp timeIntervalSinceNow]) < kMinimumOrphanAge) { + DDLogInfo(@"Skipping orphan attachment due to age: %f", + fabs([attachmentStream.creationTimestamp timeIntervalSinceNow])); + continue; + } + DDLogInfo(@"Removing orphan attachment: %@", attachmentStream.uniqueId); + [attachmentStream removeWithTransaction:transaction]; + } + }]; + + for (NSString *filePath in orphanDiskFilePaths) { + NSError *error; + NSDictionary *attributes = [[NSFileManager defaultManager] attributesOfItemAtPath:filePath error:&error]; + if (!attributes || error) { + OWSFail(@"Could not get attributes of file at: %@", filePath); + continue; + } + // Don't delete files which were created in the last N minutes. + if (fabs([attributes.fileModificationDate timeIntervalSinceNow]) < kMinimumOrphanAge) { + DDLogInfo(@"Skipping orphan attachment file due to age: %f", + fabs([attributes.fileModificationDate timeIntervalSinceNow])); + continue; } - } - }]; - NSArray *filenamesToDelete = [attachmentIdFilenames allValues]; - NSMutableArray *absolutePathsToDelete = [NSMutableArray arrayWithCapacity:[filenamesToDelete count]]; - for (NSString *filename in filenamesToDelete) { - NSString *absolutePath = [[TSAttachmentStream attachmentsFolder] stringByAppendingFormat:@"/%@", filename]; - [absolutePathsToDelete addObject:absolutePath]; + DDLogInfo(@"Removing orphan attachment file: %@", filePath); + [[NSFileManager defaultManager] removeItemAtPath:filePath error:&error]; + if (error) { + OWSFail(@"Could not remove orphan file at: %@", filePath); + } + } } +} - return [absolutePathsToDelete copy]; ++ (void)printPaths:(NSArray *)paths label:(NSString *)label +{ + for (NSString *path in [paths sortedArrayUsingSelector:@selector(compare:)]) { + DDLogDebug(@"%@: %@", label, path); + } } @end From 762f9151795e813d04c97ed54f0666abd715badb Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Thu, 6 Jul 2017 13:30:20 -0400 Subject: [PATCH 2/4] Respond to CR. // FREEBIE --- src/Storage/OWSOrphanedDataCleaner.m | 99 +++++++++++++++------------- 1 file changed, 53 insertions(+), 46 deletions(-) diff --git a/src/Storage/OWSOrphanedDataCleaner.m b/src/Storage/OWSOrphanedDataCleaner.m index d436dfa4b..003698bfb 100644 --- a/src/Storage/OWSOrphanedDataCleaner.m +++ b/src/Storage/OWSOrphanedDataCleaner.m @@ -31,6 +31,11 @@ // * Orphan attachments (with no message). // * Orphan attachment files (with no attachment). // * Missing attachment files (cannot be cleaned up). +// These are attachments which have no file on disk. They should be extremely rare - +// the only cases I have seen are probably due to debugging. +// They can't be cleaned up - we don't want to delete the TSAttachmentStream or +// its corresponding message. Better that the broken message shows up in the +// conversation view. + (void)auditAndCleanup:(BOOL)shouldCleanup { NSString *attachmentsFolder = [TSAttachmentStream attachmentsFolder]; @@ -152,59 +157,61 @@ // being created/written, so we don't clean up anything recent. const NSTimeInterval kMinimumOrphanAge = 15 * 60.f; - if (shouldCleanup) { - [databaseConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *_Nonnull transaction) { - for (NSString *interactionId in orphanInteractionIds) { - TSInteraction *interaction = [TSInteraction fetchObjectWithUniqueID:interactionId]; - if (!interaction) { - // This could just be a race condition, but it should be very unlikely. - OWSFail(@"Could not load interaction: %@", interactionId); - continue; - } - DDLogInfo(@"Removing orphan message: %@", interaction.uniqueId); - [interaction removeWithTransaction:transaction]; + if (!shouldCleanup) { + return; + } + + [databaseConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *_Nonnull transaction) { + for (NSString *interactionId in orphanInteractionIds) { + TSInteraction *interaction = [TSInteraction fetchObjectWithUniqueID:interactionId]; + if (!interaction) { + // This could just be a race condition, but it should be very unlikely. + OWSFail(@"Could not load interaction: %@", interactionId); + continue; } - for (NSString *attachmentId in orphanAttachmentIds) { - TSAttachment *attachment = [TSAttachment fetchObjectWithUniqueID:attachmentId]; - if (!attachment) { - // This could just be a race condition, but it should be very unlikely. - OWSFail(@"Could not load attachment: %@", attachmentId); - continue; - } - if (![attachment isKindOfClass:[TSAttachmentStream class]]) { - continue; - } - TSAttachmentStream *attachmentStream = (TSAttachmentStream *)attachment; - // Don't delete attachments which were created in the last N minutes. - if (fabs([attachmentStream.creationTimestamp timeIntervalSinceNow]) < kMinimumOrphanAge) { - DDLogInfo(@"Skipping orphan attachment due to age: %f", - fabs([attachmentStream.creationTimestamp timeIntervalSinceNow])); - continue; - } - DDLogInfo(@"Removing orphan attachment: %@", attachmentStream.uniqueId); - [attachmentStream removeWithTransaction:transaction]; + DDLogInfo(@"Removing orphan message: %@", interaction.uniqueId); + [interaction removeWithTransaction:transaction]; + } + for (NSString *attachmentId in orphanAttachmentIds) { + TSAttachment *attachment = [TSAttachment fetchObjectWithUniqueID:attachmentId]; + if (!attachment) { + // This could just be a race condition, but it should be very unlikely. + OWSFail(@"Could not load attachment: %@", attachmentId); + continue; } - }]; - - for (NSString *filePath in orphanDiskFilePaths) { - NSError *error; - NSDictionary *attributes = [[NSFileManager defaultManager] attributesOfItemAtPath:filePath error:&error]; - if (!attributes || error) { - OWSFail(@"Could not get attributes of file at: %@", filePath); + if (![attachment isKindOfClass:[TSAttachmentStream class]]) { continue; } - // Don't delete files which were created in the last N minutes. - if (fabs([attributes.fileModificationDate timeIntervalSinceNow]) < kMinimumOrphanAge) { - DDLogInfo(@"Skipping orphan attachment file due to age: %f", - fabs([attributes.fileModificationDate timeIntervalSinceNow])); + TSAttachmentStream *attachmentStream = (TSAttachmentStream *)attachment; + // Don't delete attachments which were created in the last N minutes. + if (fabs([attachmentStream.creationTimestamp timeIntervalSinceNow]) < kMinimumOrphanAge) { + DDLogInfo(@"Skipping orphan attachment due to age: %f", + fabs([attachmentStream.creationTimestamp timeIntervalSinceNow])); continue; } + DDLogInfo(@"Removing orphan attachment: %@", attachmentStream.uniqueId); + [attachmentStream removeWithTransaction:transaction]; + } + }]; - DDLogInfo(@"Removing orphan attachment file: %@", filePath); - [[NSFileManager defaultManager] removeItemAtPath:filePath error:&error]; - if (error) { - OWSFail(@"Could not remove orphan file at: %@", filePath); - } + for (NSString *filePath in orphanDiskFilePaths) { + NSError *error; + NSDictionary *attributes = [[NSFileManager defaultManager] attributesOfItemAtPath:filePath error:&error]; + if (!attributes || error) { + OWSFail(@"Could not get attributes of file at: %@", filePath); + continue; + } + // Don't delete files which were created in the last N minutes. + if (fabs([attributes.fileModificationDate timeIntervalSinceNow]) < kMinimumOrphanAge) { + DDLogInfo(@"Skipping orphan attachment file due to age: %f", + fabs([attributes.fileModificationDate timeIntervalSinceNow])); + continue; + } + + DDLogInfo(@"Removing orphan attachment file: %@", filePath); + [[NSFileManager defaultManager] removeItemAtPath:filePath error:&error]; + if (error) { + OWSFail(@"Could not remove orphan file at: %@", filePath); } } } From 7a50d6b996cb7a9f9e0a78c2dc6c420f5040b224 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 7 Jul 2017 10:26:09 -0400 Subject: [PATCH 3/4] Fix broken tests. // FREEBIE --- Example/TSKitiOSTestApp/Podfile.lock | 2 +- src/Messages/Attachments/TSAttachmentStream.h | 1 - src/Messages/Attachments/TSAttachmentStream.m | 13 -- src/Storage/OWSOrphanedDataCleaner.h | 19 ++- src/Storage/OWSOrphanedDataCleaner.m | 154 +++++++++++------- tests/Storage/OWSOrphanedDataCleanerTest.m | 87 ++++++++-- 6 files changed, 188 insertions(+), 88 deletions(-) diff --git a/Example/TSKitiOSTestApp/Podfile.lock b/Example/TSKitiOSTestApp/Podfile.lock index ddf365882..02217da28 100644 --- a/Example/TSKitiOSTestApp/Podfile.lock +++ b/Example/TSKitiOSTestApp/Podfile.lock @@ -111,7 +111,7 @@ EXTERNAL SOURCES: AxolotlKit: :git: https://github.com/WhisperSystems/SignalProtocolKit.git SignalServiceKit: - :path: "../../SignalServiceKit.podspec" + :path: ../../SignalServiceKit.podspec SocketRocket: :git: https://github.com/facebook/SocketRocket.git diff --git a/src/Messages/Attachments/TSAttachmentStream.h b/src/Messages/Attachments/TSAttachmentStream.h index 10d381500..3b60f6ff5 100644 --- a/src/Messages/Attachments/TSAttachmentStream.h +++ b/src/Messages/Attachments/TSAttachmentStream.h @@ -46,7 +46,6 @@ NS_ASSUME_NONNULL_BEGIN + (void)deleteAttachments; + (NSString *)attachmentsFolder; -+ (NSUInteger)numberOfItemsInAttachmentsFolder; - (CGSize)imageSizeWithTransaction:(YapDatabaseReadWriteTransaction *)transaction; - (CGSize)imageSizeWithoutTransaction; diff --git a/src/Messages/Attachments/TSAttachmentStream.m b/src/Messages/Attachments/TSAttachmentStream.m index 8951cdff3..47ec187c9 100644 --- a/src/Messages/Attachments/TSAttachmentStream.m +++ b/src/Messages/Attachments/TSAttachmentStream.m @@ -189,19 +189,6 @@ NS_ASSUME_NONNULL_BEGIN return attachmentsFolder; } -+ (NSUInteger)numberOfItemsInAttachmentsFolder -{ - NSError *error; - NSUInteger count = - [[[NSFileManager defaultManager] contentsOfDirectoryAtPath:[self attachmentsFolder] error:&error] count]; - - if (error) { - DDLogError(@"Unable to count attachments in attachments folder. Error: %@", error); - } - - return count; -} - - (nullable NSString *)filePath { if (!self.localRelativeFilePath) { diff --git a/src/Storage/OWSOrphanedDataCleaner.h b/src/Storage/OWSOrphanedDataCleaner.h index b84698a5d..960256a10 100644 --- a/src/Storage/OWSOrphanedDataCleaner.h +++ b/src/Storage/OWSOrphanedDataCleaner.h @@ -2,12 +2,29 @@ // Copyright (c) 2017 Open Whisper Systems. All rights reserved. // +NS_ASSUME_NONNULL_BEGIN + +// Notes: +// +// * On disk, we only bother cleaning up files, not directories. +// * For code simplicity, we don't guarantee that everything is +// cleaned up in a single pass. If an interaction is cleaned up, +// it's attachments might not be cleaned up until the next pass. +// If an attachment is cleaned up, it's file on disk might not +// be cleaned up until the next pass. @interface OWSOrphanedDataCleaner : NSObject - (instancetype)init NS_UNAVAILABLE; + (void)auditAsync; -+ (void)auditAndCleanupAsync; +// completion, if present, will be invoked on the main thread. ++ (void)auditAndCleanupAsync:(void (^_Nullable)())completion; + ++ (NSSet *)filePathsInAttachmentsFolder; + ++ (long long)fileSizeOfFilePaths:(NSArray *)filePaths; @end + +NS_ASSUME_NONNULL_END diff --git a/src/Storage/OWSOrphanedDataCleaner.m b/src/Storage/OWSOrphanedDataCleaner.m index 003698bfb..76d0b8064 100644 --- a/src/Storage/OWSOrphanedDataCleaner.m +++ b/src/Storage/OWSOrphanedDataCleaner.m @@ -9,19 +9,29 @@ #import "TSStorageManager.h" #import "TSThread.h" +NS_ASSUME_NONNULL_BEGIN + +#ifdef SSK_BUILDING_FOR_TESTS +#define CleanupLogDebug NSLog +#define CleanupLogInfo NSLog +#else +#define CleanupLogDebug DDLogDebug +#define CleanupLogInfo DDLogInfo +#endif + @implementation OWSOrphanedDataCleaner + (void)auditAsync { dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ - [OWSOrphanedDataCleaner auditAndCleanup:NO]; + [OWSOrphanedDataCleaner auditAndCleanup:NO completion:nil]; }); } -+ (void)auditAndCleanupAsync ++ (void)auditAndCleanupAsync:(void (^_Nullable)())completion { dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ - [OWSOrphanedDataCleaner auditAndCleanup:YES]; + [OWSOrphanedDataCleaner auditAndCleanup:YES completion:completion]; }); } @@ -36,45 +46,11 @@ // They can't be cleaned up - we don't want to delete the TSAttachmentStream or // its corresponding message. Better that the broken message shows up in the // conversation view. -+ (void)auditAndCleanup:(BOOL)shouldCleanup ++ (void)auditAndCleanup:(BOOL)shouldCleanup completion:(void (^_Nullable)())completion { - NSString *attachmentsFolder = [TSAttachmentStream attachmentsFolder]; - DDLogDebug(@"attachmentsFolder: %@", attachmentsFolder); - - __block int fileCount = 0; - __block long long totalFileSize = 0; - NSMutableSet *diskFilePaths = [NSMutableSet new]; - __unsafe_unretained __block void (^visitAttachmentFilesRecursable)(NSString *); - void (^visitAttachmentFiles)(NSString *); - visitAttachmentFiles = ^(NSString *dirPath) { - NSError *error; - NSArray *fileNames = - [[NSFileManager defaultManager] contentsOfDirectoryAtPath:dirPath error:&error]; - if (error) { - OWSFail(@"contentsOfDirectoryAtPath error: %@", error); - return; - } - for (NSString *fileName in fileNames) { - NSString *filePath = [dirPath stringByAppendingPathComponent:fileName]; - BOOL isDirectory; - [[NSFileManager defaultManager] fileExistsAtPath:filePath isDirectory:&isDirectory]; - if (isDirectory) { - visitAttachmentFilesRecursable(filePath); - } else { - NSNumber *fileSize = - [[NSFileManager defaultManager] attributesOfItemAtPath:filePath error:&error][NSFileSize]; - if (error) { - OWSFail(@"attributesOfItemAtPath: %@ error: %@", filePath, error); - continue; - } - totalFileSize += fileSize.longLongValue; - fileCount++; - [diskFilePaths addObject:filePath]; - } - } - }; - visitAttachmentFilesRecursable = visitAttachmentFiles; - visitAttachmentFiles(attachmentsFolder); + NSSet *diskFilePaths = [self filePathsInAttachmentsFolder]; + long long totalFileSize = [self fileSizeOfFilePaths:diskFilePaths.allObjects]; + NSUInteger fileCount = diskFilePaths.count; TSStorageManager *storageManager = [TSStorageManager sharedManager]; YapDatabaseConnection *databaseConnection = storageManager.newDatabaseConnection; @@ -98,18 +74,18 @@ }]; }]; - DDLogDebug(@"fileCount: %d", fileCount); - DDLogDebug(@"totalFileSize: %lld", totalFileSize); - DDLogDebug(@"attachmentStreams: %d", attachmentStreamCount); - DDLogDebug(@"attachmentStreams with file paths: %zd", attachmentFilePaths.count); + CleanupLogDebug(@"fileCount: %zd", fileCount); + CleanupLogDebug(@"totalFileSize: %lld", totalFileSize); + CleanupLogDebug(@"attachmentStreams: %d", attachmentStreamCount); + CleanupLogDebug(@"attachmentStreams with file paths: %zd", attachmentFilePaths.count); NSMutableSet *orphanDiskFilePaths = [diskFilePaths mutableCopy]; [orphanDiskFilePaths minusSet:attachmentFilePaths]; NSMutableSet *missingAttachmentFilePaths = [attachmentFilePaths mutableCopy]; [missingAttachmentFilePaths minusSet:diskFilePaths]; - DDLogDebug(@"orphan disk file paths: %zd", orphanDiskFilePaths.count); - DDLogDebug(@"missing attachment file paths: %zd", missingAttachmentFilePaths.count); + CleanupLogDebug(@"orphan disk file paths: %zd", orphanDiskFilePaths.count); + CleanupLogDebug(@"missing attachment file paths: %zd", missingAttachmentFilePaths.count); [self printPaths:orphanDiskFilePaths.allObjects label:@"orphan disk file paths"]; [self printPaths:missingAttachmentFilePaths.allObjects label:@"missing attachment file paths"]; @@ -141,21 +117,25 @@ }]; }]; - DDLogDebug(@"attachmentIds: %zd", attachmentIds.count); - DDLogDebug(@"messageAttachmentIds: %zd", messageAttachmentIds.count); + CleanupLogDebug(@"attachmentIds: %zd", attachmentIds.count); + CleanupLogDebug(@"messageAttachmentIds: %zd", messageAttachmentIds.count); NSMutableSet *orphanAttachmentIds = [attachmentIds mutableCopy]; [orphanAttachmentIds minusSet:messageAttachmentIds]; NSMutableSet *missingAttachmentIds = [messageAttachmentIds mutableCopy]; [missingAttachmentIds minusSet:attachmentIds]; - DDLogDebug(@"orphan attachmentIds: %zd", orphanAttachmentIds.count); - DDLogDebug(@"missing attachmentIds: %zd", missingAttachmentIds.count); - DDLogDebug(@"orphan interactions: %zd", orphanInteractionIds.count); + CleanupLogDebug(@"orphan attachmentIds: %zd", orphanAttachmentIds.count); + CleanupLogDebug(@"missing attachmentIds: %zd", missingAttachmentIds.count); + CleanupLogDebug(@"orphan interactions: %zd", orphanInteractionIds.count); // We need to avoid cleaning up new attachments and files that are still in the process of // being created/written, so we don't clean up anything recent. +#ifdef SSK_BUILDING_FOR_TESTS + const NSTimeInterval kMinimumOrphanAge = 0.f; +#else const NSTimeInterval kMinimumOrphanAge = 15 * 60.f; +#endif if (!shouldCleanup) { return; @@ -169,7 +149,7 @@ OWSFail(@"Could not load interaction: %@", interactionId); continue; } - DDLogInfo(@"Removing orphan message: %@", interaction.uniqueId); + CleanupLogInfo(@"Removing orphan message: %@", interaction.uniqueId); [interaction removeWithTransaction:transaction]; } for (NSString *attachmentId in orphanAttachmentIds) { @@ -185,11 +165,11 @@ TSAttachmentStream *attachmentStream = (TSAttachmentStream *)attachment; // Don't delete attachments which were created in the last N minutes. if (fabs([attachmentStream.creationTimestamp timeIntervalSinceNow]) < kMinimumOrphanAge) { - DDLogInfo(@"Skipping orphan attachment due to age: %f", + CleanupLogInfo(@"Skipping orphan attachment due to age: %f", fabs([attachmentStream.creationTimestamp timeIntervalSinceNow])); continue; } - DDLogInfo(@"Removing orphan attachment: %@", attachmentStream.uniqueId); + CleanupLogInfo(@"Removing orphan attachment: %@", attachmentStream.uniqueId); [attachmentStream removeWithTransaction:transaction]; } }]; @@ -203,24 +183,82 @@ } // Don't delete files which were created in the last N minutes. if (fabs([attributes.fileModificationDate timeIntervalSinceNow]) < kMinimumOrphanAge) { - DDLogInfo(@"Skipping orphan attachment file due to age: %f", + CleanupLogInfo(@"Skipping orphan attachment file due to age: %f", fabs([attributes.fileModificationDate timeIntervalSinceNow])); continue; } - DDLogInfo(@"Removing orphan attachment file: %@", filePath); + CleanupLogInfo(@"Removing orphan attachment file: %@", filePath); [[NSFileManager defaultManager] removeItemAtPath:filePath error:&error]; if (error) { OWSFail(@"Could not remove orphan file at: %@", filePath); } } + + if (completion) { + dispatch_async(dispatch_get_main_queue(), ^{ + completion(); + }); + } } + (void)printPaths:(NSArray *)paths label:(NSString *)label { for (NSString *path in [paths sortedArrayUsingSelector:@selector(compare:)]) { - DDLogDebug(@"%@: %@", label, path); + CleanupLogDebug(@"%@: %@", label, path); + } +} + ++ (NSSet *)filePathsInAttachmentsFolder +{ + NSString *attachmentsFolder = [TSAttachmentStream attachmentsFolder]; + CleanupLogDebug(@"attachmentsFolder: %@", attachmentsFolder); + + return [self filePathsInDirectory:attachmentsFolder]; +} + ++ (NSSet *)filePathsInDirectory:(NSString *)dirPath +{ + NSMutableSet *filePaths = [NSMutableSet new]; + NSError *error; + NSArray *fileNames = [[NSFileManager defaultManager] contentsOfDirectoryAtPath:dirPath error:&error]; + if (error) { + OWSFail(@"contentsOfDirectoryAtPath error: %@", error); + return [NSSet new]; } + for (NSString *fileName in fileNames) { + NSString *filePath = [dirPath stringByAppendingPathComponent:fileName]; + BOOL isDirectory; + [[NSFileManager defaultManager] fileExistsAtPath:filePath isDirectory:&isDirectory]; + if (isDirectory) { + [filePaths addObjectsFromArray:[self filePathsInDirectory:filePath].allObjects]; + } else { + [filePaths addObject:filePath]; + } + } + return filePaths; +} + ++ (long long)fileSizeOfFilePath:(NSString *)filePath +{ + NSError *error; + NSNumber *fileSize = [[NSFileManager defaultManager] attributesOfItemAtPath:filePath error:&error][NSFileSize]; + if (error) { + OWSFail(@"attributesOfItemAtPath: %@ error: %@", filePath, error); + return 0; + } + return fileSize.longLongValue; +} + ++ (long long)fileSizeOfFilePaths:(NSArray *)filePaths +{ + long long result = 0; + for (NSString *filePath in filePaths) { + result += [self fileSizeOfFilePath:filePath]; + } + return result; } @end + +NS_ASSUME_NONNULL_END diff --git a/tests/Storage/OWSOrphanedDataCleanerTest.m b/tests/Storage/OWSOrphanedDataCleanerTest.m index 0f4bb7fff..0abda3f4b 100644 --- a/tests/Storage/OWSOrphanedDataCleanerTest.m +++ b/tests/Storage/OWSOrphanedDataCleanerTest.m @@ -14,6 +14,8 @@ @end +#pragma mark - + @implementation OWSOrphanedDataCleanerTest - (void)setUp @@ -24,7 +26,7 @@ // Set up initial conditions & Sanity check [TSAttachmentStream deleteAttachments]; - XCTAssertEqual(0, [TSAttachmentStream numberOfItemsInAttachmentsFolder]); + XCTAssertEqual(0, [self numberOfItemsInAttachmentsFolder]); [TSAttachmentStream removeAllObjectsInCollection]; XCTAssertEqual(0, [TSAttachmentStream numberOfKeysInCollection]); [TSIncomingMessage removeAllObjectsInCollection]; @@ -38,6 +40,11 @@ [super tearDown]; } +- (NSUInteger)numberOfItemsInAttachmentsFolder +{ + return [OWSOrphanedDataCleaner filePathsInAttachmentsFolder].count; +} + - (void)testInteractionsWithoutThreadAreDeleted { // This thread is intentionally not saved. It's meant to recreate a situation we've seen where interactions exist @@ -53,7 +60,17 @@ [incomingMessage save]; XCTAssertEqual(1, [TSIncomingMessage numberOfKeysInCollection]); - [[OWSOrphanedDataCleaner new] removeOrphanedData]; + XCTestExpectation *expectation = [self expectationWithDescription:@"Cleanup"]; + [OWSOrphanedDataCleaner auditAndCleanupAsync:^{ + [expectation fulfill]; + }]; + [self waitForExpectationsWithTimeout:5.0 + handler:^(NSError *error) { + if (error) { + XCTFail(@"Expectation Failed with error: %@", error); + } + }]; + XCTAssertEqual(0, [TSIncomingMessage numberOfKeysInCollection]); } @@ -70,14 +87,24 @@ [incomingMessage save]; XCTAssertEqual(1, [TSIncomingMessage numberOfKeysInCollection]); - [[OWSOrphanedDataCleaner new] removeOrphanedData]; + XCTestExpectation *expectation = [self expectationWithDescription:@"Cleanup"]; + [OWSOrphanedDataCleaner auditAndCleanupAsync:^{ + [expectation fulfill]; + }]; + [self waitForExpectationsWithTimeout:5.0 + handler:^(NSError *error) { + if (error) { + XCTFail(@"Expectation Failed with error: %@", error); + } + }]; + XCTAssertEqual(1, [TSIncomingMessage numberOfKeysInCollection]); } - (void)testFilesWithoutInteractionsAreDeleted { // sanity check - XCTAssertEqual(0, [TSAttachmentStream numberOfItemsInAttachmentsFolder]); + XCTAssertEqual(0, [self numberOfItemsInAttachmentsFolder]); NSError *error; TSAttachmentStream *attachmentStream = [[TSAttachmentStream alloc] initWithContentType:@"image/jpeg" sourceFilename:nil]; @@ -86,12 +113,25 @@ NSString *orphanedFilePath = [attachmentStream filePath]; BOOL fileExists = [[NSFileManager defaultManager] fileExistsAtPath:orphanedFilePath]; XCTAssert(fileExists); - XCTAssertEqual(1, [TSAttachmentStream numberOfItemsInAttachmentsFolder]); + XCTAssertEqual(1, [self numberOfItemsInAttachmentsFolder]); + + // Do multiple cleanup passes. + for (int i = 0; i < 2; i++) { + XCTestExpectation *expectation = [self expectationWithDescription:@"Cleanup"]; + [OWSOrphanedDataCleaner auditAndCleanupAsync:^{ + [expectation fulfill]; + }]; + [self waitForExpectationsWithTimeout:5.0 + handler:^(NSError *error) { + if (error) { + XCTFail(@"Expectation Failed with error: %@", error); + } + }]; + } - [[OWSOrphanedDataCleaner new] removeOrphanedData]; fileExists = [[NSFileManager defaultManager] fileExistsAtPath:orphanedFilePath]; XCTAssertFalse(fileExists); - XCTAssertEqual(0, [TSAttachmentStream numberOfItemsInAttachmentsFolder]); + XCTAssertEqual(0, [self numberOfItemsInAttachmentsFolder]); } - (void)testFilesWithInteractionsAreNotDeleted @@ -116,13 +156,22 @@ NSString *attachmentFilePath = [attachmentStream filePath]; BOOL fileExists = [[NSFileManager defaultManager] fileExistsAtPath:attachmentFilePath]; XCTAssert(fileExists); - XCTAssertEqual(1, [TSAttachmentStream numberOfItemsInAttachmentsFolder]); - - [[OWSOrphanedDataCleaner new] removeOrphanedData]; + XCTAssertEqual(1, [self numberOfItemsInAttachmentsFolder]); + + XCTestExpectation *expectation = [self expectationWithDescription:@"Cleanup"]; + [OWSOrphanedDataCleaner auditAndCleanupAsync:^{ + [expectation fulfill]; + }]; + [self waitForExpectationsWithTimeout:5.0 + handler:^(NSError *error) { + if (error) { + XCTFail(@"Expectation Failed with error: %@", error); + } + }]; fileExists = [[NSFileManager defaultManager] fileExistsAtPath:attachmentFilePath]; XCTAssert(fileExists); - XCTAssertEqual(1, [TSAttachmentStream numberOfItemsInAttachmentsFolder]); + XCTAssertEqual(1, [self numberOfItemsInAttachmentsFolder]); } - (void)testFilesWithoutAttachmentStreamsAreDeleted @@ -135,12 +184,22 @@ NSString *orphanedFilePath = [attachmentStream filePath]; BOOL fileExists = [[NSFileManager defaultManager] fileExistsAtPath:orphanedFilePath]; XCTAssert(fileExists); - XCTAssertEqual(1, [TSAttachmentStream numberOfItemsInAttachmentsFolder]); + XCTAssertEqual(1, [self numberOfItemsInAttachmentsFolder]); + + XCTestExpectation *expectation = [self expectationWithDescription:@"Cleanup"]; + [OWSOrphanedDataCleaner auditAndCleanupAsync:^{ + [expectation fulfill]; + }]; + [self waitForExpectationsWithTimeout:5.0 + handler:^(NSError *error) { + if (error) { + XCTFail(@"Expectation Failed with error: %@", error); + } + }]; - [[OWSOrphanedDataCleaner new] removeOrphanedData]; fileExists = [[NSFileManager defaultManager] fileExistsAtPath:orphanedFilePath]; XCTAssertFalse(fileExists); - XCTAssertEqual(0, [TSAttachmentStream numberOfItemsInAttachmentsFolder]); + XCTAssertEqual(0, [self numberOfItemsInAttachmentsFolder]); } @end From 6fa3fac4aed9bf36d14568f4d41a70424abd3721 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 7 Jul 2017 11:00:52 -0400 Subject: [PATCH 4/4] Fix broken tests. // FREEBIE --- Example/TSKitiOSTestApp/Podfile | 3 ++- Example/TSKitiOSTestApp/Podfile.lock | 2 +- .../xcshareddata/xcschemes/TSKitiOSTestApp.xcscheme | 2 +- src/Network/API/TSNetworkManager.h | 2 +- src/Network/API/TSNetworkManager.m | 3 ++- 5 files changed, 7 insertions(+), 5 deletions(-) diff --git a/Example/TSKitiOSTestApp/Podfile b/Example/TSKitiOSTestApp/Podfile index 96619ecd8..ac9917f84 100644 --- a/Example/TSKitiOSTestApp/Podfile +++ b/Example/TSKitiOSTestApp/Podfile @@ -21,7 +21,8 @@ post_install do |installer| existing_definitions = "$(inheritied)" end - config.build_settings['GCC_PREPROCESSOR_DEFINITIONS'] = "#{existing_definitions} SSK_BUILDING_FOR_TESTS=1" + config.build_settings['GCC_PREPROCESSOR_DEFINITIONS'] = "#{existing_definitions} SSK_BUILDING_FOR_TESTS=1" + config.build_settings['OTHER_SWIFT_FLAGS'] = ['$(inherited)', '-DSSK_BUILDING_FOR_TESTS'] end end end diff --git a/Example/TSKitiOSTestApp/Podfile.lock b/Example/TSKitiOSTestApp/Podfile.lock index 02217da28..c66d52d58 100644 --- a/Example/TSKitiOSTestApp/Podfile.lock +++ b/Example/TSKitiOSTestApp/Podfile.lock @@ -140,6 +140,6 @@ SPEC CHECKSUMS: UnionFind: c33be5adb12983981d6e827ea94fc7f9e370f52d YapDatabase: cd911121580ff16675f65ad742a9eb0ab4d9e266 -PODFILE CHECKSUM: 124c05542083fefccb75f4b7afbdd839e27ff5ab +PODFILE CHECKSUM: a0f4507b6b4e6f9da3250901b06187a67236e083 COCOAPODS: 1.2.1 diff --git a/Example/TSKitiOSTestApp/TSKitiOSTestApp.xcodeproj/xcshareddata/xcschemes/TSKitiOSTestApp.xcscheme b/Example/TSKitiOSTestApp/TSKitiOSTestApp.xcodeproj/xcshareddata/xcschemes/TSKitiOSTestApp.xcscheme index faa3aee10..17bb32fd8 100644 --- a/Example/TSKitiOSTestApp/TSKitiOSTestApp.xcodeproj/xcshareddata/xcschemes/TSKitiOSTestApp.xcscheme +++ b/Example/TSKitiOSTestApp/TSKitiOSTestApp.xcodeproj/xcshareddata/xcschemes/TSKitiOSTestApp.xcscheme @@ -28,7 +28,7 @@ buildForAnalyzing = "YES"> diff --git a/src/Network/API/TSNetworkManager.h b/src/Network/API/TSNetworkManager.h index 0bd654ec2..aca526709 100644 --- a/src/Network/API/TSNetworkManager.h +++ b/src/Network/API/TSNetworkManager.h @@ -30,7 +30,7 @@ extern NSString *const TSNetworkManagerDomain; - (instancetype)init NS_UNAVAILABLE; -+ (id)sharedManager; ++ (instancetype)sharedManager; - (void)makeRequest:(TSRequest *)request success:(void (^)(NSURLSessionDataTask *task, id responseObject))success diff --git a/src/Network/API/TSNetworkManager.m b/src/Network/API/TSNetworkManager.m index 3005dbe17..1fd8e8232 100644 --- a/src/Network/API/TSNetworkManager.m +++ b/src/Network/API/TSNetworkManager.m @@ -22,7 +22,8 @@ typedef void (^failureBlock)(NSURLSessionDataTask *task, NSError *error); #pragma mark Singleton implementation -+ (id)sharedManager { ++ (instancetype)sharedManager +{ static TSNetworkManager *sharedMyManager = nil; static dispatch_once_t onceToken; dispatch_once(&onceToken, ^{