Move log and non-fatal persistence to disk write worker (#6133)

Other metadata persistence is already on the right work from the
previous pr #6120
- Move log and non-fatal persistence to disk write worker
- Refine unit tests
- Remove unused functions and interface

Author: themiswang

firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreTest.java

@@ -70,6 +70,10 @@ public void whenAvailable(
 
   private CrashlyticsCore crashlyticsCore;
   private BreadcrumbSource mockBreadcrumbSource;
+  private static final CrashlyticsWorker commonWorker =
+      new CrashlyticsWorker(TestOnlyExecutors.background());
+  private static final CrashlyticsWorker diskWriteWorker =
+      new CrashlyticsWorker(TestOnlyExecutors.background());
 
   @Override
   protected void setUp() throws Exception {
@@ -93,7 +97,7 @@ public void testCustomAttributes() throws Exception {
 
     final String id = "id012345";
     crashlyticsCore.setUserId(id);
-    crashlyticsCore.commonWorker.await();
+    commonWorker.await();
     assertEquals(id, metadata.getUserId());
 
     final StringBuffer idBuffer = new StringBuffer(id);
@@ -104,13 +108,13 @@ public void testCustomAttributes() throws Exception {
     final String superLongId = longId + "more chars";
 
     crashlyticsCore.setUserId(superLongId);
-    crashlyticsCore.commonWorker.await();
+    commonWorker.await();
     assertEquals(longId, metadata.getUserId());
 
     final String key1 = "key1";
     final String value1 = "value1";
     crashlyticsCore.setCustomKey(key1, value1);
-    crashlyticsCore.commonWorker.await();
+    commonWorker.await();
     assertEquals(value1, metadata.getCustomKeys().get(key1));
 
     // Adding an existing key with the same value should return false
@@ -124,7 +128,7 @@ public void testCustomAttributes() throws Exception {
 
     // test truncation of custom keys and attributes
     crashlyticsCore.setCustomKey(superLongId, superLongValue);
-    crashlyticsCore.commonWorker.await();
+    commonWorker.await();
     assertNull(metadata.getCustomKeys().get(superLongId));
     assertEquals(longValue, metadata.getCustomKeys().get(longId));
 
@@ -133,28 +137,28 @@ public void testCustomAttributes() throws Exception {
       final String key = "key" + i;
       final String value = "value" + i;
       crashlyticsCore.setCustomKey(key, value);
-      crashlyticsCore.commonWorker.await();
+      commonWorker.await();
       assertEquals(value, metadata.getCustomKeys().get(key));
     }
     // should be full now, extra key, value pairs will be dropped.
     final String key = "new key";
     crashlyticsCore.setCustomKey(key, "some value");
-    crashlyticsCore.commonWorker.await();
+    commonWorker.await();
     assertFalse(metadata.getCustomKeys().containsKey(key));
 
     // should be able to update existing keys
     crashlyticsCore.setCustomKey(key1, longValue);
-    crashlyticsCore.commonWorker.await();
+    commonWorker.await();
     assertEquals(longValue, metadata.getCustomKeys().get(key1));
 
     // when we set a key to null, it should still exist with an empty value
     crashlyticsCore.setCustomKey(key1, null);
-    crashlyticsCore.commonWorker.await();
+    commonWorker.await();
     assertEquals("", metadata.getCustomKeys().get(key1));
 
     // keys and values are trimmed.
     crashlyticsCore.setCustomKey(" " + key1 + " ", " " + longValue + " ");
-    crashlyticsCore.commonWorker.await();
+    commonWorker.await();
     assertTrue(metadata.getCustomKeys().containsKey(key1));
     assertEquals(longValue, metadata.getCustomKeys().get(key1));
   }
@@ -205,7 +209,7 @@ public void testBulkCustomKeys() throws Exception {
     keysAndValues.put(intKey, String.valueOf(intValue));
 
     crashlyticsCore.setCustomKeys(keysAndValues);
-    crashlyticsCore.commonWorker.await();
+    commonWorker.await();
 
     assertEquals(stringValue, metadata.getCustomKeys().get(stringKey));
     assertEquals(trimmedValue, metadata.getCustomKeys().get(trimmedKey));
@@ -226,7 +230,7 @@ public void testBulkCustomKeys() throws Exception {
       addlKeysAndValues.put(key, value);
     }
     crashlyticsCore.setCustomKeys(addlKeysAndValues);
-    crashlyticsCore.commonWorker.await();
+    commonWorker.await();
 
     // Ensure all keys have been set
     assertEquals(UserMetadata.MAX_ATTRIBUTES, metadata.getCustomKeys().size(), DELTA);
@@ -244,7 +248,7 @@ public void testBulkCustomKeys() throws Exception {
       extraKeysAndValues.put(key, value);
     }
     crashlyticsCore.setCustomKeys(extraKeysAndValues);
-    crashlyticsCore.commonWorker.await();
+    commonWorker.await();
 
     // Make sure the extra keys were not added
     for (int i = UserMetadata.MAX_ATTRIBUTES; i < UserMetadata.MAX_ATTRIBUTES + 10; ++i) {
@@ -270,7 +274,7 @@ public void testBulkCustomKeys() throws Exception {
     updatedKeysAndValues.put(intKey, String.valueOf(updatedIntValue));
 
     crashlyticsCore.setCustomKeys(updatedKeysAndValues);
-    crashlyticsCore.commonWorker.await();
+    commonWorker.await();
 
     assertEquals(updatedStringValue, metadata.getCustomKeys().get(stringKey));
     assertFalse(Boolean.parseBoolean(metadata.getCustomKeys().get(booleanKey)));
@@ -443,8 +447,8 @@ CrashlyticsCore build(Context context) {
               new FileStore(context),
               mock(CrashlyticsAppQualitySessionsSubscriber.class),
               mock(RemoteConfigDeferredProxy.class),
-              new CrashlyticsWorker(TestOnlyExecutors.background()),
-              new CrashlyticsWorker(TestOnlyExecutors.background()));
+              commonWorker,
+              diskWriteWorker);
       return crashlyticsCore;
     }
   }

firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinatorTest.java

@@ -33,6 +33,8 @@
 
 import com.google.android.gms.tasks.Task;
 import com.google.android.gms.tasks.Tasks;
+import com.google.firebase.concurrent.TestOnlyExecutors;
+import com.google.firebase.crashlytics.internal.CrashlyticsWorker;
 import com.google.firebase.crashlytics.internal.metadata.LogFileManager;
 import com.google.firebase.crashlytics.internal.metadata.UserMetadata;
 import com.google.firebase.crashlytics.internal.model.CrashlyticsReport;
@@ -69,6 +71,8 @@ public class SessionReportingCoordinatorTest {
 
   private SessionReportingCoordinator reportingCoordinator;
 
+  private CrashlyticsWorker diskWriteWorker = new CrashlyticsWorker(TestOnlyExecutors.background());
+
   @Before
   public void setUp() {
     MockitoAnnotations.initMocks(this);
@@ -80,7 +84,8 @@ public void setUp() {
             reportSender,
             logFileManager,
             reportMetadata,
-            idManager);
+            idManager,
+            diskWriteWorker);
   }
 
   @Test
@@ -116,7 +121,8 @@ public void testFatalEvent_persistsHighPriorityEventWithAllThreadsForSessionId()
   }
 
   @Test
-  public void testNonFatalEvent_persistsNormalPriorityEventWithoutAllThreadsForSessionId() {
+  public void testNonFatalEvent_persistsNormalPriorityEventWithoutAllThreadsForSessionId()
+      throws Exception {
     final String eventType = "error";
     final String sessionId = "testSessionId";
     final long timestamp = System.currentTimeMillis();
@@ -126,6 +132,8 @@ public void testNonFatalEvent_persistsNormalPriorityEventWithoutAllThreadsForSes
     reportingCoordinator.onBeginSession(sessionId, timestamp);
     reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp);
 
+    diskWriteWorker.await();
+
     final boolean expectedAllThreads = false;
     final boolean expectedHighPriority = false;
 
@@ -136,7 +144,7 @@ public void testNonFatalEvent_persistsNormalPriorityEventWithoutAllThreadsForSes
   }
 
   @Test
-  public void testNonFatalEvent_addsLogsToEvent() {
+  public void testNonFatalEvent_addsLogsToEvent() throws Exception {
     long timestamp = System.currentTimeMillis();
 
     mockEventInteractions();
@@ -149,14 +157,16 @@ public void testNonFatalEvent_addsLogsToEvent() {
     reportingCoordinator.onBeginSession(sessionId, timestamp);
     reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp);
 
+    diskWriteWorker.await();
+
     verify(mockEventBuilder)
         .setLog(CrashlyticsReport.Session.Event.Log.builder().setContent(testLog).build());
     verify(mockEventBuilder).build();
     verify(logFileManager, never()).clearLog();
   }
 
   @Test
-  public void testNonFatalEvent_addsNoLogsToEventWhenNoneAvailable() {
+  public void testNonFatalEvent_addsNoLogsToEventWhenNoneAvailable() throws Exception {
     long timestamp = System.currentTimeMillis();
 
     mockEventInteractions();
@@ -168,6 +178,8 @@ public void testNonFatalEvent_addsNoLogsToEventWhenNoneAvailable() {
     reportingCoordinator.onBeginSession(sessionId, timestamp);
     reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp);
 
+    diskWriteWorker.await();
+
     verify(mockEventBuilder, never()).setLog(any(CrashlyticsReport.Session.Event.Log.class));
     verify(mockEventBuilder).build();
     verify(logFileManager, never()).clearLog();
@@ -212,7 +224,7 @@ public void testFatalEvent_addsNoLogsToEventWhenNoneAvailable() {
   }
 
   @Test
-  public void testNonFatalEvent_addsSortedKeysToEvent() {
+  public void testNonFatalEvent_addsSortedKeysToEvent() throws Exception {
     final long timestamp = System.currentTimeMillis();
 
     mockEventInteractions();
@@ -243,6 +255,8 @@ public void testNonFatalEvent_addsSortedKeysToEvent() {
     reportingCoordinator.onBeginSession(sessionId, timestamp);
     reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp);
 
+    diskWriteWorker.await();
+
     verify(mockEventAppBuilder).setCustomAttributes(expectedCustomAttributes);
     verify(mockEventAppBuilder).setInternalKeys(expectedCustomAttributes);
     verify(mockEventAppBuilder).build();
@@ -252,7 +266,7 @@ public void testNonFatalEvent_addsSortedKeysToEvent() {
   }
 
   @Test
-  public void testNonFatalEvent_addsNoKeysToEventWhenNoneAvailable() {
+  public void testNonFatalEvent_addsNoKeysToEventWhenNoneAvailable() throws Exception {
     final long timestamp = System.currentTimeMillis();
 
     mockEventInteractions();
@@ -266,6 +280,8 @@ public void testNonFatalEvent_addsNoKeysToEventWhenNoneAvailable() {
     reportingCoordinator.onBeginSession(sessionId, timestamp);
     reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp);
 
+    diskWriteWorker.await();
+
     verify(mockEventAppBuilder, never()).setCustomAttributes(anyList());
     verify(mockEventAppBuilder, never()).build();
     verify(mockEventBuilder, never()).setApp(mockEventApp);
@@ -274,7 +290,7 @@ public void testNonFatalEvent_addsNoKeysToEventWhenNoneAvailable() {
   }
 
   @Test
-  public void testNonFatalEvent_addRolloutsEvent() {
+  public void testNonFatalEvent_addRolloutsEvent() throws Exception {
     long timestamp = System.currentTimeMillis();
     String sessionId = "testSessionId";
     mockEventInteractions();
@@ -287,6 +303,8 @@ public void testNonFatalEvent_addRolloutsEvent() {
     reportingCoordinator.onBeginSession(sessionId, timestamp);
     reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp);
 
+    diskWriteWorker.await();
+
     verify(mockEventAppBuilder, never()).setCustomAttributes(anyList());
     verify(mockEventAppBuilder, never()).build();
     verify(mockEventBuilder, never()).setApp(mockEventApp);
@@ -417,35 +435,6 @@ public void testFatalEvent_addRolloutsToEvent() {
     verify(mockEventBuilder, times(2)).build();
   }
 
-  @Test
-  public void onLog_writesToLogFileManager() {
-    long timestamp = System.currentTimeMillis();
-    String log = "this is a log";
-
-    reportingCoordinator.onLog(timestamp, log);
-
-    verify(logFileManager).writeToLog(timestamp, log);
-  }
-
-  @Test
-  public void onCustomKey_writesToReportMetadata() {
-    final String key = "key";
-    final String value = "value";
-
-    reportingCoordinator.onCustomKey(key, value);
-
-    verify(reportMetadata).setCustomKey(key, value);
-  }
-
-  @Test
-  public void onUserId_writesUserToReportMetadata() {
-    final String userId = "testUser";
-
-    reportingCoordinator.onUserId(userId);
-
-    verify(reportMetadata).setUserId(userId);
-  }
-
   @Test
   public void testFinalizeSessionWithNativeEvent_createsCrashlyticsReportWithNativePayload() {
     byte[] testBytes = {0, 2, 20, 10};

firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCore.java

@@ -96,8 +96,8 @@ public class CrashlyticsCore {
 
   private final RemoteConfigDeferredProxy remoteConfigDeferredProxy;
 
-  @VisibleForTesting final CrashlyticsWorker commonWorker;
-  @VisibleForTesting final CrashlyticsWorker diskWriteWorker;
+  private final CrashlyticsWorker commonWorker;
+  private final CrashlyticsWorker diskWriteWorker;
 
   // region Constructors
 
@@ -172,7 +172,8 @@ public boolean onPreExecute(AppData appData, SettingsProvider settingsProvider)
               stackTraceTrimmingStrategy,
               settingsProvider,
               onDemandCounter,
-              sessionsSubscriber);
+              sessionsSubscriber,
+              diskWriteWorker);
 
       controller =
           new CrashlyticsController(
@@ -337,7 +338,11 @@ public void logException(@NonNull Throwable throwable) {
    */
   public void log(final String msg) {
     final long timestamp = System.currentTimeMillis() - startTime;
-    commonWorker.submit(() -> controller.writeToLog(timestamp, msg));
+    // queuing up on common worker to maintain the order
+    commonWorker.submit(
+        () -> {
+          diskWriteWorker.submit(() -> controller.writeToLog(timestamp, msg));
+        });
   }
 
   public void setUserId(String identifier) {