Merge pull request #1753 from bugsnag/integration/deliver-on-crash

Introduce option to attempt delivery on crash
bugsnag · Oct 6, 2022 · bd1e25b · bd1e25b
2 parents 14cef4e + db2aa9f
commit bd1e25b
Show file tree

Hide file tree

Showing 13 changed files with 148 additions and 9 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,13 @@
 # Changelog
 
+## TBD
+
+### Enhancements
+
+* Setting `Configuration.attemptDeliveryOnCrash` will cause Bugsnag to attempt error delivery during some crashes.
+  Use of this feature is discouraged, see the method JavaDoc for more information.
+  [#1749](https://github.com/bugsnag/bugsnag-android/pull/1749)
+
 ## 5.26.0 (2022-08-18)
 
 ### Enhancements

diff --git a/bugsnag-android-core/src/androidTest/java/com/bugsnag/android/ManifestConfigLoaderTest.kt b/bugsnag-android-core/src/androidTest/java/com/bugsnag/android/ManifestConfigLoaderTest.kt
@@ -92,6 +92,7 @@ class ManifestConfigLoaderTest {
             putBoolean("com.bugsnag.android.SEND_LAUNCH_CRASHES_SYNCHRONOUSLY", false)
             putString("com.bugsnag.android.APP_TYPE", "react-native")
             putString("com.bugsnag.android.CODE_BUNDLE_ID", "123")
+            putBoolean("com.bugsnag.android.ATTEMPT_DELIVERY_ON_CRASH", true)
         }
 
         val config = configLoader.load(data, null)
@@ -128,6 +129,7 @@ class ManifestConfigLoaderTest {
             assertEquals(launchDurationMillis, 7000)
             assertFalse(sendLaunchCrashesSynchronously)
             assertEquals("react-native", appType)
+            assertTrue(isAttemptDeliveryOnCrash)
         }
     }
 

diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/ConfigInternal.kt b/bugsnag-android-core/src/main/java/com/bugsnag/android/ConfigInternal.kt
@@ -57,6 +57,8 @@ internal class ConfigInternal(
     var projectPackages: Set<String> = emptySet()
     var persistenceDirectory: File? = null
 
+    var attemptDeliveryOnCrash: Boolean = false
+
     val notifier: Notifier = Notifier()
 
     protected val plugins = HashSet<Plugin>()
@@ -103,6 +105,9 @@ internal class ConfigInternal(
     }
 
     fun getConfigDifferences(): Map<String, Any> {
+        // allocate a local ConfigInternal with all-defaults to compare against
+        val defaultConfig = ConfigInternal("")
+
         return listOfNotNull(
             if (plugins.count() > 0) "pluginCount" to plugins.count() else null,
             if (autoDetectErrors != defaultConfig.autoDetectErrors)
@@ -136,6 +141,8 @@ internal class ConfigInternal(
                 "persistenceDirectorySet" to true else null,
             if (sendThreads != defaultConfig.sendThreads)
                 "sendThreads" to sendThreads else null,
+            if (attemptDeliveryOnCrash != defaultConfig.attemptDeliveryOnCrash)
+                "attemptDeliveryOnCrash" to attemptDeliveryOnCrash else null
         ).toMap()
     }
 
@@ -153,7 +160,5 @@ internal class ConfigInternal(
         protected fun load(context: Context, apiKey: String?): Configuration {
             return ManifestConfigLoader().load(context, apiKey)
         }
-
-        private val defaultConfig = ConfigInternal("")
     }
 }
diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/Configuration.java b/bugsnag-android-core/src/main/java/com/bugsnag/android/Configuration.java
@@ -7,7 +7,6 @@
 import androidx.annotation.VisibleForTesting;
 
 import java.io.File;
-import java.util.Locale;
 import java.util.Map;
 import java.util.Set;
 
@@ -1112,6 +1111,39 @@ public void addPlugin(@NonNull Plugin plugin) {
         }
     }
 
