More safely manage background tasks (#3633) (#3635)

* More safely manage background tasks Delete a test that can't be run now. * Move state saving back to being async.
firebase · Aug 20, 2019 · eb715c7 · eb715c7
1 parent 51fea2a
commit eb715c7
Show file tree

Hide file tree

Showing 6 changed files with 53 additions and 42 deletions.
diff --git a/GoogleDataTransport/GDTLibrary/GDTPlatform.m b/GoogleDataTransport/GDTLibrary/GDTPlatform.m
@@ -105,7 +105,9 @@ - (GDTBackgroundIdentifier)beginBackgroundTaskWithExpirationHandler:(void (^)(vo
 }
 
 - (void)endBackgroundTask:(GDTBackgroundIdentifier)bgID {
-  [[self sharedApplicationForBackgroundTask] endBackgroundTask:bgID];
+  if (bgID != GDTBackgroundIdentifierInvalid) {
+    [[self sharedApplicationForBackgroundTask] endBackgroundTask:bgID];
+  }
 }
 
 #pragma mark - App environment helpers

diff --git a/GoogleDataTransport/GDTLibrary/GDTStorage.m b/GoogleDataTransport/GDTLibrary/GDTStorage.m
@@ -76,11 +76,12 @@ - (instancetype)init {
 - (void)storeEvent:(GDTEvent *)event {
   [self createEventDirectoryIfNotExists];
 
-  if (_backgroundID == GDTBackgroundIdentifierInvalid) {
-    _backgroundID = [[GDTApplication sharedApplication] beginBackgroundTaskWithExpirationHandler:^{
-      if (self->_backgroundID != GDTBackgroundIdentifierInvalid) {
-        [[GDTApplication sharedApplication] endBackgroundTask:self->_backgroundID];
-        self->_backgroundID = GDTBackgroundIdentifierInvalid;
+  __block GDTBackgroundIdentifier bgID = GDTBackgroundIdentifierInvalid;
+  if (_runningInBackground) {
+    bgID = [[GDTApplication sharedApplication] beginBackgroundTaskWithExpirationHandler:^{
+      if (bgID != GDTBackgroundIdentifierInvalid) {
+        [[GDTApplication sharedApplication] endBackgroundTask:bgID];
+        bgID = GDTBackgroundIdentifierInvalid;
       }
     }];
   }
@@ -111,8 +112,8 @@ - (void)storeEvent:(GDTEvent *)event {
       [self.uploadCoordinator forceUploadForTarget:target];
     }
 
-    // If running in the background, save state to disk and end the associated background task.
-    if (self->_backgroundID != GDTBackgroundIdentifierInvalid) {
+    // Write state to disk.
+    if (self->_runningInBackground) {
       if (@available(macOS 10.13, iOS 11.0, tvOS 11.0, *)) {
         NSData *data = [NSKeyedArchiver archivedDataWithRootObject:self
                                              requiringSecureCoding:YES
@@ -123,8 +124,12 @@ - (void)storeEvent:(GDTEvent *)event {
         [NSKeyedArchiver archiveRootObject:self toFile:[GDTStorage archivePath]];
 #endif
       }
-      [[GDTApplication sharedApplication] endBackgroundTask:self->_backgroundID];
-      self->_backgroundID = GDTBackgroundIdentifierInvalid;
+    }
+
+    // If running in the background, save state to disk and end the associated background task.
+    if (bgID != GDTBackgroundIdentifierInvalid) {
+      [[GDTApplication sharedApplication] endBackgroundTask:bgID];
+      bgID = GDTBackgroundIdentifierInvalid;
     }
   });
 }
@@ -216,14 +221,7 @@ - (void)appWillForeground:(GDTApplication *)app {
 }
 
 - (void)appWillBackground:(GDTApplication *)app {
-  if (_backgroundID == GDTBackgroundIdentifierInvalid) {
-    _backgroundID = [[GDTApplication sharedApplication] beginBackgroundTaskWithExpirationHandler:^{
-      if (self->_backgroundID != GDTBackgroundIdentifierInvalid) {
-        [[GDTApplication sharedApplication] endBackgroundTask:self->_backgroundID];
-        self->_backgroundID = GDTBackgroundIdentifierInvalid;
-      }
-    }];
-  }
+  self->_runningInBackground = YES;
   dispatch_async(_storageQueue, ^{
     if (@available(macOS 10.13, iOS 11.0, tvOS 11.0, *)) {
       NSData *data = [NSKeyedArchiver archivedDataWithRootObject:self
@@ -236,10 +234,18 @@ - (void)appWillBackground:(GDTApplication *)app {
 #endif
     }
   });
+
+  // Create an immediate background task to run until the end of the current queue of work.
+  __block GDTBackgroundIdentifier bgID = [app beginBackgroundTaskWithExpirationHandler:^{
+    if (bgID != GDTBackgroundIdentifierInvalid) {
+      [app endBackgroundTask:bgID];
+      bgID = GDTBackgroundIdentifierInvalid;
+    }
+  }];
   dispatch_async(_storageQueue, ^{
-    if (self->_backgroundID != GDTBackgroundIdentifierInvalid) {
-      [app endBackgroundTask:self->_backgroundID];
-      self->_backgroundID = GDTBackgroundIdentifierInvalid;
+    if (bgID != GDTBackgroundIdentifierInvalid) {
+      [app endBackgroundTask:bgID];
+      bgID = GDTBackgroundIdentifierInvalid;
     }
   });
 }

diff --git a/GoogleDataTransport/GDTLibrary/GDTTransformer.m b/GoogleDataTransport/GDTLibrary/GDTTransformer.m
@@ -51,7 +51,10 @@ - (void)transformEvent:(GDTEvent *)event
   __block GDTBackgroundIdentifier bgID = GDTBackgroundIdentifierInvalid;
   if (_runningInBackground) {
     bgID = [[GDTApplication sharedApplication] beginBackgroundTaskWithExpirationHandler:^{
-      [[GDTApplication sharedApplication] endBackgroundTask:bgID];
+      if (bgID != GDTBackgroundIdentifierInvalid) {
+        [[GDTApplication sharedApplication] endBackgroundTask:bgID];
+        bgID = GDTBackgroundIdentifierInvalid;
+      }
     }];
   }
   dispatch_async(_eventWritingQueue, ^{
@@ -71,6 +74,7 @@ - (void)transformEvent:(GDTEvent *)event
     [self.storageInstance storeEvent:transformedEvent];
     if (self->_runningInBackground) {
       [[GDTApplication sharedApplication] endBackgroundTask:bgID];
+      bgID = GDTBackgroundIdentifierInvalid;
     }
   });
 }
@@ -86,10 +90,16 @@ - (void)appWillForeground:(GDTApplication *)app {
 - (void)appWillBackground:(GDTApplication *)app {
   // Create an immediate background task to run until the end of the current queue of work.
   __block GDTBackgroundIdentifier bgID = [app beginBackgroundTaskWithExpirationHandler:^{
-    [app endBackgroundTask:bgID];
+    if (bgID != GDTBackgroundIdentifierInvalid) {
+      [app endBackgroundTask:bgID];
+      bgID = GDTBackgroundIdentifierInvalid;
+    }
   }];
   dispatch_async(_eventWritingQueue, ^{
-    [app endBackgroundTask:bgID];
+    if (bgID != GDTBackgroundIdentifierInvalid) {
+      [app endBackgroundTask:bgID];
+      bgID = GDTBackgroundIdentifierInvalid;
+    }
   });
 }
 

diff --git a/GoogleDataTransport/GDTLibrary/GDTUploadCoordinator.m b/GoogleDataTransport/GDTLibrary/GDTUploadCoordinator.m
@@ -192,10 +192,16 @@ - (void)appWillBackground:(GDTApplication *)app {
 
   // Create an immediate background task to run until the end of the current queue of work.
   __block GDTBackgroundIdentifier bgID = [app beginBackgroundTaskWithExpirationHandler:^{
-    [app endBackgroundTask:bgID];
+    if (bgID != GDTBackgroundIdentifierInvalid) {
+      [app endBackgroundTask:bgID];
+      bgID = GDTBackgroundIdentifierInvalid;
+    }
   }];
   dispatch_async(_coordinationQueue, ^{
-    [app endBackgroundTask:bgID];
+    if (bgID != GDTBackgroundIdentifierInvalid) {
+      [app endBackgroundTask:bgID];
+      bgID = GDTBackgroundIdentifierInvalid;
+    }
   });
 }
 

diff --git a/GoogleDataTransport/GDTLibrary/Private/GDTStorage_Private.h b/GoogleDataTransport/GDTLibrary/Private/GDTStorage_Private.h
@@ -35,8 +35,9 @@ NS_ASSUME_NONNULL_BEGIN
 /** The upload coordinator instance used by this storage instance. */
 @property(nonatomic) GDTUploadCoordinator *uploadCoordinator;
 
-/** The background identifier, not invalid if running in the background. */
-@property(nonatomic) GDTBackgroundIdentifier backgroundID;
+/** 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/GDTTests/Unit/GDTStorageTest.m b/GoogleDataTransport/GDTTests/Unit/GDTStorageTest.m
@@ -360,18 +360,4 @@ - (void)testQoSTierFast {
   });
 }
 
-/** Tests a deadlock condition in which a background signal is sent during -storeEvent:. */
-- (void)testStoreEventDeadlockFromBackgrounding {
-  [GDTStorage sharedInstance].backgroundID = 1234;
-  for (int i = 0; i < 10000; i++) {
-    GDTEvent *event = [[GDTEvent alloc] initWithMappingID:@"404" target:kGDTTargetCCT];
-    event.dataObjectTransportBytes = [@"testString" dataUsingEncoding:NSUTF8StringEncoding];
-    event.qosTier = GDTEventQoSFast;
-    event.clockSnapshot = [GDTClock snapshot];
-    [[GDTStorage sharedInstance] storeEvent:event];
-  }
-  dispatch_sync(dispatch_get_global_queue(QOS_CLASS_USER_INTERACTIVE, 0), ^{
-                });
-}
-
 @end