diff --git a/GoogleDataTransport/GDTCORLibrary/GDTCORFlatFileStorage.m b/GoogleDataTransport/GDTCORLibrary/GDTCORFlatFileStorage.m index 0a46c74515f..2148b74da21 100644 --- a/GoogleDataTransport/GDTCORLibrary/GDTCORFlatFileStorage.m +++ b/GoogleDataTransport/GDTCORLibrary/GDTCORFlatFileStorage.m @@ -27,6 +27,8 @@ #import "GoogleDataTransport/GDTCORLibrary/Private/GDTCORRegistrar_Private.h" #import "GoogleDataTransport/GDTCORLibrary/Private/GDTCORUploadCoordinator.h" +NS_ASSUME_NONNULL_BEGIN + /** A library data key this class uses to track batchIDs. */ static NSString *const gBatchIDCounterKey = @"GDTCORFlatFileStorageBatchIDCounter"; @@ -225,52 +227,7 @@ - (void)removeBatchWithID:(nonnull NSNumber *)batchID deleteEvents:(BOOL)deleteEvents onComplete:(void (^_Nullable)(void))onComplete { dispatch_async(_storageQueue, ^{ - NSError *error; - NSArray *batchDirPaths = [self batchDirPathsForBatchID:batchID error:&error]; - - if (batchDirPaths == nil) { - if (onComplete) { - onComplete(); - } - return; - } - - NSFileManager *fileManager = [NSFileManager defaultManager]; - - void (^removeBatchDir)(NSString *batchDirPath) = ^(NSString *batchDirPath) { - NSError *error; - if ([fileManager removeItemAtPath:batchDirPath error:&error]) { - GDTCORLogDebug(@"Batch removed at path: %@", batchDirPath); - } else { - GDTCORLogDebug(@"Failed to remove batch at path: %@", batchDirPath); - } - }; - - for (NSString *batchDirPath in batchDirPaths) { - if (deleteEvents) { - removeBatchDir(batchDirPath); - } else { - NSString *batchDirName = [batchDirPath lastPathComponent]; - NSDictionary *components = [self batchComponentsFromFilename:batchDirName]; - NSNumber *target = components[kGDTCORBatchComponentsTargetKey]; - NSString *destinationPath = [[GDTCORFlatFileStorage eventDataStoragePath] - stringByAppendingPathComponent:target.stringValue]; - - // `- [NSFileManager moveItemAtPath:toPath:error:] method fails if an item by the - // destination path already exists (which usually is the case for the current method). Move - // the events one by one instead. - if ([self moveContentsOfDirectoryAtPath:batchDirPath to:destinationPath error:&error]) { - GDTCORLogDebug(@"Batched events at path: %@ moved back to the storage: %@", batchDirPath, - destinationPath); - } else { - GDTCORLogDebug(@"Error encountered whilst moving events back: %@", error); - } - - // Even if not all events where moved back to the storage, there is not much can be done at - // this point, so cleanup batch directory now to avoid clattering. - removeBatchDir(batchDirPath); - } - } + [self syncThreadUnsafeRemoveBatchWithID:batchID deleteEvents:deleteEvents]; if (onComplete) { onComplete(); @@ -384,26 +341,16 @@ - (void)hasEventsForTarget:(GDTCORTarget)target onComplete:(void (^)(BOOL hasEve - (void)checkForExpirations { dispatch_async(_storageQueue, ^{ - NSMutableSet *pathsToDelete = [[NSMutableSet alloc] init]; GDTCORLogDebug(@"%@", @"Checking for expired events and batches"); NSTimeInterval now = [NSDate date].timeIntervalSince1970; NSFileManager *fileManager = [NSFileManager defaultManager]; - NSString *eventDataPath = [GDTCORFlatFileStorage eventDataStoragePath]; - NSDirectoryEnumerator *enumerator = [fileManager enumeratorAtPath:eventDataPath]; - NSString *path; - while ((path = [enumerator nextObject])) { - NSString *fileName = [path lastPathComponent]; - NSDictionary *eventComponents = [self eventComponentsFromFilename:fileName]; - NSDate *expirationDate = eventComponents[kGDTCOREventComponentsExpirationKey]; - if (expirationDate != nil && expirationDate.timeIntervalSince1970 < now) { - [pathsToDelete addObject:[eventDataPath stringByAppendingPathComponent:path]]; - } - } - // TODO: Events from expired batches with not expired events must be moved back to queue to - // avoid data loss. // TODO: Storage may not have enough context to remove batches because a batch may be being // uploaded but the storage has not context of it. + + // Find expired batches and move their events back to the main storage. + // If a batch contains expired events they are expected to be removed further in the method + // together with other expired events in the main storage. NSString *batchDataPath = [GDTCORFlatFileStorage batchDataStoragePath]; NSArray *batchDataPaths = [fileManager contentsOfDirectoryAtPath:batchDataPath error:nil]; @@ -411,18 +358,31 @@ - (void)checkForExpirations { NSString *fileName = [path lastPathComponent]; NSDictionary *batchComponents = [self batchComponentsFromFilename:fileName]; NSDate *expirationDate = batchComponents[kGDTCORBatchComponentsExpirationKey]; - if (expirationDate != nil && expirationDate.timeIntervalSince1970 < now) { - [pathsToDelete addObject:[batchDataPath stringByAppendingPathComponent:path]]; + NSNumber *batchID = batchComponents[kGDTCORBatchComponentsBatchIDKey]; + if (expirationDate != nil && expirationDate.timeIntervalSince1970 < now && batchID != nil) { + NSNumber *batchID = batchComponents[kGDTCORBatchComponentsBatchIDKey]; + // Move all events from the expired batch back to the main storage. + [self syncThreadUnsafeRemoveBatchWithID:batchID deleteEvents:NO]; } } - for (NSString *path in pathsToDelete) { - NSError *error; - [fileManager removeItemAtPath:path error:&error]; - if (error != nil) { - GDTCORLogDebug(@"There was an error deleting an expired item: %@", error); - } else { - GDTCORLogDebug(@"Item deleted because it expired: %@", path); + // Find expired events and remove them from the storage. + NSString *eventDataPath = [GDTCORFlatFileStorage eventDataStoragePath]; + NSDirectoryEnumerator *enumerator = [fileManager enumeratorAtPath:eventDataPath]; + NSString *path; + while ((path = [enumerator nextObject])) { + NSString *fileName = [path lastPathComponent]; + NSDictionary *eventComponents = [self eventComponentsFromFilename:fileName]; + NSDate *expirationDate = eventComponents[kGDTCOREventComponentsExpirationKey]; + if (expirationDate != nil && expirationDate.timeIntervalSince1970 < now) { + NSString *pathToDelete = [eventDataPath stringByAppendingPathComponent:path]; + NSError *error; + [fileManager removeItemAtPath:pathToDelete error:&error]; + if (error != nil) { + GDTCORLogDebug(@"There was an error deleting an expired item: %@", error); + } else { + GDTCORLogDebug(@"Item deleted because it expired: %@", pathToDelete); + } } } }); @@ -539,6 +499,53 @@ - (BOOL)moveContentsOfDirectoryAtPath:(NSString *)sourcePath } } +- (void)syncThreadUnsafeRemoveBatchWithID:(nonnull NSNumber *)batchID + deleteEvents:(BOOL)deleteEvents { + NSError *error; + NSArray *batchDirPaths = [self batchDirPathsForBatchID:batchID error:&error]; + + if (batchDirPaths == nil) { + return; + } + + NSFileManager *fileManager = [NSFileManager defaultManager]; + + void (^removeBatchDir)(NSString *batchDirPath) = ^(NSString *batchDirPath) { + NSError *error; + if ([fileManager removeItemAtPath:batchDirPath error:&error]) { + GDTCORLogDebug(@"Batch removed at path: %@", batchDirPath); + } else { + GDTCORLogDebug(@"Failed to remove batch at path: %@", batchDirPath); + } + }; + + for (NSString *batchDirPath in batchDirPaths) { + if (deleteEvents) { + removeBatchDir(batchDirPath); + } else { + NSString *batchDirName = [batchDirPath lastPathComponent]; + NSDictionary *components = [self batchComponentsFromFilename:batchDirName]; + NSNumber *target = components[kGDTCORBatchComponentsTargetKey]; + NSString *destinationPath = [[GDTCORFlatFileStorage eventDataStoragePath] + stringByAppendingPathComponent:target.stringValue]; + + // `- [NSFileManager moveItemAtPath:toPath:error:]` method fails if an item by the + // destination path already exists (which usually is the case for the current method). Move + // the events one by one instead. + if ([self moveContentsOfDirectoryAtPath:batchDirPath to:destinationPath error:&error]) { + GDTCORLogDebug(@"Batched events at path: %@ moved back to the storage: %@", batchDirPath, + destinationPath); + } else { + GDTCORLogDebug(@"Error encountered whilst moving events back: %@", error); + } + + // Even if not all events where moved back to the storage, there is not much can be done at + // this point, so cleanup batch directory now to avoid clattering. + removeBatchDir(batchDirPath); + } + } +} + #pragma mark - Private helper methods + (NSString *)eventDataStoragePath { @@ -766,3 +773,5 @@ - (void)appWillTerminate:(GDTCORApplication *)application { } @end + +NS_ASSUME_NONNULL_END diff --git a/GoogleDataTransport/GDTCORLibrary/GDTCORUploadCoordinator.m b/GoogleDataTransport/GDTCORLibrary/GDTCORUploadCoordinator.m index dd6223ea258..b0ee0b8e74e 100644 --- a/GoogleDataTransport/GDTCORLibrary/GDTCORUploadCoordinator.m +++ b/GoogleDataTransport/GDTCORLibrary/GDTCORUploadCoordinator.m @@ -63,10 +63,16 @@ - (void)forceUploadForTarget:(GDTCORTarget)target { */ - (void)startTimer { dispatch_async(_coordinationQueue, ^{ + if (self->_timer) { + // The timer has been already started. + return; + } + self->_timer = dispatch_source_create(DISPATCH_SOURCE_TYPE_TIMER, 0, 0, self->_coordinationQueue); dispatch_source_set_timer(self->_timer, DISPATCH_TIME_NOW, self->_timerInterval, self->_timerLeeway); + dispatch_source_set_event_handler(self->_timer, ^{ if (![[GDTCORApplication sharedApplication] isRunningInBackground]) { GDTCORUploadConditions conditions = [self uploadConditions]; @@ -83,6 +89,7 @@ - (void)startTimer { - (void)stopTimer { if (_timer) { dispatch_source_cancel(_timer); + _timer = nil; } } diff --git a/GoogleDataTransport/GDTCORLibrary/Private/GDTCORUploadCoordinator.h b/GoogleDataTransport/GDTCORLibrary/Private/GDTCORUploadCoordinator.h index a18579de15d..1346f08da45 100644 --- a/GoogleDataTransport/GDTCORLibrary/Private/GDTCORUploadCoordinator.h +++ b/GoogleDataTransport/GDTCORLibrary/Private/GDTCORUploadCoordinator.h @@ -39,7 +39,7 @@ NS_ASSUME_NONNULL_BEGIN @property(nonatomic, readonly) dispatch_queue_t coordinationQueue; /** A timer that will causes regular checks for events to upload. */ -@property(nonatomic, readonly) dispatch_source_t timer; +@property(nonatomic, readonly, nullable) dispatch_source_t timer; /** The interval the timer will fire. */ @property(nonatomic, readonly) uint64_t timerInterval; diff --git a/GoogleDataTransport/GDTCORTests/Common/Categories/GDTCORUploadCoordinator+Testing.m b/GoogleDataTransport/GDTCORTests/Common/Categories/GDTCORUploadCoordinator+Testing.m index 4a92b8a0786..50cfeb78fde 100644 --- a/GoogleDataTransport/GDTCORTests/Common/Categories/GDTCORUploadCoordinator+Testing.m +++ b/GoogleDataTransport/GDTCORTests/Common/Categories/GDTCORUploadCoordinator+Testing.m @@ -31,12 +31,20 @@ - (void)reset { - (void)setTimerInterval:(uint64_t)timerInterval { [self setValue:@(timerInterval) forKey:@"_timerInterval"]; - dispatch_source_set_timer(self.timer, DISPATCH_TIME_NOW, timerInterval, self.timerLeeway); + + dispatch_source_t timer = self.timer; + if (timer) { + dispatch_source_set_timer(timer, DISPATCH_TIME_NOW, timerInterval, self.timerLeeway); + } } - (void)setTimerLeeway:(uint64_t)timerLeeway { [self setValue:@(timerLeeway) forKey:@"_timerLeeway"]; - dispatch_source_set_timer(self.timer, DISPATCH_TIME_NOW, self.timerInterval, timerLeeway); + + dispatch_source_t timer = self.timer; + if (timer) { + dispatch_source_set_timer(timer, DISPATCH_TIME_NOW, self.timerInterval, timerLeeway); + } } @end diff --git a/GoogleDataTransport/GDTCORTests/Unit/GDTCORFlatFileStorageTest.m b/GoogleDataTransport/GDTCORTests/Unit/GDTCORFlatFileStorageTest.m index 2788449815e..a17a17bb2a8 100644 --- a/GoogleDataTransport/GDTCORTests/Unit/GDTCORFlatFileStorageTest.m +++ b/GoogleDataTransport/GDTCORTests/Unit/GDTCORFlatFileStorageTest.m @@ -92,15 +92,21 @@ - (void)tearDown { - (NSSet *)generateEventsForStorageTesting { NSMutableSet *generatedEvents = [[NSMutableSet alloc] init]; // Generate 100 test target events - [generatedEvents unionSet:[self generateEventsForTarget:kGDTCORTargetTest count:100]]; + [generatedEvents unionSet:[self generateEventsForTarget:kGDTCORTargetTest + expiringIn:1000 + count:100]]; // Generate 50 FLL target events. - [generatedEvents unionSet:[self generateEventsForTarget:kGDTCORTargetFLL count:50]]; + [generatedEvents unionSet:[self generateEventsForTarget:kGDTCORTargetFLL + expiringIn:1000 + count:50]]; return generatedEvents; } -- (NSSet *)generateEventsForTarget:(GDTCORTarget)target count:(NSInteger)count { +- (NSSet *)generateEventsForTarget:(GDTCORTarget)target + expiringIn:(NSTimeInterval)eventsExpireIn + count:(NSInteger)count { GDTCORFlatFileStorage *storage = [GDTCORFlatFileStorage sharedInstance]; NSMutableSet *generatedEvents = [[NSMutableSet alloc] init]; @@ -112,6 +118,7 @@ - (void)tearDown { GDTCOREvent *event = [GDTCOREventGenerator generateEventForTarget:target qosTier:nil mappingID:nil]; + event.expirationDate = [NSDate dateWithTimeIntervalSinceNow:eventsExpireIn]; [generatedEvents addObject:event]; [storage storeEvent:event onComplete:^(BOOL wasWritten, NSError *_Nullable error) { @@ -783,8 +790,27 @@ - (void)testBatchIDWithTarget { [self waitForExpectations:@[ expectation ] timeout:10]; } +- (void)testBatchIDsForTarget { + __auto_type expectedBatch = [self generateAndBatchEvents]; + + XCTestExpectation *batchIDsExpectation = [self expectationWithDescription:@"batchIDsExpectation"]; + + [[GDTCORFlatFileStorage sharedInstance] + batchIDsForTarget:kGDTCORTargetTest + onComplete:^(NSSet *_Nullable batchIDs) { + [batchIDsExpectation fulfill]; + + XCTAssertEqual(batchIDs.count, 1); + XCTAssertEqualObjects([expectedBatch.allKeys firstObject], [batchIDs anyObject]); + }]; + + [self waitForExpectations:@[ batchIDsExpectation ] timeout:5]; +} + +#pragma mark - Expiration tests + /** Tests events expiring at a given time. */ -- (void)testCheckEventExpiration { +- (void)testCheckForExpirations_WhenEventsExpire { NSTimeInterval delay = 10.0; XCTestExpectation *expectation = [self expectationWithDescription:@"hasEvent completion called"]; [[GDTCORFlatFileStorage sharedInstance] hasEventsForTarget:kGDTCORTargetTest @@ -823,78 +849,86 @@ - (void)testCheckEventExpiration { [self waitForExpectations:@[ expectation ] timeout:10]; } -- (void)testBatchIDsForTarget { - __auto_type expectedBatch = [self generateAndBatchEvents]; - - XCTestExpectation *batchIDsExpectation = [self expectationWithDescription:@"batchIDsExpectation"]; +- (void)testCheckForExpirations_WhenBatchWithNotExpiredEventsExpires { + NSTimeInterval batchExpiresIn = 0.5; + // 0.1. Generate and batch events + __auto_type generatedBatch = [self generateAndBatchEventsExpiringIn:1000 + batchExpiringIn:batchExpiresIn]; + NSNumber *generatedBatchID = [[generatedBatch allKeys] firstObject]; + NSSet *generatedEvents = generatedBatch[generatedBatchID]; + // 0.2. Wait for batch expiration. + [[NSRunLoop currentRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:batchExpiresIn]]; + + // 1. Check for expiration. + [[GDTCORFlatFileStorage sharedInstance] checkForExpirations]; + // 2. Check events. + // 2.1. Expect no batches left. + XCTestExpectation *getBatchesExpectation = + [self expectationWithDescription:@"getBatchesExpectation"]; [[GDTCORFlatFileStorage sharedInstance] batchIDsForTarget:kGDTCORTargetTest onComplete:^(NSSet *_Nullable batchIDs) { - [batchIDsExpectation fulfill]; - - XCTAssertEqual(batchIDs.count, 1); - XCTAssertEqualObjects([expectedBatch.allKeys firstObject], [batchIDs anyObject]); + [getBatchesExpectation fulfill]; + XCTAssertEqual(batchIDs.count, 0); }]; - [self waitForExpectations:@[ batchIDsExpectation ] timeout:5]; + // 2.2. Expect the events back in the main storage. + XCTestExpectation *getEventsExpectation = + [self expectationWithDescription:@"getEventsExpectation"]; + [[GDTCORFlatFileStorage sharedInstance] + batchWithEventSelector:[GDTCORStorageEventSelector eventSelectorForTarget:kGDTCORTargetTest] + batchExpiration:[NSDate dateWithTimeIntervalSinceNow:1000] + onComplete:^(NSNumber *_Nullable newBatchID, + NSSet *_Nullable batchEvents) { + [getEventsExpectation fulfill]; + XCTAssertNotNil(newBatchID); + NSSet *batchEventsIDs = [batchEvents valueForKeyPath:@"eventID"]; + NSSet *generatedEventsIDs = + [generatedEvents valueForKeyPath:@"eventID"]; + XCTAssertEqualObjects(batchEventsIDs, generatedEventsIDs); + }]; + + [self waitForExpectations:@[ getBatchesExpectation, getEventsExpectation ] timeout:0.5]; } -/** Tests batch expiring at a given time. */ -- (void)testCheckBatchExpiration { - GDTCORFlatFileStorage *storage = [GDTCORFlatFileStorage sharedInstance]; - NSTimeInterval delay = 10.0; - [self generateEventsForStorageTesting]; - XCTestExpectation *expectation = [self expectationWithDescription:@"hasEvent completion called"]; - [storage hasEventsForTarget:kGDTCORTargetTest - onComplete:^(BOOL hasEvents) { - XCTAssertTrue(hasEvents); - [expectation fulfill]; - }]; - [self waitForExpectations:@[ expectation ] timeout:10]; +- (void)testCheckForExpirations_WhenBatchWithExpiredEventsExpires { + NSTimeInterval batchExpiresIn = 0.5; + NSTimeInterval eventsExpireIn = 0.5; + // 0.1. Generate and batch events + __unused __auto_type generatedBatch = [self generateAndBatchEventsExpiringIn:eventsExpireIn + batchExpiringIn:batchExpiresIn]; + // 0.2. Wait for batch expiration. + [[NSRunLoop currentRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:batchExpiresIn]]; - expectation = [self expectationWithDescription:@"no batches exist"]; - [storage batchIDsForTarget:kGDTCORTargetTest - onComplete:^(NSSet *_Nonnull newBatchIDs) { - XCTAssertEqual(newBatchIDs.count, 0); - [expectation fulfill]; - }]; - [self waitForExpectations:@[ expectation ] timeout:10]; - expectation = [self expectationWithDescription:@"batch created"]; - __block NSNumber *batchID; - [storage + // 1. Check for expiration. + [[GDTCORFlatFileStorage sharedInstance] checkForExpirations]; + + // 2. Check events. + // 2.1. Expect no batches left. + XCTestExpectation *getBatchesExpectation = + [self expectationWithDescription:@"getBatchesExpectation"]; + [[GDTCORFlatFileStorage sharedInstance] + batchIDsForTarget:kGDTCORTargetTest + onComplete:^(NSSet *_Nullable batchIDs) { + [getBatchesExpectation fulfill]; + XCTAssertEqual(batchIDs.count, 0); + }]; + + // 2.2. Expect events to be deleted. + XCTestExpectation *getEventsExpectation = + [self expectationWithDescription:@"getEventsExpectation"]; + [[GDTCORFlatFileStorage sharedInstance] batchWithEventSelector:[GDTCORStorageEventSelector eventSelectorForTarget:kGDTCORTargetTest] - batchExpiration:[NSDate dateWithTimeIntervalSinceNow:delay] + batchExpiration:[NSDate dateWithTimeIntervalSinceNow:1000] onComplete:^(NSNumber *_Nullable newBatchID, - NSSet *_Nullable events) { - batchID = newBatchID; - XCTAssertGreaterThan(events.count, 0); - [expectation fulfill]; + NSSet *_Nullable batchEvents) { + [getEventsExpectation fulfill]; + XCTAssertNil(newBatchID); + XCTAssertEqual(batchEvents.count, 0); }]; - [self waitForExpectations:@[ expectation ] timeout:10]; - expectation = [self expectationWithDescription:@"a batch now exists"]; - [storage batchIDsForTarget:kGDTCORTargetTest - onComplete:^(NSSet *_Nonnull newBatchIDs) { - XCTAssertEqual(newBatchIDs.count, 1); - [expectation fulfill]; - }]; - [self waitForExpectations:@[ expectation ] timeout:10]; - [[NSRunLoop currentRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:delay]]; - [[GDTCORFlatFileStorage sharedInstance] checkForExpirations]; - expectation = [self expectationWithDescription:@"no batch exists after expiration"]; - [storage batchIDsForTarget:kGDTCORTargetTest - onComplete:^(NSSet *_Nonnull newBatchIDs) { - XCTAssertEqual(newBatchIDs.count, 0); - [expectation fulfill]; - }]; - [self waitForExpectations:@[ expectation ] timeout:30]; - expectation = [self expectationWithDescription:@"hasEvent completion called"]; - [storage hasEventsForTarget:kGDTCORTargetTest - onComplete:^(BOOL hasEvents) { - XCTAssertFalse(hasEvents); - [expectation fulfill]; - }]; - [self waitForExpectations:@[ expectation ] timeout:10]; + + [self waitForExpectations:@[ getBatchesExpectation, getEventsExpectation ] timeout:0.5]; } #pragma mark - Remove Batch tests @@ -1096,8 +1130,16 @@ - (void)testRemoveBatchWithIDDeletingEventsStorageSize { #pragma mark - Helpers - (NSDictionary *> *)generateAndBatchEvents { + return [self generateAndBatchEventsExpiringIn:1000 batchExpiringIn:1000]; +} + +- (NSDictionary *> *) + generateAndBatchEventsExpiringIn:(NSTimeInterval)eventsExpireIn + batchExpiringIn:(NSTimeInterval)batchExpiresIn { GDTCORFlatFileStorage *storage = [GDTCORFlatFileStorage sharedInstance]; - NSSet *events = [self generateEventsForTarget:kGDTCORTargetTest count:100]; + NSSet *events = [self generateEventsForTarget:kGDTCORTargetTest + expiringIn:eventsExpireIn + count:100]; XCTestExpectation *eventsGeneratedExpectation = [self expectationWithDescription:@"eventsGeneratedExpectation"]; [storage hasEventsForTarget:kGDTCORTargetTest @@ -1113,7 +1155,7 @@ - (void)testRemoveBatchWithIDDeletingEventsStorageSize { __block NSNumber *batchID; [storage batchWithEventSelector:[GDTCORStorageEventSelector eventSelectorForTarget:kGDTCORTargetTest] - batchExpiration:[NSDate dateWithTimeIntervalSinceNow:1000] + batchExpiration:[NSDate dateWithTimeIntervalSinceNow:batchExpiresIn] onComplete:^(NSNumber *_Nullable newBatchID, NSSet *_Nullable events) { batchID = newBatchID;