Merge pull request #301 from Microsoft/fix_crash_in_database_count

Fix a crash while counting logs and disabling a module

sdk/mobile-center-crashes/src/androidTest/java/com/microsoft/azure/mobile/crashes/CrashesAndroidTest.java

@@ -240,6 +240,7 @@ public void onResult(ErrorReport errorReport) {
 
         assertNotNull(groupListener.get());
         groupListener.get().onSuccess(log.get());
+        waitForCrashesHandlerTasksToComplete();
         assertEquals(0, ErrorLogHelper.getErrorStorageDirectory().listFiles().length);
         verify(channel, never()).enqueue(any(Log.class), anyString());
         verify(crashesListener).onBeforeSending(any(ErrorReport.class));

sdk/mobile-center/src/main/java/com/microsoft/azure/mobile/channel/DefaultChannel.java

@@ -164,21 +164,31 @@ public synchronized void addGroup(final String groupName, int maxLogsPerBatch, l
 
         /* Init group. */
         MobileCenterLog.debug(LOG_TAG, "addGroup(" + groupName + ")");
-        mGroupStates.put(groupName, new GroupState(groupName, maxLogsPerBatch, batchTimeInterval, maxParallelBatches, groupListener));
+        final GroupState groupState = new GroupState(groupName, maxLogsPerBatch, batchTimeInterval, maxParallelBatches, groupListener);
+        mGroupStates.put(groupName, groupState);
 
         /* Count pending logs. */
         mPersistence.countLogs(groupName, new AbstractDatabasePersistenceAsyncCallback() {
 
             @Override
             public void onSuccess(Object result) {
-                mGroupStates.get(groupName).mPendingLogCount = (Integer) result;
-
-                /* Schedule sending any pending log. */
-                checkPendingLogs(groupName);
+                checkPendingLogsAfterCounting(groupState, (Integer) result);
             }
         });
     }
 
+    private synchronized void checkPendingLogsAfterCounting(GroupState groupState, int logCount) {
+
+        /* Check group was not replaced in the mean time. */
+        String groupName = groupState.mName;
+        if (groupState == mGroupStates.get(groupName)) {
+            groupState.mPendingLogCount = logCount;
+
+            /* Schedule sending any pending log. */
+            checkPendingLogs(groupName);
+        }
+    }
+
     @Override
     public synchronized void removeGroup(String groupName) {
         GroupState groupState = mGroupStates.remove(groupName);

sdk/mobile-center/src/test/java/com/microsoft/azure/mobile/channel/DefaultChannelTest.java

@@ -40,6 +40,8 @@
 import java.util.ArrayList;
 import java.util.List;
 import java.util.UUID;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.Semaphore;
 import java.util.concurrent.atomic.AtomicReference;
 
 import static com.microsoft.azure.mobile.persistence.DatabasePersistenceAsync.THREAD_NAME;
@@ -49,6 +51,7 @@
 import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.anyInt;
 import static org.mockito.Matchers.anyList;
+import static org.mockito.Matchers.anyListOf;
 import static org.mockito.Matchers.anyLong;
 import static org.mockito.Matchers.anyString;
 import static org.mockito.Matchers.argThat;
@@ -63,6 +66,7 @@
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoMoreInteractions;
 import static org.mockito.Mockito.when;
+import static org.powermock.api.mockito.PowerMockito.doAnswer;
 import static org.powermock.api.mockito.PowerMockito.mockStatic;
 import static org.powermock.api.mockito.PowerMockito.verifyStatic;
 import static org.powermock.api.mockito.PowerMockito.whenNew;
@@ -1077,4 +1081,114 @@ public boolean matches(Object argument) {
             }
         }));
     }
