From 284509eba39c29cea83ef5cc695f1e4578ef0241 Mon Sep 17 00:00:00 2001 From: Jason Date: Fri, 30 Sep 2022 13:32:01 +0100 Subject: [PATCH 1/3] feat(delivery): introduced `EventStore.writeAndDeliver` to both store and then immediately schedule the delivery of a given payload --- .../java/com/bugsnag/android/EventStore.java | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) 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 index 5ae0c04c50..593003ce99 100644 --- 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 storedFiles) { return launchCrashes.isEmpty() ? null : launchCrashes.get(launchCrashes.size() - 1); } + @Nullable + Future writeAndDeliver(@NonNull final JsonStream.Streamable streamable) { + final String filename = write(streamable); + + if (filename != null) { + try { + return bgTaskSevice.submitTask(TaskType.ERROR_REQUEST, new Callable() { + 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 storedReports) { } } - private void flushEventFile(File eventFile) { + void flushEventFile(File eventFile) { try { EventFilenameInfo eventInfo = EventFilenameInfo.fromFile(eventFile, config); String apiKey = eventInfo.getApiKey(); From bf249d79a73183339773932a38c76c95c9564afc Mon Sep 17 00:00:00 2001 From: Jason Date: Fri, 30 Sep 2022 16:20:56 +0100 Subject: [PATCH 2/3] feat(delivery): added `attemptDeliveryOnCrash` to `Configuration` and `ImmutableConfig` and allow a 3 second timeout during crash to attempt delivery --- CHANGELOG.md | 8 +++++ .../android/ManifestConfigLoaderTest.kt | 2 ++ .../com/bugsnag/android/ConfigInternal.kt | 2 ++ .../com/bugsnag/android/Configuration.java | 34 +++++++++++++++++- .../com/bugsnag/android/DeliveryDelegate.java | 35 ++++++++++++++++--- .../bugsnag/android/ManifestConfigLoader.kt | 5 +++ .../android/internal/ImmutableConfig.kt | 2 ++ .../java/com/bugsnag/android/TestData.java | 1 + .../mazerunner/app/proguard-rules.pro | 1 + .../jvm-scenarios/detekt-baseline.xml | 1 + .../scenarios/DeliverOnCrashScenario.kt | 27 ++++++++++++++ features/full_tests/crash_handler.feature | 8 +++++ 12 files changed, 120 insertions(+), 6 deletions(-) create mode 100644 features/fixtures/mazerunner/jvm-scenarios/src/main/java/com/bugsnag/android/mazerunner/scenarios/DeliverOnCrashScenario.kt diff --git a/CHANGELOG.md b/CHANGELOG.md index 1e4ff78f0b..07300a16a6 100644 --- 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 index 9b1566db6f..3ebe2f3b96 100644 --- 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 index 32599967b4..b9e311e201 100644 --- 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 = emptySet() var persistenceDirectory: File? = null + var attemptDeliveryOnCrash: Boolean = false + val notifier: Notifier = Notifier() protected val plugins = HashSet() 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 index 0be3d0ff9a..d1d5436cf5 100644 --- 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 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 index d9dd1aa168..7e4d1b207b 100644 --- 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 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/ManifestConfigLoader.kt b/bugsnag-android-core/src/main/java/com/bugsnag/android/ManifestConfigLoader.kt index ad166e35b8..b121cbf83a 100644 --- 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 index 06558cfca2..1cde96634d 100644 --- 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, 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 index 7b92180584..4408361a25 100644 --- 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 index c82de9bb2b..ba642cad1e 100644 --- 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 index 20bbeafdcd..3ebc1a1b02 100644 --- a/features/fixtures/mazerunner/jvm-scenarios/detekt-baseline.xml +++ b/features/fixtures/mazerunner/jvm-scenarios/detekt-baseline.xml @@ -31,6 +31,7 @@ ThrowingExceptionsWithoutMessageOrCause:TrimmedStacktraceScenario.kt$TrimmedStacktraceScenario$RuntimeException() TooGenericExceptionThrown:CrashHandlerScenario.kt$CrashHandlerScenario$throw RuntimeException("CrashHandlerScenario") TooGenericExceptionThrown:CustomHttpClientFlushScenario.kt$CustomHttpClientFlushScenario$throw RuntimeException("ReportCacheScenario") + TooGenericExceptionThrown:DeliverOnCrashScenario.kt$DeliverOnCrashScenario$throw RuntimeException("DeliverOnCrashScenario") TooGenericExceptionThrown:DisableAutoDetectErrorsScenario.kt$DisableAutoDetectErrorsScenario$throw RuntimeException("Should never appear") TooGenericExceptionThrown:FeatureFlagScenario.kt$FeatureFlagScenario$throw RuntimeException("FeatureFlagScenario unhandled") TooGenericExceptionThrown:OnSendCallbackScenario.kt$OnSendCallbackScenario$throw RuntimeException("Unhandled Error") diff --git a/features/fixtures/mazerunner/jvm-scenarios/src/main/java/com/bugsnag/android/mazerunner/scenarios/DeliverOnCrashScenario.kt b/features/fixtures/mazerunner/jvm-scenarios/src/main/java/com/bugsnag/android/mazerunner/scenarios/DeliverOnCrashScenario.kt new file mode 100644 index 0000000000..7854c95662 --- /dev/null +++ b/features/fixtures/mazerunner/jvm-scenarios/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 index a401483129..0ca3ee3957 100644 --- a/features/full_tests/crash_handler.feature +++ b/features/full_tests/crash_handler.feature @@ -12,3 +12,11 @@ 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" From c680081d4041b37a6618fa18943513582de4b31f Mon Sep 17 00:00:00 2001 From: Jason Date: Mon, 3 Oct 2022 09:23:55 +0100 Subject: [PATCH 3/3] feat(delivery): added `attemptDeliveryOnCrash` telemetry to event payloads --- .../src/main/java/com/bugsnag/android/ConfigInternal.kt | 7 +++++-- features/full_tests/crash_handler.feature | 1 + 2 files changed, 6 insertions(+), 2 deletions(-) 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 index b9e311e201..1fc62957e3 100644 --- a/bugsnag-android-core/src/main/java/com/bugsnag/android/ConfigInternal.kt +++ b/bugsnag-android-core/src/main/java/com/bugsnag/android/ConfigInternal.kt @@ -105,6 +105,9 @@ internal class ConfigInternal( } fun getConfigDifferences(): Map { + // 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) @@ -138,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() } @@ -155,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/features/full_tests/crash_handler.feature b/features/full_tests/crash_handler.feature index 0ca3ee3957..98b6ff4e8b 100644 --- a/features/full_tests/crash_handler.feature +++ b/features/full_tests/crash_handler.feature @@ -20,3 +20,4 @@ Feature: Reporting with other exception handlers installed 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