Skip to content

Commit 1973700

Browse files
jaeklimjaeklim
authored
Merge pull request #891 from Microsoft/fix/recoverable-errors-suspend-but-not-fatal
Fix/recoverable errors suspend but not fatal
2 parents c1dd056 + f6b6a86 commit 1973700

File tree

6 files changed

+118
-33
lines changed

6 files changed

+118
-33
lines changed

‎AppCenter/AppCenter/Internals/Sender/MSHttpSender.m

Lines changed: 25 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -241,7 +241,7 @@ - (void)sendCallAsync:(MSSenderCall *)call {
241241
}
242242
}
243243
MSLogVerbose([MSAppCenter logTag], @"HTTP response received with status code=%lu and payload=%@",
244-
(unsigned long)statusCode, payload);
244+
(unsigned long)statusCode, payload);
245245

246246
// Call handles the completion.
247247
if (call) {
@@ -257,19 +257,10 @@ - (void)sendCallAsync:(MSSenderCall *)call {
257257
}
258258
}
259259

260-
- (void)call:(MSSenderCall *)call completedWithFatalError:(BOOL)fatalError {
260+
- (void)call:(MSSenderCall *)call completedWithResult:(MSSenderCallResult)result {
261261
@synchronized(self) {
262-
NSString *callId = call.callId;
263-
if (callId.length == 0) {
264-
MSLogWarning([MSAppCenter logTag], @"Call object is invalid");
265-
return;
266-
}
267-
[self.pendingCalls removeObjectForKey:callId];
268-
MSLogInfo([MSAppCenter logTag], @"Removed call id:%@ from pending calls:%@", callId,
269-
[self.pendingCalls description]);
270-
271-
// Process fatal error.
272-
if (fatalError) {
262+
switch (result) {
263+
case MSSenderCallResultFatalError: {
273264

274265
// Disable and delete data.
275266
[self setEnabled:NO andDeleteDataOnDisabled:YES];
@@ -279,7 +270,27 @@ - (void)call:(MSSenderCall *)call completedWithFatalError:(BOOL)fatalError {
279270
withBlock:^(id<MSSenderDelegate> delegate) {
280271
[delegate senderDidReceiveFatalError:self];
281272
}];
273+
break;
274+
}
275+
case MSSenderCallResultRecoverableError:
276+
277+
// Disable and do not delete data. Do not notify the delegates as this will cause data to be deleted.
278+
[self setEnabled:NO andDeleteDataOnDisabled:NO];
279+
break;
280+
case MSSenderCallResultSuccess:
281+
break;
282282
}
283+
284+
// Remove call from pending call. This needs to happen after calling setEnabled:andDeleteDataOnDisabled:
285+
// FIXME: Refactor dependency between calling setEnabled:andDeleteDataOnDisabled: and suspending the sender.
286+
NSString *callId = call.callId;
287+
if (callId.length == 0) {
288+
MSLogWarning([MSAppCenter logTag], @"Call object is invalid");
289+
return;
290+
}
291+
[self.pendingCalls removeObjectForKey:callId];
292+
MSLogInfo([MSAppCenter logTag], @"Removed call id:%@ from pending calls:%@", callId,
293+
[self.pendingCalls description]);
283294
}
284295
}
285296

@@ -354,7 +365,7 @@ - (NSURLSession *)session {
354365
NSURLSessionConfiguration *sessionConfiguration = [NSURLSessionConfiguration defaultSessionConfiguration];
355366
sessionConfiguration.timeoutIntervalForRequest = kRequestTimeout;
356367
_session = [NSURLSession sessionWithConfiguration:sessionConfiguration];
357-
368+
358369
/*
359370
* Limit callbacks execution concurrency to avoid race condition. This queue is used only for
360371
* delegate method calls and completion handlers.

‎AppCenter/AppCenter/Internals/Sender/MSSenderCall.m

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,9 @@ - (BOOL)hasReachedMaxRetries {
2323
}
2424

2525
- (uint32_t)delayForRetryCount:(NSUInteger)retryCount {
26-
if (retryCount >= self.retryIntervals.count)
26+
if (retryCount >= self.retryIntervals.count) {
2727
return 0;
28+
}
2829

2930
// Create a random delay.
3031
uint32_t delay = [self.retryIntervals[retryCount] unsignedIntValue] / 2;
@@ -73,6 +74,7 @@ - (void)sender:(id<MSSender>)sender
7374
error:(NSError *)error {
7475
BOOL internetIsDown = [MSSenderUtil isNoInternetConnectionError:error];
7576
BOOL couldNotEstablishSecureConnection = [MSSenderUtil isSSLConnectionError:error];
77+
7678
if (internetIsDown || couldNotEstablishSecureConnection) {
7779

7880
// Reset the retry count, will retry once the (secure) connection is established again.
@@ -87,9 +89,8 @@ - (void)sender:(id<MSSender>)sender
8789
[self startRetryTimerWithStatusCode:statusCode];
8890
}
8991

90-
// Callback to Channel.
92+
// Call was a) successful, b) we exhausted retries for a recoverable error or c) have an unrecoverable error.
9193
else {
92-
BOOL fatalError;
9394

9495
// Wrap the status code in an error whenever the call failed.
9596
if (!error && statusCode != MSHTTPCodesNo200OK) {
@@ -100,14 +101,30 @@ - (void)sender:(id<MSSender>)sender
100101
error = [NSError errorWithDomain:kMSACErrorDomain code:kMSACConnectionHttpErrorCode userInfo:userInfo];
101102
}
102103

103-
// Detect fatal error.
104-
fatalError = (error && error.code != NSURLErrorCancelled);
104+
// Check for error.
105+
BOOL recoverableError = ([MSSenderUtil isRecoverableError:statusCode] && [self hasReachedMaxRetries]);
106+
BOOL fatalError;
107+
fatalError = recoverableError ? NO : (error && error.code != NSURLErrorCancelled);
105108

106-
// Call completion.
107-
self.completionHandler(self.callId, statusCode, data, error);
109+
// Call completion handler.
110+
if (self.completionHandler) {
111+
self.completionHandler(self.callId, statusCode, data, error);
112+
}
108113

109-
// Remove call from sender.
110-
[sender call:self completedWithFatalError:fatalError];
114+
// Handle recoverable error.
115+
if (recoverableError) {
116+
[sender call:self completedWithResult:MSSenderCallResultRecoverableError];
117+
}
118+
119+
// Handle fatal error.
120+
else if (fatalError) {
121+
[sender call:self completedWithResult:MSSenderCallResultFatalError];
122+
}
123+
124+
// Handle success case.
125+
else {
126+
[sender call:self completedWithResult:MSSenderCallResultSuccess];
127+
}
111128
}
112129
}
113130

‎AppCenter/AppCenter/Internals/Sender/MSSenderCallDelegate.h

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
#import <Foundation/Foundation.h>
22

3+
#import "MSConstants+Internal.h"
4+
35
@class MSSenderCall;
46

57
@protocol MSSenderCallDelegate <NSObject>
@@ -15,7 +17,8 @@
1517
* Call completed callback.
1618
*
1719
* @param call Call object.
20+
* @param result Enum indicating the result of the call.
1821
*/
19-
- (void)call:(MSSenderCall *)call completedWithFatalError:(BOOL)fatalError;
22+
- (void)call:(MSSenderCall *)call completedWithResult:(MSSenderCallResult)result;
2023

2124
@end

‎AppCenter/AppCenter/Internals/Util/MSConstants+Internal.h

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,3 +123,12 @@ typedef NS_ENUM(NSInteger, MSHTTPCodesNo) {
123123
MSHTTPCodesNo598NetworkReadTimeoutErrorUnknown = 598,
124124
MSHTTPCodesNo599NetworkConnectTimeoutErrorUnknown = 599
125125
};
126+
127+
/**
128+
* Enum indicating result of a MSSenderCall.
129+
*/
130+
typedef NS_ENUM(NSInteger, MSSenderCallResult) {
131+
MSSenderCallResultSuccess = 100,
132+
MSSenderCallResultRecoverableError = 500,
133+
MSSenderCallResultFatalError = 999
134+
};

‎AppCenter/AppCenterTests/MSIngestionSenderTests.m

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -352,6 +352,59 @@ - (void)testTasksRunningOnSenderResumed {
352352
}];
353353
}
354354

355+
- (void)testSuspendWhenAllRetriesUsed {
356+
357+
// If
358+
XCTestExpectation *responseReceivedExcpectation = [self expectationWithDescription:@"Used all retries."];
359+
NSString *containerId = @"1";
360+
MSLogContainer *container = [self createLogContainerWithId:containerId];
361+
362+
// Mock the call to intercept the retry.
363+
NSArray *intervals = @[ @(0.5), @(1) ];
364+
MSSenderCall *mockedCall = OCMPartialMock([[MSSenderCall alloc] initWithRetryIntervals:intervals]);
365+
mockedCall.delegate = self.sut;
366+
mockedCall.data = container;
367+
mockedCall.callId = container.batchId;
368+
369+
#pragma clang diagnostic push
370+
#pragma clang diagnostic ignored "-Wnonnull"
371+
mockedCall.completionHandler = nil;
372+
#pragma clang diagnostic pop
373+
374+
OCMStub([mockedCall sender:self.sut
375+
callCompletedWithStatus:MSHTTPCodesNo500InternalServerError
376+
data:OCMOCK_ANY
377+
error:OCMOCK_ANY])
378+
.andForwardToRealObject()
379+
.andDo(^(__attribute__((unused)) NSInvocation *invocation) {
380+
381+
/*
382+
* Don't fulfill the expectation immediatelly as the sender won't be suspended yet. Instead of using a delay to wait
383+
* for the retries, we use the retryCount as it retryCount will only be 0 before the first failed sending and after
384+
* we've exhausted the retry attempts. The first one won't be the case during unit tests as the request will fail
385+
* immediatelly, so the expectation will only by fulfilled once retries have been exhausted.
386+
*/
387+
if(mockedCall.retryCount == 0) {
388+
[responseReceivedExcpectation fulfill];
389+
}
390+
});
391+
self.sut.pendingCalls[containerId] = mockedCall;
392+
393+
// Respond with a retryable error.
394+
[MSHttpTestUtil stubHttp500Response];
395+
396+
// Send the call.
397+
[self.sut sendCallAsync:mockedCall];
398+
[self waitForExpectationsWithTimeout:20
399+
handler:^(NSError *error) {
400+
XCTAssertTrue(self.sut.suspended);
401+
XCTAssertTrue([self.sut.pendingCalls count] == 0);
402+
if (error) {
403+
XCTFail(@"Expectation Failed with error: %@", error);
404+
}
405+
}];
406+
}
407+
355408
- (void)testRetryStoppedWhileSuspended {
356409

357410
// If

‎AppCenterDistribute/AppCenterDistribute.xcodeproj/project.pbxproj

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,6 @@
154154
046BE64B1F8C1ACE00310886 /* ja */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = ja; path = Resources/ja.lproj/AppCenterDistribute.strings; sourceTree = "<group>"; };
155155
046BE64C1F8C1AE100310886 /* it */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = it; path = Resources/it.lproj/AppCenterDistribute.strings; sourceTree = "<group>"; };
156156
046BE64D1F8C1AF200310886 /* pl */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = pl; path = Resources/pl.lproj/AppCenterDistribute.strings; sourceTree = "<group>"; };
157-
048210261FD08C42006824F7 /* pt */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = pt; path = Resources/pt.lproj/AppCenterDistribute.strings; sourceTree = "<group>"; };
158157
046BE64E1F8C1B0900310886 /* es */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = es; path = Resources/es.lproj/AppCenterDistribute.strings; sourceTree = "<group>"; };
159158
046BE64F1F8C1B1E00310886 /* cs */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = cs; path = Resources/cs.lproj/AppCenterDistribute.strings; sourceTree = "<group>"; };
160159
046BE6501F8C1B9900310886 /* tr */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = tr; path = Resources/tr.lproj/AppCenterDistribute.strings; sourceTree = "<group>"; };
@@ -167,6 +166,7 @@
167166
0471B93B1E4557620022F951 /* MSServiceAbstract.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = MSServiceAbstract.h; path = ../AppCenter/AppCenter/MSServiceAbstract.h; sourceTree = "<group>"; };
168167
0471B93D1E4557820022F951 /* MSLogManager.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = MSLogManager.h; path = ../AppCenter/AppCenter/Internals/Channel/MSLogManager.h; sourceTree = "<group>"; };
169168
047D137E1EAFF87500D699BA /* MSReleaseDetailsPrivate.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MSReleaseDetailsPrivate.h; sourceTree = "<group>"; };
169+
048210261FD08C42006824F7 /* pt */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = pt; path = Resources/pt.lproj/AppCenterDistribute.strings; sourceTree = "<group>"; };
170170
048BB2FC1E60A40D009D92C2 /* MSReleaseDetails.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MSReleaseDetails.h; sourceTree = "<group>"; };
171171
048BB2FD1E60A40D009D92C2 /* MSReleaseDetails.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = MSReleaseDetails.m; sourceTree = "<group>"; };
172172
048BB2FF1E60A40D009D92C2 /* MSDistributionGroup.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = MSDistributionGroup.h; path = ../Sender/MSDistributionGroup.h; sourceTree = "<group>"; };
@@ -408,7 +408,6 @@
408408
38A683631E774DCE00A63BC8 /* MSSemVerPreReleaseIdTest.m */,
409409
38A683621E774DCE00A63BC8 /* MSSemVerTests.m */,
410410
80324E2F1F4DC81E005F4E11 /* MSDistributeAppDelegateTests.m */,
411-
386A69EE1FD88A6B0057B316 /* DataMigration */,
412411
040AF0101E523AC2005C1174 /* Fixtures */,
413412
04FD4A491E453930009B4468 /* Frameworks */,
414413
04A140BE1ECE84B7001CEE94 /* Model */,
@@ -447,13 +446,6 @@
447446
path = ../../Vendor/iOS/OCHamcrest;
448447
sourceTree = "<group>";
449448
};
450-
386A69EE1FD88A6B0057B316 /* DataMigration */ = {
451-
isa = PBXGroup;
452-
children = (
453-
);
454-
name = DataMigration;
455-
sourceTree = "<group>";
456-
};
457449
389638A21E79F557007C3809 /* Util */ = {
458450
isa = PBXGroup;
459451
children = (

0 commit comments

Comments
 (0)