address comments

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/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
 
@@ -338,7 +338,11 @@ public void logException(@NonNull Throwable throwable) {
    */
   public void log(final String msg) {
     final long timestamp = System.currentTimeMillis() - startTime;
-    diskWriteWorker.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) {

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

@@ -48,10 +48,10 @@
 import java.util.concurrent.Executor;
 
 /**
- * This class handles Crashlytics lifecycle events and coordinates session data capture and
+ * This class coordinates Crashlytics session data capture and
  * persistence, as well as sending of reports to Firebase Crashlytics.
  */
-public class SessionReportingCoordinator implements CrashlyticsLifecycleEvents {
+public class SessionReportingCoordinator {
 
   private static final String EVENT_TYPE_CRASH = "crash";
   private static final String EVENT_TYPE_LOGGED = "error";
@@ -95,7 +95,7 @@ public static SessionReportingCoordinator create(
   private final UserMetadata reportMetadata;
   private final IdManager idManager;
 
-  final CrashlyticsWorker diskWriteWorker;
+  private final CrashlyticsWorker diskWriteWorker;
 
   SessionReportingCoordinator(
       CrashlyticsReportDataCapture dataCapture,
@@ -114,7 +114,6 @@ public static SessionReportingCoordinator create(
     this.diskWriteWorker = diskWriteWorker;
   }
 
-  @Override
   public void onBeginSession(@NonNull String sessionId, long timestampSeconds) {
     final CrashlyticsReport capturedReport =
         dataCapture.captureReportData(sessionId, timestampSeconds);
@@ -323,7 +322,7 @@ private void persistEvent(
       @NonNull String sessionId,
       @NonNull String eventType,
       long timestamp,
-      boolean includeAllThreads) {
+      boolean isFatal) {
 
     final boolean isHighPriority = eventType.equals(EVENT_TYPE_CRASH);
 
@@ -335,12 +334,11 @@ private void persistEvent(
             timestamp,
             EVENT_THREAD_IMPORTANCE,
             MAX_CHAINED_EXCEPTION_DEPTH,
-            includeAllThreads);
+            isFatal);
     CrashlyticsReport.Session.Event finallizedEvent = addMetaDataToEvent(capturedEvent);
 
-    // Non-fatal case, non-fatal does not include all threads
-    // We move to diskWriteWorker for persistence
-    if (!includeAllThreads) {
+    // Non-fatal, persistence write task we move to diskWriteWorker
+    if (!isFatal) {
       diskWriteWorker.submit(
           () -> {
             reportPersistence.persistEvent(finallizedEvent, sessionId, isHighPriority);