Release 6.28.0 - GDT cherry pick of #6010 and #6032 (#6035)

* [GDTCORUploadCoordinator startTimer]: don't restart timer if already started (#6032)

* [GDTCORUploadCoordinator startTimer]: don't restart timer if already started

* style

* `GDTCORUploadCoordinator.timer` - nullable

* GDTCORUploadCoordinator test utils thread safety.

* GDTCORFlatFileStorage: keep not expired events when expired batch removed (#6010)

* GDTCORFlatFileStorage: expiration clean up tests

* GDTCORFlatFileStorage: keep not expired events when expired batch removed

* Cleanup

* Cleanup

* Comments

* Comments

* Remove additional delays in tests

* style

* Changelog

* Changelog: remove 7.0.1

Author: maksymmalyhin

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<NSString *> *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<NSString *, id> *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,45 +341,48 @@ - (void)hasEventsForTarget:(GDTCORTarget)target onComplete:(void (^)(BOOL hasEve
 
 - (void)checkForExpirations {
   dispatch_async(_storageQueue, ^{
-    NSMutableSet<NSString *> *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<NSString *, id> *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<NSString *> *batchDataPaths = [fileManager contentsOfDirectoryAtPath:batchDataPath
                                                                            error:nil];
     for (NSString *path in batchDataPaths) {
       NSString *fileName = [path lastPathComponent];
       NSDictionary<NSString *, id> *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<NSString *, id> *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<NSString *> *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<NSString *, id> *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

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;
   }
 }
 

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;

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