Ensure fetcher authorization delays support stop callbacks.

google · Dec 2, 2024 · ff266b4 · ff266b4
1 parent 7f04a23
commit ff266b4
Show file tree

Hide file tree

Showing 2 changed files with 56 additions and 21 deletions.
diff --git a/Sources/Core/GTMSessionFetcher.m b/Sources/Core/GTMSessionFetcher.m
@@ -1788,8 +1788,27 @@ - (void)endBackgroundTask {
 - (void)authorizeRequest {
   GTMSessionCheckNotSynchronized(self);
 
+  BOOL stopped = NO;
   @synchronized(self) {
-    _delayState = kDelayStateAuthorizing;
+    if (_userStoppedFetching) {
+      stopped = YES;
+    } else {
+      // Go into the delayed state for getting the authorization.
+      _delayState = kDelayStateAuthorizing;
+    }
+  }
+
+  if (stopped) {
+    // We end up here if someone calls `stopFetching` from another thread/queue while
+    // the fetch was being started up, so while `stopFetching` did the needed shutdown
+    // we have to ensure the requested callback was triggered.
+    if (self.stopFetchingTriggersCompletionHandler) {
+      NSError *error = [NSError errorWithDomain:kGTMSessionFetcherErrorDomain
+                                           code:GTMSessionFetcherErrorUserCancelled
+                                       userInfo:nil];
+      [self finishWithError:error shouldRetry:NO];
+    }
+    return;
   }
 
   id authorizer = self.authorizer;
@@ -1827,13 +1846,26 @@ - (void)authorizer:(nullable id __unused)auth
     finishedWithError:(nullable NSError *)error {
   GTMSessionCheckNotSynchronized(self);
 
+  @synchronized(self) {
+    // If `stopFetching` was called, do nothing, since the fetch was in a delay state
+    // any needed callback already happened.
+    if (_userStoppedFetching) {
+      return;
+    }
+    if (error == nil) {
+      _request = authorizedRequest;
+
+      // If `stopFetching` wasn't called, clear the `_delayState`, so a call after this
+      // point will trigger a callback as needed. This also ensure if this is going to
+      // error below a cancel callback couldn't also trigger.
+      _delayState = kDelayStateNotDelayed;
+    }
+  }
+
   if (error != nil) {
     // We can't fetch without authorization
     [self failToBeginFetchWithError:(NSError *_Nonnull)error];
   } else {
-    @synchronized(self) {
-      _request = authorizedRequest;
-    }
     [self beginFetchMayDelay:NO mayAuthorize:NO mayDecorate:YES];
   }
 }
@@ -2080,7 +2112,12 @@ - (void)stopFetching {
     // `stopFetchReleasingCallbacks:` will dequeue it if there is a sevice throttled
     // delay, so the canceled callback needs to be directly triggered since the serivce
     // won't attempt to restart it.
-    triggerCallback = _delayState == kDelayStateServiceDelayed && self.stopFetchingTriggersCompletionHandler;
+    //
+    // And the authorization delay assumes all stop notifications needed will be done
+    // from here.
+    triggerCallback =
+        (_delayState == kDelayStateServiceDelayed || _delayState == kDelayStateAuthorizing) &&
+        self.stopFetchingTriggersCompletionHandler;
   }  // @synchronized(self)
 
   if (triggerCallback) {

diff --git a/UnitTests/GTMSessionFetcherFetchingTest.m b/UnitTests/GTMSessionFetcherFetchingTest.m
@@ -1585,57 +1585,47 @@ - (void)testDelayedSyncAuthCancelFetchWithCallback_WithoutFetcherService {
 }
 
 - (void)testImmediateSyncAuthCancelFetchWithCallback {
-  XCTSkip(@"Has failed on CI, but not locally, needs investigation.");
   [self internalCancelFetchWithCallback:0 authorizer:[TestAuthorizer syncAuthorizer]];
 }
 
 - (void)testImmediateSyncAuthCancelFetchWithCallback_WithoutFetcherService {
   _fetcherService = nil;
-  XCTSkip(@"Has failed on CI, but not locally, needs investigation.");
   [self internalCancelFetchWithCallback:0 authorizer:[TestAuthorizer syncAuthorizer]];
 }
 
 - (void)testDelayedAsyncAuthCancelFetchWithCallback {
-  XCTSkip(@"Currently fails, needs fixing.");
   [self internalCancelFetchWithCallback:1 authorizer:[TestAuthorizer asyncAuthorizer]];
 }
 
 - (void)testDelayedAsyncAuthCancelFetchWithCallback_WithoutFetcherService {
   _fetcherService = nil;
-  XCTSkip(@"Currently fails, needs fixing.");
   [self internalCancelFetchWithCallback:1 authorizer:[TestAuthorizer asyncAuthorizer]];
 }
 
 - (void)testImmediateAsyncAuthCancelFetchWithCallback {
-  XCTSkip(@"Currently fails, needs fixing.");
   [self internalCancelFetchWithCallback:0 authorizer:[TestAuthorizer asyncAuthorizer]];
 }
 
 - (void)testImmediateAsyncAuthCancelFetchWithCallback_WithoutFetcherService {
   _fetcherService = nil;
-  XCTSkip(@"Currently fails, needs fixing.");
   [self internalCancelFetchWithCallback:0 authorizer:[TestAuthorizer asyncAuthorizer]];
 }
 
 - (void)testDelayedAsyncDelayedAuthCancelFetchWithCallback {
-  XCTSkip(@"Currently fails, needs fixing.");
   [self internalCancelFetchWithCallback:1 authorizer:[TestAuthorizer asyncAuthorizerDelayed:2]];
 }
 
 - (void)testDelayedAsyncDelayedAuthCancelFetchWithCallback_WithoutFetcherService {
   _fetcherService = nil;
-  XCTSkip(@"Currently fails, needs fixing.");
   [self internalCancelFetchWithCallback:1 authorizer:[TestAuthorizer asyncAuthorizerDelayed:2]];
 }
 
 - (void)testImmediateAsyncDelayedAuthCancelFetchWithCallback {
-  XCTSkip(@"Currently fails, needs fixing.");
   [self internalCancelFetchWithCallback:0 authorizer:[TestAuthorizer asyncAuthorizerDelayed:1]];
 }
 
 - (void)testImmediateAsyncDelayedAuthCancelFetchWithCallback_WithoutFetcherService {
   _fetcherService = nil;
-  XCTSkip(@"Currently fails, needs fixing.");
   [self internalCancelFetchWithCallback:0 authorizer:[TestAuthorizer asyncAuthorizerDelayed:1]];
 }
 
@@ -1649,7 +1639,17 @@ - (void)internalCancelFetchWithCallback:(unsigned int)sleepTime
 #pragma clang diagnostic pop
   if (!_isServerRunning) return;
 
-  CREATE_START_STOP_NOTIFICATION_EXPECTATIONS(1, 1);
+  // If the authorizer is async, then the fetch won't fully begin, and there won't ever be
+  // a start notification (and thus stop notification).
+  int expectedNotificationCount = ((TestAuthorizer*)authorizer).isAsync ? 0 : 1;
+  XCTestExpectation *fetcherStartedExpectation = nil;
+  XCTestExpectation *fetcherStoppedExpectation = nil;
+  if (expectedNotificationCount) {
+    fetcherStartedExpectation =
+        [[XCTNSNotificationExpectation alloc] initWithName:kGTMSessionFetcherStartedNotification];
+    fetcherStoppedExpectation =
+        [[XCTNSNotificationExpectation alloc] initWithName:kGTMSessionFetcherStoppedNotification];
+  }
 
   FetcherNotificationsCounter *fnctr = [[FetcherNotificationsCounter alloc] init];
 
@@ -1674,19 +1674,17 @@ - (void)internalCancelFetchWithCallback:(unsigned int)sleepTime
   }
   [fetcher stopFetching];
 
-  WAIT_FOR_START_STOP_NOTIFICATION_EXPECTATIONS();
-
   [self waitForExpectationsWithTimeout:_timeoutInterval handler:nil];
 
   [self assertCallbacksReleasedForFetcher:fetcher];
 
   // Check the notifications.
-  XCTAssertEqual(fnctr.fetchStarted, 1, @"%@", fnctr.fetchersStartedDescriptions);
-  XCTAssertEqual(fnctr.fetchStopped, 1, @"%@", fnctr.fetchersStoppedDescriptions);
+  XCTAssertEqual(fnctr.fetchStarted, expectedNotificationCount, @"%@", fnctr.fetchersStartedDescriptions);
+  XCTAssertEqual(fnctr.fetchStopped, fnctr.fetchStarted, @"%@", fnctr.fetchersStoppedDescriptions);
   XCTAssertEqual(fnctr.fetchCompletionInvoked, 1);
 #if GTM_BACKGROUND_TASK_FETCHING
   [self waitForBackgroundTaskEndedNotifications:fnctr];
-  XCTAssertEqual(fnctr.backgroundTasksStarted.count, (NSUInteger)1);
+  XCTAssertEqual(fnctr.backgroundTasksStarted.count, (NSUInteger)expectedNotificationCount);
   XCTAssertEqualObjects(fnctr.backgroundTasksStarted, fnctr.backgroundTasksEnded);
 #endif
 }