Update synchronous crash reporting logic

CHANGELOG.md

@@ -18,6 +18,7 @@
   [#1185](https://github.com/bugsnag/bugsnag-android/pull/1185)
   [#1186](https://github.com/bugsnag/bugsnag-android/pull/1186)
   [#1180](https://github.com/bugsnag/bugsnag-android/pull/1180)
+  [#1188](https://github.com/bugsnag/bugsnag-android/pull/1188)
 
 ## 5.7.1 (2021-03-03)
 

bugsnag-android-core/src/androidTest/java/com/bugsnag/android/EventStoreConfinementTest.kt

@@ -22,7 +22,7 @@ internal class EventStoreConfinementTest {
     @Before
     fun setUp() {
         // setup delivery for interception
-        retainingDelivery = RetainingDelivery()
+        retainingDelivery = RetainingDelivery(EVENT_CONFINEMENT_ATTEMPTS)
         val cfg = BugsnagTestUtils.generateConfiguration().apply {
             autoTrackSessions = false
             delivery = retainingDelivery
@@ -39,7 +39,7 @@ internal class EventStoreConfinementTest {
         repeat(EVENT_CONFINEMENT_ATTEMPTS) { count ->
             client.notify(RuntimeException("$count"))
         }
-        retainingDelivery.latch.await(1, TimeUnit.SECONDS)
+        retainingDelivery.latch.await(10, TimeUnit.SECONDS)
 
         // confirm that no dupe requests are sent and that the request order is deterministic
         val payloads = retainingDelivery.payloads
@@ -80,41 +80,13 @@ internal class EventStoreConfinementTest {
         }
     }
 
-    /**
-     * Calling flushOnLaunch() is confined to a single thread
-     */
-    @Test
-    fun flushOnLaunchIsThreadConfined() {
-        val eventStore = client.eventStore
-
-        // send 20 errors
-        repeat(EVENT_CONFINEMENT_ATTEMPTS) { count ->
-            val event = BugsnagTestUtils.generateEvent().apply {
-                apiKey = "$count"
-            }
-            eventStore.write(event)
-            eventStore.flushOnLaunch()
-        }
-        retainingDelivery.latch.await(5, TimeUnit.SECONDS)
-
-        // confirm that no dupe requests are sent
-        val filenames = retainingDelivery.files
-        assertEquals(EVENT_CONFINEMENT_ATTEMPTS, filenames.size)
-        assertEquals(EVENT_CONFINEMENT_ATTEMPTS, filenames.toSet().size)
-
-        retainingDelivery.files.forEachIndexed { index, file ->
-            val eventInfo = EventFilenameInfo.fromFile(file, client.immutableConfig)
-            assertEquals("$index", eventInfo.apiKey)
-        }
-    }
-
     /**
      * Retains all the sent error payloads
      */
-    private class RetainingDelivery : Delivery {
+    private class RetainingDelivery(attempts: Int) : Delivery {
         val files = mutableListOf<File>()
         val payloads = mutableListOf<Event>()
-        val latch = CountDownLatch(EVENT_CONFINEMENT_ATTEMPTS)
+        val latch = CountDownLatch(attempts)
 
         override fun deliver(payload: Session, deliveryParams: DeliveryParams) =
             DeliveryStatus.DELIVERED

bugsnag-android-core/src/main/java/com/bugsnag/android/Client.java

@@ -234,6 +234,7 @@ public Unit invoke(String activity, Map<String, ?> metadata) {
 
         // Flush any on-disk errors and sessions
         eventStore.flushOnLaunch();
+        eventStore.flushAsync();
         sessionTracker.flushAsync();
 
         lastRunInfoStore = new LastRunInfoStore(immutableConfig);

bugsnag-android-core/src/main/java/com/bugsnag/android/EventFilenameInfo.kt

@@ -62,7 +62,7 @@ internal data class EventFilenameInfo(
                 sanitizedApiKey,
                 uuid,
                 timestamp,
-                findSuffixForEvent(obj, config),
+                findSuffixForEvent(obj),
                 findErrorTypesForEvent(obj)
             )
         }
@@ -140,24 +140,18 @@ internal data class EventFilenameInfo(
         /**
          * Calculates the suffix for the given event
          */
-        private fun findSuffixForEvent(obj: Any, config: ImmutableConfig): String {
+        private fun findSuffixForEvent(obj: Any): String {
             return when (obj) {
                 is Event -> {
-                    val duration = obj.app.duration
-                    if (duration != null && isStartupCrash(duration.toLong(), config)) {
-                        STARTUP_CRASH
-                    } else {
-                        ""
+                    when (obj.app.isLaunching) {
+                        true -> STARTUP_CRASH
+                        else -> ""
                     }
                 }
                 else -> {
                     NON_JVM_CRASH
                 }
             }
         }
-
-        private fun isStartupCrash(durationMs: Long, config: ImmutableConfig): Boolean {
-            return durationMs < config.launchDurationMillis
-        }
     }
 }

bugsnag-android-core/src/main/java/com/bugsnag/android/EventStore.java

@@ -1,16 +1,20 @@
 package com.bugsnag.android;
 
 import androidx.annotation.NonNull;
+import androidx.annotation.Nullable;
 
 import java.io.File;
-import java.lang.Thread;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.List;
 import java.util.Locale;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Future;
 import java.util.concurrent.RejectedExecutionException;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
 
 /**
  * Store and flush Event reports which couldn't be sent immediately due to
@@ -19,13 +23,11 @@
 class EventStore extends FileStore {
 
     private static final long LAUNCH_CRASH_TIMEOUT_MS = 2000;
-    private static final int LAUNCH_CRASH_POLL_MS = 50;
 
-    volatile boolean flushOnLaunchCompleted = false;
     private final ImmutableConfig config;
     private final Delegate delegate;
     private final Notifier notifier;
-    private final BackgroundTaskService backgroundTaskService;
+    private final BackgroundTaskService bgTaskSevice;
     final Logger logger;
 
     static final Comparator<File> EVENT_COMPARATOR = new Comparator<File>() {
@@ -47,7 +49,7 @@ public int compare(File lhs, File rhs) {
     EventStore(@NonNull ImmutableConfig config,
                @NonNull Logger logger,
                Notifier notifier,
-               BackgroundTaskService backgroundTaskService,
+               BackgroundTaskService bgTaskSevice,
                Delegate delegate) {
         super(new File(config.getPersistenceDirectory(), "bugsnag-errors"),
                 config.getMaxPersistedEvents(),
@@ -58,64 +60,78 @@ public int compare(File lhs, File rhs) {
         this.logger = logger;
         this.delegate = delegate;
         this.notifier = notifier;
-        this.backgroundTaskService = backgroundTaskService;
+        this.bgTaskSevice = bgTaskSevice;
     }
 
+    /**
+     * Flush startup crashes synchronously on the main thread
+     */
     void flushOnLaunch() {
-        if (config.getLaunchDurationMillis() != 0) {
-            List<File> storedFiles = findStoredFiles();
-            final List<File> crashReports = findLaunchCrashReports(storedFiles);
-
-            // cancel non-launch crash reports
-            storedFiles.removeAll(crashReports);
-            cancelQueuedFiles(storedFiles);
-
-            if (!crashReports.isEmpty()) {
-
-                // Block the main thread for a 2 second interval as the app may crash very soon.
-                // The request itself will run in a background thread and will continue after the 2
-                // second period until the request completes, or the app crashes.
-                flushOnLaunchCompleted = false;
-                logger.i("Attempting to send launch crash reports");
-
-                try {
-                    backgroundTaskService.submitTask(TaskType.ERROR_REQUEST, new Runnable() {
-                        @Override
-                        public void run() {
-                            flushReports(crashReports);
-                            flushOnLaunchCompleted = true;
-                        }
-                    });
-                } catch (RejectedExecutionException ex) {
-                    logger.w("Failed to flush launch crash reports", ex);
-                    flushOnLaunchCompleted = true;
+        if (!config.getSendLaunchCrashesSynchronously()) {
+            return;
+        }
+        Future<?> future = null;
+        try {
+            future = bgTaskSevice.submitTask(TaskType.ERROR_REQUEST, new Runnable() {
+                @Override
+                public void run() {
+                    flushLaunchCrashReport();
                 }
+            });
+        } catch (RejectedExecutionException exc) {
+            logger.d("Failed to flush launch crash reports, continuing.", exc);
+        }
+
+        try {
+            if (future != null) {
+                future.get(LAUNCH_CRASH_TIMEOUT_MS, TimeUnit.MILLISECONDS);
+            }
+        } catch (InterruptedException | ExecutionException | TimeoutException exc) {
+            logger.d("Failed to send launch crash reports within 2s timeout, continuing.", exc);
+        }
+    }
 
-                long waitMs = 0;
+    void flushLaunchCrashReport() {
+        List<File> storedFiles = findStoredFiles();
+        File launchCrashReport = findLaunchCrashReport(storedFiles);
 
-                while (!flushOnLaunchCompleted && waitMs < LAUNCH_CRASH_TIMEOUT_MS) {
-                    try {
-                        Thread.sleep(LAUNCH_CRASH_POLL_MS);
-                        waitMs += LAUNCH_CRASH_POLL_MS;
-                    } catch (InterruptedException exception) {
-                        logger.w("Interrupted while waiting for launch crash report request");
-                    }
-                }
-                logger.i("Continuing with Bugsnag initialisation");
-            } else {
-                logger.d("No startupcrash events to flush to Bugsnag.");
+        // cancel non-launch crash reports
+        if (launchCrashReport != null) {
+            storedFiles.remove(launchCrashReport);
+        }
+        cancelQueuedFiles(storedFiles);
+
+        if (launchCrashReport != null) {
+            logger.i("Attempting to send the most recent launch crash report");
+            flushReports(Collections.singletonList(launchCrashReport));
+            logger.i("Continuing with Bugsnag initialisation");
+        } else {
+            logger.d("No startupcrash events to flush to Bugsnag.");
+        }
+    }
+
+    @Nullable
+    File findLaunchCrashReport(Collection<File> storedFiles) {
+        List<File> launchCrashes = new ArrayList<>();
+
+        for (File file : storedFiles) {
+            EventFilenameInfo filenameInfo = EventFilenameInfo.Companion.fromFile(file, config);
+            if (filenameInfo.isLaunchCrashReport()) {
+                launchCrashes.add(file);
             }
         }
 
-        flushAsync(); // flush any remaining errors async that weren't delivered
+        // sort to get most recent timestamp
+        Collections.sort(launchCrashes, EVENT_COMPARATOR);
+        return launchCrashes.isEmpty() ? null : launchCrashes.get(launchCrashes.size() - 1);
     }
 
     /**
      * Flush any on-disk errors to Bugsnag
      */
     void flushAsync() {
         try {
-            backgroundTaskService.submitTask(TaskType.ERROR_REQUEST, new Runnable() {
+            bgTaskSevice.submitTask(TaskType.ERROR_REQUEST, new Runnable() {
                 @Override
                 public void run() {
                     List<File> storedFiles = findStoredFiles();
@@ -133,7 +149,7 @@ public void run() {
     void flushReports(Collection<File> storedReports) {
         if (!storedReports.isEmpty()) {
             logger.i(String.format(Locale.US,
-                "Sending %d saved error(s) to Bugsnag", storedReports.size()));
+                    "Sending %d saved error(s) to Bugsnag", storedReports.size()));
 
             for (File eventFile : storedReports) {
                 flushEventFile(eventFile);
@@ -147,7 +163,8 @@ private void flushEventFile(File eventFile) {
             String apiKey = eventInfo.getApiKey();
             EventPayload payload = new EventPayload(apiKey, null, eventFile, notifier, config);
             DeliveryParams deliveryParams = config.getErrorApiDeliveryParams(payload);
-            DeliveryStatus deliveryStatus = config.getDelivery().deliver(payload, deliveryParams);
+            Delivery delivery = config.getDelivery();
+            DeliveryStatus deliveryStatus = delivery.deliver(payload, deliveryParams);
 
             switch (deliveryStatus) {
                 case DELIVERED:
@@ -178,31 +195,19 @@ private void handleEventFlushFailure(Exception exc, File eventFile) {
         deleteStoredFiles(Collections.singleton(eventFile));
     }
 
-    private List<File> findLaunchCrashReports(Collection<File> storedFiles) {
-        List<File> launchCrashes = new ArrayList<>();
-
-        for (File file : storedFiles) {
-            EventFilenameInfo filenameInfo = EventFilenameInfo.Companion.fromFile(file, config);
-            if (filenameInfo.isLaunchCrashReport()) {
-                launchCrashes.add(file);
-            }
-        }
-        return launchCrashes;
-    }
-
     @NonNull
     @Override
     String getFilename(Object object) {
         EventFilenameInfo eventInfo
                 = EventFilenameInfo.Companion.fromEvent(object, null, config);
         String encodedInfo = eventInfo.encode();
-        return String.format(Locale.US, "%s.json", encodedInfo);
+        return String.format(Locale.US, "%s", encodedInfo);
     }
 
     String getNdkFilename(Object object, String apiKey) {
         EventFilenameInfo eventInfo
                 = EventFilenameInfo.Companion.fromEvent(object, apiKey, config);
         String encodedInfo = eventInfo.encode();
-        return String.format(Locale.US, "%s.json", encodedInfo);
+        return String.format(Locale.US, "%s", encodedInfo);
     }
 }