+
+    @Test
+    public void removeGroupWhileCounting() throws Exception {
+
+        /* Set up mocking. */
+        final CountDownLatch latch = new CountDownLatch(1);
+        final Semaphore semaphore = new Semaphore(0);
+        Persistence mockPersistence = mock(Persistence.class);
+        DatabasePersistenceAsync mockPersistenceAsync = mock(DatabasePersistenceAsync.class);
+        whenNew(DatabasePersistenceAsync.class).withArguments(mockPersistence).thenReturn(mockPersistenceAsync);
+        doAnswer(new Answer<Object>() {
+
+            @Override
+            public Object answer(final InvocationOnMock invocation) throws Throwable {
+                new Thread() {
+
+                    @Override
+                    public void run() {
+
+                        /* Wait for add/remove calls to be done. */
+                        try {
+                            latch.await();
+                        } catch (InterruptedException e) {
+                            throw new RuntimeException(e);
+                        }
+
+                        /* Return the count. */
+                        DatabasePersistenceAsync.DatabasePersistenceAsyncCallback callback = (DatabasePersistenceAsync.DatabasePersistenceAsyncCallback) invocation.getArguments()[1];
+                        callback.onSuccess(1);
+
+                        /* Mark call as done. */
+                        semaphore.release();
+                    }
+                }.start();
+                return null;
+            }
+        }).when(mockPersistenceAsync).countLogs(anyString(), any(DatabasePersistenceAsync.DatabasePersistenceAsyncCallback.class));
+
+        /* Simulate enable module then disable before persistence can return results. */
+        DefaultChannel channel = new DefaultChannel(mock(Context.class), UUIDUtils.randomUUID().toString(), mockPersistence, mock(Ingestion.class));
+        channel.addGroup(TEST_GROUP, 1, BATCH_TIME_INTERVAL, MAX_PARALLEL_BATCHES, mock(Channel.GroupListener.class));
+        channel.removeGroup(TEST_GROUP);
+
+        /* We wait on this object to make persistence return results after the groups add/remove calls. */
+        latch.countDown();
+
+        /* We wait on database call to return to verify behavior. */
+        semaphore.acquireUninterruptibly();
+
+        /* Verify we didn't get logs after counting since group was removed. */
+        verify(mockPersistenceAsync, never()).getLogs(eq(TEST_GROUP), eq(1), anyListOf(Log.class), any(DatabasePersistenceAsync.DatabasePersistenceAsyncCallback.class));
+    }
+
+    @Test
+    public void replaceGroupWhileCounting() throws Exception {
+
+        /* Set up mocking. */
+        final CountDownLatch latch = new CountDownLatch(1);
+        final Semaphore semaphore = new Semaphore(0);
+        Persistence mockPersistence = mock(Persistence.class);
+        DatabasePersistenceAsync mockPersistenceAsync = mock(DatabasePersistenceAsync.class);
+        whenNew(DatabasePersistenceAsync.class).withArguments(mockPersistence).thenReturn(mockPersistenceAsync);
+        doAnswer(new Answer<Object>() {
+
+            private int count;
+
+            @Override
+            public Object answer(final InvocationOnMock invocation) throws Throwable {
+                new Thread() {
+
+                    @Override
+                    public void run() {
+
+                        /* Wait for add/remove calls to be done. */
+                        try {
+                            latch.await();
+                        } catch (InterruptedException e) {
+                            throw new RuntimeException(e);
+                        }
+
+                        /* First call returns 1 then 2. */
+                        DatabasePersistenceAsync.DatabasePersistenceAsyncCallback callback = (DatabasePersistenceAsync.DatabasePersistenceAsyncCallback) invocation.getArguments()[1];
+                        callback.onSuccess(++count);
+
+                        /* Mark call as done. */
+                        semaphore.release();
+                    }
+                }.start();
+                return null;
+            }
+        }).when(mockPersistenceAsync).countLogs(anyString(), any(DatabasePersistenceAsync.DatabasePersistenceAsyncCallback.class));
+
+        /* Simulate enable module, disable, then re-enable before persistence can return results. */
+        DefaultChannel channel = new DefaultChannel(mock(Context.class), UUIDUtils.randomUUID().toString(), mockPersistence, mock(Ingestion.class));
+        channel.addGroup(TEST_GROUP, 1, BATCH_TIME_INTERVAL, MAX_PARALLEL_BATCHES, mock(Channel.GroupListener.class));
+        channel.removeGroup(TEST_GROUP);
+
+        /* We enable again with a different batching count to be able to check behavior on getLogs with different count values. */
+        channel.addGroup(TEST_GROUP, 2, BATCH_TIME_INTERVAL, MAX_PARALLEL_BATCHES, mock(Channel.GroupListener.class));
+
+        /* We wait on this object to make persistence return results after the groups add/remove calls. */
+        latch.countDown();
+
+        /* We wait on both database calls to return to verify behavior. */
+        semaphore.acquireUninterruptibly(2);
+
+        /* Verify only the second call has an effect, when the group was added a second time. */
+        verify(mockPersistenceAsync, never()).getLogs(eq(TEST_GROUP), eq(1), anyListOf(Log.class), any(DatabasePersistenceAsync.DatabasePersistenceAsyncCallback.class));
+        verify(mockPersistenceAsync).getLogs(eq(TEST_GROUP), eq(2), anyListOf(Log.class), any(DatabasePersistenceAsync.DatabasePersistenceAsyncCallback.class));
+    }
 }