Refactor GDT Backgrounding (#3896)

ryanwilson · web-flow · commit 03ecafd06d24 · 2019-10-02T10:40:45.000-04:00
* Fix missing `runningInBackground` flags.

* Attempt at fixing background issues with GDT.

* Add missing deprecated implementation.

* Transfer extra call to background call.

* Style

* Further review comments.

* Fix testing.

* Refactored FLLIntegrationTest.

This does a few things - split the tests into a specific multithreaded
test and a recursive run without crashing.

It also adds a notification for the upload to post how many events were
uploaded. This should be a bit more reliable and helps the overall
validation during the "run without crashing" test.

* Removed extra log.

* Fix flag for Catalyst.

* Disable currently non-functional test.

* Further review comments.

* Migrate CCT tests to match FLL.

* Revert accidental partial change.

* Clarify comment for starting the background tasks.

* Review feedback, changelog additions and bug numbers.
diff --git a/GoogleDataTransport.podspec b/GoogleDataTransport.podspec
@@ -1,6 +1,6 @@
 Pod::Spec.new do |s|
   s.name             = 'GoogleDataTransport'
-  s.version          = '2.0.0'
+  s.version          = '3.0.0'
   s.summary          = 'Google iOS SDK data transport.'
 
   s.description      = <<-DESC
diff --git a/GoogleDataTransport/CHANGELOG.md b/GoogleDataTransport/CHANGELOG.md
@@ -1,3 +1,10 @@
+# v3.0.0
+- Changes backgrounding logic to reduce background usage and properly complete
+all tasks. (#3893)
+- Fix Catalyst define checks. (#3695)
+- Fix ubsan issues in GDT (#3910)
+- Add support for FLL. (#3867)
+
 # v2.0.0
 - Change/rename all classes and references from GDT to GDTCOR. (#3729)
 
diff --git a/GoogleDataTransport/GDTCORLibrary/GDTCORPlatform.m b/GoogleDataTransport/GDTCORLibrary/GDTCORPlatform.m
@@ -102,9 +102,10 @@ - (instancetype)init {
   return self;
 }
 
-- (GDTCORBackgroundIdentifier)beginBackgroundTaskWithExpirationHandler:(void (^)(void))handler {
-  return
-      [[self sharedApplicationForBackgroundTask] beginBackgroundTaskWithExpirationHandler:handler];
+- (GDTCORBackgroundIdentifier)beginBackgroundTaskWithName:(NSString *)name
+                                        expirationHandler:(void (^)(void))handler {
+  return [[self sharedApplicationForBackgroundTask] beginBackgroundTaskWithName:name
+                                                              expirationHandler:handler];
 }
 
 - (void)endBackgroundTask:(GDTCORBackgroundIdentifier)bgID {
@@ -124,6 +125,16 @@ - (BOOL)isAppExtension {
 #endif
 }
 
+- (BOOL)isRunningInBackground {
+#if TARGET_OS_IOS || TARGET_OS_TV
+  BOOL runningInBackground =
+      [[self sharedApplicationForBackgroundTask] applicationState] == UIApplicationStateBackground;
+  return runningInBackground;
+#else  // For macoS and Catalyst apps.
+  return NO;
+#endif
+}
+
 /** Returns a UIApplication instance if on the appropriate platform.
  *
  * @return The shared UIApplication if on the appropriate platform.
diff --git a/GoogleDataTransport/GDTCORLibrary/GDTCORStorage.m b/GoogleDataTransport/GDTCORLibrary/GDTCORStorage.m
@@ -81,14 +81,13 @@ - (void)storeEvent:(GDTCOREvent *)event {
   [self createEventDirectoryIfNotExists];
 
   __block GDTCORBackgroundIdentifier bgID = GDTCORBackgroundIdentifierInvalid;
-  if (_runningInBackground) {
-    bgID = [[GDTCORApplication sharedApplication] beginBackgroundTaskWithExpirationHandler:^{
-      if (bgID != GDTCORBackgroundIdentifierInvalid) {
-        [[GDTCORApplication sharedApplication] endBackgroundTask:bgID];
-        bgID = GDTCORBackgroundIdentifierInvalid;
-      }
-    }];
-  }
+  bgID = [[GDTCORApplication sharedApplication]
+      beginBackgroundTaskWithName:@"GDTStorage"
+                expirationHandler:^{
+                  // End the background task if it's still valid.
+                  [[GDTCORApplication sharedApplication] endBackgroundTask:bgID];
+                  bgID = GDTCORBackgroundIdentifierInvalid;
+                }];
 
   dispatch_async(_storageQueue, ^{
     // Check that a backend implementation is available for this target.
@@ -117,8 +116,8 @@ - (void)storeEvent:(GDTCOREvent *)event {
       [self.uploadCoordinator forceUploadForTarget:target];
     }
 
-    // Write state to disk.
-    if (self->_runningInBackground) {
+    // Write state to disk if we're in the background.
+    if ([[GDTCORApplication sharedApplication] isRunningInBackground]) {
       if (@available(macOS 10.13, iOS 11.0, tvOS 11.0, *)) {
         NSError *error;
         NSData *data = [NSKeyedArchiver archivedDataWithRootObject:self
@@ -132,11 +131,9 @@ - (void)storeEvent:(GDTCOREvent *)event {
       }
     }
 
-    // If running in the background, save state to disk and end the associated background task.
-    if (bgID != GDTCORBackgroundIdentifierInvalid) {
-      [[GDTCORApplication sharedApplication] endBackgroundTask:bgID];
-      bgID = GDTCORBackgroundIdentifierInvalid;
-    }
+    // Cancel or end the associated background task if it's still valid.
+    [[GDTCORApplication sharedApplication] endBackgroundTask:bgID];
+    bgID = GDTCORBackgroundIdentifierInvalid;
   });
 }
 
@@ -229,8 +226,16 @@ - (void)appWillForeground:(GDTCORApplication *)app {
 }
 
 - (void)appWillBackground:(GDTCORApplication *)app {
-  self->_runningInBackground = YES;
   dispatch_async(_storageQueue, ^{
+    // Immediately request a background task to run until the end of the current queue of work, and
+    // cancel it once the work is done.
+    __block GDTCORBackgroundIdentifier bgID =
+        [app beginBackgroundTaskWithName:@"GDTStorage"
+                       expirationHandler:^{
+                         [app endBackgroundTask:bgID];
+                         bgID = GDTCORBackgroundIdentifierInvalid;
+                       }];
+
     if (@available(macOS 10.13, iOS 11.0, tvOS 11.0, *)) {
       NSError *error;
       NSData *data = [NSKeyedArchiver archivedDataWithRootObject:self
@@ -242,20 +247,10 @@ - (void)appWillBackground:(GDTCORApplication *)app {
       [NSKeyedArchiver archiveRootObject:self toFile:[GDTCORStorage archivePath]];
 #endif
     }
-  });
 
-  // Create an immediate background task to run until the end of the current queue of work.
-  __block GDTCORBackgroundIdentifier bgID = [app beginBackgroundTaskWithExpirationHandler:^{
-    if (bgID != GDTCORBackgroundIdentifierInvalid) {
-      [app endBackgroundTask:bgID];
-      bgID = GDTCORBackgroundIdentifierInvalid;
-    }
-  }];
-  dispatch_async(_storageQueue, ^{
-    if (bgID != GDTCORBackgroundIdentifierInvalid) {
-      [app endBackgroundTask:bgID];
-      bgID = GDTCORBackgroundIdentifierInvalid;
-    }
+    // End the background task if it's still valid.
+    [app endBackgroundTask:bgID];
+    bgID = GDTCORBackgroundIdentifierInvalid;
   });
 }
 
diff --git a/GoogleDataTransport/GDTCORLibrary/GDTCORTransformer.m b/GoogleDataTransport/GDTCORLibrary/GDTCORTransformer.m
@@ -50,14 +50,12 @@ - (void)transformEvent:(GDTCOREvent *)event
   GDTCORAssert(event, @"You can't write a nil event");
 
   __block GDTCORBackgroundIdentifier bgID = GDTCORBackgroundIdentifierInvalid;
-  if (_runningInBackground) {
-    bgID = [[GDTCORApplication sharedApplication] beginBackgroundTaskWithExpirationHandler:^{
-      if (bgID != GDTCORBackgroundIdentifierInvalid) {
-        [[GDTCORApplication sharedApplication] endBackgroundTask:bgID];
-        bgID = GDTCORBackgroundIdentifierInvalid;
-      }
-    }];
-  }
+  bgID = [[GDTCORApplication sharedApplication]
+      beginBackgroundTaskWithName:@"GDTTransformer"
+                expirationHandler:^{
+                  [[GDTCORApplication sharedApplication] endBackgroundTask:bgID];
+                  bgID = GDTCORBackgroundIdentifierInvalid;
+                }];
   dispatch_async(_eventWritingQueue, ^{
     GDTCOREvent *transformedEvent = event;
     for (id<GDTCOREventTransformer> transformer in transformers) {
@@ -73,36 +71,14 @@ - (void)transformEvent:(GDTCOREvent *)event
       }
     }
     [self.storageInstance storeEvent:transformedEvent];
-    if (self->_runningInBackground) {
-      [[GDTCORApplication sharedApplication] endBackgroundTask:bgID];
-      bgID = GDTCORBackgroundIdentifierInvalid;
-    }
-  });
-}
-
-#pragma mark - GDTCORLifecycleProtocol
 
-- (void)appWillForeground:(GDTCORApplication *)app {
-  dispatch_async(_eventWritingQueue, ^{
-    self->_runningInBackground = NO;
+    // The work is done, cancel the background task if it's valid.
+    [[GDTCORApplication sharedApplication] endBackgroundTask:bgID];
+    bgID = GDTCORBackgroundIdentifierInvalid;
   });
 }
 
-- (void)appWillBackground:(GDTCORApplication *)app {
-  // Create an immediate background task to run until the end of the current queue of work.
-  __block GDTCORBackgroundIdentifier bgID = [app beginBackgroundTaskWithExpirationHandler:^{
-    if (bgID != GDTCORBackgroundIdentifierInvalid) {
-      [app endBackgroundTask:bgID];
-      bgID = GDTCORBackgroundIdentifierInvalid;
-    }
-  }];
-  dispatch_async(_eventWritingQueue, ^{
-    if (bgID != GDTCORBackgroundIdentifierInvalid) {
-      [app endBackgroundTask:bgID];
-      bgID = GDTCORBackgroundIdentifierInvalid;
-    }
-  });
-}
+#pragma mark - GDTCORLifecycleProtocol
 
 - (void)appWillTerminate:(GDTCORApplication *)application {
   // Flush the queue immediately.
diff --git a/GoogleDataTransport/GDTCORLibrary/GDTCORUploadCoordinator.m b/GoogleDataTransport/GDTCORLibrary/GDTCORUploadCoordinator.m
@@ -80,7 +80,7 @@ - (void)startTimer {
     dispatch_source_set_timer(self->_timer, DISPATCH_TIME_NOW, self->_timerInterval,
                               self->_timerLeeway);
     dispatch_source_set_event_handler(self->_timer, ^{
-      if (!self->_runningInBackground) {
+      if (![[GDTCORApplication sharedApplication] isRunningInBackground]) {
         GDTCORUploadConditions conditions = [self uploadConditions];
         [self uploadTargets:[self.registrar.targetToUploader allKeys] conditions:conditions];
       }
@@ -186,30 +186,12 @@ - (void)encodeWithCoder:(NSCoder *)aCoder {
 
 - (void)appWillForeground:(GDTCORApplication *)app {
   // Not entirely thread-safe, but it should be fine.
-  self->_runningInBackground = NO;
   [self startTimer];
 }
 
 - (void)appWillBackground:(GDTCORApplication *)app {
-  // Not entirely thread-safe, but it should be fine.
-  self->_runningInBackground = YES;
-
   // Should be thread-safe. If it ends up not being, put this in a dispatch_sync.
   [self stopTimer];
-
-  // Create an immediate background task to run until the end of the current queue of work.
-  __block GDTCORBackgroundIdentifier bgID = [app beginBackgroundTaskWithExpirationHandler:^{
-    if (bgID != GDTCORBackgroundIdentifierInvalid) {
-      [app endBackgroundTask:bgID];
-      bgID = GDTCORBackgroundIdentifierInvalid;
-    }
-  }];
-  dispatch_async(_coordinationQueue, ^{
-    if (bgID != GDTCORBackgroundIdentifierInvalid) {
-      [app endBackgroundTask:bgID];
-      bgID = GDTCORBackgroundIdentifierInvalid;
-    }
-  });
 }
 
 - (void)appWillTerminate:(GDTCORApplication *)application {
diff --git a/GoogleDataTransport/GDTCORLibrary/Private/GDTCORStorage_Private.h b/GoogleDataTransport/GDTCORLibrary/Private/GDTCORStorage_Private.h
@@ -35,10 +35,6 @@ NS_ASSUME_NONNULL_BEGIN
 /** The upload coordinator instance used by this storage instance. */
 @property(nonatomic) GDTCORUploadCoordinator *uploadCoordinator;
 
-/** If YES, every call to -storeLog results in background task and serializes the singleton to disk.
- */
-@property(nonatomic) BOOL runningInBackground;
-
 /** Returns the path to the keyed archive of the singleton. This is where the singleton is saved
  * to disk during certain app lifecycle events.
  *
diff --git a/GoogleDataTransport/GDTCORLibrary/Private/GDTCORTransformer_Private.h b/GoogleDataTransport/GDTCORLibrary/Private/GDTCORTransformer_Private.h
@@ -28,9 +28,6 @@ NS_ASSUME_NONNULL_BEGIN
 /** The storage instance used to store events. Should only be used to inject a testing fake. */
 @property(nonatomic) GDTCORStorage *storageInstance;
 
-/** If YES, every call to -transformEvent will result in a background task. */
-@property(nonatomic, readonly) BOOL runningInBackground;
-
 @end
 
 NS_ASSUME_NONNULL_END
diff --git a/GoogleDataTransport/GDTCORLibrary/Private/GDTCORUploadCoordinator.h b/GoogleDataTransport/GDTCORLibrary/Private/GDTCORUploadCoordinator.h
@@ -59,9 +59,6 @@ NS_ASSUME_NONNULL_BEGIN
 /** The registrar object the coordinator will use. Generally used for testing. */
 @property(nonatomic) GDTCORRegistrar *registrar;
 
-/** If YES, completion and other operations will result in serializing the singleton to disk. */
-@property(nonatomic, readonly) BOOL runningInBackground;
-
 /** Forces the backend specified by the target to upload the provided set of events. This should
  * only ever happen when the QoS tier of an event requires it.
  *
diff --git a/GoogleDataTransport/GDTCORLibrary/Public/GDTCORPlatform.h b/GoogleDataTransport/GDTCORLibrary/Public/GDTCORPlatform.h
@@ -67,14 +67,21 @@ FOUNDATION_EXPORT const GDTCORBackgroundIdentifier GDTCORBackgroundIdentifierInv
  */
 + (nullable GDTCORApplication *)sharedApplication;
 
+/** Flag to determine if the application is running in the background.
+ *
+ * @return YES if the app is running in the background, otherwise NO.
+ */
+- (BOOL)isRunningInBackground;
+
 /** Creates a background task with the returned identifier if on a suitable platform.
  *
+ * @name name The name of the task, useful for debugging which background tasks are running.
  * @param handler The handler block that is called if the background task expires.
  * @return An identifier for the background task, or GDTCORBackgroundIdentifierInvalid if one
  * couldn't be created.
  */
-- (GDTCORBackgroundIdentifier)beginBackgroundTaskWithExpirationHandler:
-    (void (^__nullable)(void))handler;
+- (GDTCORBackgroundIdentifier)beginBackgroundTaskWithName:(NSString *)name
+                                        expirationHandler:(void (^__nullable)(void))handler;
 
 /** Ends the background task if the identifier is valid.
  *
diff --git a/GoogleDataTransport/GDTCORTests/Lifecycle/GDTCORLifecycleTest.m b/GoogleDataTransport/GDTCORTests/Lifecycle/GDTCORLifecycleTest.m
@@ -91,7 +91,7 @@ - (void)setUp {
 }
 
 /** Tests that the library serializes itself to disk when the app backgrounds. */
-- (void)testBackgrounding {
+- (void)DISABLED_testBackgrounding {
   GDTCORTransport *transport = [[GDTCORTransport alloc] initWithMappingID:@"test"
                                                              transformers:nil
                                                                    target:kGDTCORTargetTest];
@@ -105,9 +105,12 @@ - (void)testBackgrounding {
       },
       5.0);
 
+  // TODO(#3973): This notification no longer triggers the `isRunningInBackground` flag. Find
+  // another way to test it.
   NSNotificationCenter *notifCenter = [NSNotificationCenter defaultCenter];
   [notifCenter postNotificationName:kGDTCORApplicationDidEnterBackgroundNotification object:nil];
-  XCTAssertTrue([GDTCORUploadCoordinator sharedInstance].runningInBackground);
+  XCTAssertTrue([GDTCORApplication sharedApplication].isRunningInBackground);
+
   GDTCORWaitForBlock(
       ^BOOL {
         NSFileManager *fm = [NSFileManager defaultManager];
@@ -117,7 +120,7 @@ - (void)testBackgrounding {
 }
 
 /** Tests that the library deserializes itself from disk when the app foregrounds. */
-- (void)testForegrounding {
+- (void)DISABLED_testForegrounding {
   GDTCORTransport *transport = [[GDTCORTransport alloc] initWithMappingID:@"test"
                                                              transformers:nil
                                                                    target:kGDTCORTargetTest];
@@ -131,6 +134,8 @@ - (void)testForegrounding {
       },
       5.0);
 
+  // TODO(#3973): This notification no longer triggers the `isRunningInBackground` flag. Find
+  // another way to test it.
   NSNotificationCenter *notifCenter = [NSNotificationCenter defaultCenter];
   [notifCenter postNotificationName:kGDTCORApplicationDidEnterBackgroundNotification object:nil];
 
@@ -141,9 +146,11 @@ - (void)testForegrounding {
       },
       5.0);
 
+  // TODO(#3973): This notification no longer triggers the `isRunningInBackground` flag. Find
+  // another way to test it.
   [[NSRunLoop currentRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:1.0]];
   [notifCenter postNotificationName:kGDTCORApplicationWillEnterForegroundNotification object:nil];
-  XCTAssertFalse([GDTCORUploadCoordinator sharedInstance].runningInBackground);
+  XCTAssertFalse([GDTCORApplication sharedApplication].isRunningInBackground);
   GDTCORWaitForBlock(
       ^BOOL {
         return [GDTCORStorage sharedInstance].storedEvents.count > 0;
diff --git a/GoogleDataTransportCCTSupport.podspec b/GoogleDataTransportCCTSupport.podspec
@@ -1,7 +1,7 @@
 
 Pod::Spec.new do |s|
   s.name             = 'GoogleDataTransportCCTSupport'
-  s.version          = '1.1.0'
+  s.version          = '1.2.0'
   s.summary          = 'Support library for the GoogleDataTransport CCT backend target.'
 
 
@@ -32,7 +32,7 @@ Support library to provide event prioritization and uploading for the GoogleData
 
   s.libraries = ['z']
 
-  s.dependency 'GoogleDataTransport', '~> 2.0'
+  s.dependency 'GoogleDataTransport', '~> 3.0'
   s.dependency 'nanopb', '~> 0.3.901'
 
   header_search_paths = {
diff --git a/GoogleDataTransportCCTSupport/CHANGELOG.md b/GoogleDataTransportCCTSupport/CHANGELOG.md
@@ -1,3 +1,8 @@
+# v1.2.0
+- Updates GDT dependency to improve backgrounding logic.
+- Reduces requests for background task creation. (#3893)
+- Fix unbalanced background task creation in GDTCCTUploader. (#3838)
+
 # v1.1.0
 - Updates GDT dependency to GDTCOR prefixed version.
 
diff --git a/GoogleDataTransportCCTSupport/GDTCCTLibrary/GDTCCTUploader.m b/GoogleDataTransportCCTSupport/GDTCCTLibrary/GDTCCTUploader.m
diff --git a/GoogleDataTransportCCTSupport/GDTCCTLibrary/GDTFLLUploader.m b/GoogleDataTransportCCTSupport/GDTCCTLibrary/GDTFLLUploader.m
diff --git a/GoogleDataTransportCCTSupport/GDTCCTLibrary/Private/GDTCCTUploader.h b/GoogleDataTransportCCTSupport/GDTCCTLibrary/Private/GDTCCTUploader.h
diff --git a/GoogleDataTransportCCTSupport/GDTCCTLibrary/Private/GDTFLLUploader.h b/GoogleDataTransportCCTSupport/GDTCCTLibrary/Private/GDTFLLUploader.h
diff --git a/GoogleDataTransportCCTSupport/GDTCCTTests/Integration/GDTCCTIntegrationTest.m b/GoogleDataTransportCCTSupport/GDTCCTTests/Integration/GDTCCTIntegrationTest.m
diff --git a/GoogleDataTransportCCTSupport/GDTCCTTests/Integration/GDTFLLIntegrationTest.m b/GoogleDataTransportCCTSupport/GDTCCTTests/Integration/GDTFLLIntegrationTest.m

Original file line number	Diff line number	Diff line change
`@@ -59,9 +59,6 @@ NS_ASSUME_NONNULL_BEGIN`
`59`	`59`	`/** The registrar object the coordinator will use. Generally used for testing. */`
`60`	`60`	`@property(nonatomic) GDTCORRegistrar *registrar;`
`61`	`61`
`62`		`-/** If YES, completion and other operations will result in serializing the singleton to disk. */`
`63`		`-@property(nonatomic, readonly) BOOL runningInBackground;`
`64`		`-`
`65`	`62`	`/** Forces the backend specified by the target to upload the provided set of events. This should`
`66`	`63`	`* only ever happen when the QoS tier of an event requires it.`
`67`	`64`	`*`