Merge pull request #1215 from Microsoft/fix/save-log-error-skip-counting

Don't increment/check pending logs on save error
microsoft · Nov 13, 2018 · dbdb045 · dbdb045
2 parents d0303b9 + b4949ec
commit dbdb045
Show file tree

Hide file tree

Showing 2 changed files with 47 additions and 3 deletions.
diff --git a/AppCenter/AppCenter/Internals/Channel/MSChannelUnitDefault.m b/AppCenter/AppCenter/Internals/Channel/MSChannelUnitDefault.m
@@ -166,13 +166,19 @@ - (void)enqueueItem:(id<MSLog>)item flags:(MSFlags)flags {
 
       // Save the log first.
       MSLogDebug([MSAppCenter logTag], @"Saving log, type: %@, flags: %u.", item.type, (unsigned int)flags);
-      [self.storage saveLog:item withGroupId:self.configuration.groupId flags:flags];
-      self.itemsCount += 1;
+      bool success = [self.storage saveLog:item withGroupId:self.configuration.groupId flags:flags];
+
+      // Notify delegates of completion (whatever the result is).
       [self enumerateDelegatesForSelector:@selector(channel:didCompleteEnqueueingLog:internalId:)
                                 withBlock:^(id<MSChannelDelegate> delegate) {
                                   [delegate channel:self didCompleteEnqueueingLog:item internalId:internalLogId];
                                 }];
-      [self checkPendingLogs];
+
+      // If successful, check if logs can be sent now.
+      if (success) {
+        self.itemsCount += 1;
+        [self checkPendingLogs];
+      }
     }
   });
 }

diff --git a/AppCenter/AppCenterTests/MSChannelUnitDefaultTests.m b/AppCenter/AppCenterTests/MSChannelUnitDefaultTests.m
@@ -190,6 +190,7 @@ - (void)testLogsSentWithFailure {
 
   // Stub the storage load for that log.
   id storageMock = OCMProtocolMock(@protocol(MSStorage));
+  OCMStub([storageMock saveLog:OCMOCK_ANY withGroupId:OCMOCK_ANY flags:MSFlagsDefault]).andReturn(YES);
   OCMStub([storageMock loadLogsWithGroupId:kMSTestGroupId limit:batchSizeLimit excludedTargetKeys:OCMOCK_ANY completionHandler:OCMOCK_ANY])
       .andDo(^(NSInvocation *invocation) {
         MSLoadDataCompletionHandler loadCallback;
@@ -280,6 +281,41 @@ - (void)testEnqueuingItemsWillIncreaseCounter {
                                }];
 }
 
+- (void)testNotCheckingPendingLogsOnEnqueueFailure {
+
+  // If
+  [self initChannelEndJobExpectation];
+  self.configMock = [[MSChannelUnitConfiguration alloc] initWithGroupId:kMSTestGroupId
+                                                               priority:MSPriorityDefault
+                                                          flushInterval:5
+                                                         batchSizeLimit:10
+                                                    pendingBatchesLimit:3];
+  self.storageMock = OCMProtocolMock(@protocol(MSStorage));
+  OCMStub([self.storageMock saveLog:OCMOCK_ANY withGroupId:OCMOCK_ANY flags:MSFlagsDefault]).andReturn(NO);
+  self.sut = [[MSChannelUnitDefault alloc] initWithIngestion:self.ingestionMock
+                                                     storage:self.storageMock
+                                               configuration:self.configMock
+                                           logsDispatchQueue:self.logsDispatchQueue];
+  id channelUnitMock = OCMPartialMock(self.sut);
+  OCMReject([channelUnitMock checkPendingLogs]);
+  int itemsToAdd = 3;
+
+  // When
+  for (int i = 1; i <= itemsToAdd; i++) {
+    [self.sut enqueueItem:[self getValidMockLog] flags:MSFlagsDefault];
+  }
+  [self enqueueChannelEndJobExpectation];
+
+  // Then
+  [self waitForExpectationsWithTimeout:1
+                               handler:^(NSError *error) {
+                                 assertThatUnsignedLong(self.sut.itemsCount, equalToInt(0));
+                                 if (error) {
+                                   XCTFail(@"Expectation Failed with error: %@", error);
+                                 }
+                               }];
+}
+
 - (void)testEnqueueCriticalItem {
 
   // If
@@ -403,6 +439,7 @@ - (void)testBatchQueueLimit {
     }
   });
   id storageMock = OCMProtocolMock(@protocol(MSStorage));
+  OCMStub([storageMock saveLog:OCMOCK_ANY withGroupId:OCMOCK_ANY flags:MSFlagsDefault]).andReturn(YES);
   OCMStub([storageMock loadLogsWithGroupId:kMSTestGroupId limit:batchSizeLimit excludedTargetKeys:OCMOCK_ANY completionHandler:OCMOCK_ANY])
       .andDo(^(NSInvocation *invocation) {
         MSLoadDataCompletionHandler loadCallback;
@@ -464,6 +501,7 @@ - (void)testNextBatchSentIfPendingQueueGotRoomAgain {
 
   // Stub the storage load for that log.
   id storageMock = OCMProtocolMock(@protocol(MSStorage));
+  OCMStub([storageMock saveLog:OCMOCK_ANY withGroupId:OCMOCK_ANY flags:MSFlagsDefault]).andReturn(YES);
   OCMStub([storageMock loadLogsWithGroupId:kMSTestGroupId limit:batchSizeLimit excludedTargetKeys:OCMOCK_ANY completionHandler:OCMOCK_ANY])
       .andDo(^(NSInvocation *invocation) {
         MSLoadDataCompletionHandler loadCallback;