Skip to content

Commit 9535f94

Browse files
mrobermrober
authored
Add a CrashlyticsWorkers container to manage the workers (firebase#6178)
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.
1 parent 08deb69 commit 9535f94

26 files changed

+740
-612
lines changed

‎firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerTest.java‎

Lines changed: 19 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@
4040
import com.google.firebase.crashlytics.internal.DevelopmentPlatformProvider;
4141
import com.google.firebase.crashlytics.internal.NativeSessionFileProvider;
4242
import com.google.firebase.crashlytics.internal.analytics.AnalyticsEventLogger;
43-
import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorker;
43+
import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers;
4444
import com.google.firebase.crashlytics.internal.metadata.LogFileManager;
4545
import com.google.firebase.crashlytics.internal.metadata.UserMetadata;
4646
import com.google.firebase.crashlytics.internal.model.CrashlyticsReport;
@@ -63,8 +63,8 @@ public class CrashlyticsControllerTest extends CrashlyticsTestCase {
6363
private static final String GOOGLE_APP_ID = "google:app:id";
6464
private static final String SESSION_ID = "session_id";
6565

66-
private final CrashlyticsWorker commonWorker =
67-
new CrashlyticsWorker(TestOnlyExecutors.background());
66+
private final CrashlyticsWorkers crashlyticsWorkers =
67+
new CrashlyticsWorkers(TestOnlyExecutors.background(), TestOnlyExecutors.blocking());
6868

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

77-
private CrashlyticsWorker diskWriteWorker = new CrashlyticsWorker(TestOnlyExecutors.background());
78-
7977
@Override
8078
protected void setUp() throws Exception {
8179
super.setUp();
@@ -108,7 +106,7 @@ protected void setUp() throws Exception {
108106
@Override
109107
protected void tearDown() throws Exception {
110108
super.tearDown();
111-
commonWorker.await();
109+
crashlyticsWorkers.common.await();
112110
}
113111

114112
/** A convenience class for building CrashlyticsController instances for testing. */
@@ -177,7 +175,6 @@ public CrashlyticsController build() {
177175
final CrashlyticsController controller =
178176
new CrashlyticsController(
179177
testContext.getApplicationContext(),
180-
commonWorker,
181178
idManager,
182179
dataCollectionArbiter,
183180
testFileStore,
@@ -189,7 +186,7 @@ public CrashlyticsController build() {
189186
nativeComponent,
190187
analyticsEventLogger,
191188
mock(CrashlyticsAppQualitySessionsSubscriber.class),
192-
diskWriteWorker);
189+
crashlyticsWorkers);
193190
return controller;
194191
}
195192
}
@@ -218,7 +215,7 @@ public void testWriteNonFatal_callsSessionReportingCoordinatorPersistNonFatal()
218215
controller.writeNonFatalException(thread, nonFatal);
219216
controller.doCloseSessions(testSettingsProvider);
220217

221-
commonWorker.await();
218+
crashlyticsWorkers.common.await();
222219

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

260-
commonWorker.await();
257+
crashlyticsWorkers.common.await();
261258

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

339-
commonWorker.submit(() -> controller.finalizeSessions(testSettingsProvider));
340-
commonWorker.await();
336+
crashlyticsWorkers.common.submit(() -> controller.finalizeSessions(testSettingsProvider));
337+
crashlyticsWorkers.common.await();
341338

342339
verify(mockSessionReportingCoordinator)
343340
.finalizeSessionWithNativeEvent(eq(previousSessionId), any(), any());
@@ -348,8 +345,8 @@ public File getOsFile() {
348345
@SdkSuppress(minSdkVersion = 30) // ApplicationExitInfo
349346
public void testMissingNativeComponentCausesNoReports() throws Exception {
350347
final CrashlyticsController controller = createController();
351-
commonWorker.submit(() -> controller.finalizeSessions(testSettingsProvider));
352-
commonWorker.await();
348+
crashlyticsWorkers.common.submit(() -> controller.finalizeSessions(testSettingsProvider));
349+
crashlyticsWorkers.common.await();
353350

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

387384
// This should not throw.
388-
diskWriteWorker.submit(() -> controller.writeToLog(System.currentTimeMillis(), "Hi"));
389-
diskWriteWorker.await();
385+
crashlyticsWorkers.diskWrite.submit(
386+
() -> controller.writeToLog(System.currentTimeMillis(), "Hi"));
387+
crashlyticsWorkers.diskWrite.await();
390388
}
391389

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

403401
// This should not throw.
404-
commonWorker.submit(() -> controller.finalizeSessions(testSettingsProvider));
405-
commonWorker.await();
402+
crashlyticsWorkers.common.submit(() -> controller.finalizeSessions(testSettingsProvider));
403+
crashlyticsWorkers.common.await();
406404
}
407405

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

446444
final DataCollectionArbiter arbiter = mock(DataCollectionArbiter.class);
447445
when(arbiter.isAutomaticDataCollectionEnabled()).thenReturn(false);
448-
when(arbiter.waitForDataCollectionPermission(any(Executor.class)))
446+
when(arbiter.waitForDataCollectionPermission())
449447
.thenReturn(new TaskCompletionSource<Void>().getTask());
450448
when(arbiter.waitForAutomaticDataCollectionEnabled())
451449
.thenReturn(new TaskCompletionSource<Void>().getTask());
@@ -566,14 +564,14 @@ public void testFatalEvent_sendsAppExceptionEvent() throws Exception {
566564
when(mockSessionReportingCoordinator.listSortedOpenSessionIds())
567565
.thenReturn(new TreeSet<>(Collections.singleton(sessionId)));
568566

569-
commonWorker.submit(
567+
crashlyticsWorkers.common.submit(
570568
() -> {
571569
controller.openSession(SESSION_ID);
572570
controller.handleUncaughtException(
573571
testSettingsProvider, Thread.currentThread(), new RuntimeException("Fatal"));
574572
controller.finalizeSessions(testSettingsProvider);
575573
});
576-
commonWorker.await();
574+
crashlyticsWorkers.common.await();
577575

578576
assertFirebaseAnalyticsCrashEvent(mockFirebaseAnalyticsLogger);
579577
}

‎firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreInitializationTest.java‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@
3535
import com.google.firebase.crashlytics.internal.RemoteConfigDeferredProxy;
3636
import com.google.firebase.crashlytics.internal.analytics.UnavailableAnalyticsEventLogger;
3737
import com.google.firebase.crashlytics.internal.breadcrumbs.DisabledBreadcrumbSource;
38-
import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorker;
38+
import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers;
3939
import com.google.firebase.crashlytics.internal.persistence.FileStore;
4040
import com.google.firebase.crashlytics.internal.settings.Settings;
4141
import com.google.firebase.crashlytics.internal.settings.SettingsController;
@@ -91,8 +91,7 @@ private static final class CoreBuilder {
9191
private CrashlyticsNativeComponent nativeComponent;
9292
private DataCollectionArbiter arbiter;
9393
private FileStore fileStore;
94-
private CrashlyticsWorker commonWorker;
95-
private CrashlyticsWorker diskWriteWorker;
94+
private CrashlyticsWorkers crashlyticsWorkers;
9695

9796
public CoreBuilder(Context context, FirebaseOptions firebaseOptions) {
9897
app = mock(FirebaseApp.class);
@@ -121,8 +120,8 @@ public void whenAvailable(
121120
arbiter = mock(DataCollectionArbiter.class);
122121
when(arbiter.isAutomaticDataCollectionEnabled()).thenReturn(true);
123122

124-
commonWorker = new CrashlyticsWorker(TestOnlyExecutors.background());
125-
diskWriteWorker = new CrashlyticsWorker(TestOnlyExecutors.background());
123+
crashlyticsWorkers =
124+
new CrashlyticsWorkers(TestOnlyExecutors.background(), TestOnlyExecutors.blocking());
126125
fileStore = new FileStore(context);
127126
}
128127

@@ -147,8 +146,7 @@ public CrashlyticsCore build() {
147146
fileStore,
148147
mock(CrashlyticsAppQualitySessionsSubscriber.class),
149148
mock(RemoteConfigDeferredProxy.class),
150-
commonWorker,
151-
diskWriteWorker);
149+
crashlyticsWorkers);
152150
}
153151
}
154152

‎firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreTest.java‎

Lines changed: 17 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@
3939
import com.google.firebase.crashlytics.internal.breadcrumbs.BreadcrumbHandler;
4040
import com.google.firebase.crashlytics.internal.breadcrumbs.BreadcrumbSource;
4141
import com.google.firebase.crashlytics.internal.breadcrumbs.DisabledBreadcrumbSource;
42-
import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorker;
42+
import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers;
4343
import com.google.firebase.crashlytics.internal.metadata.UserMetadata;
4444
import com.google.firebase.crashlytics.internal.persistence.FileStore;
4545
import com.google.firebase.crashlytics.internal.settings.Settings;
@@ -70,10 +70,8 @@ public void whenAvailable(
7070

7171
private CrashlyticsCore crashlyticsCore;
7272
private BreadcrumbSource mockBreadcrumbSource;
73-
private static final CrashlyticsWorker commonWorker =
74-
new CrashlyticsWorker(TestOnlyExecutors.background());
75-
private static final CrashlyticsWorker diskWriteWorker =
76-
new CrashlyticsWorker(TestOnlyExecutors.background());
73+
private static final CrashlyticsWorkers crashlyticsWorkers =
74+
new CrashlyticsWorkers(TestOnlyExecutors.background(), TestOnlyExecutors.blocking());
7775

7876
@Override
7977
protected void setUp() throws Exception {
@@ -97,7 +95,7 @@ public void testCustomAttributes() throws Exception {
9795

9896
final String id = "id012345";
9997
crashlyticsCore.setUserId(id);
100-
commonWorker.await();
98+
crashlyticsWorkers.common.await();
10199
assertEquals(id, metadata.getUserId());
102100

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

110108
crashlyticsCore.setUserId(superLongId);
111-
commonWorker.await();
109+
crashlyticsWorkers.common.await();
112110
assertEquals(longId, metadata.getUserId());
113111

114112
final String key1 = "key1";
115113
final String value1 = "value1";
116114
crashlyticsCore.setCustomKey(key1, value1);
117-
commonWorker.await();
115+
crashlyticsWorkers.common.await();
118116
assertEquals(value1, metadata.getCustomKeys().get(key1));
119117

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

129127
// test truncation of custom keys and attributes
130128
crashlyticsCore.setCustomKey(superLongId, superLongValue);
131-
commonWorker.await();
129+
crashlyticsWorkers.common.await();
132130
assertNull(metadata.getCustomKeys().get(superLongId));
133131
assertEquals(longValue, metadata.getCustomKeys().get(longId));
134132

@@ -137,28 +135,28 @@ public void testCustomAttributes() throws Exception {
137135
final String key = "key" + i;
138136
final String value = "value" + i;
139137
crashlyticsCore.setCustomKey(key, value);
140-
commonWorker.await();
138+
crashlyticsWorkers.common.await();
141139
assertEquals(value, metadata.getCustomKeys().get(key));
142140
}
143141
// should be full now, extra key, value pairs will be dropped.
144142
final String key = "new key";
145143
crashlyticsCore.setCustomKey(key, "some value");
146-
commonWorker.await();
144+
crashlyticsWorkers.common.await();
147145
assertFalse(metadata.getCustomKeys().containsKey(key));
148146

149147
// should be able to update existing keys
150148
crashlyticsCore.setCustomKey(key1, longValue);
151-
commonWorker.await();
149+
crashlyticsWorkers.common.await();
152150
assertEquals(longValue, metadata.getCustomKeys().get(key1));
153151

154152
// when we set a key to null, it should still exist with an empty value
155153
crashlyticsCore.setCustomKey(key1, null);
156-
commonWorker.await();
154+
crashlyticsWorkers.common.await();
157155
assertEquals("", metadata.getCustomKeys().get(key1));
158156

159157
// keys and values are trimmed.
160158
crashlyticsCore.setCustomKey(" " + key1 + " ", " " + longValue + " ");
161-
commonWorker.await();
159+
crashlyticsWorkers.common.await();
162160
assertTrue(metadata.getCustomKeys().containsKey(key1));
163161
assertEquals(longValue, metadata.getCustomKeys().get(key1));
164162
}
@@ -209,7 +207,7 @@ public void testBulkCustomKeys() throws Exception {
209207
keysAndValues.put(intKey, String.valueOf(intValue));
210208

211209
crashlyticsCore.setCustomKeys(keysAndValues);
212-
commonWorker.await();
210+
crashlyticsWorkers.common.await();
213211

214212
assertEquals(stringValue, metadata.getCustomKeys().get(stringKey));
215213
assertEquals(trimmedValue, metadata.getCustomKeys().get(trimmedKey));
@@ -230,7 +228,7 @@ public void testBulkCustomKeys() throws Exception {
230228
addlKeysAndValues.put(key, value);
231229
}
232230
crashlyticsCore.setCustomKeys(addlKeysAndValues);
233-
commonWorker.await();
231+
crashlyticsWorkers.common.await();
234232

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

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

276274
crashlyticsCore.setCustomKeys(updatedKeysAndValues);
277-
commonWorker.await();
275+
crashlyticsWorkers.common.await();
278276

279277
assertEquals(updatedStringValue, metadata.getCustomKeys().get(stringKey));
280278
assertFalse(Boolean.parseBoolean(metadata.getCustomKeys().get(booleanKey)));
@@ -447,8 +445,7 @@ CrashlyticsCore build(Context context) {
447445
new FileStore(context),
448446
mock(CrashlyticsAppQualitySessionsSubscriber.class),
449447
mock(RemoteConfigDeferredProxy.class),
450-
commonWorker,
451-
diskWriteWorker);
448+
crashlyticsWorkers);
452449
return crashlyticsCore;
453450
}
454451
}

0 commit comments

Comments
 (0)