Skip to content

Commit

Permalink
Add a CrashlyticsWorkers container to manage the workers (#6178)
Browse files Browse the repository at this point in the history 
Add a CrashlyticsWorkers container to manage the workers, and define
what each one is for. Also added a `submitTaskOnSuccess` method for
convenience. This will be useful for any of the user actions that get
triggered by send unsent reports.

Also found and fixed a bug in the worker queue. If a task gets cancelled
and is followed by a runnable or callable already in the queue, it would
have been skipped. Now it's not a problem, and a test case added.
  • Loading branch information
@mrober
mrober committed Aug 12, 2024
1 parent 08deb69 commit 9535f94
Show file tree
Hide file tree
Showing 26 changed files with 740 additions and 612 deletions.
40 changes: 19 additions & 21 deletions ...dTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
import com.google.firebase.crashlytics.internal.DevelopmentPlatformProvider;
import com.google.firebase.crashlytics.internal.NativeSessionFileProvider;
import com.google.firebase.crashlytics.internal.analytics.AnalyticsEventLogger;
import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorker;
import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers;
import com.google.firebase.crashlytics.internal.metadata.LogFileManager;
import com.google.firebase.crashlytics.internal.metadata.UserMetadata;
import com.google.firebase.crashlytics.internal.model.CrashlyticsReport;
Expand All @@ -63,8 +63,8 @@ public class CrashlyticsControllerTest extends CrashlyticsTestCase {
private static final String GOOGLE_APP_ID = "google:app:id";
private static final String SESSION_ID = "session_id";

private final CrashlyticsWorker commonWorker =
new CrashlyticsWorker(TestOnlyExecutors.background());
private final CrashlyticsWorkers crashlyticsWorkers =
new CrashlyticsWorkers(TestOnlyExecutors.background(), TestOnlyExecutors.blocking());

private Context testContext;
private IdManager idManager;
Expand All @@ -74,8 +74,6 @@ public class CrashlyticsControllerTest extends CrashlyticsTestCase {
private DataCollectionArbiter mockDataCollectionArbiter;
private CrashlyticsNativeComponent mockNativeComponent = mock(CrashlyticsNativeComponent.class);

private CrashlyticsWorker diskWriteWorker = new CrashlyticsWorker(TestOnlyExecutors.background());

@Override
protected void setUp() throws Exception {
super.setUp();
Expand Down Expand Up @@ -108,7 +106,7 @@ protected void setUp() throws Exception {
@Override
protected void tearDown() throws Exception {
super.tearDown();
commonWorker.await();
crashlyticsWorkers.common.await();
}

/** A convenience class for building CrashlyticsController instances for testing. */
Expand Down Expand Up @@ -177,7 +175,6 @@ public CrashlyticsController build() {
final CrashlyticsController controller =
new CrashlyticsController(
testContext.getApplicationContext(),
commonWorker,
idManager,
dataCollectionArbiter,
testFileStore,
Expand All @@ -189,7 +186,7 @@ public CrashlyticsController build() {
nativeComponent,
analyticsEventLogger,
mock(CrashlyticsAppQualitySessionsSubscriber.class),
diskWriteWorker);
crashlyticsWorkers);
return controller;
}
}
Expand Down Expand Up @@ -218,7 +215,7 @@ public void testWriteNonFatal_callsSessionReportingCoordinatorPersistNonFatal()
controller.writeNonFatalException(thread, nonFatal);
controller.doCloseSessions(testSettingsProvider);

commonWorker.await();
crashlyticsWorkers.common.await();

verify(mockSessionReportingCoordinator)
.persistNonFatalEvent(eq(nonFatal), eq(thread), eq(sessionId), anyLong());
Expand Down Expand Up @@ -257,7 +254,7 @@ public void testOnDemandFatal_callLogFatalException() throws Exception {
controller.enableExceptionHandling(SESSION_ID, exceptionHandler, testSettingsProvider);
controller.logFatalException(thread, fatal);

commonWorker.await();
crashlyticsWorkers.common.await();

verify(mockUserMetadata).setNewSession(not(eq(SESSION_ID)));
}
Expand Down Expand Up @@ -336,8 +333,8 @@ public File getOsFile() {
final CrashlyticsController controller =
builder().setNativeComponent(mockNativeComponent).setLogFileManager(logFileManager).build();

commonWorker.submit(() -> controller.finalizeSessions(testSettingsProvider));
commonWorker.await();
crashlyticsWorkers.common.submit(() -> controller.finalizeSessions(testSettingsProvider));
crashlyticsWorkers.common.await();

verify(mockSessionReportingCoordinator)
.finalizeSessionWithNativeEvent(eq(previousSessionId), any(), any());
Expand All @@ -348,8 +345,8 @@ public File getOsFile() {
@SdkSuppress(minSdkVersion = 30) // ApplicationExitInfo
public void testMissingNativeComponentCausesNoReports() throws Exception {
final CrashlyticsController controller = createController();
commonWorker.submit(() -> controller.finalizeSessions(testSettingsProvider));
commonWorker.await();
crashlyticsWorkers.common.submit(() -> controller.finalizeSessions(testSettingsProvider));
crashlyticsWorkers.common.await();

List<String> sessions = testFileStore.getAllOpenSessionIds();
for (String sessionId : sessions) {
Expand Down Expand Up @@ -385,8 +382,9 @@ public void testLogStringAfterCrashOk() throws Exception {
testSettingsProvider, Thread.currentThread(), new RuntimeException());

// This should not throw.
diskWriteWorker.submit(() -> controller.writeToLog(System.currentTimeMillis(), "Hi"));
diskWriteWorker.await();
crashlyticsWorkers.diskWrite.submit(
() -> controller.writeToLog(System.currentTimeMillis(), "Hi"));
crashlyticsWorkers.diskWrite.await();
}

/**
Expand All @@ -401,8 +399,8 @@ public void testFinalizeSessionAfterCrashOk() throws Exception {
testSettingsProvider, Thread.currentThread(), new RuntimeException());

// This should not throw.
commonWorker.submit(() -> controller.finalizeSessions(testSettingsProvider));
commonWorker.await();
crashlyticsWorkers.common.submit(() -> controller.finalizeSessions(testSettingsProvider));
crashlyticsWorkers.common.await();
}

@SdkSuppress(minSdkVersion = 30) // ApplicationExitInfo
Expand Down Expand Up @@ -445,7 +443,7 @@ public void testUploadDisabledThenOptIn() throws Exception {

final DataCollectionArbiter arbiter = mock(DataCollectionArbiter.class);
when(arbiter.isAutomaticDataCollectionEnabled()).thenReturn(false);
when(arbiter.waitForDataCollectionPermission(any(Executor.class)))
when(arbiter.waitForDataCollectionPermission())
.thenReturn(new TaskCompletionSource<Void>().getTask());
when(arbiter.waitForAutomaticDataCollectionEnabled())
.thenReturn(new TaskCompletionSource<Void>().getTask());
Expand Down Expand Up @@ -566,14 +564,14 @@ public void testFatalEvent_sendsAppExceptionEvent() throws Exception {
when(mockSessionReportingCoordinator.listSortedOpenSessionIds())
.thenReturn(new TreeSet<>(Collections.singleton(sessionId)));

commonWorker.submit(
crashlyticsWorkers.common.submit(
() -> {
controller.openSession(SESSION_ID);
controller.handleUncaughtException(
testSettingsProvider, Thread.currentThread(), new RuntimeException("Fatal"));
controller.finalizeSessions(testSettingsProvider);
});
commonWorker.await();
crashlyticsWorkers.common.await();

assertFirebaseAnalyticsCrashEvent(mockFirebaseAnalyticsLogger);
}
Expand Down
12 changes: 5 additions & 7 deletions ...va/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreInitializationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
import com.google.firebase.crashlytics.internal.RemoteConfigDeferredProxy;
import com.google.firebase.crashlytics.internal.analytics.UnavailableAnalyticsEventLogger;
import com.google.firebase.crashlytics.internal.breadcrumbs.DisabledBreadcrumbSource;
import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorker;
import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers;
import com.google.firebase.crashlytics.internal.persistence.FileStore;
import com.google.firebase.crashlytics.internal.settings.Settings;
import com.google.firebase.crashlytics.internal.settings.SettingsController;
Expand Down Expand Up @@ -91,8 +91,7 @@ private static final class CoreBuilder {
private CrashlyticsNativeComponent nativeComponent;
private DataCollectionArbiter arbiter;
private FileStore fileStore;
private CrashlyticsWorker commonWorker;
private CrashlyticsWorker diskWriteWorker;
private CrashlyticsWorkers crashlyticsWorkers;

public CoreBuilder(Context context, FirebaseOptions firebaseOptions) {
app = mock(FirebaseApp.class);
Expand Down Expand Up @@ -121,8 +120,8 @@ public void whenAvailable(
arbiter = mock(DataCollectionArbiter.class);
when(arbiter.isAutomaticDataCollectionEnabled()).thenReturn(true);

commonWorker = new CrashlyticsWorker(TestOnlyExecutors.background());
diskWriteWorker = new CrashlyticsWorker(TestOnlyExecutors.background());
crashlyticsWorkers =
new CrashlyticsWorkers(TestOnlyExecutors.background(), TestOnlyExecutors.blocking());
fileStore = new FileStore(context);
}

Expand All @@ -147,8 +146,7 @@ public CrashlyticsCore build() {
fileStore,
mock(CrashlyticsAppQualitySessionsSubscriber.class),
mock(RemoteConfigDeferredProxy.class),
commonWorker,
diskWriteWorker);
crashlyticsWorkers);
}
}

Expand Down
37 changes: 17 additions & 20 deletions ...androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
import com.google.firebase.crashlytics.internal.breadcrumbs.BreadcrumbHandler;
import com.google.firebase.crashlytics.internal.breadcrumbs.BreadcrumbSource;
import com.google.firebase.crashlytics.internal.breadcrumbs.DisabledBreadcrumbSource;
import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorker;
import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers;
import com.google.firebase.crashlytics.internal.metadata.UserMetadata;
import com.google.firebase.crashlytics.internal.persistence.FileStore;
import com.google.firebase.crashlytics.internal.settings.Settings;
Expand Down Expand Up @@ -70,10 +70,8 @@ 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());
private static final CrashlyticsWorkers crashlyticsWorkers =
new CrashlyticsWorkers(TestOnlyExecutors.background(), TestOnlyExecutors.blocking());

@Override
protected void setUp() throws Exception {
Expand All @@ -97,7 +95,7 @@ public void testCustomAttributes() throws Exception {

final String id = "id012345";
crashlyticsCore.setUserId(id);
commonWorker.await();
crashlyticsWorkers.common.await();
assertEquals(id, metadata.getUserId());

final StringBuffer idBuffer = new StringBuffer(id);
Expand All @@ -108,13 +106,13 @@ public void testCustomAttributes() throws Exception {
final String superLongId = longId + "more chars";

crashlyticsCore.setUserId(superLongId);
commonWorker.await();
crashlyticsWorkers.common.await();
assertEquals(longId, metadata.getUserId());

final String key1 = "key1";
final String value1 = "value1";
crashlyticsCore.setCustomKey(key1, value1);
commonWorker.await();
crashlyticsWorkers.common.await();
assertEquals(value1, metadata.getCustomKeys().get(key1));

// Adding an existing key with the same value should return false
Expand All @@ -128,7 +126,7 @@ public void testCustomAttributes() throws Exception {

// test truncation of custom keys and attributes
crashlyticsCore.setCustomKey(superLongId, superLongValue);
commonWorker.await();
crashlyticsWorkers.common.await();
assertNull(metadata.getCustomKeys().get(superLongId));
assertEquals(longValue, metadata.getCustomKeys().get(longId));

Expand All @@ -137,28 +135,28 @@ public void testCustomAttributes() throws Exception {
final String key = "key" + i;
final String value = "value" + i;
crashlyticsCore.setCustomKey(key, value);
commonWorker.await();
crashlyticsWorkers.common.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");
commonWorker.await();
crashlyticsWorkers.common.await();
assertFalse(metadata.getCustomKeys().containsKey(key));

// should be able to update existing keys
crashlyticsCore.setCustomKey(key1, longValue);
commonWorker.await();
crashlyticsWorkers.common.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);
commonWorker.await();
crashlyticsWorkers.common.await();
assertEquals("", metadata.getCustomKeys().get(key1));

// keys and values are trimmed.
crashlyticsCore.setCustomKey(" " + key1 + " ", " " + longValue + " ");
commonWorker.await();
crashlyticsWorkers.common.await();
assertTrue(metadata.getCustomKeys().containsKey(key1));
assertEquals(longValue, metadata.getCustomKeys().get(key1));
}
Expand Down Expand Up @@ -209,7 +207,7 @@ public void testBulkCustomKeys() throws Exception {
keysAndValues.put(intKey, String.valueOf(intValue));

crashlyticsCore.setCustomKeys(keysAndValues);
commonWorker.await();
crashlyticsWorkers.common.await();

assertEquals(stringValue, metadata.getCustomKeys().get(stringKey));
assertEquals(trimmedValue, metadata.getCustomKeys().get(trimmedKey));
Expand All @@ -230,7 +228,7 @@ public void testBulkCustomKeys() throws Exception {
addlKeysAndValues.put(key, value);
}
crashlyticsCore.setCustomKeys(addlKeysAndValues);
commonWorker.await();
crashlyticsWorkers.common.await();

// Ensure all keys have been set
assertEquals(UserMetadata.MAX_ATTRIBUTES, metadata.getCustomKeys().size(), DELTA);
Expand All @@ -248,7 +246,7 @@ public void testBulkCustomKeys() throws Exception {
extraKeysAndValues.put(key, value);
}
crashlyticsCore.setCustomKeys(extraKeysAndValues);
commonWorker.await();
crashlyticsWorkers.common.await();

// Make sure the extra keys were not added
for (int i = UserMetadata.MAX_ATTRIBUTES; i < UserMetadata.MAX_ATTRIBUTES + 10; ++i) {
Expand All @@ -274,7 +272,7 @@ public void testBulkCustomKeys() throws Exception {
updatedKeysAndValues.put(intKey, String.valueOf(updatedIntValue));

crashlyticsCore.setCustomKeys(updatedKeysAndValues);
commonWorker.await();
crashlyticsWorkers.common.await();

assertEquals(updatedStringValue, metadata.getCustomKeys().get(stringKey));
assertFalse(Boolean.parseBoolean(metadata.getCustomKeys().get(booleanKey)));
Expand Down Expand Up @@ -447,8 +445,7 @@ CrashlyticsCore build(Context context) {
new FileStore(context),
mock(CrashlyticsAppQualitySessionsSubscriber.class),
mock(RemoteConfigDeferredProxy.class),
commonWorker,
diskWriteWorker);
crashlyticsWorkers);
return crashlyticsCore;
}
}
Expand Down
Loading

0 comments on commit 9535f94

Please sign in to comment.