+    /**
+     * Whether Bugsnag should try to send crashing errors prior to app termination.
+     *
+     * Delivery will only be attempted for uncaught Java / Kotlin exceptions or errors, and
+     * while in progress will block the crashing thread for up to 3 seconds.
+     *
+     * Delivery on crash should be considered unreliable due to the necessary short timeout and
+     * potential for generating "errors on errors".
+     *
+     * Use of this feature is discouraged because it:
+     * - may cause Application Not Responding (ANR) errors on-top of existing crashes
+     * - will result in duplicate errors in your Dashboard when errors are not detected as sent
+     *   before termination
+     * - may prevent other error handlers from detecting or reporting a crash
+     *
+     * By default this value is {@code false}.
+     *
+     * @param attemptDeliveryOnCrash {@code true} if Bugsnag should try to send crashing errors
+     *                               prior to app termination
+     */
+    public void setAttemptDeliveryOnCrash(boolean attemptDeliveryOnCrash) {
+        impl.setAttemptDeliveryOnCrash(attemptDeliveryOnCrash);
+    }
+
+    /**
+     * Whether Bugsnag should try to send crashing errors prior to app termination.
+     * 
+     * @see #setAttemptDeliveryOnCrash(boolean)
+     */
+    public boolean isAttemptDeliveryOnCrash() {
+        return impl.getAttemptDeliveryOnCrash();
+    }
+
     Set<Plugin> getPlugins() {
         return impl.getPlugins();
     }

diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/DeliveryDelegate.java b/bugsnag-android-core/src/main/java/com/bugsnag/android/DeliveryDelegate.java
@@ -7,14 +7,15 @@
 import androidx.annotation.NonNull;
 import androidx.annotation.VisibleForTesting;
 
-import java.util.Date;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
+import java.util.concurrent.Future;
 import java.util.concurrent.RejectedExecutionException;
