Skip to content

Commit

Permalink
refactor: deliver most recent launch crash synchronously
Browse files Browse the repository at this point in the history
  • Loading branch information
@fractalwrench
fractalwrench committed Mar 16, 2021
1 parent 7a4f8e8 commit 03ed228
Show file tree
Hide file tree
Showing 7 changed files with 307 additions and 109 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
36 changes: 4 additions & 32 deletions bugsnag-android-core/src/androidTest/java/com/bugsnag/android/EventStoreConfinementTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions bugsnag-android-core/src/main/java/com/bugsnag/android/Client.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
16 changes: 5 additions & 11 deletions bugsnag-android-core/src/main/java/com/bugsnag/android/EventFilenameInfo.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ internal data class EventFilenameInfo(
sanitizedApiKey,
uuid,
timestamp,
findSuffixForEvent(obj, config),
findSuffixForEvent(obj),
findErrorTypesForEvent(obj)
)
}
Expand Down Expand Up @@ -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
}
}
}
131 changes: 68 additions & 63 deletions bugsnag-android-core/src/main/java/com/bugsnag/android/EventStore.java
View file
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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>() {
Expand All @@ -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(),
Expand All @@ -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();
Expand All @@ -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);
Expand All @@ -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:
Expand Down Expand Up @@ -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);
}
}
Loading

0 comments on commit 03ed228

Please sign in to comment.