Skip to content

Commit 3a4ec18

Browse files
themiswangthemiswang
authored
User action refactor (#6120)
- Creating common and disk worker based on Firebase background common executor - Move all user action directly to background thread and remove some unnecessary task submit - Add unit tests dependency for firebase common executor - Decouple crashlytics backend handler from user metadata and use disk writing working for persistence write.
1 parent 96b702f commit 3a4ec18

File tree

6 files changed

+87
-79
lines changed

6 files changed

+87
-79
lines changed

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

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,8 @@ 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+
7779
@Override
7880
protected void setUp() throws Exception {
7981
super.setUp();
@@ -186,7 +188,8 @@ public CrashlyticsController build() {
186188
sessionReportingCoordinator,
187189
nativeComponent,
188190
analyticsEventLogger,
189-
mock(CrashlyticsAppQualitySessionsSubscriber.class));
191+
mock(CrashlyticsAppQualitySessionsSubscriber.class),
192+
diskWriteWorker);
190193
return controller;
191194
}
192195
}
@@ -376,13 +379,14 @@ public void testLoggedExceptionsAfterCrashOk() {
376379
*/
377380
// FIXME: Validate this test works as intended
378381
@SdkSuppress(minSdkVersion = 30) // ApplicationExitInfo
379-
public void testLogStringAfterCrashOk() {
382+
public void testLogStringAfterCrashOk() throws Exception {
380383
final CrashlyticsController controller = builder().build();
381384
controller.handleUncaughtException(
382385
testSettingsProvider, Thread.currentThread(), new RuntimeException());
383386

384387
// This should not throw.
385-
controller.writeToLog(System.currentTimeMillis(), "Hi");
388+
diskWriteWorker.submit(() -> controller.writeToLog(System.currentTimeMillis(), "Hi"));
389+
diskWriteWorker.await();
386390
}
387391

388392
/**

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

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ public void testCustomAttributes() throws Exception {
9393

9494
final String id = "id012345";
9595
crashlyticsCore.setUserId(id);
96-
96+
crashlyticsCore.commonWorker.await();
9797
assertEquals(id, metadata.getUserId());
9898

9999
final StringBuffer idBuffer = new StringBuffer(id);
@@ -104,11 +104,13 @@ public void testCustomAttributes() throws Exception {
104104
final String superLongId = longId + "more chars";
105105

106106
crashlyticsCore.setUserId(superLongId);
107+
crashlyticsCore.commonWorker.await();
107108
assertEquals(longId, metadata.getUserId());
108109

109110
final String key1 = "key1";
110111
final String value1 = "value1";
111112
crashlyticsCore.setCustomKey(key1, value1);
113+
crashlyticsCore.commonWorker.await();
112114
assertEquals(value1, metadata.getCustomKeys().get(key1));
113115

114116
// Adding an existing key with the same value should return false
@@ -122,6 +124,7 @@ public void testCustomAttributes() throws Exception {
122124

123125
// test truncation of custom keys and attributes
124126
crashlyticsCore.setCustomKey(superLongId, superLongValue);
127+
crashlyticsCore.commonWorker.await();
125128
assertNull(metadata.getCustomKeys().get(superLongId));
126129
assertEquals(longValue, metadata.getCustomKeys().get(longId));
127130

@@ -130,23 +133,28 @@ public void testCustomAttributes() throws Exception {
130133
final String key = "key" + i;
131134
final String value = "value" + i;
132135
crashlyticsCore.setCustomKey(key, value);
136+
crashlyticsCore.commonWorker.await();
133137
assertEquals(value, metadata.getCustomKeys().get(key));
134138
}
135139
// should be full now, extra key, value pairs will be dropped.
136140
final String key = "new key";
137141
crashlyticsCore.setCustomKey(key, "some value");
142+
crashlyticsCore.commonWorker.await();
138143
assertFalse(metadata.getCustomKeys().containsKey(key));
139144

140145
// should be able to update existing keys
141146
crashlyticsCore.setCustomKey(key1, longValue);
147+
crashlyticsCore.commonWorker.await();
142148
assertEquals(longValue, metadata.getCustomKeys().get(key1));
143149

144150
// when we set a key to null, it should still exist with an empty value
145151
crashlyticsCore.setCustomKey(key1, null);
152+
crashlyticsCore.commonWorker.await();
146153
assertEquals("", metadata.getCustomKeys().get(key1));
147154

148155
// keys and values are trimmed.
149156
crashlyticsCore.setCustomKey(" " + key1 + " ", " " + longValue + " ");
157+
crashlyticsCore.commonWorker.await();
150158
assertTrue(metadata.getCustomKeys().containsKey(key1));
151159
assertEquals(longValue, metadata.getCustomKeys().get(key1));
152160
}
@@ -197,6 +205,7 @@ public void testBulkCustomKeys() throws Exception {
197205
keysAndValues.put(intKey, String.valueOf(intValue));
198206

199207
crashlyticsCore.setCustomKeys(keysAndValues);
208+
crashlyticsCore.commonWorker.await();
200209

201210
assertEquals(stringValue, metadata.getCustomKeys().get(stringKey));
202211
assertEquals(trimmedValue, metadata.getCustomKeys().get(trimmedKey));
@@ -217,6 +226,7 @@ public void testBulkCustomKeys() throws Exception {
217226
addlKeysAndValues.put(key, value);
218227
}
219228
crashlyticsCore.setCustomKeys(addlKeysAndValues);
229+
crashlyticsCore.commonWorker.await();
220230

221231
// Ensure all keys have been set
222232
assertEquals(UserMetadata.MAX_ATTRIBUTES, metadata.getCustomKeys().size(), DELTA);
@@ -234,6 +244,7 @@ public void testBulkCustomKeys() throws Exception {
234244
extraKeysAndValues.put(key, value);
235245
}
236246
crashlyticsCore.setCustomKeys(extraKeysAndValues);
247+
crashlyticsCore.commonWorker.await();
237248

238249
// Make sure the extra keys were not added
239250
for (int i = UserMetadata.MAX_ATTRIBUTES; i < UserMetadata.MAX_ATTRIBUTES + 10; ++i) {
@@ -259,6 +270,7 @@ public void testBulkCustomKeys() throws Exception {
259270
updatedKeysAndValues.put(intKey, String.valueOf(updatedIntValue));
260271

261272
crashlyticsCore.setCustomKeys(updatedKeysAndValues);
273+
crashlyticsCore.commonWorker.await();
262274

263275
assertEquals(updatedStringValue, metadata.getCustomKeys().get(stringKey));
264276
assertFalse(Boolean.parseBoolean(metadata.getCustomKeys().get(booleanKey)));

‎firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java

Lines changed: 18 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,8 @@ class CrashlyticsController {
8686

8787
private final CrashlyticsWorker backgroundWorker;
8888

89+
private final CrashlyticsWorker diskWriteWorker;
90+
8991
private final IdManager idManager;
9092
private final FileStore fileStore;
9193

@@ -129,7 +131,8 @@ class CrashlyticsController {
129131
SessionReportingCoordinator sessionReportingCoordinator,
130132
CrashlyticsNativeComponent nativeComponent,
131133
AnalyticsEventLogger analyticsEventLogger,
132-
CrashlyticsAppQualitySessionsSubscriber sessionsSubscriber) {
134+
CrashlyticsAppQualitySessionsSubscriber sessionsSubscriber,
135+
CrashlyticsWorker diskWriteWorker) {
133136
this.context = context;
134137
this.backgroundWorker = commonWorker;
135138
this.idManager = idManager;
@@ -143,6 +146,7 @@ class CrashlyticsController {
143146
this.analyticsEventLogger = analyticsEventLogger;
144147
this.sessionsSubscriber = sessionsSubscriber;
145148
this.reportingCoordinator = sessionReportingCoordinator;
149+
this.diskWriteWorker = diskWriteWorker;
146150
}
147151

148152
private Context getContext() {
@@ -418,16 +422,9 @@ public Task<Void> then(@Nullable Settings appSettingsData)
418422

419423
/** Log a timestamped string to the log file. */
420424
void writeToLog(final long timestamp, final String msg) {
421-
backgroundWorker.submit(
422-
new Callable<Void>() {
423-
@Override
424-
public Void call() throws Exception {
425-
if (!isHandlingException()) {
426-
logFileManager.writeToLog(timestamp, msg);
427-
}
428-
return null;
429-
}
430-
});
425+
if (!isHandlingException()) {
426+
logFileManager.writeToLog(timestamp, msg);
427+
}
431428
}
432429

433430
/** Log a caught exception - write out Throwable as event section of protobuf */
@@ -436,23 +433,15 @@ void writeNonFatalException(@NonNull final Thread thread, @NonNull final Throwab
436433
// rather than the time at which the task executes.
437434
final long timestampMillis = System.currentTimeMillis();
438435

439-
backgroundWorker.submit(
440-
new Runnable() {
441-
@Override
442-
public void run() {
443-
if (!isHandlingException()) {
444-
long timestampSeconds = getTimestampSeconds(timestampMillis);
445-
final String currentSessionId = getCurrentSessionId();
446-
if (currentSessionId == null) {
447-
Logger.getLogger()
448-
.w("Tried to write a non-fatal exception while no session was open.");
449-
return;
450-
}
451-
reportingCoordinator.persistNonFatalEvent(
452-
ex, thread, currentSessionId, timestampSeconds);
453-
}
454-
}
455-
});
436+
if (!isHandlingException()) {
437+
long timestampSeconds = getTimestampSeconds(timestampMillis);
438+
final String currentSessionId = getCurrentSessionId();
439+
if (currentSessionId == null) {
440+
Logger.getLogger().w("Tried to write a non-fatal exception while no session was open.");
441+
return;
442+
}
443+
reportingCoordinator.persistNonFatalEvent(ex, thread, currentSessionId, timestampSeconds);
444+
}
456445
}
457446

458447
void logFatalException(Thread thread, Throwable ex) {
@@ -941,7 +930,7 @@ private void writeApplicationExitInfoEventIfRelevant(String sessionId) {
941930
if (applicationExitInfoList.size() != 0) {
942931
final LogFileManager relevantSessionLogManager = new LogFileManager(fileStore, sessionId);
943932
final UserMetadata relevantUserMetadata =
944-
UserMetadata.loadFromExistingSession(sessionId, fileStore, backgroundWorker);
933+
UserMetadata.loadFromExistingSession(sessionId, fileStore, diskWriteWorker);
945934
reportingCoordinator.persistRelevantAppExitInfoEvent(
946935
sessionId, applicationExitInfoList, relevantSessionLogManager, relevantUserMetadata);
947936
} else {

‎firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCore.java

Lines changed: 21 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -96,8 +96,8 @@ public class CrashlyticsCore {
9696

9797
private final RemoteConfigDeferredProxy remoteConfigDeferredProxy;
9898

99-
private final CrashlyticsWorker commonWorker;
100-
private final CrashlyticsWorker diskWriteWorker;
99+
@VisibleForTesting final CrashlyticsWorker commonWorker;
100+
@VisibleForTesting final CrashlyticsWorker diskWriteWorker;
101101

102102
// region Constructors
103103

@@ -153,7 +153,7 @@ public boolean onPreExecute(AppData appData, SettingsProvider settingsProvider)
153153
initializationMarker = new CrashlyticsFileMarker(INITIALIZATION_MARKER_FILE_NAME, fileStore);
154154

155155
final UserMetadata userMetadata =
156-
new UserMetadata(sessionIdentifier, fileStore, commonWorker);
156+
new UserMetadata(sessionIdentifier, fileStore, diskWriteWorker);
157157
final LogFileManager logFileManager = new LogFileManager(fileStore);
158158
final StackTraceTrimmingStrategy stackTraceTrimmingStrategy =
159159
new MiddleOutFallbackStrategy(
@@ -188,7 +188,8 @@ public boolean onPreExecute(AppData appData, SettingsProvider settingsProvider)
188188
sessionReportingCoordinator,
189189
nativeComponent,
190190
analyticsEventLogger,
191-
sessionsSubscriber);
191+
sessionsSubscriber,
192+
diskWriteWorker);
192193

193194
// If the file is present at this point, then the previous run's initialization
194195
// did not complete, and we want to perform initialization synchronously this time.
@@ -321,7 +322,7 @@ public static String getVersion() {
321322
* safe to invoke this method from the main thread.
322323
*/
323324
public void logException(@NonNull Throwable throwable) {
324-
controller.writeNonFatalException(Thread.currentThread(), throwable);
325+
commonWorker.submit(() -> controller.writeNonFatalException(Thread.currentThread(), throwable));
325326
}
326327

327328
/**
@@ -336,11 +337,11 @@ public void logException(@NonNull Throwable throwable) {
336337
*/
337338
public void log(final String msg) {
338339
final long timestamp = System.currentTimeMillis() - startTime;
339-
controller.writeToLog(timestamp, msg);
340+
commonWorker.submit(() -> controller.writeToLog(timestamp, msg));
340341
}
341342

342343
public void setUserId(String identifier) {
343-
controller.setUserId(identifier);
344+
commonWorker.submit(() -> controller.setUserId(identifier));
344345
}
345346

346347
/**
@@ -353,7 +354,7 @@ public void setUserId(String identifier) {
353354
* @throws NullPointerException if key is null.
354355
*/
355356
public void setCustomKey(String key, String value) {
356-
controller.setCustomKey(key, value);
357+
commonWorker.submit(() -> controller.setCustomKey(key, value));
357358
}
358359

359360
/**
@@ -370,7 +371,7 @@ public void setCustomKey(String key, String value) {
370371
* @throws NullPointerException if any key in keysAndValues is null.
371372
*/
372373
public void setCustomKeys(Map<String, String> keysAndValues) {
373-
controller.setCustomKeys(keysAndValues);
374+
commonWorker.submit(() -> controller.setCustomKeys(keysAndValues));
374375
}
375376

376377
// endregion
@@ -390,7 +391,7 @@ public void setCustomKeys(Map<String, String> keysAndValues) {
390391
* @throws NullPointerException if key is null.
391392
*/
392393
public void setInternalKey(String key, String value) {
393-
controller.setInternalKey(key, value);
394+
commonWorker.submit(() -> controller.setInternalKey(key, value));
394395
}
395396

396397
/** Logs a fatal Throwable on the Crashlytics servers on-demand. */
@@ -399,11 +400,16 @@ public void logFatalException(Throwable throwable) {
399400
.d("Recorded on-demand fatal events: " + onDemandCounter.getRecordedOnDemandExceptions());
400401
Logger.getLogger()
401402
.d("Dropped on-demand fatal events: " + onDemandCounter.getDroppedOnDemandExceptions());
402-
controller.setInternalKey(
403-
ON_DEMAND_RECORDED_KEY, Integer.toString(onDemandCounter.getRecordedOnDemandExceptions()));
404-
controller.setInternalKey(
405-
ON_DEMAND_DROPPED_KEY, Integer.toString(onDemandCounter.getDroppedOnDemandExceptions()));
406-
controller.logFatalException(Thread.currentThread(), throwable);
403+
commonWorker.submit(
404+
() -> {
405+
controller.setInternalKey(
406+
ON_DEMAND_RECORDED_KEY,
407+
Integer.toString(onDemandCounter.getRecordedOnDemandExceptions()));
408+
controller.setInternalKey(
409+
ON_DEMAND_DROPPED_KEY,
410+
Integer.toString(onDemandCounter.getDroppedOnDemandExceptions()));
411+
controller.logFatalException(Thread.currentThread(), throwable);
412+
});
407413
}
408414

409415
// endregion

0 commit comments

Comments
 (0)