+import java.util.concurrent.TimeUnit;
 
 class DeliveryDelegate extends BaseObservable {
 
+    @VisibleForTesting
+    static long DELIVERY_TIMEOUT = 3000L;
+
     final Logger logger;
     private final EventStore eventStore;
     private final ImmutableConfig immutableConfig;
@@ -55,7 +56,13 @@ void deliver(@NonNull Event event) {
             String severityReasonType = event.getImpl().getSeverityReasonType();
             boolean promiseRejection = REASON_PROMISE_REJECTION.equals(severityReasonType);
             boolean anr = event.getImpl().isAnr(event);
-            cacheEvent(event, anr || promiseRejection);
+            if (anr || promiseRejection) {
+                cacheEvent(event, true);
+            } else if (immutableConfig.getAttemptDeliveryOnCrash()) {
+                cacheAndSendSynchronously(event);
+            } else {
+                cacheEvent(event, false);
+            }
         } else if (callbackState.runOnSendTasks(event, logger)) {
             // Build the eventPayload
             String apiKey = event.getApiKey();
@@ -107,6 +114,24 @@ DeliveryStatus deliverPayloadInternal(@NonNull EventPayload payload, @NonNull Ev
         return deliveryStatus;
     }
 
+    private void cacheAndSendSynchronously(@NonNull Event event) {
+        long cutoffTime = System.currentTimeMillis() + DELIVERY_TIMEOUT;
+        Future<String> task = eventStore.writeAndDeliver(event);
+
+        long timeout = cutoffTime - System.currentTimeMillis();
+        if (task != null && timeout > 0) {
+            try {
+                task.get(timeout, TimeUnit.MILLISECONDS);
+            } catch (Exception ex) {
+                logger.w("failed to immediately deliver event", ex);
+            }
+
+            if (!task.isDone()) {
+                task.cancel(true);
+            }
+        }
+    }
+
     private void cacheEvent(@NonNull Event event, boolean attemptSend) {
         eventStore.write(event);
         if (attemptSend) {

diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/EventStore.java b/bugsnag-android-core/src/main/java/com/bugsnag/android/EventStore.java
@@ -13,6 +13,7 @@
 import java.util.Comparator;
 import java.util.Date;
 import java.util.List;
+import java.util.concurrent.Callable;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Future;
 import java.util.concurrent.RejectedExecutionException;
@@ -132,6 +133,26 @@ File findLaunchCrashReport(Collection<File> storedFiles) {
         return launchCrashes.isEmpty() ? null : launchCrashes.get(launchCrashes.size() - 1);
     }
 
+    @Nullable
+    Future<String> writeAndDeliver(@NonNull final JsonStream.Streamable streamable) {
+        final String filename = write(streamable);
+
+        if (filename != null) {
+            try {
+                return bgTaskSevice.submitTask(TaskType.ERROR_REQUEST, new Callable<String>() {
+                    public String call() {
+                        flushEventFile(new File(filename));
+                        return filename;
+                    }
+                });
+            } catch (RejectedExecutionException exception) {
+                logger.w("Failed to flush all on-disk errors, retaining unsent errors for later.");
+            }
+        }
+
+        return null;
+    }
+
     /**
      * Flush any on-disk errors to Bugsnag
      */
@@ -163,7 +184,7 @@ void flushReports(Collection<File> storedReports) {
         }
     }
 
-    private void flushEventFile(File eventFile) {
+    void flushEventFile(File eventFile) {
         try {
             EventFilenameInfo eventInfo = EventFilenameInfo.fromFile(eventFile, config);
             String apiKey = eventInfo.getApiKey();

diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/ManifestConfigLoader.kt b/bugsnag-android-core/src/main/java/com/bugsnag/android/ManifestConfigLoader.kt
@@ -42,6 +42,7 @@ internal class ManifestConfigLoader {
         private const val LAUNCH_DURATION_MILLIS = "$BUGSNAG_NS.LAUNCH_DURATION_MILLIS"
         private const val SEND_LAUNCH_CRASHES_SYNCHRONOUSLY = "$BUGSNAG_NS.SEND_LAUNCH_CRASHES_SYNCHRONOUSLY"
         private const val APP_TYPE = "$BUGSNAG_NS.APP_TYPE"
+        private const val ATTEMPT_DELIVERY_ON_CRASH = "$BUGSNAG_NS.ATTEMPT_DELIVERY_ON_CRASH"
     }
 
     fun load(ctx: Context, userSuppliedApiKey: String?): Configuration {
@@ -91,6 +92,10 @@ internal class ManifestConfigLoader {
                     SEND_LAUNCH_CRASHES_SYNCHRONOUSLY,
                     sendLaunchCrashesSynchronously
                 )
+                isAttemptDeliveryOnCrash = data.getBoolean(
+                    ATTEMPT_DELIVERY_ON_CRASH,
+                    isAttemptDeliveryOnCrash
+                )
             }
         }
         return config

diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/internal/ImmutableConfig.kt b/bugsnag-android-core/src/main/java/com/bugsnag/android/internal/ImmutableConfig.kt
@@ -52,6 +52,7 @@ data class ImmutableConfig(
     val maxReportedThreads: Int,
     val persistenceDirectory: Lazy<File>,
     val sendLaunchCrashesSynchronously: Boolean,
+    val attemptDeliveryOnCrash: Boolean,
 
     // results cached here to avoid unnecessary lookups in Client.
     val packageInfo: PackageInfo?,
@@ -167,6 +168,7 @@ internal fun convertToImmutableConfig(
         telemetry = config.telemetry.toSet(),
         persistenceDirectory = persistenceDir,
         sendLaunchCrashesSynchronously = config.sendLaunchCrashesSynchronously,
+        attemptDeliveryOnCrash = config.isAttemptDeliveryOnCrash,
         packageInfo = packageInfo,
         appInfo = appInfo,
         redactedKeys = config.redactedKeys.toSet()

diff --git a/bugsnag-plugin-react-native/src/test/java/com/bugsnag/android/TestData.java b/bugsnag-plugin-react-native/src/test/java/com/bugsnag/android/TestData.java
@@ -51,6 +51,7 @@ public File invoke() {
                     }
                 }),
                 true,
+                true,
                 null,
                 null,
                 Collections.singleton("password")

diff --git a/features/fixtures/mazerunner/app/proguard-rules.pro b/features/fixtures/mazerunner/app/proguard-rules.pro
@@ -1 +1,2 @@
 -keep class com.bugsnag.android.mazerunner.** {*;}
+-keep class com.bugsnag.android.DeliveryDelegate {*;}
diff --git a/features/fixtures/mazerunner/jvm-scenarios/detekt-baseline.xml b/features/fixtures/mazerunner/jvm-scenarios/detekt-baseline.xml
@@ -31,6 +31,7 @@
     <ID>ThrowingExceptionsWithoutMessageOrCause:TrimmedStacktraceScenario.kt$TrimmedStacktraceScenario$RuntimeException()</ID>
     <ID>TooGenericExceptionThrown:CrashHandlerScenario.kt$CrashHandlerScenario$throw RuntimeException("CrashHandlerScenario")</ID>
     <ID>TooGenericExceptionThrown:CustomHttpClientFlushScenario.kt$CustomHttpClientFlushScenario$throw RuntimeException("ReportCacheScenario")</ID>
+    <ID>TooGenericExceptionThrown:DeliverOnCrashScenario.kt$DeliverOnCrashScenario$throw RuntimeException("DeliverOnCrashScenario")</ID>
     <ID>TooGenericExceptionThrown:DisableAutoDetectErrorsScenario.kt$DisableAutoDetectErrorsScenario$throw RuntimeException("Should never appear")</ID>
     <ID>TooGenericExceptionThrown:FeatureFlagScenario.kt$FeatureFlagScenario$throw RuntimeException("FeatureFlagScenario unhandled")</ID>
     <ID>TooGenericExceptionThrown:OnSendCallbackScenario.kt$OnSendCallbackScenario$throw RuntimeException("Unhandled Error")</ID>

diff --git a/...cenarios/src/main/java/com/bugsnag/android/mazerunner/scenarios/DeliverOnCrashScenario.kt b/...cenarios/src/main/java/com/bugsnag/android/mazerunner/scenarios/DeliverOnCrashScenario.kt
@@ -0,0 +1,27 @@
+package com.bugsnag.android.mazerunner.scenarios
+
+import android.content.Context
+import com.bugsnag.android.Configuration
+
+private const val TIMEOUT = 15_000L
+
+internal class DeliverOnCrashScenario(
+    config: Configuration,
+    context: Context,
+    eventMetadata: String
+) : Scenario(config, context, eventMetadata) {
+
+    init {
+        config.isAttemptDeliveryOnCrash = true
+        val deliveryDelegate = Class.forName("com.bugsnag.android.DeliveryDelegate")
+        deliveryDelegate.getDeclaredField("DELIVERY_TIMEOUT").apply {
+            isAccessible = true
+            set(null, TIMEOUT)
+        }
+    }
+
+    override fun startScenario() {
+        super.startScenario()
+        throw RuntimeException("DeliverOnCrashScenario")
+    }
+}
diff --git a/features/full_tests/crash_handler.feature b/features/full_tests/crash_handler.feature
@@ -12,3 +12,12 @@ Feature: Reporting with other exception handlers installed
     And the exception "errorClass" equals "java.lang.RuntimeException"
     And the exception "message" equals "CrashHandlerScenario"
     And the event "metaData.customHandler.invoked" is true
+
+  Scenario: Delivers crashes synchronously when configured
+    When I run "DeliverOnCrashScenario"
+    And I wait to receive an error
+    Then the error is valid for the error reporting API version "4.0" for the "Android Bugsnag Notifier" notifier
+    And the error payload field "events" is an array with 1 elements
+    And the exception "errorClass" equals "java.lang.RuntimeException"
+    And the exception "message" equals "DeliverOnCrashScenario"
+    And the event "usage.config.attemptDeliveryOnCrash" is true
-Original file line number
+Diff line change
@@ Expand Up / @@ -51,6 +51,7 @@ public File invoke() { @@
                         }
                     }),
                     true,
+                    true,
                     null,
                     null,
                     Collections.singleton("password")
@@ Expand Down @@
Original file line number	Diff line number	Diff line change
		@@ -1 +1,2 @@
		-keep class com.bugsnag.android.mazerunner.** {*;}
		-keep class com.bugsnag.android.DeliveryDelegate {*;}