Skip to content

Add a CrashlyticsWorkers container to manage the workers #6178

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Aug 12, 2024
Merged

Add a CrashlyticsWorkers container to manage the workers #6178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ef591d4
Add CrashlyticsWorkers container and plumb it through
mrober Aug 12, 2024
a0cee58
Delete CrashlyticsBackgroundWorker.java
mrober Aug 12, 2024
f2ab709
Make CrashlyticsWorker an Executor, also fixed cancellation bug
mrober Aug 12, 2024
7005ce8
Add submitTaskOnSuccess method to CrashlyticsWorker
mrober Aug 12, 2024
ebbf526
Clean up CrashlyticsWorkers, and fix some comments
mrober Aug 12, 2024
fded039
Format
mrober Aug 12, 2024
03fb48d
Address feedback
mrober Aug 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 19 additions & 21 deletions ...dTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerTest.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading