Skip to content

Commit

Permalink
Ensure stopped callback happen on service delayed fetches.
Browse files Browse the repository at this point in the history
Also update/enable tests to check/match this.
  • Loading branch information
thomasvl committed Nov 18, 2024
1 parent 7c579d3 commit 295c3f6
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 13 deletions.
48 changes: 40 additions & 8 deletions Sources/Core/GTMSessionFetcher.m
Original file line number Diff line number Diff line change
Expand Up @@ -706,11 +706,16 @@ - (void)beginFetchMayDelay:(BOOL)mayDelay
mayDelay = NO;
}
if (mayDelay && _service) {
// Set the delayed state so there can't be a race incase between it getting queued in
// the `fetcherShouldBeginFetching:` call and some other thread completing a different
// fetch and thus starting this one (if we were trying to set the state based on the
// return result).
@synchronized(self) { _delayState = kDelayStateServiceDelayed; }
BOOL savedStoppedState;
@synchronized(self) {
savedStoppedState = _userStoppedFetching;
// Set the delayed state so there can't be a race incase between it getting queued in
// the `fetcherShouldBeginFetching:` call and some other thread completing a different
// fetch and thus starting this one. If we were trying to set the state based on the
// return result, there would be a small window for that race.
_delayState = kDelayStateServiceDelayed;
}

BOOL shouldFetchNow = [_service fetcherShouldBeginFetching:self];
if (!shouldFetchNow) {
// The fetch is deferred, but will happen later.
Expand All @@ -726,8 +731,21 @@ - (void)beginFetchMayDelay:(BOOL)mayDelay
}
return;
}
// Per comment above, correct state since it wasn't delayed.
@synchronized(self) { _delayState = kDelayStateNotDelayed; }

@synchronized(self) {
// Per comment above, correct state since it wasn't delayed.
_delayState = kDelayStateNotDelayed;

// If a `-stopFetching` came in while the service check was made, then the side effect of the
// state setting caused the handler (if needed) to already be made, so there we want to just
// exit and not continue the fetch.
//
// TODO(thomasvl): If `savedStoppedState` was already `YES`, then it's a more general problem
// which will be handled elsewhere/later.
if (!savedStoppedState && savedStoppedState != _userStoppedFetching) {
return;
}
}
}

if ([fetchRequest valueForHTTPHeaderField:@"User-Agent"] == nil) {
Expand Down Expand Up @@ -2051,14 +2069,28 @@ - (void)forgetSessionIdentifierForFetcherWithoutSyncCheck {

// External stop method
- (void)stopFetching {
BOOL triggerCallback;
@synchronized(self) {
GTMSessionMonitorSynchronized(self);

// Prevent enqueued callbacks from executing. The completion handler will still execute if
// the property `stopFetchingTriggersCompletionHandler` is `YES`.
_userStoppedFetching = YES;

// `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;
} // @synchronized(self)
[self stopFetchReleasingCallbacks:!self.stopFetchingTriggersCompletionHandler];

if (triggerCallback) {
NSError *error = [NSError errorWithDomain:kGTMSessionFetcherErrorDomain
code:GTMSessionFetcherErrorUserCancelled
userInfo:nil];
[self finishWithError:error shouldRetry:NO];
} else {
[self stopFetchReleasingCallbacks:!self.stopFetchingTriggersCompletionHandler];
}
}

// Cancel the fetch of the URL that's currently in progress.
Expand Down
13 changes: 8 additions & 5 deletions UnitTests/GTMSessionFetcherServiceTest.m
Original file line number Diff line number Diff line change
Expand Up @@ -346,11 +346,9 @@ - (void)testFetcherService {
for (int idx = 1; idx <= 5; idx++) [urlArray addObject:validFileURL];
for (int idx = 1; idx <= 3; idx++) [urlArray addObject:stopSilentFileURL];
for (int idx = 1; idx <= 5; idx++) [urlArray addObject:altValidURL];
// TODO(thomasvl) currently fail: The current code paths remove these from
// pending, but doesn't actually get the completions ever called as requested.
// Will be fixed in follow up PRs.
// for (int idx = 1; idx <= 3; idx++) [urlArray addObject:stopCallbackFileURL];
for (int idx = 1; idx <= 3; idx++) [urlArray addObject:stopCallbackFileURL];
for (int idx = 1; idx <= 5; idx++) [urlArray addObject:validFileURL];
for (int idx = 1; idx <= 2; idx++) [urlArray addObject:stopCallbackFileURL];
for (int idx = 1; idx <= 2; idx++) [urlArray addObject:stopSilentFileURL];
NSUInteger totalNumberOfFetchers = urlArray.count;

Expand Down Expand Up @@ -572,6 +570,10 @@ - (void)testStopAllFetchersSeparately {
[self stopAllFetchersHelperUseStopAllAPI:NO callbacksAfterStop:NO];
}

- (void)testStopAllFetchersWithCallbacks {
[self stopAllFetchersHelperUseStopAllAPI:YES callbacksAfterStop:YES];
}

- (void)testStopAllFetchersSeparatelyWithCallbacks {
[self stopAllFetchersHelperUseStopAllAPI:NO callbacksAfterStop:YES];
}
Expand Down Expand Up @@ -599,9 +601,10 @@ - (void)stopAllFetchersHelperUseStopAllAPI:(BOOL)useStopAllAPI
XCTestExpectation *fetcherCallbackExpectation =
[[XCTNSNotificationExpectation alloc] initWithName:@"callback"];
if (doStopCallbacks) {
fetcherCallbackExpectation.expectedFulfillmentCount = 4;
fetcherCallbackExpectation.expectedFulfillmentCount = urlArray.count;
fetcherCallbackExpectation.assertForOverFulfill = YES;
} else {
// No callbacks, just complete this expectation now.
[fetcherCallbackExpectation fulfill];
}

Expand Down

0 comments on commit 295c3f6

Please sign in to comment.