From 6e7e04e1b52b531b8ac95555c1adda90393aded0 Mon Sep 17 00:00:00 2001 From: themiswang Date: Wed, 18 Sep 2024 09:28:27 -0400 Subject: [PATCH 1/4] Crashlytics exception handler (#6267) Revert the old reversion https://github.com/firebase/firebase-android-sdk/pull/6191 merge back feature branch --------- Co-authored-by: Matthew Robertson --- firebase-crashlytics-ndk/gradle.properties | 2 +- firebase-crashlytics/CHANGELOG.md | 5 +- .../firebase-crashlytics.gradle | 1 + firebase-crashlytics/gradle.properties | 2 +- .../internal/CrashlyticsWorkerTest.java | 429 -------- .../common/CrashlyticsControllerTest.java | 87 +- .../CrashlyticsCoreInitializationTest.java | 12 +- .../internal/common/CrashlyticsCoreTest.java | 33 +- .../CrashlyticsReportDataCaptureTest.java | 20 +- .../internal/common/IdManagerTest.java | 63 +- .../SessionReportingCoordinatorTest.java | 62 +- .../concurrency/ConcurrencyTesting.java | 45 + .../concurrency/CrashlyticsTasksTest.java | 240 +++++ .../concurrency/CrashlyticsWorkerTest.java | 918 ++++++++++++++++++ .../internal/metadata/MetaDataStoreTest.java | 227 +++-- .../DefaultSettingsControllerTest.java | 40 +- .../settings/DefaultSettingsSpiCallTest.java | 8 +- .../crashlytics/CrashlyticsRegistrar.java | 12 +- .../crashlytics/FirebaseCrashlytics.java | 39 +- .../internal/CrashlyticsPreconditions.kt | 86 -- .../internal/CrashlyticsWorker.java | 128 --- .../internal/common/CommonUtils.java | 16 +- .../common/CrashlyticsBackgroundWorker.java | 166 ---- .../common/CrashlyticsController.java | 180 ++-- .../internal/common/CrashlyticsCore.java | 155 ++- .../common/CrashlyticsLifecycleEvents.java | 51 - .../common/DataCollectionArbiter.java | 10 +- .../internal/common/IdManager.java | 16 +- .../common/SessionReportingCoordinator.java | 57 +- .../crashlytics/internal/common/Utils.java | 74 +- .../concurrency/CrashlyticsTasks.java | 69 ++ .../concurrency/CrashlyticsWorker.java | 212 ++++ .../concurrency/CrashlyticsWorkers.kt | 100 ++ .../internal/concurrency/package-info.java | 18 + .../internal/metadata/UserMetadata.java | 66 +- .../internal/network/HttpGetRequest.java | 2 + .../settings/DefaultSettingsSpiCall.java | 2 + .../internal/settings/SettingsController.java | 28 +- .../CrashlyticsControllerRobolectricTest.java | 37 +- ...onReportingCoordinatorRobolectricTest.java | 14 +- 40 files changed, 2276 insertions(+), 1456 deletions(-) delete mode 100644 firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/CrashlyticsWorkerTest.java create mode 100644 firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/concurrency/ConcurrencyTesting.java create mode 100644 firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsTasksTest.java create mode 100644 firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsWorkerTest.java delete mode 100644 firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/CrashlyticsPreconditions.kt delete mode 100644 firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/CrashlyticsWorker.java delete mode 100644 firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsBackgroundWorker.java delete mode 100644 firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsLifecycleEvents.java create mode 100644 firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsTasks.java create mode 100644 firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsWorker.java create mode 100644 firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsWorkers.kt create mode 100644 firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/package-info.java diff --git a/firebase-crashlytics-ndk/gradle.properties b/firebase-crashlytics-ndk/gradle.properties index 971c9e334fe..4deda310a06 100644 --- a/firebase-crashlytics-ndk/gradle.properties +++ b/firebase-crashlytics-ndk/gradle.properties @@ -1,2 +1,2 @@ version=19.1.1 -latestReleasedVersion=19.1.0 +latestReleasedVersion=19.1.0 \ No newline at end of file diff --git a/firebase-crashlytics/CHANGELOG.md b/firebase-crashlytics/CHANGELOG.md index 30835e8fcc5..372e47310ef 100644 --- a/firebase-crashlytics/CHANGELOG.md +++ b/firebase-crashlytics/CHANGELOG.md @@ -1,5 +1,8 @@ # Unreleased - +* [fixed] Improved data consistency for rapid user actions. +* [changed] Internal changes to improve startup time. +* [changed] Internal changes to the way background tasks are scheduled. +* [changed] Migrated SDK to use standard Firebase executors. # 19.1.0 * [feature] Added the `isCrashlyticsCollectionEnabled` API to check if Crashlytics collection is diff --git a/firebase-crashlytics/firebase-crashlytics.gradle b/firebase-crashlytics/firebase-crashlytics.gradle index 3c450c0d68a..11656334809 100644 --- a/firebase-crashlytics/firebase-crashlytics.gradle +++ b/firebase-crashlytics/firebase-crashlytics.gradle @@ -98,6 +98,7 @@ dependencies { testImplementation(libs.mockito.core) testImplementation(libs.robolectric) testImplementation(libs.truth) + testImplementation(project(":integ-testing")) androidTestImplementation(libs.androidx.test.core) androidTestImplementation(libs.androidx.test.runner) diff --git a/firebase-crashlytics/gradle.properties b/firebase-crashlytics/gradle.properties index 971c9e334fe..4deda310a06 100644 --- a/firebase-crashlytics/gradle.properties +++ b/firebase-crashlytics/gradle.properties @@ -1,2 +1,2 @@ version=19.1.1 -latestReleasedVersion=19.1.0 +latestReleasedVersion=19.1.0 \ No newline at end of file diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/CrashlyticsWorkerTest.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/CrashlyticsWorkerTest.java deleted file mode 100644 index 63c9a9ce6d2..00000000000 --- a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/CrashlyticsWorkerTest.java +++ /dev/null @@ -1,429 +0,0 @@ -/* - * Copyright 2024 Google LLC - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.google.firebase.crashlytics.internal; - -import static com.google.common.truth.Truth.assertThat; -import static org.junit.Assert.assertThrows; - -import com.google.android.gms.tasks.Task; -import com.google.android.gms.tasks.Tasks; -import com.google.firebase.concurrent.TestOnlyExecutors; -import java.io.IOException; -import java.util.ArrayList; -import java.util.HashSet; -import java.util.List; -import java.util.Set; -import java.util.concurrent.CancellationException; -import java.util.concurrent.ExecutionException; -import java.util.concurrent.Executors; -import java.util.concurrent.ThreadPoolExecutor; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.TimeoutException; -import java.util.concurrent.atomic.AtomicBoolean; -import org.junit.After; -import org.junit.Before; -import org.junit.Test; - -public class CrashlyticsWorkerTest { - private CrashlyticsWorker crashlyticsWorker; - - @Before - public void setUp() { - crashlyticsWorker = new CrashlyticsWorker(TestOnlyExecutors.background()); - } - - @After - public void tearDown() throws Exception { - // Drain the worker, just in case any test cases would fail but didn't await. - crashlyticsWorker.await(); - } - - @Test - public void executesTasksOnThreadPool() throws Exception { - Set threads = new HashSet<>(); - - // Find thread names by adding the names we touch to the set. - for (int i = 0; i < 100; i++) { - crashlyticsWorker.submit(() -> threads.add(Thread.currentThread().getName())); - } - - crashlyticsWorker.await(); - - // Verify that we touched at lease some of the expected background threads. - assertThat(threads) - .containsAnyOf( - "Firebase Background Thread #0", - "Firebase Background Thread #1", - "Firebase Background Thread #2", - "Firebase Background Thread #3"); - } - - @Test - public void executesTasksInOrder() throws Exception { - List list = new ArrayList<>(); - - // Add sequential numbers to the list to validate tasks execute in order. - for (int i = 0; i < 100; i++) { - int sequential = i; - crashlyticsWorker.submit(() -> list.add(sequential)); - } - - crashlyticsWorker.await(); - - // Verify that the tasks executed in order. - assertThat(list).isInOrder(); - } - - @Test - public void executesTasksSequentially() throws Exception { - List list = new ArrayList<>(); - AtomicBoolean reentrant = new AtomicBoolean(false); - - for (int i = 0; i < 100; i++) { - int sequential = i; - crashlyticsWorker.submit( - () -> { - if (reentrant.get()) { - // Return early if two runnables ran at the same time. - return; - } - - reentrant.set(true); - // Sleep a bit to simulate some work. - sleep(5); - list.add(sequential); - reentrant.set(false); - }); - } - - crashlyticsWorker.await(); - - // Verify that all the runnable tasks executed, one at a time, and in order. - assertThat(list).hasSize(100); - assertThat(list).isInOrder(); - } - - @Test - public void submitCallableThatReturns() throws Exception { - String ender = "Remember, the enemy's gate is down."; - Task task = crashlyticsWorker.submit(() -> ender); - - String result = Tasks.await(task); - - assertThat(result).isEqualTo(ender); - } - - @Test - public void submitCallableThatReturnsNull() throws Exception { - Task task = crashlyticsWorker.submit(() -> null); - - String result = Tasks.await(task); - - assertThat(result).isNull(); - } - - @Test - public void submitCallableThatThrows() { - Task task = - crashlyticsWorker.submit( - () -> { - throw new Exception("I threw in the callable"); - }); - - ExecutionException thrown = assertThrows(ExecutionException.class, () -> Tasks.await(task)); - - assertThat(thrown).hasCauseThat().hasMessageThat().isEqualTo("I threw in the callable"); - } - - @Test - public void submitCallableThatThrowsThenReturns() throws Exception { - Task throwingTask = - crashlyticsWorker.submit( - () -> { - throw new IOException(); - }); - - assertThrows(ExecutionException.class, () -> Tasks.await(throwingTask)); - - String hiro = - "When you are wrestling for possession of a sword, the man with the handle always wins."; - Task task = crashlyticsWorker.submitTask(() -> Tasks.forResult(hiro)); - - String result = Tasks.await(task); - - assertThat(result).isEqualTo(hiro); - } - - @Test - public void submitRunnable() throws Exception { - Task task = crashlyticsWorker.submit(() -> {}); - - Void result = Tasks.await(task); - - // A Runnable does not return, so the task evaluates to null. - assertThat(result).isNull(); - } - - @Test - public void submitRunnableThatThrows() { - Task task = - crashlyticsWorker.submit( - (Runnable) - () -> { - throw new RuntimeException("I threw in the runnable"); - }); - - ExecutionException thrown = assertThrows(ExecutionException.class, () -> Tasks.await(task)); - - assertThat(thrown).hasCauseThat().hasMessageThat().isEqualTo("I threw in the runnable"); - } - - @Test - public void submitRunnableThatThrowsThenReturns() throws Exception { - Task thowingTask = - crashlyticsWorker.submit( - (Runnable) - () -> { - throw new IllegalArgumentException(); - }); - - assertThrows(ExecutionException.class, () -> Tasks.await(thowingTask)); - - Task task = crashlyticsWorker.submit(() -> {}); - - Void result = Tasks.await(task); - - assertThat(result).isNull(); - } - - @Test - public void submitTaskThatReturns() throws Exception { - String skippy = "Think of the problem as an enemy, and defeat them in detail."; - Task task = crashlyticsWorker.submitTask(() -> Tasks.forResult(skippy)); - - String result = Tasks.await(task); - - assertThat(result).isEqualTo(skippy); - } - - @Test - public void submitTaskThatReturnsNull() throws Exception { - Task task = crashlyticsWorker.submitTask(() -> Tasks.forResult(null)); - - String result = Tasks.await(task); - - assertThat(result).isNull(); - } - - @Test - public void submitTaskThatThrows() { - Task task = - crashlyticsWorker.submitTask( - () -> Tasks.forException(new Exception("Thrown from a task."))); - - ExecutionException thrown = assertThrows(ExecutionException.class, () -> Tasks.await(task)); - - assertThat(thrown).hasCauseThat().hasMessageThat().isEqualTo("Thrown from a task."); - } - - @Test - public void submitTaskThatThrowsThenReturns() throws Exception { - crashlyticsWorker.submitTask(() -> Tasks.forException(new IllegalStateException())); - Task task = crashlyticsWorker.submitTask(() -> Tasks.forResult("The Hail Mary")); - - String result = Tasks.await(task); - - assertThat(result).isEqualTo("The Hail Mary"); - } - - @Test - public void submitTaskThatCancels() { - Task task = crashlyticsWorker.submitTask(Tasks::forCanceled); - - CancellationException thrown = - assertThrows(CancellationException.class, () -> Tasks.await(task)); - - assertThat(task.isCanceled()).isTrue(); - assertThat(thrown).hasMessageThat().contains("Task is already canceled"); - } - - @Test - public void submitTaskThatCancelsThenReturns() throws Exception { - crashlyticsWorker.submitTask(Tasks::forCanceled); - Task task = crashlyticsWorker.submitTask(() -> Tasks.forResult("Flying Dutchman")); - - String result = Tasks.await(task); - - assertThat(task.isCanceled()).isFalse(); - assertThat(result).isEqualTo("Flying Dutchman"); - } - - @Test - public void submitTaskThatCancelsThenAwaitsThenReturns() throws Exception { - Task cancelled = crashlyticsWorker.submitTask(Tasks::forCanceled); - - // Await on the cancelled task to force the exception to propagate. - assertThrows(CancellationException.class, () -> Tasks.await(cancelled)); - - // Submit another task. - Task task = crashlyticsWorker.submitTask(() -> Tasks.forResult("Valkyrie")); - - String result = Tasks.await(task); - - assertThat(cancelled.isCanceled()).isTrue(); - assertThat(task.isCanceled()).isFalse(); - assertThat(result).isEqualTo("Valkyrie"); - } - - @Test - public void submitTaskThatCancelsThenAwaitsThenCallable() throws Exception { - Task cancelled = crashlyticsWorker.submitTask(Tasks::forCanceled); - - // Await on the cancelled task to force the exception to propagate. - assertThrows(CancellationException.class, () -> Tasks.await(cancelled)); - - // Submit a simple callable. - Task task = crashlyticsWorker.submit(() -> true); - - boolean result = Tasks.await(task); - - assertThat(cancelled.isCanceled()).isTrue(); - assertThat(task.isCanceled()).isFalse(); - assertThat(result).isTrue(); - } - - @Test - public void submitTaskThatCancelsThenAwaitsThenRunnable() throws Exception { - Task cancelled = crashlyticsWorker.submitTask(Tasks::forCanceled); - - // Await on the cancelled task to force the exception to propagate. - assertThrows(CancellationException.class, () -> Tasks.await(cancelled)); - - // Submit an empty runnable. - Task task = crashlyticsWorker.submit(() -> {}); - - Void result = Tasks.await(task); - - assertThat(cancelled.isCanceled()).isTrue(); - assertThat(task.isCanceled()).isFalse(); - assertThat(result).isNull(); - } - - @Test - public void submitTaskFromAnotherWorker() throws Exception { - Task otherTask = - new CrashlyticsWorker(TestOnlyExecutors.blocking()) - .submit(() -> "Dog's fine. Just sleeping."); - - // This will not use a background thread while waiting for the task on blocking thread. - Task task = crashlyticsWorker.submitTask(() -> otherTask); - - String result = Tasks.await(task); - assertThat(result).isEqualTo("Dog's fine. Just sleeping."); - } - - @Test - public void submitTaskFromAnotherWorkerThatThrows() throws Exception { - Task otherTask = - new CrashlyticsWorker(TestOnlyExecutors.blocking()) - .submitTask(() -> Tasks.forException(new IndexOutOfBoundsException())); - - // Await on the throwing task to force the exception to propagate threw the local worker. - Task task = crashlyticsWorker.submitTask(() -> otherTask); - assertThrows(ExecutionException.class, () -> Tasks.await(task)); - - // Submit another task to local worker to verify the chain did not break. - Task localTask = crashlyticsWorker.submitTask(() -> Tasks.forResult(0x5f375a86)); - - int localResult = Tasks.await(localTask); - - assertThat(otherTask.isSuccessful()).isFalse(); - assertThat(localTask.isSuccessful()).isTrue(); - assertThat(localResult).isEqualTo(0x5f375a86); - } - - @Test - public void submitTaskFromAnotherWorkerThatCancels() throws Exception { - Task otherCancelled = - new CrashlyticsWorker(TestOnlyExecutors.blocking()).submitTask(Tasks::forCanceled); - - // Await on the cancelled task to force the exception to propagate threw the local worker. - Task task = crashlyticsWorker.submitTask(() -> otherCancelled); - assertThrows(CancellationException.class, () -> Tasks.await(task)); - - // Submit another task to local worker to verify the chain did not break. - Task localTask = crashlyticsWorker.submitTask(() -> Tasks.forResult(0x5fe6eb50c7b537a9L)); - - long localResult = Tasks.await(localTask); - - assertThat(otherCancelled.isCanceled()).isTrue(); - assertThat(localTask.isCanceled()).isFalse(); - assertThat(localResult).isEqualTo(0x5fe6eb50c7b537a9L); - } - - @Test - public void submitTaskFromAnotherWorkerDoesNotUseLocalThreads() throws Exception { - // Setup a "local" worker. - ThreadPoolExecutor localExecutor = (ThreadPoolExecutor) Executors.newFixedThreadPool(4); - CrashlyticsWorker localWorker = new CrashlyticsWorker(localExecutor); - - // Use a task off crashlyticsWorker to represent an other task. - Task otherTask = - crashlyticsWorker.submit( - () -> { - sleep(30); - return localExecutor.getActiveCount(); - }); - - // No active threads yet. - assertThat(localExecutor.getActiveCount()).isEqualTo(0); - - // 1 active thread when doing a local task. - assertThat(Tasks.await(localWorker.submit(localExecutor::getActiveCount))).isEqualTo(1); - - // 0 active local threads when waiting for other task. - // Waiting for a task from another worker does not block a local thread. - assertThat(Tasks.await(localWorker.submitTask(() -> otherTask))).isEqualTo(0); - - // 1 active thread when doing a task. - assertThat(Tasks.await(localWorker.submit(localExecutor::getActiveCount))).isEqualTo(1); - - // No active threads after. - assertThat(localExecutor.getActiveCount()).isEqualTo(0); - } - - @Test - public void submitTaskWhenThreadPoolFull() { - // Fill the underlying executor thread pool. - for (int i = 0; i < 10; i++) { - crashlyticsWorker.getExecutor().execute(() -> sleep(40)); - } - - Task task = crashlyticsWorker.submitTask(() -> Tasks.forResult(42)); - - // The underlying thread pool is full with tasks that will take longer than this timeout. - assertThrows(TimeoutException.class, () -> Tasks.await(task, 30, TimeUnit.MILLISECONDS)); - } - - private static void sleep(long millis) { - try { - Thread.sleep(millis); - } catch (InterruptedException ex) { - Thread.currentThread().interrupt(); - } - } -} diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerTest.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerTest.java index 904ad8bb97b..fc46a35090c 100644 --- a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerTest.java +++ b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerTest.java @@ -34,11 +34,13 @@ import com.google.android.gms.tasks.TaskCompletionSource; import com.google.android.gms.tasks.Tasks; import com.google.firebase.FirebaseApp; +import com.google.firebase.concurrent.TestOnlyExecutors; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponent; import com.google.firebase.crashlytics.internal.CrashlyticsTestCase; 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.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; @@ -55,13 +57,15 @@ import java.util.TreeSet; import java.util.concurrent.Executor; import java.util.concurrent.TimeUnit; -import org.junit.Test; import org.mockito.ArgumentCaptor; 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 CrashlyticsWorkers crashlyticsWorkers = + new CrashlyticsWorkers(TestOnlyExecutors.background(), TestOnlyExecutors.blocking()); + private Context testContext; private IdManager idManager; private SettingsProvider testSettingsProvider; @@ -99,6 +103,12 @@ protected void setUp() throws Exception { when(testSettingsProvider.getSettingsAsync()).thenReturn(Tasks.forResult(testSettings)); } + @Override + protected void tearDown() throws Exception { + super.tearDown(); + crashlyticsWorkers.common.await(); + } + /** A convenience class for building CrashlyticsController instances for testing. */ private class ControllerBuilder { private DataCollectionArbiter dataCollectionArbiter; @@ -106,7 +116,6 @@ private class ControllerBuilder { private AnalyticsEventLogger analyticsEventLogger; private SessionReportingCoordinator sessionReportingCoordinator; - private CrashlyticsBackgroundWorker backgroundWorker; private LogFileManager logFileManager = null; private UserMetadata userMetadata = null; @@ -118,8 +127,6 @@ private class ControllerBuilder { analyticsEventLogger = mock(AnalyticsEventLogger.class); sessionReportingCoordinator = mockSessionReportingCoordinator; - - backgroundWorker = new CrashlyticsBackgroundWorker(new SameThreadExecutorService()); } ControllerBuilder setDataCollectionArbiter(DataCollectionArbiter arbiter) { @@ -168,7 +175,6 @@ public CrashlyticsController build() { final CrashlyticsController controller = new CrashlyticsController( testContext.getApplicationContext(), - backgroundWorker, idManager, dataCollectionArbiter, testFileStore, @@ -179,7 +185,8 @@ public CrashlyticsController build() { sessionReportingCoordinator, nativeComponent, analyticsEventLogger, - mock(CrashlyticsAppQualitySessionsSubscriber.class)); + mock(CrashlyticsAppQualitySessionsSubscriber.class), + crashlyticsWorkers); return controller; } } @@ -206,7 +213,10 @@ public void testWriteNonFatal_callsSessionReportingCoordinatorPersistNonFatal() .thenReturn(new TreeSet<>(Collections.singleton(sessionId))); controller.writeNonFatalException(thread, nonFatal); - controller.doCloseSessions(testSettingsProvider); + crashlyticsWorkers.common.submit(() -> controller.doCloseSessions(testSettingsProvider)); + + crashlyticsWorkers.common.await(); + crashlyticsWorkers.diskWrite.await(); verify(mockSessionReportingCoordinator) .persistNonFatalEvent(eq(nonFatal), eq(thread), eq(sessionId), anyLong()); @@ -228,9 +238,8 @@ public void testFatalException_callsSessionReportingCoordinatorPersistFatal() th .persistFatalEvent(eq(fatal), eq(thread), eq(sessionId), anyLong()); } - @Test @SdkSuppress(minSdkVersion = 30) // ApplicationExitInfo - public void testOnDemandFatal_callLogFatalException() { + public void testOnDemandFatal_callLogFatalException() throws Exception { Thread thread = Thread.currentThread(); Exception fatal = new RuntimeException("Fatal"); Thread.UncaughtExceptionHandler exceptionHandler = mock(Thread.UncaughtExceptionHandler.class); @@ -246,6 +255,8 @@ public void testOnDemandFatal_callLogFatalException() { controller.enableExceptionHandling(SESSION_ID, exceptionHandler, testSettingsProvider); controller.logFatalException(thread, fatal); + crashlyticsWorkers.common.await(); + verify(mockUserMetadata).setNewSession(not(eq(SESSION_ID))); } @@ -323,7 +334,9 @@ public File getOsFile() { final CrashlyticsController controller = builder().setNativeComponent(mockNativeComponent).setLogFileManager(logFileManager).build(); - controller.finalizeSessions(testSettingsProvider); + crashlyticsWorkers.common.submit(() -> controller.finalizeSessions(testSettingsProvider)); + crashlyticsWorkers.common.await(); + verify(mockSessionReportingCoordinator) .finalizeSessionWithNativeEvent(eq(previousSessionId), any(), any()); verify(mockSessionReportingCoordinator, never()) @@ -331,9 +344,10 @@ public File getOsFile() { } @SdkSuppress(minSdkVersion = 30) // ApplicationExitInfo - public void testMissingNativeComponentCausesNoReports() { + public void testMissingNativeComponentCausesNoReports() throws Exception { final CrashlyticsController controller = createController(); - controller.finalizeSessions(testSettingsProvider); + crashlyticsWorkers.common.submit(() -> controller.finalizeSessions(testSettingsProvider)); + crashlyticsWorkers.common.await(); List sessions = testFileStore.getAllOpenSessionIds(); for (String sessionId : sessions) { @@ -363,13 +377,15 @@ public void testLoggedExceptionsAfterCrashOk() { */ // FIXME: Validate this test works as intended @SdkSuppress(minSdkVersion = 30) // ApplicationExitInfo - public void testLogStringAfterCrashOk() { + public void testLogStringAfterCrashOk() throws Exception { final CrashlyticsController controller = builder().build(); controller.handleUncaughtException( testSettingsProvider, Thread.currentThread(), new RuntimeException()); // This should not throw. - controller.writeToLog(System.currentTimeMillis(), "Hi"); + crashlyticsWorkers.diskWrite.submit( + () -> controller.writeToLog(System.currentTimeMillis(), "Hi")); + crashlyticsWorkers.diskWrite.await(); } /** @@ -384,7 +400,8 @@ public void testFinalizeSessionAfterCrashOk() throws Exception { testSettingsProvider, Thread.currentThread(), new RuntimeException()); // This should not throw. - controller.finalizeSessions(testSettingsProvider); + crashlyticsWorkers.common.submit(() -> controller.finalizeSessions(testSettingsProvider)); + crashlyticsWorkers.common.await(); } @SdkSuppress(minSdkVersion = 30) // ApplicationExitInfo @@ -393,9 +410,9 @@ public void testUploadWithNoReports() throws Exception { final CrashlyticsController controller = createController(); - Task task = controller.submitAllReports(testSettingsProvider.getSettingsAsync()); + controller.submitAllReports(testSettingsProvider.getSettingsAsync()); - await(task); + crashlyticsWorkers.common.await(); verify(mockSessionReportingCoordinator).hasReportsToSend(); verifyNoMoreInteractions(mockSessionReportingCoordinator); @@ -409,9 +426,9 @@ public void testUploadWithDataCollectionAlwaysEnabled() throws Exception { final CrashlyticsController controller = createController(); - final Task task = controller.submitAllReports(testSettingsProvider.getSettingsAsync()); + controller.submitAllReports(testSettingsProvider.getSettingsAsync()); - await(task); + crashlyticsWorkers.common.await(); verify(mockSessionReportingCoordinator).hasReportsToSend(); verify(mockDataCollectionArbiter).isAutomaticDataCollectionEnabled(); @@ -427,7 +444,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().getTask()); when(arbiter.waitForAutomaticDataCollectionEnabled()) .thenReturn(new TaskCompletionSource().getTask()); @@ -435,15 +452,13 @@ public void testUploadDisabledThenOptIn() throws Exception { final ControllerBuilder builder = builder(); builder.setDataCollectionArbiter(arbiter); final CrashlyticsController controller = builder.build(); - - final Task task = controller.submitAllReports(testSettingsProvider.getSettingsAsync()); - + controller.submitAllReports(testSettingsProvider.getSettingsAsync()); verify(arbiter).isAutomaticDataCollectionEnabled(); verify(mockSessionReportingCoordinator).hasReportsToSend(); verify(mockSessionReportingCoordinator, never()).sendReports(any(Executor.class)); await(controller.sendUnsentReports()); - await(task); + crashlyticsWorkers.common.await(); verify(mockSessionReportingCoordinator).sendReports(any(Executor.class)); verifyNoMoreInteractions(mockSessionReportingCoordinator); @@ -464,14 +479,16 @@ public void testUploadDisabledThenOptOut() throws Exception { builder.setDataCollectionArbiter(arbiter); final CrashlyticsController controller = builder.build(); - final Task task = controller.submitAllReports(testSettingsProvider.getSettingsAsync()); + controller.submitAllReports(testSettingsProvider.getSettingsAsync()); verify(arbiter).isAutomaticDataCollectionEnabled(); verify(mockSessionReportingCoordinator).hasReportsToSend(); verify(mockSessionReportingCoordinator, never()).removeAllReports(); await(controller.deleteUnsentReports()); - await(task); + crashlyticsWorkers.common.await(); + + crashlyticsWorkers.diskWrite.await(); verify(mockSessionReportingCoordinator).removeAllReports(); verifyNoMoreInteractions(mockSessionReportingCoordinator); @@ -506,7 +523,7 @@ public void testUploadDisabledThenEnabled() throws Exception { builder.setDataCollectionArbiter(arbiter); final CrashlyticsController controller = builder.build(); - final Task task = controller.submitAllReports(testSettingsProvider.getSettingsAsync()); + controller.submitAllReports(testSettingsProvider.getSettingsAsync()); verify(mockSessionReportingCoordinator).hasReportsToSend(); verify(mockSessionReportingCoordinator, never()).sendReports(any(Executor.class)); @@ -528,14 +545,14 @@ public void testUploadDisabledThenEnabled() throws Exception { when(app.isDataCollectionDefaultEnabled()).thenReturn(false); assertFalse(arbiter.isAutomaticDataCollectionEnabled()); - await(task); + crashlyticsWorkers.common.await(); verify(mockSessionReportingCoordinator).sendReports(any(Executor.class)); verifyNoMoreInteractions(mockSessionReportingCoordinator); } @SdkSuppress(minSdkVersion = 30) // ApplicationExitInfo - public void testFatalEvent_sendsAppExceptionEvent() { + public void testFatalEvent_sendsAppExceptionEvent() throws Exception { final String sessionId = "sessionId"; final LogFileManager logFileManager = new LogFileManager(testFileStore); final AnalyticsEventLogger mockFirebaseAnalyticsLogger = mock(AnalyticsEventLogger.class); @@ -548,10 +565,14 @@ public void testFatalEvent_sendsAppExceptionEvent() { when(mockSessionReportingCoordinator.listSortedOpenSessionIds()) .thenReturn(new TreeSet<>(Collections.singleton(sessionId))); - controller.openSession(SESSION_ID); - controller.handleUncaughtException( - testSettingsProvider, Thread.currentThread(), new RuntimeException("Fatal")); - controller.finalizeSessions(testSettingsProvider); + crashlyticsWorkers.common.submit( + () -> { + controller.openSession(SESSION_ID); + controller.handleUncaughtException( + testSettingsProvider, Thread.currentThread(), new RuntimeException("Fatal")); + controller.finalizeSessions(testSettingsProvider); + }); + crashlyticsWorkers.common.await(); assertFirebaseAnalyticsCrashEvent(mockFirebaseAnalyticsLogger); } diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreInitializationTest.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreInitializationTest.java index 9b579a66a62..1bdb3fc183d 100644 --- a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreInitializationTest.java +++ b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreInitializationTest.java @@ -27,6 +27,7 @@ import com.google.android.gms.tasks.Tasks; import com.google.firebase.FirebaseApp; import com.google.firebase.FirebaseOptions; +import com.google.firebase.concurrent.TestOnlyExecutors; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponent; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponentDeferredProxy; import com.google.firebase.crashlytics.internal.CrashlyticsTestCase; @@ -34,6 +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.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; @@ -44,7 +46,6 @@ import java.io.IOException; import java.util.ArrayList; import java.util.List; -import java.util.concurrent.ExecutorService; public class CrashlyticsCoreInitializationTest extends CrashlyticsTestCase { @@ -89,8 +90,8 @@ private static final class CoreBuilder { private IdManager idManager; private CrashlyticsNativeComponent nativeComponent; private DataCollectionArbiter arbiter; - private ExecutorService crashHandlerExecutor; private FileStore fileStore; + private CrashlyticsWorkers crashlyticsWorkers; public CoreBuilder(Context context, FirebaseOptions firebaseOptions) { app = mock(FirebaseApp.class); @@ -119,7 +120,8 @@ public void whenAvailable( arbiter = mock(DataCollectionArbiter.class); when(arbiter.isAutomaticDataCollectionEnabled()).thenReturn(true); - crashHandlerExecutor = new SameThreadExecutorService(); + crashlyticsWorkers = + new CrashlyticsWorkers(TestOnlyExecutors.background(), TestOnlyExecutors.blocking()); fileStore = new FileStore(context); } @@ -142,9 +144,9 @@ public CrashlyticsCore build() { new DisabledBreadcrumbSource(), new UnavailableAnalyticsEventLogger(), fileStore, - crashHandlerExecutor, mock(CrashlyticsAppQualitySessionsSubscriber.class), - mock(RemoteConfigDeferredProxy.class)); + mock(RemoteConfigDeferredProxy.class), + crashlyticsWorkers); } } diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreTest.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreTest.java index 17064da9f37..33046b3f74c 100644 --- a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreTest.java +++ b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCoreTest.java @@ -22,12 +22,11 @@ import android.content.Context; import android.text.TextUtils; import androidx.annotation.NonNull; -import androidx.annotation.Nullable; -import com.google.android.gms.tasks.SuccessContinuation; import com.google.android.gms.tasks.Task; import com.google.android.gms.tasks.Tasks; import com.google.firebase.FirebaseApp; import com.google.firebase.FirebaseOptions; +import com.google.firebase.concurrent.TestOnlyExecutors; import com.google.firebase.crashlytics.BuildConfig; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponent; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponentDeferredProxy; @@ -38,6 +37,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.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; @@ -68,6 +68,8 @@ public void whenAvailable( private CrashlyticsCore crashlyticsCore; private BreadcrumbSource mockBreadcrumbSource; + private static final CrashlyticsWorkers crashlyticsWorkers = + new CrashlyticsWorkers(TestOnlyExecutors.background(), TestOnlyExecutors.blocking()); @Override protected void setUp() throws Exception { @@ -91,7 +93,7 @@ public void testCustomAttributes() throws Exception { final String id = "id012345"; crashlyticsCore.setUserId(id); - + crashlyticsWorkers.common.await(); assertEquals(id, metadata.getUserId()); final StringBuffer idBuffer = new StringBuffer(id); @@ -102,11 +104,13 @@ public void testCustomAttributes() throws Exception { final String superLongId = longId + "more chars"; crashlyticsCore.setUserId(superLongId); + crashlyticsWorkers.common.await(); assertEquals(longId, metadata.getUserId()); final String key1 = "key1"; final String value1 = "value1"; crashlyticsCore.setCustomKey(key1, value1); + crashlyticsWorkers.common.await(); assertEquals(value1, metadata.getCustomKeys().get(key1)); // Adding an existing key with the same value should return false @@ -120,6 +124,7 @@ public void testCustomAttributes() throws Exception { // test truncation of custom keys and attributes crashlyticsCore.setCustomKey(superLongId, superLongValue); + crashlyticsWorkers.common.await(); assertNull(metadata.getCustomKeys().get(superLongId)); assertEquals(longValue, metadata.getCustomKeys().get(longId)); @@ -128,23 +133,28 @@ public void testCustomAttributes() throws Exception { final String key = "key" + i; final String value = "value" + i; crashlyticsCore.setCustomKey(key, value); + 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"); + crashlyticsWorkers.common.await(); assertFalse(metadata.getCustomKeys().containsKey(key)); // should be able to update existing keys crashlyticsCore.setCustomKey(key1, longValue); + 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); + crashlyticsWorkers.common.await(); assertEquals("", metadata.getCustomKeys().get(key1)); // keys and values are trimmed. crashlyticsCore.setCustomKey(" " + key1 + " ", " " + longValue + " "); + crashlyticsWorkers.common.await(); assertTrue(metadata.getCustomKeys().containsKey(key1)); assertEquals(longValue, metadata.getCustomKeys().get(key1)); } @@ -195,6 +205,7 @@ public void testBulkCustomKeys() throws Exception { keysAndValues.put(intKey, String.valueOf(intValue)); crashlyticsCore.setCustomKeys(keysAndValues); + crashlyticsWorkers.common.await(); assertEquals(stringValue, metadata.getCustomKeys().get(stringKey)); assertEquals(trimmedValue, metadata.getCustomKeys().get(trimmedKey)); @@ -215,6 +226,7 @@ public void testBulkCustomKeys() throws Exception { addlKeysAndValues.put(key, value); } crashlyticsCore.setCustomKeys(addlKeysAndValues); + crashlyticsWorkers.common.await(); // Ensure all keys have been set assertEquals(UserMetadata.MAX_ATTRIBUTES, metadata.getCustomKeys().size(), DELTA); @@ -232,6 +244,7 @@ public void testBulkCustomKeys() throws Exception { extraKeysAndValues.put(key, value); } crashlyticsCore.setCustomKeys(extraKeysAndValues); + crashlyticsWorkers.common.await(); // Make sure the extra keys were not added for (int i = UserMetadata.MAX_ATTRIBUTES; i < UserMetadata.MAX_ATTRIBUTES + 10; ++i) { @@ -257,6 +270,7 @@ public void testBulkCustomKeys() throws Exception { updatedKeysAndValues.put(intKey, String.valueOf(updatedIntValue)); crashlyticsCore.setCustomKeys(updatedKeysAndValues); + crashlyticsWorkers.common.await(); assertEquals(updatedStringValue, metadata.getCustomKeys().get(stringKey)); assertFalse(Boolean.parseBoolean(metadata.getCustomKeys().get(booleanKey))); @@ -363,14 +377,7 @@ private Task startCoreAsync(CrashlyticsCore crashlyticsCore) { return crashlyticsCore .doBackgroundInitializationAsync(mockSettingsController) - .onSuccessTask( - new SuccessContinuation() { - @NonNull - @Override - public Task then(@Nullable Void aVoid) throws Exception { - return Tasks.forResult(crashlyticsCore); - } - }); + .onSuccessTask(unused -> Tasks.forResult(crashlyticsCore)); } /** Helper class for building CrashlyticsCore instances. */ @@ -427,9 +434,9 @@ CrashlyticsCore build(Context context) { breadcrumbSource, new UnavailableAnalyticsEventLogger(), new FileStore(context), - new SameThreadExecutorService(), mock(CrashlyticsAppQualitySessionsSubscriber.class), - mock(RemoteConfigDeferredProxy.class)); + mock(RemoteConfigDeferredProxy.class), + crashlyticsWorkers); return crashlyticsCore; } } diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsReportDataCaptureTest.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsReportDataCaptureTest.java index 80904908632..56eae6b3f42 100644 --- a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsReportDataCaptureTest.java +++ b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/CrashlyticsReportDataCaptureTest.java @@ -37,8 +37,11 @@ import android.os.BatteryManager; import androidx.test.core.app.ApplicationProvider; import androidx.test.ext.junit.runners.AndroidJUnit4; +import com.google.android.gms.tasks.Task; import com.google.android.gms.tasks.Tasks; +import com.google.firebase.concurrent.TestOnlyExecutors; import com.google.firebase.crashlytics.internal.DevelopmentPlatformProvider; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers; import com.google.firebase.crashlytics.internal.model.CrashlyticsReport; import com.google.firebase.crashlytics.internal.model.CrashlyticsReport.Session.Event.Application.Execution; import com.google.firebase.crashlytics.internal.settings.Settings; @@ -66,6 +69,8 @@ public class CrashlyticsReportDataCaptureTest { private int eventThreadImportance; private int maxChainedExceptions; private SettingsProvider testSettingsProvider; + private final CrashlyticsWorkers crashlyticsWorkers = + new CrashlyticsWorkers(TestOnlyExecutors.background(), TestOnlyExecutors.blocking()); @Mock private DevelopmentPlatformProvider developmentPlatformProvider; @@ -118,7 +123,9 @@ public void testCaptureReport_containsUnityVersionInDeveloperPlatformFieldsWhenA .thenReturn(expectedUnityVersion); initDataCapture(); - final CrashlyticsReport report = dataCapture.captureReportData("sessionId", 0); + Task task = + crashlyticsWorkers.common.submit(() -> dataCapture.captureReportData("sessionId", 0)); + CrashlyticsReport report = Tasks.await(task); assertNotNull(report.getSession()); assertEquals(UNITY_PLATFORM, report.getSession().getApp().getDevelopmentPlatform()); @@ -132,7 +139,9 @@ public void testCaptureReport_containsNoDeveloperPlatformFieldsWhenUnityIsMissin when(developmentPlatformProvider.getDevelopmentPlatform()).thenReturn(null); initDataCapture(); - final CrashlyticsReport report = dataCapture.captureReportData("sessionId", 0); + Task task = + crashlyticsWorkers.common.submit(() -> dataCapture.captureReportData("sessionId", 0)); + CrashlyticsReport report = Tasks.await(task); assertNotNull(report.getSession()); assertNull(report.getSession().getApp().getDevelopmentPlatform()); @@ -307,10 +316,13 @@ public void testCaptureAnrEvent_noBuildIdInAppData_buildIdsNull() throws Excepti } @Test - public void testCaptureReportSessionFields() { + public void testCaptureReportSessionFields() throws Exception { final String sessionId = "sessionId"; final long timestamp = System.currentTimeMillis(); - final CrashlyticsReport report = dataCapture.captureReportData(sessionId, timestamp); + + Task task = + crashlyticsWorkers.common.submit(() -> dataCapture.captureReportData(sessionId, timestamp)); + CrashlyticsReport report = Tasks.await(task); assertEquals(sessionId, report.getSession().getIdentifier()); assertEquals(timestamp, report.getSession().getStartedAt()); diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/IdManagerTest.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/IdManagerTest.java index 93a3e334fe3..d51a07fbf7f 100644 --- a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/IdManagerTest.java +++ b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/IdManagerTest.java @@ -21,14 +21,20 @@ import android.content.SharedPreferences; import com.google.android.gms.tasks.Tasks; +import com.google.firebase.concurrent.TestOnlyExecutors; import com.google.firebase.crashlytics.internal.CrashlyticsTestCase; +import com.google.firebase.crashlytics.internal.common.InstallIdProvider.InstallIds; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers; import com.google.firebase.installations.FirebaseInstallationsApi; import java.util.concurrent.TimeoutException; public class IdManagerTest extends CrashlyticsTestCase { - SharedPreferences prefs; - SharedPreferences legacyPrefs; + private SharedPreferences prefs; + private SharedPreferences legacyPrefs; + + private final CrashlyticsWorkers crashlyticsWorkers = + new CrashlyticsWorkers(TestOnlyExecutors.background(), TestOnlyExecutors.blocking()); @Override public void setUp() throws Exception { @@ -66,10 +72,10 @@ private IdManager createIdManager(String instanceId, DataCollectionArbiter arbit return new IdManager(getContext(), getContext().getPackageName(), iid, arbiter); } - public void testCreateUUID() { + public void testCreateUUID() throws Exception { final String fid = "test_fid"; final IdManager idManager = createIdManager(fid, MOCK_ARBITER_ENABLED); - final String installId = idManager.getInstallIds().getCrashlyticsInstallId(); + final String installId = getInstallIds(idManager).getCrashlyticsInstallId(); assertNotNull(installId); assertEquals(installId, prefs.getString(IdManager.PREFKEY_INSTALLATION_UUID, null)); @@ -77,10 +83,10 @@ public void testCreateUUID() { assertEquals(fid, prefs.getString(IdManager.PREFKEY_FIREBASE_IID, null)); // subsequent calls should return the same id - assertEquals(installId, idManager.getInstallIds().getCrashlyticsInstallId()); + assertEquals(installId, getInstallIds(idManager).getCrashlyticsInstallId()); } - public void testGetIdExceptionalCase_doesNotRotateInstallId() { + public void testGetIdExceptionalCase_doesNotRotateInstallId() throws Exception { FirebaseInstallationsApi fis = mock(FirebaseInstallationsApi.class); final String expectedInstallId = "expectedInstallId"; when(fis.getId()) @@ -93,12 +99,12 @@ public void testGetIdExceptionalCase_doesNotRotateInstallId() { final IdManager idManager = new IdManager(getContext(), getContext().getPackageName(), fis, MOCK_ARBITER_ENABLED); - final String actualInstallId = idManager.getInstallIds().getCrashlyticsInstallId(); + final String actualInstallId = getInstallIds(idManager).getCrashlyticsInstallId(); assertNotNull(actualInstallId); assertEquals(expectedInstallId, actualInstallId); } - public void testInstanceIdChanges_dataCollectionEnabled() { + public void testInstanceIdChanges_dataCollectionEnabled() throws Exception { // Set up the initial state with a valid iid and uuid. final String oldUuid = "old_uuid"; final String newFid = "new_test_fid"; @@ -111,7 +117,7 @@ public void testInstanceIdChanges_dataCollectionEnabled() { // Initialize the manager with a different FID. IdManager idManager = createIdManager(newFid, MOCK_ARBITER_ENABLED); - String installId = idManager.getInstallIds().getCrashlyticsInstallId(); + String installId = getInstallIds(idManager).getCrashlyticsInstallId(); assertNotNull(installId); assertFalse(installId.equals(oldUuid)); @@ -119,10 +125,10 @@ public void testInstanceIdChanges_dataCollectionEnabled() { assertEquals(newFid, prefs.getString(IdManager.PREFKEY_FIREBASE_IID, null)); // subsequent calls should return the same id - assertEquals(installId, idManager.getInstallIds().getCrashlyticsInstallId()); + assertEquals(installId, getInstallIds(idManager).getCrashlyticsInstallId()); } - void validateInstanceIdDoesntChange(boolean dataCollectionEnabled) { + void validateInstanceIdDoesntChange(boolean dataCollectionEnabled) throws Exception { final String oldUuid = "test_uuid"; final String fid = dataCollectionEnabled ? "test_fid" : IdManager.createSyntheticFid(); // Set up the initial state with a valid iid and uuid. @@ -136,7 +142,7 @@ void validateInstanceIdDoesntChange(boolean dataCollectionEnabled) { IdManager idManager = createIdManager(fid, dataCollectionEnabled ? MOCK_ARBITER_ENABLED : MOCK_ARBITER_DISABLED); - String installId = idManager.getInstallIds().getCrashlyticsInstallId(); + String installId = getInstallIds(idManager).getCrashlyticsInstallId(); assertNotNull(installId); // Test that the UUID didn't change. @@ -146,18 +152,18 @@ void validateInstanceIdDoesntChange(boolean dataCollectionEnabled) { assertEquals(fid, prefs.getString(IdManager.PREFKEY_FIREBASE_IID, null)); // subsequent calls should return the same id - assertEquals(oldUuid, idManager.getInstallIds().getCrashlyticsInstallId()); + assertEquals(oldUuid, getInstallIds(idManager).getCrashlyticsInstallId()); } - public void testInstanceIdDoesntChange_dataCollectionEnabled() { + public void testInstanceIdDoesntChange_dataCollectionEnabled() throws Exception { validateInstanceIdDoesntChange(/* dataCollectionEnabled= */ true); } - public void testInstanceIdDoesntChange_dataCollectionDisabled() { + public void testInstanceIdDoesntChange_dataCollectionDisabled() throws Exception { validateInstanceIdDoesntChange(/* dataCollectionEnabled= */ false); } - public void testInstanceIdRotatesWithDataCollectionFlag() { + public void testInstanceIdRotatesWithDataCollectionFlag() throws Exception { final String originalUuid = "test_uuid"; final String originalFid = "test_fid"; // Set up the initial state with a valid iid and uuid. @@ -169,26 +175,25 @@ public void testInstanceIdRotatesWithDataCollectionFlag() { // Initialize the manager with the same FID. IdManager idManager = createIdManager(originalFid, MOCK_ARBITER_ENABLED); - String firstUuid = idManager.getInstallIds().getCrashlyticsInstallId(); + String firstUuid = getInstallIds(idManager).getCrashlyticsInstallId(); assertNotNull(firstUuid); assertEquals(originalUuid, firstUuid); // subsequent calls should return the same id - assertEquals(firstUuid, idManager.getInstallIds().getCrashlyticsInstallId()); + assertEquals(firstUuid, getInstallIds(idManager).getCrashlyticsInstallId()); assertEquals( firstUuid, - createIdManager(originalFid, MOCK_ARBITER_ENABLED) - .getInstallIds() + getInstallIds(createIdManager(originalFid, MOCK_ARBITER_ENABLED)) .getCrashlyticsInstallId()); // Disable data collection manager and confirm we get a different id idManager = createIdManager(originalFid, MOCK_ARBITER_DISABLED); - String secondUuid = idManager.getInstallIds().getCrashlyticsInstallId(); + String secondUuid = getInstallIds(idManager).getCrashlyticsInstallId(); assertNotSame(secondUuid, firstUuid); - assertEquals(secondUuid, idManager.getInstallIds().getCrashlyticsInstallId()); + assertEquals(secondUuid, getInstallIds(idManager).getCrashlyticsInstallId()); assertEquals( secondUuid, - createIdManager(null, MOCK_ARBITER_DISABLED).getInstallIds().getCrashlyticsInstallId()); + getInstallIds(createIdManager(null, MOCK_ARBITER_DISABLED)).getCrashlyticsInstallId()); // Check that we cached an synthetic FID final SharedPreferences prefs = CommonUtils.getSharedPrefs(getContext()); String cachedFid = prefs.getString(IdManager.PREFKEY_FIREBASE_IID, null); @@ -196,17 +201,21 @@ public void testInstanceIdRotatesWithDataCollectionFlag() { // re-enable data collection idManager = createIdManager(originalFid, MOCK_ARBITER_ENABLED); - String thirdUuid = idManager.getInstallIds().getCrashlyticsInstallId(); + String thirdUuid = getInstallIds(idManager).getCrashlyticsInstallId(); assertNotSame(thirdUuid, firstUuid); assertNotSame(thirdUuid, secondUuid); - assertEquals(thirdUuid, idManager.getInstallIds().getCrashlyticsInstallId()); + assertEquals(thirdUuid, getInstallIds(idManager).getCrashlyticsInstallId()); assertEquals( thirdUuid, - createIdManager(originalFid, MOCK_ARBITER_ENABLED) - .getInstallIds() + getInstallIds(createIdManager(originalFid, MOCK_ARBITER_ENABLED)) .getCrashlyticsInstallId()); // The cached ID should be back to the original cachedFid = prefs.getString(IdManager.PREFKEY_FIREBASE_IID, null); assertEquals(cachedFid, originalFid); } + + /** Get the install ids on the common worker. */ + private InstallIds getInstallIds(IdManager idManager) throws Exception { + return Tasks.await(crashlyticsWorkers.common.submit(idManager::getInstallIds)); + } } diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinatorTest.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinatorTest.java index 4aeb055eda5..4c42bdc8069 100644 --- a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinatorTest.java +++ b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinatorTest.java @@ -33,6 +33,8 @@ import com.google.android.gms.tasks.Task; import com.google.android.gms.tasks.Tasks; +import com.google.firebase.concurrent.TestOnlyExecutors; +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; @@ -69,6 +71,9 @@ public class SessionReportingCoordinatorTest { private SessionReportingCoordinator reportingCoordinator; + private CrashlyticsWorkers crashlyticsWorkers = + new CrashlyticsWorkers(TestOnlyExecutors.background(), TestOnlyExecutors.blocking()); + @Before public void setUp() { MockitoAnnotations.initMocks(this); @@ -80,7 +85,8 @@ public void setUp() { reportSender, logFileManager, reportMetadata, - idManager); + idManager, + crashlyticsWorkers); } @Test @@ -116,7 +122,8 @@ public void testFatalEvent_persistsHighPriorityEventWithAllThreadsForSessionId() } @Test - public void testNonFatalEvent_persistsNormalPriorityEventWithoutAllThreadsForSessionId() { + public void testNonFatalEvent_persistsNormalPriorityEventWithoutAllThreadsForSessionId() + throws Exception { final String eventType = "error"; final String sessionId = "testSessionId"; final long timestamp = System.currentTimeMillis(); @@ -126,6 +133,8 @@ public void testNonFatalEvent_persistsNormalPriorityEventWithoutAllThreadsForSes reportingCoordinator.onBeginSession(sessionId, timestamp); reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp); + crashlyticsWorkers.diskWrite.await(); + final boolean expectedAllThreads = false; final boolean expectedHighPriority = false; @@ -136,7 +145,7 @@ public void testNonFatalEvent_persistsNormalPriorityEventWithoutAllThreadsForSes } @Test - public void testNonFatalEvent_addsLogsToEvent() { + public void testNonFatalEvent_addsLogsToEvent() throws Exception { long timestamp = System.currentTimeMillis(); mockEventInteractions(); @@ -149,6 +158,8 @@ public void testNonFatalEvent_addsLogsToEvent() { reportingCoordinator.onBeginSession(sessionId, timestamp); reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp); + crashlyticsWorkers.diskWrite.await(); + verify(mockEventBuilder) .setLog(CrashlyticsReport.Session.Event.Log.builder().setContent(testLog).build()); verify(mockEventBuilder).build(); @@ -156,7 +167,7 @@ public void testNonFatalEvent_addsLogsToEvent() { } @Test - public void testNonFatalEvent_addsNoLogsToEventWhenNoneAvailable() { + public void testNonFatalEvent_addsNoLogsToEventWhenNoneAvailable() throws Exception { long timestamp = System.currentTimeMillis(); mockEventInteractions(); @@ -168,6 +179,8 @@ public void testNonFatalEvent_addsNoLogsToEventWhenNoneAvailable() { reportingCoordinator.onBeginSession(sessionId, timestamp); reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp); + crashlyticsWorkers.diskWrite.await(); + verify(mockEventBuilder, never()).setLog(any(CrashlyticsReport.Session.Event.Log.class)); verify(mockEventBuilder).build(); verify(logFileManager, never()).clearLog(); @@ -212,7 +225,7 @@ public void testFatalEvent_addsNoLogsToEventWhenNoneAvailable() { } @Test - public void testNonFatalEvent_addsSortedKeysToEvent() { + public void testNonFatalEvent_addsSortedKeysToEvent() throws Exception { final long timestamp = System.currentTimeMillis(); mockEventInteractions(); @@ -243,6 +256,8 @@ public void testNonFatalEvent_addsSortedKeysToEvent() { reportingCoordinator.onBeginSession(sessionId, timestamp); reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp); + crashlyticsWorkers.diskWrite.await(); + verify(mockEventAppBuilder).setCustomAttributes(expectedCustomAttributes); verify(mockEventAppBuilder).setInternalKeys(expectedCustomAttributes); verify(mockEventAppBuilder).build(); @@ -252,7 +267,7 @@ public void testNonFatalEvent_addsSortedKeysToEvent() { } @Test - public void testNonFatalEvent_addsNoKeysToEventWhenNoneAvailable() { + public void testNonFatalEvent_addsNoKeysToEventWhenNoneAvailable() throws Exception { final long timestamp = System.currentTimeMillis(); mockEventInteractions(); @@ -266,6 +281,8 @@ public void testNonFatalEvent_addsNoKeysToEventWhenNoneAvailable() { reportingCoordinator.onBeginSession(sessionId, timestamp); reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp); + crashlyticsWorkers.diskWrite.await(); + verify(mockEventAppBuilder, never()).setCustomAttributes(anyList()); verify(mockEventAppBuilder, never()).build(); verify(mockEventBuilder, never()).setApp(mockEventApp); @@ -274,7 +291,7 @@ public void testNonFatalEvent_addsNoKeysToEventWhenNoneAvailable() { } @Test - public void testNonFatalEvent_addRolloutsEvent() { + public void testNonFatalEvent_addRolloutsEvent() throws Exception { long timestamp = System.currentTimeMillis(); String sessionId = "testSessionId"; mockEventInteractions(); @@ -287,6 +304,8 @@ public void testNonFatalEvent_addRolloutsEvent() { reportingCoordinator.onBeginSession(sessionId, timestamp); reportingCoordinator.persistNonFatalEvent(mockException, mockThread, sessionId, timestamp); + crashlyticsWorkers.diskWrite.await(); + verify(mockEventAppBuilder, never()).setCustomAttributes(anyList()); verify(mockEventAppBuilder, never()).build(); verify(mockEventBuilder, never()).setApp(mockEventApp); @@ -417,35 +436,6 @@ public void testFatalEvent_addRolloutsToEvent() { verify(mockEventBuilder, times(2)).build(); } - @Test - public void onLog_writesToLogFileManager() { - long timestamp = System.currentTimeMillis(); - String log = "this is a log"; - - reportingCoordinator.onLog(timestamp, log); - - verify(logFileManager).writeToLog(timestamp, log); - } - - @Test - public void onCustomKey_writesToReportMetadata() { - final String key = "key"; - final String value = "value"; - - reportingCoordinator.onCustomKey(key, value); - - verify(reportMetadata).setCustomKey(key, value); - } - - @Test - public void onUserId_writesUserToReportMetadata() { - final String userId = "testUser"; - - reportingCoordinator.onUserId(userId); - - verify(reportMetadata).setUserId(userId); - } - @Test public void testFinalizeSessionWithNativeEvent_createsCrashlyticsReportWithNativePayload() { byte[] testBytes = {0, 2, 20, 10}; diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/concurrency/ConcurrencyTesting.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/concurrency/ConcurrencyTesting.java new file mode 100644 index 00000000000..bb086f920cd --- /dev/null +++ b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/concurrency/ConcurrencyTesting.java @@ -0,0 +1,45 @@ +/* + * Copyright 2024 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.firebase.crashlytics.internal.concurrency; + +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; + +/** Convenience methods for use in Crashlytics concurrency tests. */ +class ConcurrencyTesting { + + /** Returns the current thread's name. */ + static String getThreadName() { + return Thread.currentThread().getName(); + } + + /** Creates a simple executor that runs on a single named thread. */ + static ExecutorService newNamedSingleThreadExecutor(String name) { + return Executors.newSingleThreadExecutor(runnable -> new Thread(runnable, name)); + } + + /** Convenient sleep method that propagates the interruption, but does not throw. */ + static void sleep(long millis) { + try { + Thread.sleep(millis); + } catch (InterruptedException ex) { + Thread.currentThread().interrupt(); + } + } + + private ConcurrencyTesting() {} +} diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsTasksTest.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsTasksTest.java new file mode 100644 index 00000000000..f789172d39a --- /dev/null +++ b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsTasksTest.java @@ -0,0 +1,240 @@ +/* + * Copyright 2024 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.firebase.crashlytics.internal.concurrency; + +import static com.google.common.truth.Truth.assertThat; +import static com.google.firebase.crashlytics.internal.concurrency.ConcurrencyTesting.sleep; +import static org.junit.Assert.assertThrows; + +import com.google.android.gms.tasks.Task; +import com.google.android.gms.tasks.TaskCompletionSource; +import com.google.android.gms.tasks.Tasks; +import com.google.firebase.concurrent.TestOnlyExecutors; +import java.util.concurrent.CancellationException; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import org.junit.Test; + +public class CrashlyticsTasksTest { + + @Test + public void raceReturnsFirstResult() throws Exception { + // Create 2 tasks on different workers to race. + Task task1 = + new CrashlyticsWorker(TestOnlyExecutors.background()) + .submit( + () -> { + sleep(20); + return "first"; + }); + Task task2 = + new CrashlyticsWorker(TestOnlyExecutors.background()) + .submit( + () -> { + sleep(40); + return "slow"; + }); + + Task task = CrashlyticsTasks.race(task1, task2); + String result = Tasks.await(task); + + assertThat(result).isEqualTo("first"); + } + + @Test + public void raceReturnsFirstException() { + // Create 2 tasks on different workers to race. + Task task1 = + new CrashlyticsWorker(TestOnlyExecutors.background()) + .submitTask( + () -> { + sleep(20); + return Tasks.forException(new ArithmeticException()); + }); + Task task2 = + new CrashlyticsWorker(TestOnlyExecutors.background()) + .submitTask( + () -> { + sleep(40); + return Tasks.forException(new IllegalStateException()); + }); + + Task task = CrashlyticsTasks.race(task1, task2); + ExecutionException thrown = assertThrows(ExecutionException.class, () -> Tasks.await(task)); + + // The first task throws an ArithmeticException. + assertThat(thrown).hasCauseThat().isInstanceOf(ArithmeticException.class); + } + + @Test + public void raceFirstCancelsReturnsSecondResult() throws Exception { + // Create 2 tasks on different workers to race. + Task task1 = + new CrashlyticsWorker(TestOnlyExecutors.background()) + .submitTask( + () -> { + sleep(20); + return Tasks.forCanceled(); + }); + Task task2 = + new CrashlyticsWorker(TestOnlyExecutors.background()) + .submitTask( + () -> { + sleep(40); + return Tasks.forResult("I am slow but didn't cancel."); + }); + + Task task = CrashlyticsTasks.race(task1, task2); + String result = Tasks.await(task); + + assertThat(result).isEqualTo("I am slow but didn't cancel."); + } + + @Test + public void raceBothCancel() { + // Create 2 tasks on different workers to race. + Task task1 = + new CrashlyticsWorker(TestOnlyExecutors.background()) + .submitTask( + () -> { + sleep(20); + return Tasks.forCanceled(); + }); + Task task2 = + new CrashlyticsWorker(TestOnlyExecutors.background()) + .submitTask( + () -> { + sleep(40); + return Tasks.forCanceled(); + }); + + Task task = CrashlyticsTasks.race(task1, task2); + + // Both cancelled, so cancel the race result. + assertThrows(CancellationException.class, () -> Tasks.await(task)); + } + + @Test + public void raceTasksOnSameWorker() throws Exception { + CrashlyticsWorker worker = new CrashlyticsWorker(TestOnlyExecutors.background()); + + // Create 2 tasks on the same worker to race. + Task task1 = + worker.submit( + () -> { + sleep(20); + return "first"; + }); + Task task2 = + worker.submit( + () -> { + sleep(30); + return "second"; + }); + + Task task = CrashlyticsTasks.race(task1, task2); + String result = Tasks.await(task); + + assertThat(result).isEqualTo("first"); + } + + @Test + public void raceTasksOnSameSingleThreadWorker() throws Exception { + CrashlyticsWorker worker = new CrashlyticsWorker(Executors.newSingleThreadExecutor()); + + // Create 2 tasks on the same worker to race. + Task task1 = worker.submit(() -> "first"); + Task task2 = worker.submit(() -> "second"); + + Task task = CrashlyticsTasks.race(task1, task2); + String result = Tasks.await(task); + + // The first task is submitted to this single thread worker first, so will always be first. + assertThat(result).isEqualTo("first"); + } + + @Test + public void raceTaskOneOnWorkerAnotherNeverCompletes() throws Exception { + // Create a task on a worker, and another that never completes, to race. + Task task1 = + new CrashlyticsWorker(TestOnlyExecutors.background()).submit(() -> "first"); + Task task2 = new TaskCompletionSource().getTask(); + + Task task = CrashlyticsTasks.race(task1, task2); + String result = Tasks.await(task); + + assertThat(result).isEqualTo("first"); + } + + @Test + public void raceTaskOneOnWorkerAnotherOtherThatCompletesFirst() throws Exception { + CrashlyticsWorker worker = new CrashlyticsWorker(TestOnlyExecutors.background()); + + // Add a decoy task to the worker to take up some time. + worker.submitTask( + () -> { + sleep(20); + return Tasks.forResult(null); + }); + + // Create a task on this worker, and another, to race. + Task task1 = worker.submit(() -> "worker"); + TaskCompletionSource task2 = new TaskCompletionSource<>(); + task2.trySetResult("other"); + + Task task = CrashlyticsTasks.race(task1, task2.getTask()); + String result = Tasks.await(task); + + // The other tasks completes first because the first task is queued up later on the worker. + assertThat(result).isEqualTo("other"); + } + + @Test + public void raceNoExecutor() throws Exception { + // Create tasks with no explicit executor. + TaskCompletionSource task1 = new TaskCompletionSource<>(); + TaskCompletionSource task2 = new TaskCompletionSource<>(); + + Task task = CrashlyticsTasks.race(task1.getTask(), task2.getTask()); + + // Set a task result from another thread. + new Thread( + () -> { + sleep(30); + task1.trySetResult("yes"); + }) + .start(); + + String result = Tasks.await(task); + + assertThat(result).isEqualTo("yes"); + } + + @Test + public void raceTasksThatNeverResolve() { + // Create tasks that will never resolve. + Task task1 = new TaskCompletionSource().getTask(); + Task task2 = new TaskCompletionSource().getTask(); + + Task task = CrashlyticsTasks.race(task1, task2); + + // Since the tasks never resolve, the await will timeout. + assertThrows(TimeoutException.class, () -> Tasks.await(task, 300, TimeUnit.MILLISECONDS)); + } +} diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsWorkerTest.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsWorkerTest.java new file mode 100644 index 00000000000..96c78bb4abc --- /dev/null +++ b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsWorkerTest.java @@ -0,0 +1,918 @@ +/* + * Copyright 2024 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.firebase.crashlytics.internal.concurrency; + +import static com.google.common.truth.Truth.assertThat; +import static com.google.firebase.crashlytics.internal.concurrency.ConcurrencyTesting.getThreadName; +import static com.google.firebase.crashlytics.internal.concurrency.ConcurrencyTesting.newNamedSingleThreadExecutor; +import static com.google.firebase.crashlytics.internal.concurrency.ConcurrencyTesting.sleep; +import static org.junit.Assert.assertThrows; + +import com.google.android.gms.tasks.CancellationToken; +import com.google.android.gms.tasks.CancellationTokenSource; +import com.google.android.gms.tasks.Task; +import com.google.android.gms.tasks.TaskCompletionSource; +import com.google.android.gms.tasks.Tasks; +import com.google.firebase.concurrent.TestOnlyExecutors; +import java.io.IOException; +import java.util.Collections; +import java.util.HashSet; +import java.util.Queue; +import java.util.Set; +import java.util.concurrent.CancellationException; +import java.util.concurrent.ConcurrentLinkedQueue; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.ThreadPoolExecutor; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicBoolean; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +public class CrashlyticsWorkerTest { + private CrashlyticsWorker crashlyticsWorker; + + @Before + public void setUp() { + crashlyticsWorker = new CrashlyticsWorker(TestOnlyExecutors.background()); + } + + @After + public void tearDown() throws Exception { + // Drain the worker, just in case any test cases would fail but didn't await. + crashlyticsWorker.await(); + } + + @Test + public void executesTasksOnThreadPool() throws Exception { + Set threads = Collections.synchronizedSet(new HashSet<>()); + + // Find thread names by adding the names we touch to the set. + for (int i = 0; i < 100; i++) { + crashlyticsWorker.submit(() -> threads.add(getThreadName())); + } + + crashlyticsWorker.await(); + + // Verify that we touched at lease some of the expected background threads. + assertThat(threads) + .containsAnyOf( + "Firebase Background Thread #0", + "Firebase Background Thread #1", + "Firebase Background Thread #2", + "Firebase Background Thread #3"); + } + + @Test + public void executesTasksInOrder() throws Exception { + Queue list = new ConcurrentLinkedQueue<>(); + + // Add sequential numbers to the list to validate tasks execute in order. + for (int i = 0; i < 100; i++) { + int sequential = i; + crashlyticsWorker.submit(() -> list.add(sequential)); + } + + crashlyticsWorker.await(); + + // Verify that the tasks executed in order. + assertThat(list).isInOrder(); + } + + @Test + public void executesTasksSequentially() throws Exception { + Queue list = new ConcurrentLinkedQueue<>(); + AtomicBoolean reentrant = new AtomicBoolean(false); + + for (int i = 0; i < 100; i++) { + int sequential = i; + crashlyticsWorker.submit( + () -> { + if (reentrant.get()) { + // Return early if two runnables ran at the same time. + return; + } + + reentrant.set(true); + // Sleep a bit to simulate some work. + sleep(5); + list.add(sequential); + reentrant.set(false); + }); + } + + crashlyticsWorker.await(); + + // Verify that all the runnable tasks executed, one at a time, and in order. + assertThat(list).hasSize(100); + assertThat(list).isInOrder(); + } + + @Test + public void submitCallableThatReturns() throws Exception { + String ender = "Remember, the enemy's gate is down."; + Task task = crashlyticsWorker.submit(() -> ender); + + String result = Tasks.await(task); + + assertThat(result).isEqualTo(ender); + } + + @Test + public void submitCallableThatReturnsNull() throws Exception { + Task task = crashlyticsWorker.submit(() -> null); + + String result = Tasks.await(task); + + assertThat(result).isNull(); + } + + @Test + public void submitCallableThatThrows() { + Task task = + crashlyticsWorker.submit( + () -> { + throw new Exception("I threw in the callable"); + }); + + ExecutionException thrown = assertThrows(ExecutionException.class, () -> Tasks.await(task)); + + assertThat(thrown).hasCauseThat().hasMessageThat().isEqualTo("I threw in the callable"); + } + + @Test + public void submitCallableThatThrowsThenReturns() throws Exception { + Task throwingTask = + crashlyticsWorker.submit( + () -> { + throw new IOException(); + }); + + assertThrows(ExecutionException.class, () -> Tasks.await(throwingTask)); + + String hiro = + "When you are wrestling for possession of a sword, the man with the handle always wins."; + Task task = crashlyticsWorker.submitTask(() -> Tasks.forResult(hiro)); + + String result = Tasks.await(task); + + assertThat(result).isEqualTo(hiro); + } + + @Test + public void submitRunnable() throws Exception { + Task task = crashlyticsWorker.submit(() -> {}); + + Void result = Tasks.await(task); + + // A Runnable does not return, so the task evaluates to null. + assertThat(result).isNull(); + } + + @Test + public void submitRunnableThatThrows() { + Task task = + crashlyticsWorker.submit( + (Runnable) + () -> { + throw new RuntimeException("I threw in the runnable"); + }); + + ExecutionException thrown = assertThrows(ExecutionException.class, () -> Tasks.await(task)); + + assertThat(thrown).hasCauseThat().hasMessageThat().isEqualTo("I threw in the runnable"); + } + + @Test + public void submitRunnableThatThrowsThenReturns() throws Exception { + Task thowingTask = + crashlyticsWorker.submit( + (Runnable) + () -> { + throw new IllegalArgumentException(); + }); + + assertThrows(ExecutionException.class, () -> Tasks.await(thowingTask)); + + Task task = crashlyticsWorker.submit(() -> {}); + + Void result = Tasks.await(task); + + assertThat(result).isNull(); + } + + @Test + public void submitTaskThatReturns() throws Exception { + String skippy = "Think of the problem as an enemy, and defeat them in detail."; + Task task = crashlyticsWorker.submitTask(() -> Tasks.forResult(skippy)); + + String result = Tasks.await(task); + + assertThat(result).isEqualTo(skippy); + } + + @Test + public void submitTaskThatReturnsNull() throws Exception { + Task task = crashlyticsWorker.submitTask(() -> Tasks.forResult(null)); + + String result = Tasks.await(task); + + assertThat(result).isNull(); + } + + @Test + public void submitTaskThatThrows() { + Task task = + crashlyticsWorker.submitTask( + () -> Tasks.forException(new Exception("Thrown from a task."))); + + ExecutionException thrown = assertThrows(ExecutionException.class, () -> Tasks.await(task)); + + assertThat(thrown).hasCauseThat().hasMessageThat().isEqualTo("Thrown from a task."); + } + + @Test + public void submitTaskThatThrowsThenReturns() throws Exception { + crashlyticsWorker.submitTask(() -> Tasks.forException(new IllegalStateException())); + Task task = crashlyticsWorker.submitTask(() -> Tasks.forResult("The Hail Mary")); + + String result = Tasks.await(task); + + assertThat(result).isEqualTo("The Hail Mary"); + } + + @Test + public void submitTaskThatCancels() { + Task task = crashlyticsWorker.submitTask(Tasks::forCanceled); + + CancellationException thrown = + assertThrows(CancellationException.class, () -> Tasks.await(task)); + + assertThat(task.isCanceled()).isTrue(); + assertThat(thrown).hasMessageThat().contains("Task is already canceled"); + } + + @Test + public void submitTaskThatCancelsThenReturns() throws Exception { + crashlyticsWorker.submitTask(Tasks::forCanceled); + Task task = crashlyticsWorker.submitTask(() -> Tasks.forResult("Flying Dutchman")); + + String result = Tasks.await(task); + + assertThat(task.isCanceled()).isFalse(); + assertThat(result).isEqualTo("Flying Dutchman"); + } + + @Test + public void submitTaskThatCancelsThenAwaitsThenReturns() throws Exception { + Task cancelled = crashlyticsWorker.submitTask(Tasks::forCanceled); + + // Await on the cancelled task to force the exception to propagate. + assertThrows(CancellationException.class, () -> Tasks.await(cancelled)); + + // Submit another task. + Task task = crashlyticsWorker.submitTask(() -> Tasks.forResult("Valkyrie")); + + String result = Tasks.await(task); + + assertThat(cancelled.isCanceled()).isTrue(); + assertThat(task.isCanceled()).isFalse(); + assertThat(result).isEqualTo("Valkyrie"); + } + + @Test + public void submitTaskThatCancelsThenAwaitsThenCallable() throws Exception { + Task cancelled = crashlyticsWorker.submitTask(Tasks::forCanceled); + + // Await on the cancelled task to force the exception to propagate. + assertThrows(CancellationException.class, () -> Tasks.await(cancelled)); + + // Submit a simple callable. + Task task = crashlyticsWorker.submit(() -> true); + + boolean result = Tasks.await(task); + + assertThat(cancelled.isCanceled()).isTrue(); + assertThat(task.isCanceled()).isFalse(); + assertThat(result).isTrue(); + } + + @Test + public void submitTaskThatCancelsThenAwaitsThenRunnable() throws Exception { + Task cancelled = crashlyticsWorker.submitTask(Tasks::forCanceled); + + // Await on the cancelled task to force the exception to propagate. + assertThrows(CancellationException.class, () -> Tasks.await(cancelled)); + + // Submit an empty runnable. + Task task = crashlyticsWorker.submit(() -> {}); + + Void result = Tasks.await(task); + + assertThat(cancelled.isCanceled()).isTrue(); + assertThat(task.isCanceled()).isFalse(); + assertThat(result).isNull(); + } + + @Test + public void submitTaskFromAnotherWorker() throws Exception { + Task otherTask = + new CrashlyticsWorker(TestOnlyExecutors.blocking()) + .submit(() -> "Dog's fine. Just sleeping."); + + // This will not use a background thread while waiting for the task on blocking thread. + Task task = crashlyticsWorker.submitTask(() -> otherTask); + + String result = Tasks.await(task); + assertThat(result).isEqualTo("Dog's fine. Just sleeping."); + } + + @Test + public void submitTaskFromAnotherWorkerThatThrows() throws Exception { + Task otherTask = + new CrashlyticsWorker(TestOnlyExecutors.blocking()) + .submitTask(() -> Tasks.forException(new IndexOutOfBoundsException())); + + // Await on the throwing task to force the exception to propagate threw the local worker. + Task task = crashlyticsWorker.submitTask(() -> otherTask); + assertThrows(ExecutionException.class, () -> Tasks.await(task)); + + // Submit another task to local worker to verify the chain did not break. + Task localTask = crashlyticsWorker.submitTask(() -> Tasks.forResult(0x5f375a86)); + + int localResult = Tasks.await(localTask); + + assertThat(otherTask.isSuccessful()).isFalse(); + assertThat(localTask.isSuccessful()).isTrue(); + assertThat(localResult).isEqualTo(0x5f375a86); + } + + @Test + public void submitTaskFromAnotherWorkerThatCancels() throws Exception { + Task otherCancelled = + new CrashlyticsWorker(TestOnlyExecutors.blocking()).submitTask(Tasks::forCanceled); + + // Await on the cancelled task to force the exception to propagate threw the local worker. + Task task = crashlyticsWorker.submitTask(() -> otherCancelled); + assertThrows(CancellationException.class, () -> Tasks.await(task)); + + // Submit another task to local worker to verify the chain did not break. + Task localTask = crashlyticsWorker.submitTask(() -> Tasks.forResult(0x5fe6eb50c7b537a9L)); + + long localResult = Tasks.await(localTask); + + assertThat(otherCancelled.isCanceled()).isTrue(); + assertThat(localTask.isCanceled()).isFalse(); + assertThat(localResult).isEqualTo(0x5fe6eb50c7b537a9L); + } + + @Test + public void submitTaskFromAnotherWorkerDoesNotUseLocalThreads() throws Exception { + // Setup a "local" worker. + ThreadPoolExecutor localExecutor = (ThreadPoolExecutor) Executors.newFixedThreadPool(4); + CrashlyticsWorker localWorker = new CrashlyticsWorker(localExecutor); + + // Use a task off crashlyticsWorker to represent an other task. + Task otherTask = + crashlyticsWorker.submit( + () -> { + sleep(30); + return localExecutor.getActiveCount(); + }); + + // No active threads yet. + assertThat(localExecutor.getActiveCount()).isEqualTo(0); + + // 1 active thread when doing a local task. + assertThat(Tasks.await(localWorker.submit(localExecutor::getActiveCount))).isEqualTo(1); + + // 0 active local threads when waiting for other task. + // Waiting for a task from another worker does not block a local thread. + assertThat(Tasks.await(localWorker.submitTask(() -> otherTask))).isEqualTo(0); + + // 1 active thread when doing a task. + assertThat(Tasks.await(localWorker.submit(localExecutor::getActiveCount))).isEqualTo(1); + + // No active threads after. + assertThat(localExecutor.getActiveCount()).isEqualTo(0); + } + + @Test + public void submitTaskWhenThreadPoolFull() { + // Fill the underlying executor thread pool. + for (int i = 0; i < 10; i++) { + crashlyticsWorker.execute(() -> sleep(40)); + } + + Task task = crashlyticsWorker.submitTask(() -> Tasks.forResult(42)); + + // The underlying thread pool is full with tasks that will take longer than this timeout. + assertThrows(TimeoutException.class, () -> Tasks.await(task, 30, TimeUnit.MILLISECONDS)); + } + + @Test + public void submitTaskThatReturnsWithContinuation() throws Exception { + Task result = + crashlyticsWorker.submitTask( + () -> Tasks.forResult(1337), + task -> Tasks.forResult(Integer.toString(task.getResult()))); + + assertThat(Tasks.await(result)).isEqualTo("1337"); + } + + @Test + public void submitTaskThatThrowsWithContinuation() throws Exception { + Task result = + crashlyticsWorker.submitTask( + () -> Tasks.forException(new IndexOutOfBoundsException("Sometimes we look too far.")), + task -> { + if (task.getException() != null) { + return Tasks.forResult("Task threw."); + } + return Tasks.forResult("I dunno how I got here?"); + }); + + assertThat(Tasks.await(result)).isEqualTo("Task threw."); + } + + @Test + public void submitTaskWithContinuationThatThrows() throws Exception { + Task result = + crashlyticsWorker.submitTask( + () -> Tasks.forResult(7), task -> Tasks.forException(new IOException())); + + ExecutionException thrown = assertThrows(ExecutionException.class, () -> Tasks.await(result)); + + assertThat(thrown).hasCauseThat().isInstanceOf(IOException.class); + + // Verify the worker still executes tasks after the continuation threw. + assertThat(Tasks.await(crashlyticsWorker.submit(() -> 42))).isEqualTo(42); + } + + @Test + public void submitTaskThatCancelsWithContinuation() throws Exception { + Task result = + crashlyticsWorker.submitTask( + Tasks::forCanceled, + task -> Tasks.forResult(task.isCanceled() ? "Task cancelled." : "What?")); + + assertThat(Tasks.await(result)).isEqualTo("Task cancelled."); + } + + @Test + public void submitTaskWithContinuationThatCancels() throws Exception { + Task result = + crashlyticsWorker.submitTask(() -> Tasks.forResult(7), task -> Tasks.forCanceled()); + + assertThrows(CancellationException.class, () -> Tasks.await(result)); + + // Verify the worker still executes tasks after the continuation was cancelled. + assertThat(Tasks.await(crashlyticsWorker.submit(() -> "jk"))).isEqualTo("jk"); + } + + @Test + public void submitTaskOnSuccess() throws Exception { + TaskCompletionSource waitingSource = new TaskCompletionSource<>(); + Task waitingTask = waitingSource.getTask(); + + Task task = + crashlyticsWorker.submitTaskOnSuccess( + () -> waitingTask, + integerResult -> { + // This gets called with the result when the waiting task resolves successfully. + return Tasks.forResult(integerResult + " Success!"); + }); + + waitingSource.trySetResult(1337); + + String result = Tasks.await(task); + + assertThat(result).isEqualTo("1337 Success!"); + } + + @Test + public void submitTaskThatReturnsWithSuccessContinuation() throws Exception { + Task task = + crashlyticsWorker.submitTaskOnSuccess( + () -> Tasks.forResult(1337), integer -> Tasks.forResult(Integer.toString(integer))); + + String result = Tasks.await(task); + + assertThat(result).isEqualTo("1337"); + } + + @Test + public void submitTaskThatThrowsWithSuccessContinuation() { + Task task = + crashlyticsWorker.submitTaskOnSuccess( + () -> Tasks.forException(new IndexOutOfBoundsException()), + object -> Tasks.forResult("Still you don't believe.")); + + ExecutionException thrown = assertThrows(ExecutionException.class, () -> Tasks.await(task)); + + assertThat(thrown).hasCauseThat().isInstanceOf(IndexOutOfBoundsException.class); + } + + @Test + public void submitTaskWithSuccessContinuationThatThrows() throws Exception { + Task task = + crashlyticsWorker.submitTaskOnSuccess( + () -> Tasks.forResult(7), integer -> Tasks.forException(new IOException())); + + ExecutionException thrown = assertThrows(ExecutionException.class, () -> Tasks.await(task)); + + assertThat(thrown).hasCauseThat().isInstanceOf(IOException.class); + + // Verify the worker still executes tasks after the success continuation threw. + assertThat(Tasks.await(crashlyticsWorker.submit(() -> 42))).isEqualTo(42); + } + + @Test + public void submitTaskThatCancelsWithSuccessContinuation() throws Exception { + Task task = + crashlyticsWorker.submitTaskOnSuccess( + Tasks::forCanceled, object -> Tasks.forResult("Will set you free")); + + assertThrows(CancellationException.class, () -> Tasks.await(task)); + + // Verify the worker still executes tasks after the task cancelled. + assertThat(Tasks.await(crashlyticsWorker.submit(() -> 42))).isEqualTo(42); + } + + @Test + public void submitTaskWithSuccessContinuationThatCancels() throws Exception { + Task task = + crashlyticsWorker.submitTaskOnSuccess( + () -> Tasks.forResult(7), integer -> Tasks.forCanceled()); + + assertThrows(CancellationException.class, () -> Tasks.await(task)); + + // Verify the worker still executes tasks after the success continuation was cancelled. + assertThat(Tasks.await(crashlyticsWorker.submit(() -> "jk"))).isEqualTo("jk"); + } + + @Test + public void submitTaskWithContinuationExecutesInOrder() throws Exception { + // The integers added to the list represent the order they should be executed in. + Queue list = new ConcurrentLinkedQueue<>(); + + // Start the chain which adds 1, then kicks off tasks to add 6 & 7 later, but adds 2 before + // executing the newly added tasks in the continuation. + crashlyticsWorker.submitTask( + () -> { + list.add(1); + + // Sleep to give time for the tasks 3, 4, 5, 6 to be submitted. + sleep(3); + + // We added the 1 and will add 2 in the continuation. And 3, 4, 5, 6 have been submitted. + crashlyticsWorker.submit(() -> list.add(7)); + crashlyticsWorker.submit(() -> list.add(8)); + + return Tasks.forResult(1); + }, + task -> { + // When the task 1 completes the next number to add is 2. Because all the other tasks + // are just submitted, not executed yet. + list.add(2); + return Tasks.forResult("a"); + }); + + // Submit task to add 3 since we just added 1 and know a continuation will add the 2. + crashlyticsWorker.submit(() -> list.add(3)); + + // Submit a waiting task that blocks adding 4 and 5 so we can kick it off later. + TaskCompletionSource waitingSource = new TaskCompletionSource<>(); + Task waitingTask = waitingSource.getTask(); + + crashlyticsWorker.submitTask( + () -> + waitingTask + .continueWith(crashlyticsWorker, task -> list.add(4)) + .continueWith(crashlyticsWorker, task -> list.add(5))); + + // Submit task to add 6 after the waiting continuations to add 4, 5. + crashlyticsWorker.submit(() -> list.add(6)); + + // Kick off the waiting task to add 4, 5 now that 6 is queued up. + waitingSource.trySetResult(null); + + crashlyticsWorker.await(); + + // Verify the list is complete and in order. + assertThat(list).isInOrder(); + assertThat(list).hasSize(8); + } + + @Test + public void tasksRunOnCorrectThreads() throws Exception { + ExecutorService executor = newNamedSingleThreadExecutor("workerThread"); + CrashlyticsWorker worker = new CrashlyticsWorker(executor); + + ExecutorService otherExecutor = newNamedSingleThreadExecutor("otherThread"); + CrashlyticsWorker otherWorker = new CrashlyticsWorker(otherExecutor); + + // Submit a Runnable. + worker.submit( + () -> { + // The runnable blocks an underlying thread. + assertThat(getThreadName()).isEqualTo("workerThread"); + }); + + // Submit a Callable. + worker.submit( + () -> { + // The callable blocks an underlying thread. + assertThat(getThreadName()).isEqualTo("workerThread"); + return null; + }); + + // Submit a Callable. + worker.submitTask( + () -> { + // The callable itself blocks an underlying thread. + assertThat(getThreadName()).isEqualTo("workerThread"); + return otherWorker.submit( + () -> { + // The called task blocks an underlying thread in its own executor. + assertThat(getThreadName()).isEqualTo("otherThread"); + }); + }); + + // Submit a Callable with a Continuation. + worker.submitTask( + () -> { + // The callable itself blocks an underlying thread. + assertThat(getThreadName()).isEqualTo("workerThread"); + return otherWorker.submitTask( + () -> { + // The called task blocks an underlying thread in its own executor. + assertThat(getThreadName()).isEqualTo("otherThread"); + return Tasks.forResult(null); + }); + }, + task -> { + // The continuation blocks an underlying thread of the original worker. + assertThat(getThreadName()).isEqualTo("workerThread"); + return Tasks.forResult(null); + }); + + // Await on the worker to force all the tasks to run their assertions. + worker.await(); + } + + @Test + public void executeContinuationOnWorker() throws Exception { + Task task = + crashlyticsWorker + .submit(() -> "Hello!") + .continueWith(crashlyticsWorker, greetingTask -> getThreadName()); + + String result = Tasks.await(task); + assertThat(result).contains("Firebase Background Thread"); + } + + @Test + public void executeContinuationInsideWorker() throws Exception { + Task task = + crashlyticsWorker.submitTask( + () -> + Tasks.forResult("Hello!") + .continueWith(crashlyticsWorker, greetingTask -> getThreadName())); + + String result = Tasks.await(task); + assertThat(result).contains("Firebase Background Thread"); + } + + @Test + public void executeSuccessContinuationOnWorker() throws Exception { + Task task = + crashlyticsWorker + .submit(() -> "Ahoy-hoy!") + .onSuccessTask(crashlyticsWorker, greeting -> Tasks.forResult(getThreadName())); + + String result = Tasks.await(task); + assertThat(result).contains("Firebase Background Thread"); + } + + @Test + public void executeSuccessContinuationInsideWorker() throws Exception { + Task task = + crashlyticsWorker.submitTask( + () -> + Tasks.forResult("Ahoy-hoy!") + .onSuccessTask( + crashlyticsWorker, greeting -> Tasks.forResult(getThreadName()))); + + String result = Tasks.await(task); + assertThat(result).contains("Firebase Background Thread"); + } + + @Test + public void executeSuccessContinuationOnExceptionOnWorker() throws Exception { + Task task = + crashlyticsWorker + .submitTask(() -> Tasks.forException(new IllegalStateException())) + .onSuccessTask(crashlyticsWorker, greeting -> Tasks.forResult(getThreadName())); + + ExecutionException thrown = assertThrows(ExecutionException.class, () -> Tasks.await(task)); + assertThat(thrown).hasCauseThat().isInstanceOf(IllegalStateException.class); + + // Verify the chain did not break by adding the on success to a failed task. + assertThat(Tasks.await(crashlyticsWorker.submitTask(() -> Tasks.forResult(7)))).isEqualTo(7); + } + + @Test + public void executeSuccessContinuationOnExceptionInsideWorker() throws Exception { + Task task = + crashlyticsWorker.submitTask( + () -> + Tasks.forException(new IllegalStateException()) + .onSuccessTask( + crashlyticsWorker, greeting -> Tasks.forResult(getThreadName()))); + + ExecutionException thrown = assertThrows(ExecutionException.class, () -> Tasks.await(task)); + assertThat(thrown).hasCauseThat().isInstanceOf(IllegalStateException.class); + + // Verify the chain did not break by adding the on success to a failed task. + assertThat(Tasks.await(crashlyticsWorker.submitTask(() -> Tasks.forResult(7)))).isEqualTo(7); + } + + @Test + public void executeContinuationThatThrowsOnWorker() { + Task task = + crashlyticsWorker + .submit(() -> "Aloha") + .continueWithTask( + crashlyticsWorker, greeting -> Tasks.forException(new ArithmeticException())); + + ExecutionException thrown = assertThrows(ExecutionException.class, () -> Tasks.await(task)); + assertThat(thrown).hasCauseThat().isInstanceOf(ArithmeticException.class); + } + + @Test + public void executeContinuationThatThrowsInsideWorker() { + Task task = + crashlyticsWorker.submitTask( + () -> + Tasks.forResult("Aloha") + .continueWithTask( + crashlyticsWorker, + greeting -> Tasks.forException(new ArithmeticException()))); + + ExecutionException thrown = assertThrows(ExecutionException.class, () -> Tasks.await(task)); + assertThat(thrown).hasCauseThat().isInstanceOf(ArithmeticException.class); + } + + @Test + public void executeContinuationThatCancelsOnWorker() throws Exception { + Task task = + crashlyticsWorker + .submit(() -> "Aloha") + .continueWithTask(crashlyticsWorker, greeting -> Tasks.forCanceled()); + + assertThrows(CancellationException.class, () -> Tasks.await(task)); + + // Verify the chain did not break by the continuation cancelling. + assertThat(Tasks.await(crashlyticsWorker.submitTask(() -> Tasks.forResult(7)))).isEqualTo(7); + } + + @Test + public void executeContinuationThatCancelsInsideWorker() throws Exception { + Task task = + crashlyticsWorker.submitTask( + () -> + Tasks.forResult("Aloha") + .continueWithTask(crashlyticsWorker, greeting -> Tasks.forCanceled())); + + assertThrows(CancellationException.class, () -> Tasks.await(task)); + + // Verify the chain did not break by the continuation cancelling. + assertThat(Tasks.await(crashlyticsWorker.submitTask(() -> Tasks.forResult(7)))).isEqualTo(7); + } + + @Test + public void executeContinuationsOnOrInsideWorkerDoesNotDeadlock() throws Exception { + // Create a single thread worker to catch potential deadlocks. + CrashlyticsWorker worker = new CrashlyticsWorker(newNamedSingleThreadExecutor("single")); + + // Create a waiting task so we can queue up stuff before executing it. + TaskCompletionSource waiting = new TaskCompletionSource<>(); + + Task task = + worker + .submitTask( + () -> waiting.getTask().continueWith(worker, greetingTask -> getThreadName())) + .continueWith(worker, insideTask -> insideTask.getResult() + "-" + getThreadName()); + + // Kick off the waiting task. + waiting.trySetResult("Howdy!"); + + String result = Tasks.await(task); + + // Verify both on and inside continuations ran on the worker's underlying executor. + assertThat(result).contains("single-single"); + } + + @Test + public void executeContinuationTasksOnOrInsideWorkerDoesNotDeadlock() throws Exception { + // Create a single thread worker to catch potential deadlocks. + CrashlyticsWorker worker = new CrashlyticsWorker(newNamedSingleThreadExecutor("single")); + + // Create a waiting task so we can queue up stuff before executing it. + TaskCompletionSource waiting = new TaskCompletionSource<>(); + + Task task = + worker + .submitTask( + () -> + waiting + .getTask() + .continueWithTask(worker, greetingTask -> Tasks.forResult(getThreadName()))) + .continueWithTask( + worker, + insideTask -> Tasks.forResult(insideTask.getResult() + "-" + getThreadName())); + + // Kick off the waiting task. + waiting.trySetResult("Howdy!"); + + String result = Tasks.await(task); + + // Verify both on and inside continuations ran on the worker's underlying executor. + assertThat(result).contains("single-single"); + } + + @Test + public void cancelledTaskInMiddleDoesNotBreakChain() throws Exception { + // List to keep track of tasks that successfully executed. + Queue list = new ConcurrentLinkedQueue<>(); + + // Create a waiting task to block execution on the worker until more tasks are queued up. + TaskCompletionSource taskSource = new TaskCompletionSource<>(); + crashlyticsWorker.submitTask(taskSource::getTask); + + // Setup a waiting cancellation, so the task does not know it's cancelled when submitting more. + CancellationTokenSource cancellationSource = new CancellationTokenSource(); + CancellationToken cancellationToken = cancellationSource.getToken(); + + // Submit the first task that will cancel. + crashlyticsWorker.submitTask(() -> new TaskCompletionSource<>(cancellationToken).getTask()); + + // Submit a Runnable + crashlyticsWorker.submit( + () -> { + list.add("runnable"); + }); + + // Submit another task that will cancel. + crashlyticsWorker.submitTask(() -> new TaskCompletionSource<>(cancellationToken).getTask()); + + // Submit a Callable + crashlyticsWorker.submit(() -> list.add("callable")); + + crashlyticsWorker.submitTask(() -> new TaskCompletionSource<>(cancellationToken).getTask()); + + // Submit a Callable + crashlyticsWorker.submitTask( + () -> { + list.add("callable task"); + return Tasks.forResult(null); + }); + + crashlyticsWorker.submitTask(() -> new TaskCompletionSource<>(cancellationToken).getTask()); + + // Submit a Callable with a Continuation. + crashlyticsWorker.submitTask( + () -> Tasks.forResult(null), + task -> { + list.add("continuation"); + return Tasks.forResult(null); + }); + + // Trigger the cancellations. + cancellationSource.cancel(); + taskSource.trySetResult("go!"); + + crashlyticsWorker.await(); + + // Verify that all types of tasks executed. + assertThat(list).containsExactly("runnable", "callable", "callable task", "continuation"); + } +} diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/metadata/MetaDataStoreTest.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/metadata/MetaDataStoreTest.java index db3d6d8a1cc..f66c3e0d2d6 100644 --- a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/metadata/MetaDataStoreTest.java +++ b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/metadata/MetaDataStoreTest.java @@ -16,8 +16,9 @@ import static com.google.common.truth.Truth.assertThat; +import com.google.firebase.concurrent.TestOnlyExecutors; import com.google.firebase.crashlytics.internal.CrashlyticsTestCase; -import com.google.firebase.crashlytics.internal.common.CrashlyticsBackgroundWorker; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers; import com.google.firebase.crashlytics.internal.persistence.FileStore; import java.io.File; import java.io.IOException; @@ -57,8 +58,8 @@ public class MetaDataStoreTest extends CrashlyticsTestCase { } private FileStore fileStore; - private final CrashlyticsBackgroundWorker worker = new CrashlyticsBackgroundWorker(Runnable::run); + private CrashlyticsWorkers crashlyticsWorkers; private MetaDataStore storeUnderTest; @Override @@ -66,6 +67,13 @@ public void setUp() throws Exception { super.setUp(); fileStore = new FileStore(getContext()); storeUnderTest = new MetaDataStore(fileStore); + crashlyticsWorkers = + new CrashlyticsWorkers(TestOnlyExecutors.background(), TestOnlyExecutors.blocking()); + } + + @Override + public void tearDown() throws Exception { + fileStore.deleteAllCrashlyticsFiles(); } private UserMetadata metadataWithUserId(String sessionId) { @@ -73,113 +81,182 @@ private UserMetadata metadataWithUserId(String sessionId) { } private UserMetadata metadataWithUserId(String sessionId, String userId) { - UserMetadata metadata = new UserMetadata(sessionId, fileStore, worker); + UserMetadata metadata = new UserMetadata(sessionId, fileStore, crashlyticsWorkers); metadata.setUserId(userId); return metadata; } - public void testWriteUserData_allFields() { - storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(SESSION_ID_1).getUserId()); - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker); + @Test + public void testWriteUserData_allFields() throws Exception { + crashlyticsWorkers.diskWrite.submit( + () -> { + storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(SESSION_ID_1).getUserId()); + }); + crashlyticsWorkers.diskWrite.await(); + Thread.sleep(5); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, crashlyticsWorkers); assertEquals(USER_ID, userData.getUserId()); } - public void testWriteUserData_noFields() { - storeUnderTest.writeUserData( - SESSION_ID_1, new UserMetadata(SESSION_ID_1, fileStore, null).getUserId()); - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker); + @Test + public void testWriteUserData_noFields() throws Exception { + crashlyticsWorkers.diskWrite.submit( + () -> { + storeUnderTest.writeUserData( + SESSION_ID_1, new UserMetadata(SESSION_ID_1, fileStore, null).getUserId()); + }); + crashlyticsWorkers.diskWrite.await(); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, crashlyticsWorkers); assertNull(userData.getUserId()); } - public void testWriteUserData_singleField() { - storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(SESSION_ID_1).getUserId()); - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker); + @Test + public void testWriteUserData_singleField() throws Exception { + crashlyticsWorkers.diskWrite.submit( + () -> { + storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(SESSION_ID_1).getUserId()); + }); + crashlyticsWorkers.diskWrite.await(); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, crashlyticsWorkers); assertEquals(USER_ID, userData.getUserId()); } - public void testWriteUserData_null() { - storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(SESSION_ID_1, null).getUserId()); - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker); + @Test + public void testWriteUserData_null() throws Exception { + crashlyticsWorkers.diskWrite.submit( + () -> { + storeUnderTest.writeUserData( + SESSION_ID_1, metadataWithUserId(SESSION_ID_1, null).getUserId()); + }); + crashlyticsWorkers.diskWrite.await(); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, crashlyticsWorkers); assertNull(userData.getUserId()); } - public void testWriteUserData_emptyString() { - storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(SESSION_ID_1, "").getUserId()); - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker); + @Test + public void testWriteUserData_emptyString() throws Exception { + crashlyticsWorkers.diskWrite.submit( + () -> { + storeUnderTest.writeUserData( + SESSION_ID_1, metadataWithUserId(SESSION_ID_1, "").getUserId()); + }); + crashlyticsWorkers.diskWrite.await(); + Thread.sleep(3); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, crashlyticsWorkers); assertEquals("", userData.getUserId()); } - public void testWriteUserData_unicode() { + @Test + public void testWriteUserData_unicode() throws Exception { storeUnderTest.writeUserData( SESSION_ID_1, metadataWithUserId(SESSION_ID_1, UNICODE).getUserId()); - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker); + crashlyticsWorkers.diskWrite.await(); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, crashlyticsWorkers); assertEquals(UNICODE, userData.getUserId()); } - public void testWriteUserData_escaped() { - storeUnderTest.writeUserData( - SESSION_ID_1, metadataWithUserId(SESSION_ID_1, ESCAPED).getUserId()); - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker); + @Test + public void testWriteUserData_escaped() throws Exception { + crashlyticsWorkers.diskWrite.submit( + () -> { + storeUnderTest.writeUserData( + SESSION_ID_1, metadataWithUserId(SESSION_ID_1, ESCAPED).getUserId()); + }); + crashlyticsWorkers.diskWrite.await(); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, crashlyticsWorkers); + Thread.sleep(10); assertEquals(ESCAPED.trim(), userData.getUserId()); } + @Test public void testWriteUserData_readDifferentSession() { storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(SESSION_ID_1).getUserId()); - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_2, fileStore, worker); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_2, fileStore, crashlyticsWorkers); assertNull(userData.getUserId()); } + @Test public void testReadUserData_corruptData() throws IOException { File file = storeUnderTest.getUserDataFileForSession(SESSION_ID_1); try (PrintWriter printWriter = new PrintWriter(file)) { printWriter.println("Matt says hi!"); } - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, crashlyticsWorkers); assertNull(userData.getUserId()); assertFalse(file.exists()); } + @Test public void testReadUserData_emptyData() throws IOException { File file = storeUnderTest.getUserDataFileForSession(SESSION_ID_1); file.createNewFile(); - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, crashlyticsWorkers); assertNull(userData.getUserId()); assertFalse(file.exists()); } + @Test public void testReadUserData_noStoredData() { - UserMetadata userData = UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, worker); + UserMetadata userData = + UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, crashlyticsWorkers); assertNull(userData.getUserId()); } @Test - public void testUpdateSessionId_notPersistUserIdToNewSessionIfNoUserIdSet() { - UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, worker); + public void testUpdateSessionId_notPersistUserIdToNewSessionIfNoUserIdSet() throws Exception { + UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, crashlyticsWorkers); userMetadata.setNewSession(SESSION_ID_2); - assertThat(fileStore.getSessionFile(SESSION_ID_2, UserMetadata.USERDATA_FILENAME).exists()) - .isFalse(); + crashlyticsWorkers.diskWrite.submit( + () -> { + assertThat( + fileStore.getSessionFile(SESSION_ID_2, UserMetadata.USERDATA_FILENAME).exists()) + .isFalse(); + }); + crashlyticsWorkers.diskWrite.await(); } @Test - public void testUpdateSessionId_notPersistCustomKeysToNewSessionIfNoCustomKeysSet() { - UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, worker); + public void testUpdateSessionId_notPersistCustomKeysToNewSessionIfNoCustomKeysSet() + throws Exception { + UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, crashlyticsWorkers); userMetadata.setNewSession(SESSION_ID_2); - assertThat(fileStore.getSessionFile(SESSION_ID_2, UserMetadata.KEYDATA_FILENAME).exists()) - .isFalse(); + crashlyticsWorkers.diskWrite.submit( + () -> { + assertThat(fileStore.getSessionFile(SESSION_ID_2, UserMetadata.KEYDATA_FILENAME).exists()) + .isFalse(); + }); + crashlyticsWorkers.diskWrite.await(); } @Test - public void testUpdateSessionId_notPersistRolloutsToNewSessionIfNoRolloutsSet() { - UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, worker); + public void testUpdateSessionId_notPersistRolloutsToNewSessionIfNoRolloutsSet() throws Exception { + UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, crashlyticsWorkers); userMetadata.setNewSession(SESSION_ID_2); - assertThat( - fileStore.getSessionFile(SESSION_ID_2, UserMetadata.ROLLOUTS_STATE_FILENAME).exists()) - .isFalse(); + + crashlyticsWorkers.diskWrite.submit( + () -> { + assertThat( + fileStore + .getSessionFile(SESSION_ID_2, UserMetadata.ROLLOUTS_STATE_FILENAME) + .exists()) + .isFalse(); + }); + crashlyticsWorkers.diskWrite.await(); } @Test - public void testUpdateSessionId_persistCustomKeysToNewSessionIfCustomKeysSet() { - UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, worker); + public void testUpdateSessionId_persistCustomKeysToNewSessionIfCustomKeysSet() throws Exception { + UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, crashlyticsWorkers); final Map keys = new HashMap() { { @@ -190,34 +267,74 @@ public void testUpdateSessionId_persistCustomKeysToNewSessionIfCustomKeysSet() { }; userMetadata.setCustomKeys(keys); userMetadata.setNewSession(SESSION_ID_2); - assertThat(fileStore.getSessionFile(SESSION_ID_2, UserMetadata.KEYDATA_FILENAME).exists()) - .isTrue(); + crashlyticsWorkers.diskWrite.submit( + () -> { + assertThat(fileStore.getSessionFile(SESSION_ID_2, UserMetadata.KEYDATA_FILENAME).exists()) + .isTrue(); + }); + crashlyticsWorkers.diskWrite.await(); MetaDataStore metaDataStore = new MetaDataStore(fileStore); assertThat(metaDataStore.readKeyData(SESSION_ID_2)).isEqualTo(keys); } @Test - public void testUpdateSessionId_persistUserIdToNewSessionIfUserIdSet() { + public void testSetSameKeysRaceCondition_preserveLastEntryValue() throws Exception { + final Map keys = + new HashMap() { + { + put(KEY_1, "10000"); + put(KEY_2, "20000"); + } + }; + UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, crashlyticsWorkers); + for (int index = 0; index <= 10000; index++) { + userMetadata.setCustomKey(KEY_1, String.valueOf(index)); + userMetadata.setCustomKey(KEY_2, String.valueOf(index * 2)); + } + crashlyticsWorkers.diskWrite.submit( + () -> { + final Map readKeys = storeUnderTest.readKeyData(SESSION_ID_1); + assertThat(readKeys.get(KEY_1)).isEqualTo("10000"); + assertThat(readKeys.get(KEY_2)).isEqualTo("20000"); + assertEqualMaps(keys, readKeys); + }); + crashlyticsWorkers.diskWrite.await(); + } + + @Test + public void testUpdateSessionId_persistUserIdToNewSessionIfUserIdSet() throws Exception { String userId = "ThemisWang"; - UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, worker); + UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, crashlyticsWorkers); userMetadata.setUserId(userId); userMetadata.setNewSession(SESSION_ID_2); - assertThat(fileStore.getSessionFile(SESSION_ID_2, UserMetadata.USERDATA_FILENAME).exists()) - .isTrue(); + + crashlyticsWorkers.diskWrite.submit( + () -> { + assertThat( + fileStore.getSessionFile(SESSION_ID_2, UserMetadata.USERDATA_FILENAME).exists()) + .isTrue(); + }); + crashlyticsWorkers.diskWrite.await(); MetaDataStore metaDataStore = new MetaDataStore(fileStore); assertThat(metaDataStore.readUserId(SESSION_ID_2)).isEqualTo(userId); } @Test - public void testUpdateSessionId_persistRolloutsToNewSessionIfRolloutsSet() { - UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, worker); + public void testUpdateSessionId_persistRolloutsToNewSessionIfRolloutsSet() throws Exception { + UserMetadata userMetadata = new UserMetadata(SESSION_ID_1, fileStore, crashlyticsWorkers); userMetadata.updateRolloutsState(ROLLOUTS_STATE); userMetadata.setNewSession(SESSION_ID_2); - assertThat( - fileStore.getSessionFile(SESSION_ID_2, UserMetadata.ROLLOUTS_STATE_FILENAME).exists()) - .isTrue(); + crashlyticsWorkers.diskWrite.submit( + () -> { + assertThat( + fileStore + .getSessionFile(SESSION_ID_2, UserMetadata.ROLLOUTS_STATE_FILENAME) + .exists()) + .isTrue(); + }); + crashlyticsWorkers.diskWrite.await(); MetaDataStore metaDataStore = new MetaDataStore(fileStore); assertThat(metaDataStore.readRolloutsState(SESSION_ID_2)).isEqualTo(ROLLOUTS_STATE); diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsControllerTest.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsControllerTest.java index 3d488d924e0..83f104d9649 100644 --- a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsControllerTest.java +++ b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsControllerTest.java @@ -25,14 +25,14 @@ import com.google.android.gms.tasks.Task; import com.google.android.gms.tasks.TaskCompletionSource; import com.google.android.gms.tasks.Tasks; +import com.google.firebase.concurrent.TestOnlyExecutors; import com.google.firebase.crashlytics.internal.CrashlyticsTestCase; import com.google.firebase.crashlytics.internal.common.CurrentTimeProvider; import com.google.firebase.crashlytics.internal.common.DataCollectionArbiter; import com.google.firebase.crashlytics.internal.common.DeliveryMechanism; -import com.google.firebase.crashlytics.internal.common.ExecutorUtils; import com.google.firebase.crashlytics.internal.common.InstallIdProvider; import com.google.firebase.crashlytics.internal.common.InstallIdProvider.InstallIds; -import java.util.concurrent.Executor; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers; import java.util.concurrent.TimeUnit; import org.json.JSONObject; @@ -57,7 +57,8 @@ public class DefaultSettingsControllerTest extends CrashlyticsTestCase { private SettingsSpiCall mockSettingsSpiCall; private DataCollectionArbiter mockDataCollectionArbiter; - private Executor networkExecutor = ExecutorUtils.buildSingleThreadExecutorService("network"); + private final CrashlyticsWorkers crashlyticsWorkers = + new CrashlyticsWorkers(TestOnlyExecutors.background(), TestOnlyExecutors.blocking()); public DefaultSettingsControllerTest() {} @@ -121,7 +122,7 @@ public void testCachedSettingsLoad() throws Exception { mockDataCollectionArbiter, false); - await(controller.loadSettingsData(networkExecutor)); + await(controller.loadSettingsData(crashlyticsWorkers)); assertEquals(cachedSettings, controller.getSettingsSync()); verifyNoMoreInteractions(mockSettingsSpiCall); @@ -139,7 +140,7 @@ public void testCachedSettingsLoad_newInstanceIdentifier() throws Exception { when(mockSettingsJsonParser.parseSettingsJson(fetchedJson)).thenReturn(fetchedSettings); TaskCompletionSource dataCollectionPermission = new TaskCompletionSource<>(); - when(mockDataCollectionArbiter.waitForDataCollectionPermission(any(Executor.class))) + when(mockDataCollectionArbiter.waitForDataCollectionPermission()) .thenReturn(dataCollectionPermission.getTask()); SettingsRequest requestData = buildSettingsRequest(); @@ -153,7 +154,7 @@ public void testCachedSettingsLoad_newInstanceIdentifier() throws Exception { mockDataCollectionArbiter, true); - controller.loadSettingsData(SettingsCacheBehavior.SKIP_CACHE_LOOKUP, networkExecutor); + controller.loadSettingsData(SettingsCacheBehavior.SKIP_CACHE_LOOKUP, crashlyticsWorkers); assertNotNull(controller.getSettingsSync()); dataCollectionPermission.trySetResult(null); @@ -177,14 +178,13 @@ public void testExpiredCachedSettingsLoad() throws Exception { JSONObject cachedJson = new JSONObject(); when(mockCachedSettingsIo.readCachedSettings()).thenReturn(cachedJson); - when(mockCurrentTimeProvider.getCurrentTimeMillis()) - .thenReturn(Long.valueOf(EXPIRED_CURRENT_TIME_MILLIS)); + when(mockCurrentTimeProvider.getCurrentTimeMillis()).thenReturn(EXPIRED_CURRENT_TIME_MILLIS); when(mockSettingsJsonParser.parseSettingsJson(cachedJson)).thenReturn(cachedSettings); when(mockSettingsJsonParser.parseSettingsJson(fetchedJson)).thenReturn(fetchedSettings); TaskCompletionSource dataCollectionPermission = new TaskCompletionSource<>(); - when(mockDataCollectionArbiter.waitForDataCollectionPermission(any(Executor.class))) + when(mockDataCollectionArbiter.waitForDataCollectionPermission()) .thenReturn(dataCollectionPermission.getTask()); SettingsRequest requestData = buildSettingsRequest(); @@ -198,7 +198,7 @@ public void testExpiredCachedSettingsLoad() throws Exception { mockDataCollectionArbiter, false); - Task loadFinished = controller.loadSettingsData(networkExecutor); + Task loadFinished = controller.loadSettingsData(crashlyticsWorkers); assertEquals(cachedSettings, controller.getSettingsSync()); assertEquals(cachedSettings, await(controller.getSettingsAsync())); @@ -220,8 +220,7 @@ public void testIgnoreExpiredCachedSettingsLoad() throws Exception { JSONObject cachedJson = new JSONObject(); when(mockCachedSettingsIo.readCachedSettings()).thenReturn(cachedJson); - when(mockCurrentTimeProvider.getCurrentTimeMillis()) - .thenReturn(Long.valueOf(EXPIRED_CURRENT_TIME_MILLIS)); + when(mockCurrentTimeProvider.getCurrentTimeMillis()).thenReturn(EXPIRED_CURRENT_TIME_MILLIS); Settings cachedSettings = new TestSettings(); when(mockSettingsJsonParser.parseSettingsJson(cachedJson)).thenReturn(cachedSettings); @@ -236,7 +235,7 @@ public void testIgnoreExpiredCachedSettingsLoad() throws Exception { mockSettingsSpiCall, mockDataCollectionArbiter, false); - controller.loadSettingsData(SettingsCacheBehavior.IGNORE_CACHE_EXPIRATION, networkExecutor); + controller.loadSettingsData(SettingsCacheBehavior.IGNORE_CACHE_EXPIRATION, crashlyticsWorkers); assertEquals(cachedSettings, controller.getSettingsSync()); verifyNoMoreInteractions(mockSettingsSpiCall); @@ -256,15 +255,14 @@ public void testSkipCachedSettingsLoad() throws Exception { JSONObject expiredCachedSettingsJson = new JSONObject(); when(mockCachedSettingsIo.readCachedSettings()).thenReturn(expiredCachedSettingsJson); - when(mockCurrentTimeProvider.getCurrentTimeMillis()) - .thenReturn(Long.valueOf(EXPIRED_CURRENT_TIME_MILLIS)); + when(mockCurrentTimeProvider.getCurrentTimeMillis()).thenReturn(EXPIRED_CURRENT_TIME_MILLIS); Settings expiredCachedSettings = new TestSettings(); when(mockSettingsJsonParser.parseSettingsJson(expiredCachedSettingsJson)) .thenReturn(expiredCachedSettings); TaskCompletionSource dataCollectionPermission = new TaskCompletionSource<>(); - when(mockDataCollectionArbiter.waitForDataCollectionPermission(any(Executor.class))) + when(mockDataCollectionArbiter.waitForDataCollectionPermission()) .thenReturn(dataCollectionPermission.getTask()); SettingsRequest requestData = buildSettingsRequest(); @@ -279,7 +277,7 @@ public void testSkipCachedSettingsLoad() throws Exception { false); Task loadFinished = - controller.loadSettingsData(SettingsCacheBehavior.SKIP_CACHE_LOOKUP, networkExecutor); + controller.loadSettingsData(SettingsCacheBehavior.SKIP_CACHE_LOOKUP, crashlyticsWorkers); assertEquals(expiredCachedSettings, controller.getSettingsSync()); dataCollectionPermission.trySetResult(null); @@ -315,7 +313,7 @@ public void testLastDitchSettingsLoad() throws Exception { .thenReturn(expiredCachedSettings); TaskCompletionSource dataCollectionPermission = new TaskCompletionSource<>(); - when(mockDataCollectionArbiter.waitForDataCollectionPermission(any(Executor.class))) + when(mockDataCollectionArbiter.waitForDataCollectionPermission()) .thenReturn(dataCollectionPermission.getTask()); SettingsRequest requestData = buildSettingsRequest(); @@ -330,7 +328,7 @@ public void testLastDitchSettingsLoad() throws Exception { false); Task loadFinished = - controller.loadSettingsData(SettingsCacheBehavior.SKIP_CACHE_LOOKUP, networkExecutor); + controller.loadSettingsData(SettingsCacheBehavior.SKIP_CACHE_LOOKUP, crashlyticsWorkers); assertEquals(expiredCachedSettings, controller.getSettingsSync()); dataCollectionPermission.trySetResult(null); @@ -350,7 +348,7 @@ public void testNoAvailableSettingsLoad() throws Exception { when(mockCachedSettingsIo.readCachedSettings()).thenReturn(null); TaskCompletionSource dataCollectionPermission = new TaskCompletionSource<>(); - when(mockDataCollectionArbiter.waitForDataCollectionPermission(any(Executor.class))) + when(mockDataCollectionArbiter.waitForDataCollectionPermission()) .thenReturn(dataCollectionPermission.getTask()); SettingsRequest requestData = buildSettingsRequest(); @@ -364,7 +362,7 @@ public void testNoAvailableSettingsLoad() throws Exception { mockDataCollectionArbiter, false); - Task loadFinished = controller.loadSettingsData(networkExecutor); + Task loadFinished = controller.loadSettingsData(crashlyticsWorkers); assertNotNull(controller.getSettingsSync()); assertFalse(controller.getSettingsAsync().isComplete()); diff --git a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsSpiCallTest.java b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsSpiCallTest.java index 7131df8c8a4..9db2fb9f0ee 100644 --- a/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsSpiCallTest.java +++ b/firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsSpiCallTest.java @@ -16,6 +16,7 @@ import static org.mockito.Mockito.*; +import com.google.firebase.concurrent.TestOnlyExecutors; import com.google.firebase.crashlytics.internal.CrashlyticsTestCase; import com.google.firebase.crashlytics.internal.Logger; import com.google.firebase.crashlytics.internal.common.CommonUtils; @@ -29,6 +30,7 @@ import java.io.InputStream; import java.net.HttpURLConnection; import java.util.Map; +import java.util.concurrent.Future; public class DefaultSettingsSpiCallTest extends CrashlyticsTestCase { @@ -83,7 +85,8 @@ public HttpGetRequest buildHttpGetRequest( } }); - assertNotNull(call.invoke(requestData, true)); + Future future = TestOnlyExecutors.blocking().submit(() -> call.invoke(requestData, true)); + assertNotNull(future.get()); assertEquals(url, request.getUrl()); @@ -128,7 +131,8 @@ public HttpGetRequest buildHttpGetRequest( } }); - assertNotNull(call.invoke(requestData, true)); + Future future = TestOnlyExecutors.blocking().submit(() -> call.invoke(requestData, true)); + assertNotNull(future.get()); assertEquals(url, request.getUrl()); diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/CrashlyticsRegistrar.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/CrashlyticsRegistrar.java index 9051d22258f..b792c51784c 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/CrashlyticsRegistrar.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/CrashlyticsRegistrar.java @@ -14,8 +14,6 @@ package com.google.firebase.crashlytics; -import static com.google.firebase.crashlytics.internal.CrashlyticsPreconditions.StrictLevel.ASSERT; - import com.google.firebase.FirebaseApp; import com.google.firebase.analytics.connector.AnalyticsConnector; import com.google.firebase.annotations.concurrent.Background; @@ -26,8 +24,8 @@ import com.google.firebase.components.Dependency; import com.google.firebase.components.Qualified; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponent; -import com.google.firebase.crashlytics.internal.CrashlyticsPreconditions; import com.google.firebase.crashlytics.internal.Logger; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers; import com.google.firebase.installations.FirebaseInstallationsApi; import com.google.firebase.platforminfo.LibraryVersionComponent; import com.google.firebase.remoteconfig.interop.FirebaseRemoteConfigInterop; @@ -69,10 +67,8 @@ public List> getComponents() { } private FirebaseCrashlytics buildCrashlytics(ComponentContainer container) { - // TODO(mrober): Make this a build time configuration. Do not release like this. - CrashlyticsPreconditions.setStrictLevel(ASSERT); // Kill the process on violation for debugging. + CrashlyticsWorkers.setEnforcement(BuildConfig.DEBUG); - // CrashlyticsPreconditions.checkMainThread(); long startTime = System.currentTimeMillis(); FirebaseCrashlytics crashlytics = @@ -86,8 +82,8 @@ private FirebaseCrashlytics buildCrashlytics(ComponentContainer container) { container.get(blockingExecutorService)); long duration = System.currentTimeMillis() - startTime; - if (duration > 30) { - Logger.getLogger().i("Initializing Crashlytics blocked main for " + duration + " ms"); + if (duration > 16) { + Logger.getLogger().d("Initializing Crashlytics blocked main for " + duration + " ms"); } return crashlytics; diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/FirebaseCrashlytics.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/FirebaseCrashlytics.java index 9b0d61cd0f6..d305eb4a868 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/FirebaseCrashlytics.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/FirebaseCrashlytics.java @@ -19,12 +19,9 @@ import androidx.annotation.NonNull; import androidx.annotation.Nullable; import androidx.annotation.VisibleForTesting; -import com.google.android.gms.tasks.Continuation; import com.google.android.gms.tasks.Task; import com.google.firebase.FirebaseApp; import com.google.firebase.analytics.connector.AnalyticsConnector; -import com.google.firebase.annotations.concurrent.Background; -import com.google.firebase.annotations.concurrent.Blocking; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponent; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponentDeferredProxy; import com.google.firebase.crashlytics.internal.DevelopmentPlatformProvider; @@ -36,8 +33,8 @@ import com.google.firebase.crashlytics.internal.common.CrashlyticsAppQualitySessionsSubscriber; import com.google.firebase.crashlytics.internal.common.CrashlyticsCore; import com.google.firebase.crashlytics.internal.common.DataCollectionArbiter; -import com.google.firebase.crashlytics.internal.common.ExecutorUtils; import com.google.firebase.crashlytics.internal.common.IdManager; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers; import com.google.firebase.crashlytics.internal.network.HttpRequestFactory; import com.google.firebase.crashlytics.internal.persistence.FileStore; import com.google.firebase.crashlytics.internal.settings.SettingsController; @@ -46,7 +43,6 @@ import com.google.firebase.remoteconfig.interop.FirebaseRemoteConfigInterop; import com.google.firebase.sessions.api.FirebaseSessionsDependencies; import java.util.List; -import java.util.concurrent.Executor; import java.util.concurrent.ExecutorService; /** @@ -70,8 +66,8 @@ public class FirebaseCrashlytics { @NonNull Deferred nativeComponent, @NonNull Deferred analyticsConnector, @NonNull Deferred remoteConfigInteropDeferred, - @Background ExecutorService backgroundExecutorService, - @Blocking ExecutorService blockingExecutorService) { + ExecutorService backgroundExecutorService, + ExecutorService blockingExecutorService) { Context context = app.getApplicationContext(); final String appIdentifier = context.getPackageName(); @@ -82,6 +78,9 @@ public class FirebaseCrashlytics { + " for " + appIdentifier); + CrashlyticsWorkers crashlyticsWorkers = + new CrashlyticsWorkers(backgroundExecutorService, blockingExecutorService); + FileStore fileStore = new FileStore(context); final DataCollectionArbiter arbiter = new DataCollectionArbiter(app); final IdManager idManager = @@ -93,9 +92,6 @@ public class FirebaseCrashlytics { final AnalyticsDeferredProxy analyticsDeferredProxy = new AnalyticsDeferredProxy(analyticsConnector); - final ExecutorService crashHandlerExecutor = - ExecutorUtils.buildSingleThreadExecutorService("Crashlytics Exception Handler"); - CrashlyticsAppQualitySessionsSubscriber sessionsSubscriber = new CrashlyticsAppQualitySessionsSubscriber(arbiter, fileStore); FirebaseSessionsDependencies.register(sessionsSubscriber); @@ -112,9 +108,9 @@ public class FirebaseCrashlytics { analyticsDeferredProxy.getDeferredBreadcrumbSource(), analyticsDeferredProxy.getAnalyticsEventLogger(), fileStore, - crashHandlerExecutor, sessionsSubscriber, - remoteConfigDeferredProxy); + remoteConfigDeferredProxy, + crashlyticsWorkers); final String googleAppId = app.getOptions().getApplicationId(); final String mappingFileId = CommonUtils.getMappingFileId(context); @@ -150,10 +146,7 @@ public class FirebaseCrashlytics { Logger.getLogger().v("Installer package name is: " + appData.installerPackageName); - final Executor threadPoolExecutor = - ExecutorUtils.buildSequentialExecutor(backgroundExecutorService); - - final SettingsController settingsController = + SettingsController settingsController = SettingsController.create( context, googleAppId, @@ -166,18 +159,8 @@ public class FirebaseCrashlytics { // Kick off actually fetching the settings. settingsController - .loadSettingsData(threadPoolExecutor) - .continueWith( - threadPoolExecutor, - new Continuation() { - @Override - public Object then(@NonNull Task task) throws Exception { - if (!task.isSuccessful()) { - Logger.getLogger().e("Error fetching settings.", task.getException()); - } - return null; - } - }); + .loadSettingsData(crashlyticsWorkers) + .addOnFailureListener(ex -> Logger.getLogger().e("Error fetching settings.", ex)); final boolean finishCoreInBackground = core.onPreExecute(appData, settingsController); diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/CrashlyticsPreconditions.kt b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/CrashlyticsPreconditions.kt deleted file mode 100644 index 256dd724339..00000000000 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/CrashlyticsPreconditions.kt +++ /dev/null @@ -1,86 +0,0 @@ -/* - * Copyright 2024 Google LLC - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.google.firebase.crashlytics.internal - -import android.os.Build -import android.os.Looper -import com.google.firebase.crashlytics.internal.CrashlyticsPreconditions.StrictLevel.ASSERT -import com.google.firebase.crashlytics.internal.CrashlyticsPreconditions.StrictLevel.NONE -import com.google.firebase.crashlytics.internal.CrashlyticsPreconditions.StrictLevel.THROW -import com.google.firebase.crashlytics.internal.CrashlyticsPreconditions.StrictLevel.WARN - -/** - * Convenient preconditions specific for Crashlytics concurrency. - * - * Use GMS Core's [com.google.android.gms.common.internal.Preconditions] for general preconditions. - */ -internal object CrashlyticsPreconditions { - private val threadName - get() = Thread.currentThread().name - - // TODO(mrober): Make this a build time configuration. - @JvmStatic var strictLevel: StrictLevel = NONE - - @JvmStatic - fun checkMainThread() = - checkThread(::isMainThread) { "Must be called on the main thread, was called on $threadName." } - - @JvmStatic - fun checkBlockingThread() = - checkThread(::isBlockingThread) { - "Must be called on a blocking thread, was called on $threadName." - } - - @JvmStatic - fun checkBackgroundThread() = - checkThread(::isBackgroundThread) { - "Must be called on a background thread, was called on $threadName." - } - - private fun isMainThread() = - if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) { - Looper.getMainLooper().isCurrentThread - } else { - Looper.getMainLooper() == Looper.myLooper() - } - - private fun isBlockingThread() = threadName.contains("Firebase Blocking Thread #") - - // TODO(mrober): Remove the Crashlytics thread when fully migrated to Firebase common threads. - private fun isBackgroundThread() = - threadName.contains("Firebase Background Thread #") || - threadName.contains("Crashlytics Exception Handler") - - private fun checkThread(isCorrectThread: () -> Boolean, failureMessage: () -> String) { - if (strictLevel.level >= WARN.level && !isCorrectThread()) { - Logger.getLogger().w(failureMessage()) - assert(strictLevel.level < ASSERT.level, failureMessage) - check(strictLevel.level < THROW.level, failureMessage) - } - } - - enum class StrictLevel(val level: Int) : Comparable { - /** Do not check for violations. */ - NONE(0), - /** Log violations as warnings. */ - WARN(1), - /** Throw an exception on violation. */ - THROW(2), - /** Kill the process on violation. Useful for debugging. */ - ASSERT(3), - } -} diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/CrashlyticsWorker.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/CrashlyticsWorker.java deleted file mode 100644 index 876f018fc95..00000000000 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/CrashlyticsWorker.java +++ /dev/null @@ -1,128 +0,0 @@ -/* - * Copyright 2024 Google LLC - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.google.firebase.crashlytics.internal; - -import androidx.annotation.VisibleForTesting; -import com.google.android.gms.tasks.Task; -import com.google.android.gms.tasks.Tasks; -import java.util.concurrent.Callable; -import java.util.concurrent.ExecutionException; -import java.util.concurrent.ExecutorService; - -/** - * Helper for executing tasks sequentially on the given executor service. - * - *

Work on the queue may block, or it may return a Task, such that the underlying thread may be - * re-used while the worker queue is still blocked. - * - *

Work enqueued on this worker will be run serially, regardless of the underlying executor. - * Therefore, workers on the queue should not add new work to the queue and then block on it, as - * that would create a deadlock. In such a case, the worker can return a Task that depends on the - * future work, and run the future work on the executor's thread, but not put it in the queue as its - * own worker. - * - * @hide - */ -public class CrashlyticsWorker { - private final ExecutorService executor; - - private final Object tailLock = new Object(); - private Task tail = Tasks.forResult(null); - - public CrashlyticsWorker(ExecutorService executor) { - this.executor = executor; - } - - /** Returns the executor used by this worker. */ - public ExecutorService getExecutor() { - return executor; - } - - /** - * Submits a Callable task for asynchronous execution on the executor. - * - *

Returns a Task which will be resolved upon successful completion of the - * callable, or throws an ExecutionException if the callable throws an exception. - */ - public Task submit(Callable callable) { - synchronized (tailLock) { - // Do not propagate a cancellation. - if (tail.isCanceled()) { - tail = tail.continueWithTask(executor, task -> Tasks.forResult(null)); - } - // Chain the new callable onto the queue's tail. - Task result = tail.continueWith(executor, task -> callable.call()); - tail = result; - return result; - } - } - - /** - * Submits a Runnable task for asynchronous execution on the executor. - * - *

Returns a Task which will be resolved with null upon successful completion of - * the runnable, or throws an ExecutionException if the runnable throws an exception. - */ - public Task submit(Runnable runnable) { - synchronized (tailLock) { - // Do not propagate a cancellation. - if (tail.isCanceled()) { - tail = tail.continueWithTask(executor, task -> Tasks.forResult(null)); - } - // Chain the new runnable onto the queue's tail. - Task result = - tail.continueWith( - executor, - task -> { - runnable.run(); - return null; - }); - tail = result; - return result; - } - } - - /** - * Submits a Callable Task for asynchronous execution on the executor. - * - *

This is useful for making the worker block on an asynchronous operation, while letting the - * underlying threads be re-used. - * - *

Returns a Task which will be resolved upon successful completion of the Task - * returned by the callable, throws an ExecutionException if the callable throws an - * exception, or throws a CancellationException if the task is cancelled. - */ - public Task submitTask(Callable> callable) { - synchronized (tailLock) { - // Chain the new callable task onto the queue's tail, regardless of cancellation. - Task result = tail.continueWithTask(executor, task -> callable.call()); - tail = result; - return result; - } - } - - /** - * Blocks until all current pending tasks have completed. - * - *

This is not a shutdown, this does not stop new tasks from being submitted to the queue. - */ - @VisibleForTesting - public void await() throws ExecutionException, InterruptedException { - // Submit an empty runnable, and await on it. - Tasks.await(submit(() -> {})); - } -} diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CommonUtils.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CommonUtils.java index 33369524b2c..b29863f66c5 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CommonUtils.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CommonUtils.java @@ -114,7 +114,9 @@ enum Architecture { matcher.put("x86", X86_32); } - /** @Return {@link CommonUtils.Architecture} enum based on @param String */ + /** + * @Return {@link CommonUtils.Architecture} enum based on @param String + */ static Architecture getValue() { String arch = Build.CPU_ABI; @@ -248,7 +250,9 @@ public static boolean getProximitySensorEnabled(Context context) { } } - /** @deprecated This method will now always return false. It should not be used. */ + /** + * @deprecated This method will now always return false. It should not be used. + */ @Deprecated public static boolean isLoggingEnabled(Context context) { return false; @@ -532,7 +536,9 @@ public static void closeQuietly(Closeable closeable) { } } - /** @return if the given permission is granted */ + /** + * @return if the given permission is granted + */ public static boolean checkPermission(Context context, String permission) { final int res = context.checkCallingOrSelfPermission(permission); return (res == PackageManager.PERMISSION_GRANTED); @@ -556,7 +562,9 @@ public static boolean canTryConnection(Context context) { } } - /** @return true if s1.equals(s2), or if both are null. */ + /** + * @return true if s1.equals(s2), or if both are null. + */ public static boolean nullSafeEquals(@Nullable String s1, @Nullable String s2) { // :TODO: replace calls to this method with Objects.equals(...) when minSdkVersion is 19+ if (s1 == null) { diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsBackgroundWorker.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsBackgroundWorker.java deleted file mode 100644 index f5812bb6a4c..00000000000 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsBackgroundWorker.java +++ /dev/null @@ -1,166 +0,0 @@ -// Copyright 2020 Google LLC -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.firebase.crashlytics.internal.common; - -import androidx.annotation.NonNull; -import com.google.android.gms.tasks.Continuation; -import com.google.android.gms.tasks.Task; -import com.google.android.gms.tasks.Tasks; -import java.util.concurrent.Callable; -import java.util.concurrent.Executor; - -/** - * Helper for executing tasks on the Crashlytics background executor service. - * - *

Work on the queue may block, or it may return a Task, such that the underlying thread may be - * re-used while the worker queue is still blocked. - * - *

Work enqueued on this worker will be run serially, regardless of the underlying executor. - * Therefore, workers on the queue should not add new work to the queue and then block on it, as - * that would create a deadlock. In such a case, the worker can return a Task that depends on the - * future work, and run the future work on the executor's thread, but not put it in the queue as its - * own worker. - * - * @deprecated Use the generic CrashlyticsWorker instead. - */ -@Deprecated -public class CrashlyticsBackgroundWorker { - // TODO(mrober): Clean this up after moving everything to the generic CrashlyticsWorker. - private final Executor executor; - - private Task tail = Tasks.forResult(null); - - private final Object tailLock = new Object(); - - // A thread local to keep track of which thread belongs to this executor. - private final ThreadLocal isExecutorThread = new ThreadLocal<>(); - - public CrashlyticsBackgroundWorker(Executor executor) { - this.executor = executor; - // Queue up the first job as one that marks the thread so we can check it later. - executor.execute( - new Runnable() { - @Override - public void run() { - isExecutorThread.set(true); - } - }); - } - - /** Returns the executor used by this background worker. */ - public Executor getExecutor() { - return executor; - } - - /** Returns true if called on the thread owned by this background worker. */ - private boolean isRunningOnThread() { - return Boolean.TRUE.equals(isExecutorThread.get()); - } - - /** - * Throws an exception if called from any thread other than the background worker's. This helps - * guarantee code is being called on the intended thread. - */ - public void checkRunningOnThread() { - if (!isRunningOnThread()) { - throw new IllegalStateException("Not running on background worker thread as intended."); - } - } - - /** - * Submit a Runnable task for asynchronous execution on the Crashlytics background - * executor service. - * - *

If the runnable throws an exception, the task will be rejected with it. - * - * @return a Task which will be resolved with null upon successful completion of the - * runnable, or null if the runnable is rejected from the background executor - * service. - */ - Task submit(final Runnable runnable) { - return submit( - new Callable() { - @Override - public Void call() throws Exception { - runnable.run(); - return null; - } - }); - } - - /** Convenience method that creates a new Continuation that wraps the given callable. */ - private Continuation newContinuation(Callable callable) { - return new Continuation() { - @Override - public T then(@NonNull Task task) throws Exception { - // We can ignore the task passed in, which is just the queue's tail. - return callable.call(); - } - }; - } - - /** Convenience method that tasks a Task and convert it to a Task. */ - private Task ignoreResult(Task task) { - return task.continueWith( - executor, - new Continuation() { - @Override - public Void then(@NonNull Task task) throws Exception { - // Ignore whether the task succeeded or failed. - return null; - } - }); - } - - /** - * Submit a Callable task for asynchronous execution on the Crashlytics background - * executor service. - * - *

If the callable throws an exception, the task will be rejected with it. - * - * @return a Task which will be resolved upon successful completion of the callable. - */ - public Task submit(final Callable callable) { - synchronized (tailLock) { - // Chain the new callable onto the queue's tail. - Task toReturn = tail.continueWith(executor, newContinuation(callable)); - - // Add a new tail that swallows errors from the callable when it finishes. - tail = ignoreResult(toReturn); - return toReturn; - } - } - - /** - * Submit a Callable task for asynchronous execution on the Crashlytics background - * executor service. This method is useful for making the worker block on an asynchronous - * operation, while letting the underlying thread be re-used. - * - *

If the callable throws an exception, the task will be rejected with it. - * - * @return a Task which will be resolved upon successful completion of the Task - * returns by the callable. - */ - public Task submitTask(final Callable> callable) { - synchronized (tailLock) { - // Chain the new callable onto the queue's tail. - Task toReturn = tail.continueWithTask(executor, newContinuation(callable)); - - // Add a new tail that swallows errors from the callable when it finishes. - tail = ignoreResult(toReturn); - return toReturn; - } - } -} diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java index e26cdee31b9..7726d9e6924 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsController.java @@ -33,6 +33,8 @@ import com.google.firebase.crashlytics.internal.Logger; import com.google.firebase.crashlytics.internal.NativeSessionFileProvider; import com.google.firebase.crashlytics.internal.analytics.AnalyticsEventLogger; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsTasks; +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; @@ -51,7 +53,6 @@ import java.util.Map; import java.util.SortedSet; import java.util.concurrent.Callable; -import java.util.concurrent.Executor; import java.util.concurrent.ScheduledThreadPoolExecutor; import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeoutException; @@ -67,8 +68,6 @@ class CrashlyticsController { static final FilenameFilter APP_EXCEPTION_MARKER_FILTER = (directory, filename) -> filename.startsWith(APP_EXCEPTION_MARKER_PREFIX); - static final String NATIVE_SESSION_DIR = "native-sessions"; - static final int FIREBASE_CRASH_TYPE_FATAL = 1; private static final String GENERATOR_FORMAT = "Crashlytics Android SDK/%s"; @@ -82,7 +81,7 @@ class CrashlyticsController { private final CrashlyticsFileMarker crashMarker; private final UserMetadata userMetadata; - private final CrashlyticsBackgroundWorker backgroundWorker; + private final CrashlyticsWorkers crashlyticsWorkers; private final IdManager idManager; private final FileStore fileStore; @@ -116,7 +115,6 @@ class CrashlyticsController { CrashlyticsController( Context context, - CrashlyticsBackgroundWorker backgroundWorker, IdManager idManager, DataCollectionArbiter dataCollectionArbiter, FileStore fileStore, @@ -127,9 +125,9 @@ class CrashlyticsController { SessionReportingCoordinator sessionReportingCoordinator, CrashlyticsNativeComponent nativeComponent, AnalyticsEventLogger analyticsEventLogger, - CrashlyticsAppQualitySessionsSubscriber sessionsSubscriber) { + CrashlyticsAppQualitySessionsSubscriber sessionsSubscriber, + CrashlyticsWorkers crashlyticsWorkers) { this.context = context; - this.backgroundWorker = backgroundWorker; this.idManager = idManager; this.dataCollectionArbiter = dataCollectionArbiter; this.fileStore = fileStore; @@ -141,10 +139,7 @@ class CrashlyticsController { this.analyticsEventLogger = analyticsEventLogger; this.sessionsSubscriber = sessionsSubscriber; this.reportingCoordinator = sessionReportingCoordinator; - } - - private Context getContext() { - return context; + this.crashlyticsWorkers = crashlyticsWorkers; } // region Exception handling @@ -195,7 +190,7 @@ synchronized void handleUncaughtException( final long timestampMillis = System.currentTimeMillis(); final Task handleUncaughtExceptionTask = - backgroundWorker.submitTask( + crashlyticsWorkers.common.submitTask( new Callable>() { @Override public Task call() throws Exception { @@ -210,7 +205,6 @@ public Task call() throws Exception { // We've fatally crashed, so write the marker file that indicates a crash occurred. crashMarker.create(); - reportingCoordinator.persistFatalEvent( ex, thread, currentSessionId, timestampSeconds); @@ -224,12 +218,10 @@ public Task call() throws Exception { return Tasks.forResult(null); } - Executor executor = backgroundWorker.getExecutor(); - return settingsProvider .getSettingsAsync() .onSuccessTask( - executor, + crashlyticsWorkers.common, new SuccessContinuation() { @NonNull @Override @@ -244,7 +236,8 @@ public Task then(@Nullable Settings settings) throws Exception { return Tasks.whenAll( logAnalyticsAppExceptionEvents(), reportingCoordinator.sendReports( - executor, isOnDemand ? currentSessionId : null)); + crashlyticsWorkers.common, + isOnDemand ? currentSessionId : null)); } }); } @@ -255,6 +248,7 @@ public Task then(@Nullable Settings settings) throws Exception { try { // Never block on ODFs, in case they get triggered from the main thread. if (!isOnDemand) { + //noinspection deprecation drain the worker instead. Utils.awaitEvenIfOnMainThread(handleUncaughtExceptionTask); } } catch (TimeoutException e) { @@ -303,11 +297,12 @@ public Task then(@Nullable Void aVoid) throws Exception { Logger.getLogger().d("Waiting for send/deleteUnsentReports to be called."); // Wait for either the processReports callback to be called, or data collection to be enabled. - return Utils.race(collectionEnabled, reportActionProvided.getTask()); + return CrashlyticsTasks.race(collectionEnabled, reportActionProvided.getTask()); } /** This function must be called before opening the first session * */ boolean didCrashOnPreviousExecution() { + CrashlyticsWorkers.checkBackgroundThread(); // To not violate strict mode. if (!crashMarker.isPresent()) { // Before the first session of this execution is opened, the current session ID still refers // to the previous execution's last session, which is what we want. @@ -344,67 +339,56 @@ Task deleteUnsentReports() { return unsentReportsHandled.getTask(); } - // TODO(b/261014167): Use an explicit executor in continuations. - @SuppressLint("TaskMainThread") - Task submitAllReports(Task settingsDataTask) { + void submitAllReports(Task settingsDataTask) { if (!reportingCoordinator.hasReportsToSend()) { // Just notify the user that there are no reports and stop. Logger.getLogger().v("No crash reports are available to be sent."); unsentReportsAvailable.trySetResult(false); - return Tasks.forResult(null); + return; } Logger.getLogger().v("Crash reports are available to be sent."); - return waitForReportAction() + waitForReportAction() .onSuccessTask( + crashlyticsWorkers.common, new SuccessContinuation() { @NonNull @Override public Task then(@Nullable Boolean send) throws Exception { + if (!send) { + Logger.getLogger().v("Deleting cached crash reports..."); + deleteFiles(listAppExceptionMarkerFiles()); + reportingCoordinator.removeAllReports(); + unsentReportsHandled.trySetResult(null); - return backgroundWorker.submitTask( - new Callable>() { + return Tasks.forResult(null); + } + + Logger.getLogger().d("Sending cached crash reports..."); + + // waitForReportAction guarantees we got user permission. + boolean dataCollectionToken = send; + + // Signal to the settings fetch and onboarding that we have explicit + // permission. + dataCollectionArbiter.grantDataCollectionPermission(dataCollectionToken); + return settingsDataTask.onSuccessTask( + crashlyticsWorkers.common, + new SuccessContinuation() { + @NonNull @Override - public Task call() throws Exception { - if (!send) { - Logger.getLogger().v("Deleting cached crash reports..."); - deleteFiles(listAppExceptionMarkerFiles()); - reportingCoordinator.removeAllReports(); - unsentReportsHandled.trySetResult(null); + public Task then(@Nullable Settings appSettingsData) throws Exception { + if (appSettingsData == null) { + Logger.getLogger() + .w( + "Received null app settings at app startup. Cannot send cached reports"); return Tasks.forResult(null); } + logAnalyticsAppExceptionEvents(); + reportingCoordinator.sendReports(crashlyticsWorkers.common); + unsentReportsHandled.trySetResult(null); - Logger.getLogger().d("Sending cached crash reports..."); - - // waitForReportAction guarantees we got user permission. - boolean dataCollectionToken = send; - - // Signal to the settings fetch and onboarding that we have explicit - // permission. - dataCollectionArbiter.grantDataCollectionPermission(dataCollectionToken); - - Executor executor = backgroundWorker.getExecutor(); - - return settingsDataTask.onSuccessTask( - executor, - new SuccessContinuation() { - @NonNull - @Override - public Task then(@Nullable Settings appSettingsData) - throws Exception { - if (appSettingsData == null) { - Logger.getLogger() - .w( - "Received null app settings at app startup. Cannot send cached reports"); - return Tasks.forResult(null); - } - logAnalyticsAppExceptionEvents(); - reportingCoordinator.sendReports(executor); - unsentReportsHandled.trySetResult(null); - - return Tasks.forResult(null); - } - }); + return Tasks.forResult(null); } }); } @@ -415,16 +399,9 @@ public Task then(@Nullable Settings appSettingsData) /** Log a timestamped string to the log file. */ void writeToLog(final long timestamp, final String msg) { - backgroundWorker.submit( - new Callable() { - @Override - public Void call() throws Exception { - if (!isHandlingException()) { - logFileManager.writeToLog(timestamp, msg); - } - return null; - } - }); + if (!isHandlingException()) { + logFileManager.writeToLog(timestamp, msg); + } } /** Log a caught exception - write out Throwable as event section of protobuf */ @@ -433,23 +410,15 @@ void writeNonFatalException(@NonNull final Thread thread, @NonNull final Throwab // rather than the time at which the task executes. final long timestampMillis = System.currentTimeMillis(); - backgroundWorker.submit( - new Runnable() { - @Override - public void run() { - if (!isHandlingException()) { - long timestampSeconds = getTimestampSeconds(timestampMillis); - final String currentSessionId = getCurrentSessionId(); - if (currentSessionId == null) { - Logger.getLogger() - .w("Tried to write a non-fatal exception while no session was open."); - return; - } - reportingCoordinator.persistNonFatalEvent( - ex, thread, currentSessionId, timestampSeconds); - } - } - }); + if (!isHandlingException()) { + long timestampSeconds = getTimestampSeconds(timestampMillis); + final String currentSessionId = getCurrentSessionId(); + if (currentSessionId == null) { + Logger.getLogger().w("Tried to write a non-fatal exception while no session was open."); + return; + } + reportingCoordinator.persistNonFatalEvent(ex, thread, currentSessionId, timestampSeconds); + } } void logFatalException(Thread thread, Throwable ex) { @@ -498,14 +467,8 @@ void setInternalKey(String key, String value) { /** Open a new session on the single-threaded executor. */ void openSession(String sessionIdentifier) { - backgroundWorker.submit( - new Callable() { - @Override - public Void call() throws Exception { - doOpenSession(sessionIdentifier, /* isOnDemand= */ false); - return null; - } - }); + crashlyticsWorkers.common.submit( + () -> doOpenSession(sessionIdentifier, /* isOnDemand= */ false)); } /** @@ -531,7 +494,7 @@ private String getCurrentSessionId() { * @param settingsProvider */ boolean finalizeSessions(SettingsProvider settingsProvider) { - backgroundWorker.checkRunningOnThread(); + CrashlyticsWorkers.checkBackgroundThread(); if (isHandlingException()) { Logger.getLogger().w("Skipping session finalization because a crash has already occurred."); @@ -540,7 +503,7 @@ boolean finalizeSessions(SettingsProvider settingsProvider) { Logger.getLogger().v("Finalizing previously open sessions."); try { - doCloseSessions(true, settingsProvider); + doCloseSessions(true, settingsProvider, true); } catch (Exception e) { Logger.getLogger().e("Unable to finalize previously open sessions.", e); return false; @@ -585,15 +548,18 @@ private void doOpenSession(String sessionIdentifier, Boolean isOnDemand) { reportingCoordinator.onBeginSession(sessionIdentifier, startedAtSeconds); } + // This is only used for exception handler close session (we have another close session in + // background initialization) void doCloseSessions(SettingsProvider settingsProvider) { - doCloseSessions(false, settingsProvider); + doCloseSessions(false, settingsProvider, false); } /** - * Not synchronized/locked. Must be executed from the single thread executor service used by this - * class. + * Not synchronized/locked. Must be executed from the executor service runs tasks in serial order */ - private void doCloseSessions(boolean skipCurrentSession, SettingsProvider settingsProvider) { + private void doCloseSessions( + boolean skipCurrentSession, SettingsProvider settingsProvider, boolean isInitProcess) { + CrashlyticsWorkers.checkBackgroundThread(); final int offset = skipCurrentSession ? 1 : 0; // :TODO HW2021 this implementation can be cleaned up. @@ -607,13 +573,15 @@ private void doCloseSessions(boolean skipCurrentSession, SettingsProvider settin final String mostRecentSessionIdToClose = sortedOpenSessions.get(offset); - if (settingsProvider.getSettingsSync().featureFlagData.collectAnrs) { + // We only collect ANR info for finalize report during initialization process + if (isInitProcess && settingsProvider.getSettingsSync().featureFlagData.collectAnrs) { writeApplicationExitInfoEventIfRelevant(mostRecentSessionIdToClose); } else { Logger.getLogger().v("ANR feature disabled."); } - if (nativeComponent.hasCrashDataForSession(mostRecentSessionIdToClose)) { + // We only collect native crash info for finalize report during initialization process + if (isInitProcess && nativeComponent.hasCrashDataForSession(mostRecentSessionIdToClose)) { // We only finalize the current session if it's a Java crash, so only finalize native crash // data when we aren't including current. finalizePreviousNativeSession(mostRecentSessionIdToClose); @@ -938,7 +906,7 @@ private void writeApplicationExitInfoEventIfRelevant(String sessionId) { if (applicationExitInfoList.size() != 0) { final LogFileManager relevantSessionLogManager = new LogFileManager(fileStore, sessionId); final UserMetadata relevantUserMetadata = - UserMetadata.loadFromExistingSession(sessionId, fileStore, backgroundWorker); + UserMetadata.loadFromExistingSession(sessionId, fileStore, crashlyticsWorkers); reportingCoordinator.persistRelevantAppExitInfoEvent( sessionId, applicationExitInfoList, relevantSessionLogManager, relevantUserMetadata); } else { diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCore.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCore.java index 9e8af2f4898..8e451762642 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCore.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCore.java @@ -21,7 +21,6 @@ import androidx.annotation.Nullable; import androidx.annotation.VisibleForTesting; import com.google.android.gms.tasks.Task; -import com.google.android.gms.tasks.Tasks; import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.firebase.FirebaseApp; import com.google.firebase.crashlytics.BuildConfig; @@ -30,6 +29,7 @@ import com.google.firebase.crashlytics.internal.RemoteConfigDeferredProxy; import com.google.firebase.crashlytics.internal.analytics.AnalyticsEventLogger; import com.google.firebase.crashlytics.internal.breadcrumbs.BreadcrumbSource; +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.persistence.FileStore; @@ -39,9 +39,7 @@ import com.google.firebase.crashlytics.internal.stacktrace.RemoveRepeatsStrategy; import com.google.firebase.crashlytics.internal.stacktrace.StackTraceTrimmingStrategy; import java.util.Map; -import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; -import java.util.concurrent.ExecutorService; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; @@ -93,14 +91,14 @@ public class CrashlyticsCore { @VisibleForTesting public final BreadcrumbSource breadcrumbSource; private final AnalyticsEventLogger analyticsEventLogger; - private final ExecutorService crashHandlerExecutor; - private final CrashlyticsBackgroundWorker backgroundWorker; private final CrashlyticsAppQualitySessionsSubscriber sessionsSubscriber; private final CrashlyticsNativeComponent nativeComponent; private final RemoteConfigDeferredProxy remoteConfigDeferredProxy; + private final CrashlyticsWorkers crashlyticsWorkers; + // region Constructors public CrashlyticsCore( @@ -111,9 +109,9 @@ public CrashlyticsCore( BreadcrumbSource breadcrumbSource, AnalyticsEventLogger analyticsEventLogger, FileStore fileStore, - ExecutorService crashHandlerExecutor, CrashlyticsAppQualitySessionsSubscriber sessionsSubscriber, - RemoteConfigDeferredProxy remoteConfigDeferredProxy) { + RemoteConfigDeferredProxy remoteConfigDeferredProxy, + CrashlyticsWorkers crashlyticsWorkers) { this.app = app; this.dataCollectionArbiter = dataCollectionArbiter; this.context = app.getApplicationContext(); @@ -121,11 +119,10 @@ public CrashlyticsCore( this.nativeComponent = nativeComponent; this.breadcrumbSource = breadcrumbSource; this.analyticsEventLogger = analyticsEventLogger; - this.crashHandlerExecutor = crashHandlerExecutor; this.fileStore = fileStore; - this.backgroundWorker = new CrashlyticsBackgroundWorker(crashHandlerExecutor); this.sessionsSubscriber = sessionsSubscriber; this.remoteConfigDeferredProxy = remoteConfigDeferredProxy; + this.crashlyticsWorkers = crashlyticsWorkers; startTime = System.currentTimeMillis(); onDemandCounter = new OnDemandCounter(); @@ -154,7 +151,7 @@ public boolean onPreExecute(AppData appData, SettingsProvider settingsProvider) initializationMarker = new CrashlyticsFileMarker(INITIALIZATION_MARKER_FILE_NAME, fileStore); final UserMetadata userMetadata = - new UserMetadata(sessionIdentifier, fileStore, backgroundWorker); + new UserMetadata(sessionIdentifier, fileStore, crashlyticsWorkers); final LogFileManager logFileManager = new LogFileManager(fileStore); final StackTraceTrimmingStrategy stackTraceTrimmingStrategy = new MiddleOutFallbackStrategy( @@ -173,12 +170,12 @@ public boolean onPreExecute(AppData appData, SettingsProvider settingsProvider) stackTraceTrimmingStrategy, settingsProvider, onDemandCounter, - sessionsSubscriber); + sessionsSubscriber, + crashlyticsWorkers); controller = new CrashlyticsController( context, - backgroundWorker, idManager, dataCollectionArbiter, fileStore, @@ -189,7 +186,8 @@ public boolean onPreExecute(AppData appData, SettingsProvider settingsProvider) sessionReportingCoordinator, nativeComponent, analyticsEventLogger, - sessionsSubscriber); + sessionsSubscriber, + crashlyticsWorkers); // If the file is present at this point, then the previous run's initialization // did not complete, and we want to perform initialization synchronously this time. @@ -223,22 +221,15 @@ public boolean onPreExecute(AppData appData, SettingsProvider settingsProvider) return true; } - /** Performs background initialization asynchronously on the background worker's thread. */ + /** Performs background initialization asynchronously on the common worker. */ @CanIgnoreReturnValue public Task doBackgroundInitializationAsync(SettingsProvider settingsProvider) { - return Utils.callTask( - crashHandlerExecutor, - new Callable>() { - @Override - public Task call() throws Exception { - return doBackgroundInitialization(settingsProvider); - } - }); + return crashlyticsWorkers.common.submit(() -> doBackgroundInitialization(settingsProvider)); } /** Performs background initialization synchronously on the calling thread. */ - @CanIgnoreReturnValue - private Task doBackgroundInitialization(SettingsProvider settingsProvider) { + private void doBackgroundInitialization(SettingsProvider settingsProvider) { + CrashlyticsWorkers.checkBackgroundThread(); // create the marker for this run markInitializationStarted(); @@ -253,22 +244,17 @@ private Task doBackgroundInitialization(SettingsProvider settingsProvider) Logger.getLogger().d("Collection of crash reports disabled in Crashlytics settings."); // TODO: This isn't actually an error condition, so figure out the right way to // handle this case. - return Tasks.forException( - new RuntimeException("Collection of crash reports disabled in Crashlytics settings.")); + throw new RuntimeException("Collection of crash reports disabled in Crashlytics settings."); } if (!controller.finalizeSessions(settingsProvider)) { Logger.getLogger().w("Previous sessions could not be finalized."); } - // TODO: Move this call out of this method, so that the return value merely indicates - // initialization is complete. Callers that want to know when report sending is complete can - // handle that as a separate call. - return controller.submitAllReports(settingsProvider.getSettingsAsync()); + controller.submitAllReports(settingsProvider.getSettingsAsync()); } catch (Exception e) { Logger.getLogger() .e("Crashlytics encountered a problem during asynchronous initialization.", e); - return Tasks.forException(e); } finally { // The only thing that compels us to leave the marker and start synchronously next time // is not executing all the way through this method. That would indicate that we perhaps @@ -328,7 +314,8 @@ public static String getVersion() { * safe to invoke this method from the main thread. */ public void logException(@NonNull Throwable throwable) { - controller.writeNonFatalException(Thread.currentThread(), throwable); + crashlyticsWorkers.common.submit( + () -> controller.writeNonFatalException(Thread.currentThread(), throwable)); } /** @@ -343,11 +330,15 @@ public void logException(@NonNull Throwable throwable) { */ public void log(final String msg) { final long timestamp = System.currentTimeMillis() - startTime; - controller.writeToLog(timestamp, msg); + // queuing up on common worker to maintain the order + crashlyticsWorkers.common.submit( + () -> { + crashlyticsWorkers.diskWrite.submit(() -> controller.writeToLog(timestamp, msg)); + }); } public void setUserId(String identifier) { - controller.setUserId(identifier); + crashlyticsWorkers.common.submit(() -> controller.setUserId(identifier)); } /** @@ -360,7 +351,7 @@ public void setUserId(String identifier) { * @throws NullPointerException if key is null. */ public void setCustomKey(String key, String value) { - controller.setCustomKey(key, value); + crashlyticsWorkers.common.submit(() -> controller.setCustomKey(key, value)); } /** @@ -377,7 +368,7 @@ public void setCustomKey(String key, String value) { * @throws NullPointerException if any key in keysAndValues is null. */ public void setCustomKeys(Map keysAndValues) { - controller.setCustomKeys(keysAndValues); + crashlyticsWorkers.common.submit(() -> controller.setCustomKeys(keysAndValues)); } // endregion @@ -397,7 +388,7 @@ public void setCustomKeys(Map keysAndValues) { * @throws NullPointerException if key is null. */ public void setInternalKey(String key, String value) { - controller.setInternalKey(key, value); + crashlyticsWorkers.common.submit(() -> controller.setInternalKey(key, value)); } /** Logs a fatal Throwable on the Crashlytics servers on-demand. */ @@ -406,11 +397,16 @@ public void logFatalException(Throwable throwable) { .d("Recorded on-demand fatal events: " + onDemandCounter.getRecordedOnDemandExceptions()); Logger.getLogger() .d("Dropped on-demand fatal events: " + onDemandCounter.getDroppedOnDemandExceptions()); - controller.setInternalKey( - ON_DEMAND_RECORDED_KEY, Integer.toString(onDemandCounter.getRecordedOnDemandExceptions())); - controller.setInternalKey( - ON_DEMAND_DROPPED_KEY, Integer.toString(onDemandCounter.getDroppedOnDemandExceptions())); - controller.logFatalException(Thread.currentThread(), throwable); + crashlyticsWorkers.common.submit( + () -> { + controller.setInternalKey( + ON_DEMAND_RECORDED_KEY, + Integer.toString(onDemandCounter.getRecordedOnDemandExceptions())); + controller.setInternalKey( + ON_DEMAND_DROPPED_KEY, + Integer.toString(onDemandCounter.getDroppedOnDemandExceptions())); + controller.logFatalException(Thread.currentThread(), throwable); + }); } // endregion @@ -427,19 +423,14 @@ CrashlyticsController getController() { /** * When a startup crash occurs, Crashlytics must lock on the main thread and complete - * initializaiton to upload crash result. 4 seconds is chosen for the lock to prevent ANR + * initialization to upload crash result. 4 seconds is chosen for the lock to prevent ANR */ private void finishInitSynchronously(SettingsProvider settingsProvider) { - - final Runnable runnable = - new Runnable() { - @Override - public void run() { - doBackgroundInitialization(settingsProvider); - } - }; - - final Future future = crashHandlerExecutor.submit(runnable); + Future future = + crashlyticsWorkers + .common + .getExecutor() + .submit(() -> doBackgroundInitialization(settingsProvider)); Logger.getLogger() .d( @@ -448,18 +439,19 @@ public void run() { try { future.get(DEFAULT_MAIN_HANDLER_TIMEOUT_SEC, TimeUnit.SECONDS); - } catch (InterruptedException e) { - Logger.getLogger().e("Crashlytics was interrupted during initialization.", e); - } catch (ExecutionException e) { - Logger.getLogger().e("Crashlytics encountered a problem during initialization.", e); - } catch (TimeoutException e) { - Logger.getLogger().e("Crashlytics timed out during initialization.", e); + } catch (InterruptedException ex) { + Logger.getLogger().e("Crashlytics was interrupted during initialization.", ex); + Thread.currentThread().interrupt(); + } catch (ExecutionException ex) { + Logger.getLogger().e("Crashlytics encountered a problem during initialization.", ex); + } catch (TimeoutException ex) { + Logger.getLogger().e("Crashlytics timed out during initialization.", ex); } } /** Synchronous call to mark start of initialization */ void markInitializationStarted() { - backgroundWorker.checkRunningOnThread(); + CrashlyticsWorkers.checkBackgroundThread(); // Create the Crashlytics initialization marker file, which is used to determine // whether the app crashed before initialization could complete. @@ -469,23 +461,15 @@ void markInitializationStarted() { /** Enqueues a job to remove the Crashlytics initialization marker file */ void markInitializationComplete() { - backgroundWorker.submit( - new Callable() { - @Override - public Boolean call() throws Exception { - try { - final boolean removed = initializationMarker.remove(); - if (!removed) { - Logger.getLogger().w("Initialization marker file was not properly removed."); - } - return removed; - } catch (Exception e) { - Logger.getLogger() - .e("Problem encountered deleting Crashlytics initialization marker.", e); - return false; - } - } - }); + CrashlyticsWorkers.checkBackgroundThread(); + try { + boolean removed = initializationMarker.remove(); + if (!removed) { + Logger.getLogger().w("Initialization marker file was not properly removed."); + } + } catch (Exception ex) { + Logger.getLogger().e("Problem encountered deleting Crashlytics initialization marker.", ex); + } } boolean didPreviousInitializationFail() { @@ -495,19 +479,16 @@ boolean didPreviousInitializationFail() { // region Previous crash handling private void checkForPreviousCrash() { - Task task = - backgroundWorker.submit( - new Callable() { - @Override - public Boolean call() throws Exception { - return controller.didCrashOnPreviousExecution(); - } - }); + Future future = + crashlyticsWorkers + .common + .getExecutor() + .submit(() -> controller.didCrashOnPreviousExecution()); Boolean result; try { - result = Utils.awaitEvenIfOnMainThread(task); - } catch (Exception e) { + result = future.get(DEFAULT_MAIN_HANDLER_TIMEOUT_SEC, TimeUnit.SECONDS); + } catch (Exception ignored) { didCrashOnPreviousExecution = false; return; } diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsLifecycleEvents.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsLifecycleEvents.java deleted file mode 100644 index 3ddc3d3ab6a..00000000000 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsLifecycleEvents.java +++ /dev/null @@ -1,51 +0,0 @@ -// Copyright 2020 Google LLC -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.firebase.crashlytics.internal.common; - -import androidx.annotation.NonNull; - -/** This class defines Crashlytics lifecycle events */ -interface CrashlyticsLifecycleEvents { - - /** - * Called when a new session is opened. - * - * @param sessionId the identifier for the new session - */ - void onBeginSession(@NonNull String sessionId, long timestamp); - - /** - * Called when a message is logged by the user. - * - * @param timestamp the timestamp of the message (in milliseconds since app launch) - * @param log the log message - */ - void onLog(long timestamp, String log); - - /** - * Called when a custom key is set by the user. - * - * @param key - * @param value - */ - void onCustomKey(String key, String value); - - /** - * Called when a user ID is set by the user. - * - * @param userId - */ - void onUserId(String userId); -} diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/DataCollectionArbiter.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/DataCollectionArbiter.java index 84493f7b355..157722753fd 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/DataCollectionArbiter.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/DataCollectionArbiter.java @@ -24,7 +24,7 @@ import com.google.android.gms.tasks.TaskCompletionSource; import com.google.firebase.FirebaseApp; import com.google.firebase.crashlytics.internal.Logger; -import java.util.concurrent.Executor; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsTasks; // Determines whether automatic data collection is enabled. public class DataCollectionArbiter { @@ -129,11 +129,9 @@ public Task waitForAutomaticDataCollectionEnabled() { * Returns a task which will be resolved when either: 1) automatic data collection has been * enabled, or 2) grantDataCollectionPermission has been called. */ - public Task waitForDataCollectionPermission(Executor executor) { - return Utils.race( - executor, - dataCollectionExplicitlyApproved.getTask(), - waitForAutomaticDataCollectionEnabled()); + public Task waitForDataCollectionPermission() { + return CrashlyticsTasks.race( + dataCollectionExplicitlyApproved.getTask(), waitForAutomaticDataCollectionEnabled()); } /** diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/IdManager.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/IdManager.java index 8a985e2f3c5..25877d8d773 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/IdManager.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/IdManager.java @@ -14,13 +14,15 @@ package com.google.firebase.crashlytics.internal.common; -import static com.google.firebase.crashlytics.internal.common.Utils.awaitEvenIfOnMainThread; +import static java.util.concurrent.TimeUnit.MILLISECONDS; import android.content.Context; import android.content.SharedPreferences; import android.os.Build; import androidx.annotation.NonNull; +import com.google.android.gms.tasks.Tasks; import com.google.firebase.crashlytics.internal.Logger; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers; import com.google.firebase.installations.FirebaseInstallationsApi; import java.util.Locale; import java.util.Objects; @@ -28,6 +30,7 @@ import java.util.regex.Pattern; public class IdManager implements InstallIdProvider { + private static final int TIMEOUT_MILLIS = 10_000; public static final String DEFAULT_VERSION_NAME = "0.0"; @@ -179,19 +182,22 @@ private String readCachedCrashlyticsInstallId(SharedPreferences prefs) { */ @NonNull public FirebaseInstallationId fetchTrueFid(boolean validate) { + CrashlyticsWorkers.checkNotMainThread(); // This fetch blocks, never do it on main. String fid = null; String authToken = null; if (validate) { // Fetch the auth token first when requested, so the fid will be validated. try { - authToken = awaitEvenIfOnMainThread(firebaseInstallations.getToken(false)).getToken(); + authToken = + Tasks.await(firebaseInstallations.getToken(false), TIMEOUT_MILLIS, MILLISECONDS) + .getToken(); } catch (Exception ex) { Logger.getLogger().w("Error getting Firebase authentication token.", ex); } } try { - fid = awaitEvenIfOnMainThread(firebaseInstallations.getId()); + fid = Tasks.await(firebaseInstallations.getId(), TIMEOUT_MILLIS, MILLISECONDS); } catch (Exception ex) { Logger.getLogger().w("Error getting Firebase installation id.", ex); } @@ -213,7 +219,9 @@ private synchronized String createAndCacheCrashlyticsInstallId( return iid; } - /** @return the package name that identifies this App. */ + /** + * @return the package name that identifies this App. + */ public String getAppIdentifier() { return appIdentifier; } diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinator.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinator.java index 4018d3c0394..29e2301591a 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinator.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinator.java @@ -24,6 +24,7 @@ import com.google.android.gms.tasks.Task; import com.google.android.gms.tasks.Tasks; import com.google.firebase.crashlytics.internal.Logger; +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; @@ -47,10 +48,10 @@ import java.util.concurrent.Executor; /** - * This class handles Crashlytics lifecycle events and coordinates session data capture and - * persistence, as well as sending of reports to Firebase Crashlytics. + * This class coordinates Crashlytics session data capture and persistence, as well as sending of + * reports to Firebase Crashlytics. */ -public class SessionReportingCoordinator implements CrashlyticsLifecycleEvents { +public class SessionReportingCoordinator { private static final String EVENT_TYPE_CRASH = "crash"; private static final String EVENT_TYPE_LOGGED = "error"; @@ -68,7 +69,8 @@ public static SessionReportingCoordinator create( StackTraceTrimmingStrategy stackTraceTrimmingStrategy, SettingsProvider settingsProvider, OnDemandCounter onDemandCounter, - CrashlyticsAppQualitySessionsSubscriber sessionsSubscriber) { + CrashlyticsAppQualitySessionsSubscriber sessionsSubscriber, + CrashlyticsWorkers crashlyticsWorkers) { final CrashlyticsReportDataCapture dataCapture = new CrashlyticsReportDataCapture( context, idManager, appData, stackTraceTrimmingStrategy, settingsProvider); @@ -77,7 +79,13 @@ public static SessionReportingCoordinator create( final DataTransportCrashlyticsReportSender reportSender = DataTransportCrashlyticsReportSender.create(context, settingsProvider, onDemandCounter); return new SessionReportingCoordinator( - dataCapture, reportPersistence, reportSender, logFileManager, userMetadata, idManager); + dataCapture, + reportPersistence, + reportSender, + logFileManager, + userMetadata, + idManager, + crashlyticsWorkers); } private final CrashlyticsReportDataCapture dataCapture; @@ -87,22 +95,25 @@ public static SessionReportingCoordinator create( private final UserMetadata reportMetadata; private final IdManager idManager; + private final CrashlyticsWorkers crashlyticsWorkers; + SessionReportingCoordinator( CrashlyticsReportDataCapture dataCapture, CrashlyticsReportPersistence reportPersistence, DataTransportCrashlyticsReportSender reportsSender, LogFileManager logFileManager, UserMetadata reportMetadata, - IdManager idManager) { + IdManager idManager, + CrashlyticsWorkers crashlyticsWorkers) { this.dataCapture = dataCapture; this.reportPersistence = reportPersistence; this.reportsSender = reportsSender; this.logFileManager = logFileManager; this.reportMetadata = reportMetadata; this.idManager = idManager; + this.crashlyticsWorkers = crashlyticsWorkers; } - @Override public void onBeginSession(@NonNull String sessionId, long timestampSeconds) { final CrashlyticsReport capturedReport = dataCapture.captureReportData(sessionId, timestampSeconds); @@ -110,21 +121,6 @@ public void onBeginSession(@NonNull String sessionId, long timestampSeconds) { reportPersistence.persistReport(capturedReport); } - @Override - public void onLog(long timestamp, String log) { - logFileManager.writeToLog(timestamp, log); - } - - @Override - public void onCustomKey(String key, String value) { - reportMetadata.setCustomKey(key, value); - } - - @Override - public void onUserId(String userId) { - reportMetadata.setUserId(userId); - } - public void persistFatalEvent( @NonNull Throwable event, @NonNull Thread thread, @NonNull String sessionId, long timestamp) { Logger.getLogger().v("Persisting fatal event for session " + sessionId); @@ -326,7 +322,7 @@ private void persistEvent( @NonNull String sessionId, @NonNull String eventType, long timestamp, - boolean includeAllThreads) { + boolean isFatal) { final boolean isHighPriority = eventType.equals(EVENT_TYPE_CRASH); @@ -338,9 +334,20 @@ private void persistEvent( timestamp, EVENT_THREAD_IMPORTANCE, MAX_CHAINED_EXCEPTION_DEPTH, - includeAllThreads); + isFatal); + CrashlyticsReport.Session.Event finallizedEvent = addMetaDataToEvent(capturedEvent); + + // Non-fatal, persistence write task we move to diskWriteWorker + if (!isFatal) { + crashlyticsWorkers.diskWrite.submit( + () -> { + Logger.getLogger().d("disk worker: log non-fatal event to persistence"); + reportPersistence.persistEvent(finallizedEvent, sessionId, isHighPriority); + }); + return; + } - reportPersistence.persistEvent(addMetaDataToEvent(capturedEvent), sessionId, isHighPriority); + reportPersistence.persistEvent(finallizedEvent, sessionId, isHighPriority); } private boolean onReportSendComplete(@NonNull Task task) { diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/Utils.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/Utils.java index 8e8f19d056a..c4ca0c90274 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/Utils.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/Utils.java @@ -14,16 +14,11 @@ package com.google.firebase.crashlytics.internal.common; -import android.annotation.SuppressLint; import android.os.Looper; -import com.google.android.gms.tasks.Continuation; import com.google.android.gms.tasks.Task; -import com.google.android.gms.tasks.TaskCompletionSource; import com.google.errorprone.annotations.CanIgnoreReturnValue; -import java.util.concurrent.Callable; import java.util.concurrent.CancellationException; import java.util.concurrent.CountDownLatch; -import java.util.concurrent.Executor; import java.util.concurrent.ExecutorService; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; @@ -35,72 +30,7 @@ public final class Utils { private static final int BACKGROUND_TIMEOUT_MILLIS = 4_000; /** Timeout in milliseconds for blocking on the main thread. Be careful about ANRs. */ - private static final int MAIN_TIMEOUT_MILLIS = 2_750; - - /** - * @return A tasks that is resolved when either of the given tasks is resolved. - */ - // TODO(b/261014167): Use an explicit executor in continuations. - @SuppressLint("TaskMainThread") - public static Task race(Task t1, Task t2) { - final TaskCompletionSource result = new TaskCompletionSource<>(); - Continuation continuation = - task -> { - if (task.isSuccessful()) { - result.trySetResult(task.getResult()); - } else if (task.getException() != null) { - result.trySetException(task.getException()); - } - return null; - }; - t1.continueWith(continuation); - t2.continueWith(continuation); - return result.getTask(); - } - - /** - * @return A tasks that is resolved when either of the given tasks is resolved. - */ - public static Task race(Executor executor, Task t1, Task t2) { - final TaskCompletionSource result = new TaskCompletionSource<>(); - Continuation continuation = - task -> { - if (task.isSuccessful()) { - result.trySetResult(task.getResult()); - } else if (task.getException() != null) { - result.trySetException(task.getException()); - } - return null; - }; - t1.continueWith(executor, continuation); - t2.continueWith(executor, continuation); - return result.getTask(); - } - - /** Similar to Tasks.call, but takes a Callable that returns a Task. */ - public static Task callTask(Executor executor, Callable> callable) { - final TaskCompletionSource result = new TaskCompletionSource<>(); - executor.execute( - () -> { - try { - callable - .call() - .continueWith( - executor, - task -> { - if (task.isSuccessful()) { - result.setResult(task.getResult()); - } else if (task.getException() != null) { - result.setException(task.getException()); - } - return null; - }); - } catch (Exception e) { - result.setException(e); - } - }); - return result.getTask(); - } + private static final int MAIN_TIMEOUT_MILLIS = 3_000; /** * Blocks until the given Task completes, and then returns the value the Task was resolved with, @@ -114,7 +44,9 @@ public static Task callTask(Executor executor, Callable> callable * @return the value that was returned by the task, if successful. * @throws InterruptedException if the method was interrupted * @throws TimeoutException if the method timed out while waiting for the task. + * @deprecated Don't use this. Drain the worker instead. */ + @Deprecated public static T awaitEvenIfOnMainThread(Task task) throws InterruptedException, TimeoutException { CountDownLatch latch = new CountDownLatch(1); diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsTasks.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsTasks.java new file mode 100644 index 00000000000..61b2766197f --- /dev/null +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsTasks.java @@ -0,0 +1,69 @@ +/* + * Copyright 2024 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.firebase.crashlytics.internal.concurrency; + +import com.google.android.gms.tasks.CancellationTokenSource; +import com.google.android.gms.tasks.Continuation; +import com.google.android.gms.tasks.Task; +import com.google.android.gms.tasks.TaskCompletionSource; +import com.google.android.gms.tasks.Tasks; +import java.util.concurrent.Executor; +import java.util.concurrent.atomic.AtomicBoolean; + +/** + * Crashlytics specific utilities for dealing with Tasks. + * + * @hide + */ +public final class CrashlyticsTasks { + /** An Executor that runs on the calling thread. */ + private static final Executor DIRECT = Runnable::run; + + /** + * Returns a task that is resolved when either of the given tasks is resolved. + * + *

If both tasks are cancelled, the returned task will be cancelled. + */ + public static Task race(Task task1, Task task2) { + CancellationTokenSource cancellation = new CancellationTokenSource(); + TaskCompletionSource result = new TaskCompletionSource<>(cancellation.getToken()); + + AtomicBoolean otherTaskCancelled = new AtomicBoolean(false); + + Continuation> continuation = + task -> { + if (task.isSuccessful()) { + // Task is complete and successful. + result.trySetResult(task.getResult()); + } else if (task.getException() != null) { + // Task is complete but unsuccessful. + result.trySetException(task.getException()); + } else if (otherTaskCancelled.getAndSet(true)) { + // Both tasks are cancelled. + cancellation.cancel(); + } + return Tasks.forResult(null); + }; + + task1.continueWithTask(DIRECT, continuation); + task2.continueWithTask(DIRECT, continuation); + + return result.getTask(); + } + + private CrashlyticsTasks() {} +} diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsWorker.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsWorker.java new file mode 100644 index 00000000000..615c52389a1 --- /dev/null +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsWorker.java @@ -0,0 +1,212 @@ +/* + * Copyright 2024 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.firebase.crashlytics.internal.concurrency; + +import androidx.annotation.Discouraged; +import androidx.annotation.VisibleForTesting; +import com.google.android.gms.tasks.Continuation; +import com.google.android.gms.tasks.SuccessContinuation; +import com.google.android.gms.tasks.Task; +import com.google.android.gms.tasks.Tasks; +import com.google.errorprone.annotations.CanIgnoreReturnValue; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.Executor; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; + +/** + * Worker for executing tasks sequentially on the given executor service. + * + *

Work on the queue may suspend, or it may return a Task, such that the underlying thread may be + * re-used while the worker queue is suspended. + * + *

Work enqueued on this worker will be run serially, regardless of the underlying executor. + * Therefore, workers on the queue should not add new work to the queue and then block on it, as + * that would create a deadlock. In such a case, the worker can return a Task that depends on the + * future work, and run the future work on the executor's thread, but not put it in the queue as its + * own worker. + * + * @hide + */ +public class CrashlyticsWorker implements Executor { + private final ExecutorService executor; + + private final Object tailLock = new Object(); + private Task tail = Tasks.forResult(null); + + CrashlyticsWorker(ExecutorService executor) { + this.executor = executor; + } + + /** Returns the executor used by this worker. */ + public ExecutorService getExecutor() { + return executor; + } + + /** + * Submits a Callable task for asynchronous execution on the executor. + * + *

A blocking callable will block an underlying thread. + * + *

Returns a Task which will be resolved upon successful completion of the + * callable, or throws an ExecutionException if the callable throws an exception. + */ + @CanIgnoreReturnValue + public Task submit(Callable callable) { + synchronized (tailLock) { + // Chain the new callable onto the queue's tail. + Task result = tail.continueWithTask(executor, task -> Tasks.forResult(callable.call())); + tail = result; + return result; + } + } + + /** + * Submits a Runnable task for asynchronous execution on the executor. + * + *

A blocking runnable will block an underlying thread. + * + *

Returns a Task which will be resolved with null upon successful completion of + * the runnable, or throws an ExecutionException if the runnable throws an exception. + */ + @CanIgnoreReturnValue + public Task submit(Runnable runnable) { + synchronized (tailLock) { + // Chain the new runnable onto the queue's tail. + Task result = + tail.continueWithTask( + executor, + task -> { + runnable.run(); + return Tasks.forResult(null); + }); + tail = result; + return result; + } + } + + /** + * Submits a Callable Task for asynchronous execution on the executor. + * + *

This is useful for making the worker suspend on an asynchronous operation, while letting the + * underlying threads be re-used. + * + *

Returns a Task which will be resolved upon successful completion of the Task + * returned by the callable, throws an ExecutionException if the callable throws an + * exception, or throws a CancellationException if the task is cancelled. + */ + @CanIgnoreReturnValue + public Task submitTask(Callable> callable) { + synchronized (tailLock) { + // Chain the new callable task onto the queue's tail. + Task result = tail.continueWithTask(executor, task -> callable.call()); + tail = result; + return result; + } + } + + /** + * Submits a Callable Task followed by a Continuation for asynchronous + * execution on the executor. + * + *

This is useful for submitting a task that must be immediately followed by another task, + * regardless of more tasks being submitted in parallel. For example, settings. + * + *

Returns a Task which will be resolved upon successful completion of the Task + * returned by the callable and continued by the continuation, throws an ExecutionException + * if either task throws an exception, or throws a CancellationException if + * either task is cancelled. + */ + @CanIgnoreReturnValue + public Task submitTask( + Callable> callable, Continuation> continuation) { + synchronized (tailLock) { + // Chain the new callable task and continuation onto the queue's tail. + Task result = + tail.continueWithTask(executor, task -> callable.call()) + .continueWithTask(executor, continuation); + tail = result; + return result; + } + } + + /** + * Submits a Callable Task followed by a SuccessContinuation for + * asynchronous execution on the executor. + * + *

This is useful for submitting a task that must be immediately followed by another task, only + * if it was successful, but regardless of more tasks being submitted in parallel. + * + *

Returns a Task which will be resolved upon successful completion of the Task + * returned by the callable and continued by the continuation, throws an ExecutionException + * if either task throws an exception, or throws a CancellationException if + * the task is cancelled. + */ + @CanIgnoreReturnValue + public Task submitTaskOnSuccess( + Callable> callable, SuccessContinuation successContinuation) { + synchronized (tailLock) { + // Chain the new callable task and success continuation onto the queue's tail. + Task result = + tail.continueWithTask(executor, task -> callable.call()) + .continueWithTask( + executor, + task -> { + if (task.isSuccessful()) { + return successContinuation.then(task.getResult()); + } else if (task.getException() != null) { + return Tasks.forException(task.getException()); + } else { + return Tasks.forCanceled(); + } + }); + tail = result; + return result; + } + } + + /** + * Forwards a Runnable to the underlying executor. + * + *

This is useful for passing the worker as the executor to task continuations. + * + *

This is different than {@link #submit(Runnable)}. This will not submit the runnable to the + * worker to execute in order, this will forward the runnable to the underlying executor. If you + * are calling this directly from your code, you probably want {@link #submit(Runnable)}. + */ + @Override + @Discouraged(message = "This is probably not that you want. Use {@link #submit(Runnable)}.") + public void execute(Runnable runnable) { + executor.execute(runnable); + } + + /** + * Blocks until all current pending tasks have completed, up to 30 seconds. Only for testing. + * + *

This is not a shutdown, this does not stop new tasks from being submitted to the queue. + */ + @VisibleForTesting + public void await() throws ExecutionException, InterruptedException, TimeoutException { + // Submit an empty runnable, and await on it for 30 sec so deadlocked tests fail faster. + Tasks.await(submit(() -> {}), 30, TimeUnit.SECONDS); + + // Sleep for a bit here, instead of de-flaking individual test cases. + Thread.sleep(1); + } +} diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsWorkers.kt b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsWorkers.kt new file mode 100644 index 00000000000..4d6234fbc14 --- /dev/null +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/CrashlyticsWorkers.kt @@ -0,0 +1,100 @@ +/* + * Copyright 2024 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.firebase.crashlytics.internal.concurrency + +import android.os.Build +import android.os.Looper +import com.google.firebase.crashlytics.internal.Logger +import java.util.concurrent.ExecutorService + +/** + * Container to hold the different Crashlytics workers. + * + * @suppress @hide + */ +class CrashlyticsWorkers( + backgroundExecutorService: ExecutorService, + blockingExecutorService: ExecutorService, +) { + + /** + * The common worker is for common background tasks, like background init, user actions, and + * processing uncaught exceptions. This is the main worker of the sdk. This worker will never + * block on a disk write or network call. + */ + @JvmField val common = CrashlyticsWorker(backgroundExecutorService) + + /** + * The disk write worker is for background tasks that persisting data to local disk. No user + * action should wait on this. Use for fire and forget, safe to ignore exceptions. + */ + @JvmField val diskWrite = CrashlyticsWorker(backgroundExecutorService) + + /** + * The data collect worker is for any background tasks that send data remotely, like fetching fid, + * settings, or uploading crash reports. This worker is suspended until permission is granted. + */ + @JvmField val dataCollect = CrashlyticsWorker(backgroundExecutorService) + + /** The network worker is for making blocking network calls. */ + @JvmField val network = CrashlyticsWorker(blockingExecutorService) + + /** Convenient preconditions specific for Crashlytics worker threads. */ + companion object { + private val threadName + get() = Thread.currentThread().name + + /** When enabled, failed preconditions will cause assertion errors for debugging. */ + @JvmStatic var enforcement: Boolean = false + + @JvmStatic + fun checkNotMainThread() = + checkThread(::isNotMainThread) { + "Must not be called on a main thread, was called on $threadName." + } + + @JvmStatic + fun checkBlockingThread() = + checkThread(::isBlockingThread) { + "Must be called on a blocking thread, was called on $threadName." + } + + @JvmStatic + fun checkBackgroundThread() = + checkThread(::isBackgroundThread) { + "Must be called on a background thread, was called on $threadName." + } + + private fun isNotMainThread() = + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) { + !Looper.getMainLooper().isCurrentThread + } else { + Looper.getMainLooper() != Looper.myLooper() + } + + private fun isBlockingThread() = threadName.contains("Firebase Blocking Thread #") + + private fun isBackgroundThread() = threadName.contains("Firebase Background Thread #") + + private fun checkThread(isCorrectThread: () -> Boolean, failureMessage: () -> String) { + if (!isCorrectThread()) { + Logger.getLogger().d(failureMessage()) + assert(!enforcement, failureMessage) + } + } + } +} diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/package-info.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/package-info.java new file mode 100644 index 00000000000..bde486ae521 --- /dev/null +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/concurrency/package-info.java @@ -0,0 +1,18 @@ +/* + * Copyright 2024 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** @hide */ +package com.google.firebase.crashlytics.internal.concurrency; diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/metadata/UserMetadata.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/metadata/UserMetadata.java index e6af8a9bea3..36ca7fda8a5 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/metadata/UserMetadata.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/metadata/UserMetadata.java @@ -18,12 +18,11 @@ import androidx.annotation.VisibleForTesting; import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.firebase.crashlytics.internal.common.CommonUtils; -import com.google.firebase.crashlytics.internal.common.CrashlyticsBackgroundWorker; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers; import com.google.firebase.crashlytics.internal.model.CrashlyticsReport; import com.google.firebase.crashlytics.internal.persistence.FileStore; import java.util.List; import java.util.Map; -import java.util.concurrent.Callable; import java.util.concurrent.atomic.AtomicMarkableReference; import java.util.concurrent.atomic.AtomicReference; @@ -47,7 +46,8 @@ public class UserMetadata { @VisibleForTesting public static final int MAX_ROLLOUT_ASSIGNMENTS = 128; private final MetaDataStore metaDataStore; - private final CrashlyticsBackgroundWorker backgroundWorker; + + private final CrashlyticsWorkers crashlyticsWorkers; private String sessionIdentifier; // The following references contain a marker bit, which is true if the data maintained in the @@ -69,9 +69,9 @@ public static String readUserId(String sessionId, FileStore fileStore) { } public static UserMetadata loadFromExistingSession( - String sessionId, FileStore fileStore, CrashlyticsBackgroundWorker backgroundWorker) { + String sessionId, FileStore fileStore, CrashlyticsWorkers crashlyticsWorkers) { MetaDataStore store = new MetaDataStore(fileStore); - UserMetadata metadata = new UserMetadata(sessionId, fileStore, backgroundWorker); + UserMetadata metadata = new UserMetadata(sessionId, fileStore, crashlyticsWorkers); // We don't use the set methods in this class, because they will attempt to re-serialize the // data, which is unnecessary because we just read them from disk. metadata.customKeys.map.getReference().setKeys(store.readKeyData(sessionId, false)); @@ -82,10 +82,10 @@ public static UserMetadata loadFromExistingSession( } public UserMetadata( - String sessionIdentifier, FileStore fileStore, CrashlyticsBackgroundWorker backgroundWorker) { + String sessionIdentifier, FileStore fileStore, CrashlyticsWorkers crashlyticsWorkers) { this.sessionIdentifier = sessionIdentifier; this.metaDataStore = new MetaDataStore(fileStore); - this.backgroundWorker = backgroundWorker; + this.crashlyticsWorkers = crashlyticsWorkers; } /** @@ -99,15 +99,18 @@ public void setNewSession(String sessionId) { sessionIdentifier = sessionId; Map keyData = customKeys.getKeys(); List rolloutAssignments = rolloutsState.getRolloutAssignmentList(); - if (getUserId() != null) { - metaDataStore.writeUserData(sessionId, getUserId()); - } - if (!keyData.isEmpty()) { - metaDataStore.writeKeyData(sessionId, keyData); - } - if (!rolloutAssignments.isEmpty()) { - metaDataStore.writeRolloutState(sessionId, rolloutAssignments); - } + crashlyticsWorkers.diskWrite.submit( + () -> { + if (getUserId() != null) { + metaDataStore.writeUserData(sessionId, getUserId()); + } + if (!keyData.isEmpty()) { + metaDataStore.writeKeyData(sessionId, keyData); + } + if (!rolloutAssignments.isEmpty()) { + metaDataStore.writeRolloutState(sessionId, rolloutAssignments); + } + }); } } @@ -129,14 +132,12 @@ public void setUserId(String identifier) { } userId.set(sanitizedNewId, true); } - backgroundWorker.submit( - () -> { - serializeUserDataIfNeeded(); - return null; - }); + crashlyticsWorkers.diskWrite.submit(this::serializeUserDataIfNeeded); } - /** @return defensive copy of the custom keys. */ + /** + * @return defensive copy of the custom keys. + */ public Map getCustomKeys() { return customKeys.getKeys(); } @@ -159,7 +160,9 @@ public void setCustomKeys(Map keysAndValues) { customKeys.setKeys(keysAndValues); } - /** @return defensive copy of the internal keys. */ + /** + * @return defensive copy of the internal keys. + */ public Map getInternalKeys() { return internalKeys.getKeys(); } @@ -188,11 +191,9 @@ public boolean updateRolloutsState(List rolloutAssignments) { return false; } List updatedRolloutAssignments = rolloutsState.getRolloutAssignmentList(); - backgroundWorker.submit( - () -> { - metaDataStore.writeRolloutState(sessionIdentifier, updatedRolloutAssignments); - return null; - }); + + crashlyticsWorkers.diskWrite.submit( + () -> metaDataStore.writeRolloutState(sessionIdentifier, updatedRolloutAssignments)); return true; } } @@ -224,7 +225,7 @@ private void serializeUserDataIfNeeded() { */ private class SerializeableKeysMap { final AtomicMarkableReference map; - private final AtomicReference> queuedSerializer = new AtomicReference<>(null); + private final AtomicReference queuedSerializer = new AtomicReference<>(null); private final boolean isInternal; public SerializeableKeysMap(boolean isInternal) { @@ -262,17 +263,16 @@ public void setKeys(Map keysAndValues) { } private void scheduleSerializationTaskIfNeeded() { - Callable newCallable = + Runnable newRunnable = () -> { queuedSerializer.set(null); serializeIfMarked(); - return null; }; // Don't schedule the task if there's another queued task waiting, because the already-queued // task will write the latest data. - if (queuedSerializer.compareAndSet(null, newCallable)) { - backgroundWorker.submit(newCallable); + if (queuedSerializer.compareAndSet(null, newRunnable)) { + crashlyticsWorkers.diskWrite.submit(newRunnable); } } diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/network/HttpGetRequest.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/network/HttpGetRequest.java index 205e10fb4a5..f7fcfffc060 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/network/HttpGetRequest.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/network/HttpGetRequest.java @@ -15,6 +15,7 @@ package com.google.firebase.crashlytics.internal.network; import com.google.firebase.crashlytics.internal.Logger; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers; import java.io.BufferedReader; import java.io.IOException; import java.io.InputStream; @@ -56,6 +57,7 @@ public HttpGetRequest header(Map.Entry entry) { } public HttpResponse execute() throws IOException { + CrashlyticsWorkers.checkBlockingThread(); // Network calls must be on a blocking thread. InputStream stream = null; HttpsURLConnection connection = null; String body = null; diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsSpiCall.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsSpiCall.java index dcfbdf8d0fd..b22c01c7be3 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsSpiCall.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/settings/DefaultSettingsSpiCall.java @@ -17,6 +17,7 @@ import android.text.TextUtils; import com.google.firebase.crashlytics.internal.Logger; import com.google.firebase.crashlytics.internal.common.CrashlyticsCore; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers; import com.google.firebase.crashlytics.internal.network.HttpGetRequest; import com.google.firebase.crashlytics.internal.network.HttpRequestFactory; import com.google.firebase.crashlytics.internal.network.HttpResponse; @@ -96,6 +97,7 @@ protected HttpGetRequest createHttpGetRequest(Map queryParams) { @Override public JSONObject invoke(SettingsRequest requestData, boolean dataCollectionToken) { + CrashlyticsWorkers.checkBlockingThread(); if (!dataCollectionToken) { throw new RuntimeException("An invalid data collection token was used."); } diff --git a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/settings/SettingsController.java b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/settings/SettingsController.java index d44c37257e3..e3e4357e1e2 100644 --- a/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/settings/SettingsController.java +++ b/firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/settings/SettingsController.java @@ -30,12 +30,12 @@ import com.google.firebase.crashlytics.internal.common.DeliveryMechanism; import com.google.firebase.crashlytics.internal.common.IdManager; import com.google.firebase.crashlytics.internal.common.SystemCurrentTimeProvider; +import com.google.firebase.crashlytics.internal.concurrency.CrashlyticsWorkers; import com.google.firebase.crashlytics.internal.network.HttpRequestFactory; import com.google.firebase.crashlytics.internal.persistence.FileStore; import java.util.Locale; -import java.util.concurrent.Executor; +import java.util.concurrent.Future; import java.util.concurrent.atomic.AtomicReference; -import org.json.JSONException; import org.json.JSONObject; /** Implements the logic of when to use cached settings, and when to load them from the network. */ @@ -149,8 +149,8 @@ public Settings getSettingsSync() { * * @return a task that is resolved when loading is completely finished. */ - public Task loadSettingsData(Executor executor) { - return loadSettingsData(SettingsCacheBehavior.USE_CACHE, executor); + public Task loadSettingsData(CrashlyticsWorkers crashlyticsWorkers) { + return loadSettingsData(SettingsCacheBehavior.USE_CACHE, crashlyticsWorkers); } /** @@ -158,10 +158,10 @@ public Task loadSettingsData(Executor executor) { * * @return a task that is resolved when loading is completely finished. */ - public Task loadSettingsData(SettingsCacheBehavior cacheBehavior, Executor executor) { + public Task loadSettingsData( + SettingsCacheBehavior cacheBehavior, CrashlyticsWorkers crashlyticsWorkers) { // TODO: Refactor this so that it doesn't do the cache lookup twice when settings are // expired. - // We need to bypass the cache if this is the first time a new build has run so the // backend will know about it. if (!buildInstanceIdentifierChanged()) { @@ -186,18 +186,24 @@ public Task loadSettingsData(SettingsCacheBehavior cacheBehavior, Executor } // Kick off fetching fresh settings. + // TODO(mrober): Refactor to call worker directly, not expose executor. return dataCollectionArbiter - .waitForDataCollectionPermission(executor) + .waitForDataCollectionPermission() .onSuccessTask( - executor, + crashlyticsWorkers.common, new SuccessContinuation() { @NonNull @Override public Task then(@Nullable Void aVoid) throws Exception { // Waited for data collection permission, so this is safe. final boolean dataCollectionToken = true; - final JSONObject settingsJson = - settingsSpiCall.invoke(settingsRequest, dataCollectionToken); + Future settingsFuture = + crashlyticsWorkers + .network + .getExecutor() + .submit(() -> settingsSpiCall.invoke(settingsRequest, dataCollectionToken)); + // TODO(mrober): Should we add a timeout here, or let the entire init timeout? + JSONObject settingsJson = settingsFuture.get(); if (settingsJson != null) { final Settings fetchedSettings = @@ -256,7 +262,7 @@ private Settings getCachedSettingsData(SettingsCacheBehavior cacheBehavior) { return toReturn; } - private void logSettings(JSONObject json, String message) throws JSONException { + private void logSettings(JSONObject json, String message) { Logger.getLogger().d(message + json.toString()); } diff --git a/firebase-crashlytics/src/test/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerRobolectricTest.java b/firebase-crashlytics/src/test/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerRobolectricTest.java index 24ce37dd4b7..ed21115c075 100644 --- a/firebase-crashlytics/src/test/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerRobolectricTest.java +++ b/firebase-crashlytics/src/test/java/com/google/firebase/crashlytics/internal/common/CrashlyticsControllerRobolectricTest.java @@ -28,10 +28,12 @@ import android.content.Context; import androidx.annotation.NonNull; import androidx.test.core.app.ApplicationProvider; +import com.google.firebase.concurrent.TestOnlyExecutors; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponent; import com.google.firebase.crashlytics.internal.CrashlyticsNativeComponentDeferredProxy; import com.google.firebase.crashlytics.internal.DevelopmentPlatformProvider; import com.google.firebase.crashlytics.internal.analytics.AnalyticsEventLogger; +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.persistence.FileStore; @@ -41,6 +43,7 @@ import com.google.firebase.crashlytics.internal.settings.SettingsProvider; import com.google.firebase.inject.Deferred; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.TreeSet; @@ -63,6 +66,8 @@ public class CrashlyticsControllerRobolectricTest { @Mock private SessionReportingCoordinator mockSessionReportingCoordinator; @Mock private DataCollectionArbiter mockDataCollectionArbiter; + private CrashlyticsWorkers crashlyticsWorkers; + private static final CrashlyticsNativeComponent MISSING_NATIVE_COMPONENT = new CrashlyticsNativeComponentDeferredProxy( new Deferred() { @@ -78,17 +83,24 @@ public void setUp() { MockitoAnnotations.openMocks(this); testContext = getApplicationContext(); testFileStore = new FileStore(testContext); + crashlyticsWorkers = + new CrashlyticsWorkers(TestOnlyExecutors.background(), TestOnlyExecutors.blocking()); } @Test - public void testDoCloseSession_enabledAnrs_doesNotPersistsAppExitInfoIfItDoesntExist() { + public void testDoCloseSession_enabledAnrs_doesNotPersistsAppExitInfoIfItDoesntExist() + throws Exception { final String sessionId = "sessionId"; final CrashlyticsController controller = createController(); when(mockSessionReportingCoordinator.listSortedOpenSessionIds()) .thenReturn(new TreeSet<>(Collections.singletonList(sessionId))); mockSettingsProvider(true, false); - controller.doCloseSessions(mockSettingsProvider); + + crashlyticsWorkers.common.submit(() -> controller.doCloseSessions(mockSettingsProvider)); + // cannot use await since it check preconditions if blocking main thread + Thread.sleep(10); + // Since we haven't added any app exit info to the shadow activity manager, there won't exist a // single app exit info, and so this method won't be called. verify(mockSessionReportingCoordinator, never()) @@ -97,7 +109,8 @@ public void testDoCloseSession_enabledAnrs_doesNotPersistsAppExitInfoIfItDoesntE } @Test - public void testDoCloseSession_enabledAnrs_persistsAppExitInfoIfItExists() { + public void testDoCloseSession_enabledAnrs_persistsAppExitInfoIfItExists() throws Exception { + final String sessionIdPrevious = "sessionIdPrevious"; final String sessionId = "sessionId"; final CrashlyticsController controller = createController(); // Adds multiple AppExitInfos to confirm that Crashlytics loops through @@ -107,26 +120,30 @@ public void testDoCloseSession_enabledAnrs_persistsAppExitInfoIfItExists() { List testApplicationExitInfo = getApplicationExitInfoList(); when(mockSessionReportingCoordinator.listSortedOpenSessionIds()) - .thenReturn(new TreeSet<>(Collections.singletonList(sessionId))); + .thenReturn(new TreeSet<>(Arrays.asList(sessionId, sessionIdPrevious))); mockSettingsProvider(true, false); - controller.doCloseSessions(mockSettingsProvider); + crashlyticsWorkers.common.submit(() -> controller.finalizeSessions(mockSettingsProvider)); + // cannot use await since it check preconditions if blocking main thread + Thread.sleep(100); verify(mockSessionReportingCoordinator) .persistRelevantAppExitInfoEvent( - eq(sessionId), + eq(sessionIdPrevious), eq(testApplicationExitInfo), any(LogFileManager.class), any(UserMetadata.class)); } @Test - public void testDoCloseSession_disabledAnrs_doesNotPersistsAppExitInfo() { + public void testDoCloseSession_disabledAnrs_doesNotPersistsAppExitInfo() throws Exception { final String sessionId = "sessionId"; final CrashlyticsController controller = createController(); when(mockSessionReportingCoordinator.listSortedOpenSessionIds()) .thenReturn(new TreeSet<>(Collections.singletonList(sessionId))); mockSettingsProvider(false, false); - controller.doCloseSessions(mockSettingsProvider); + crashlyticsWorkers.common.submit(() -> controller.doCloseSessions(mockSettingsProvider)); + // cannot use await since it check preconditions if blocking main thread + Thread.sleep(10); verify(mockSessionReportingCoordinator, never()) .persistRelevantAppExitInfoEvent( eq(sessionId), any(), any(LogFileManager.class), any(UserMetadata.class)); @@ -164,7 +181,6 @@ private CrashlyticsController createController() { final CrashlyticsController controller = new CrashlyticsController( testContext, - new CrashlyticsBackgroundWorker(Runnable::run), idManager, mockDataCollectionArbiter, testFileStore, @@ -175,7 +191,8 @@ private CrashlyticsController createController() { mockSessionReportingCoordinator, MISSING_NATIVE_COMPONENT, mock(AnalyticsEventLogger.class), - mock(CrashlyticsAppQualitySessionsSubscriber.class)); + mock(CrashlyticsAppQualitySessionsSubscriber.class), + crashlyticsWorkers); controller.openSession(SESSION_ID); return controller; } diff --git a/firebase-crashlytics/src/test/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinatorRobolectricTest.java b/firebase-crashlytics/src/test/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinatorRobolectricTest.java index c29041668a6..67be4920dad 100644 --- a/firebase-crashlytics/src/test/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinatorRobolectricTest.java +++ b/firebase-crashlytics/src/test/java/com/google/firebase/crashlytics/internal/common/SessionReportingCoordinatorRobolectricTest.java @@ -25,9 +25,11 @@ import android.app.ActivityManager; import android.app.ApplicationExitInfo; import android.content.Context; -import android.os.Build; -import androidx.annotation.RequiresApi; +import android.os.Build.VERSION_CODES; import androidx.test.core.app.ApplicationProvider; +import androidx.test.filters.SdkSuppress; +import com.google.firebase.concurrent.TestOnlyExecutors; +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; @@ -61,6 +63,9 @@ public class SessionReportingCoordinatorRobolectricTest { private SessionReportingCoordinator reportingCoordinator; + private final CrashlyticsWorkers crashlyticsWorkers = + new CrashlyticsWorkers(TestOnlyExecutors.background(), TestOnlyExecutors.blocking()); + @Before public void setUp() { MockitoAnnotations.initMocks(this); @@ -72,7 +77,8 @@ public void setUp() { reportSender, logFileManager, reportMetadata, - idManager); + idManager, + crashlyticsWorkers); mockEventInteractions(); } @@ -197,7 +203,7 @@ private List getAppExitInfoList() { return activityManager.getHistoricalProcessExitReasons(null, 0, 0); } - @RequiresApi(api = Build.VERSION_CODES.R) + @SdkSuppress(minSdkVersion = VERSION_CODES.R) private static CrashlyticsReport.ApplicationExitInfo convertApplicationExitInfo( ApplicationExitInfo applicationExitInfo) { // The ApplicationExitInfo inserted by ShadowApplicationManager does not contain an input trace, From c332b1c2b264b966779b821629825d5326e42136 Mon Sep 17 00:00:00 2001 From: Rodrigo Lazo Date: Wed, 18 Sep 2024 10:31:46 -0400 Subject: [PATCH 2/4] Improve kdoc for GenerationConfig (#6261) --- .../vertexai/type/GenerationConfig.kt | 82 +++++++++++++++---- 1 file changed, 64 insertions(+), 18 deletions(-) diff --git a/firebase-vertexai/src/main/kotlin/com/google/firebase/vertexai/type/GenerationConfig.kt b/firebase-vertexai/src/main/kotlin/com/google/firebase/vertexai/type/GenerationConfig.kt index 55e1884adaa..24c64998fa5 100644 --- a/firebase-vertexai/src/main/kotlin/com/google/firebase/vertexai/type/GenerationConfig.kt +++ b/firebase-vertexai/src/main/kotlin/com/google/firebase/vertexai/type/GenerationConfig.kt @@ -19,18 +19,55 @@ package com.google.firebase.vertexai.type /** * Configuration parameters to use for content generation. * - * @property temperature The degree of randomness in token selection, typically between 0 and 1 - * @property topK The sum of probabilities to collect to during token selection - * @property topP How many tokens to select amongst the highest probabilities - * @property candidateCount The max *unique* responses to return - * @property maxOutputTokens The max tokens to generate per response - * @property stopSequences A list of strings to stop generation on occurrence of - * @property responseMimeType Response MIME type for the generated candidate text. For a list of - * supported response MIME types, see the - * [Vertex AI documentation](https://cloud.google.com/vertex-ai/docs/reference/rest/v1beta1/GenerationConfig#FIELDS.response_mime_type) - * for a list of supported types. - * @property responseSchema A schema that the response must adhere to, used with the - * `application/json` MIME type. + * @property temperature A parameter controlling the degree of randomness in token selection. A + * temperature of 0 means that the highest probability tokens are always selected. In this case, + * responses for a given prompt are mostly deterministic, but a small amount of variation is still + * possible. + * + * @property topK The `topK` parameter changes how the model selects tokens for output. A `topK` of + * 1 means the selected token is the most probable among all the tokens in the model's vocabulary, + * while a `topK` of 3 means that the next token is selected from among the 3 most probable using + * the `temperature`. For each token selection step, the `topK` tokens with the highest + * probabilities are sampled. Tokens are then further filtered based on `topP` with the final token + * selected using `temperature` sampling. Defaults to 40 if unspecified. + * + * @property topP The `topP` parameter changes how the model selects tokens for output. Tokens are + * selected from the most to least probable until the sum of their probabilities equals the `topP` + * value. For example, if tokens A, B, and C have probabilities of 0.3, 0.2, and 0.1 respectively + * and the topP value is 0.5, then the model will select either A or B as the next token by using + * the `temperature` and exclude C as a candidate. Defaults to 0.95 if unset. + * + * @property candidateCount The maximum number of generated response messages to return. This value + * must be between [1, 8], inclusive. If unset, this will default to 1. + * + * - Note: Only unique candidates are returned. Higher temperatures are more likely to produce + * unique candidates. Setting `temperature` to 0 will always produce exactly one candidate + * regardless of the `candidateCount`. + * + * @property maxOutputTokens Specifies the maximum number of tokens that can be generated in the + * response. The number of tokens per word varies depending on the language outputted. Defaults to 0 + * (unbounded). + * + * @property stopSequences A set of up to 5 `String`s that will stop output generation. If + * specified, the API will stop at the first appearance of a stop sequence. The stop sequence will + * not be included as part of the response. + * + * @property responseMimeType Output response MIME type of the generated candidate text (IANA + * standard). + * + * Supported MIME types depend on the model used, but could include: + * - `text/plain`: Text output; the default behavior if unspecified. + * - `application/json`: JSON response in the candidates. + * + * @property responseSchema Output schema of the generated candidate text. If set, a compatible + * [responseMimeType] must also be set. + * + * Compatible MIME types: + * - `application/json`: Schema for JSON response. + * + * Refer to the + * [Control generated output](https://cloud.google.com/vertex-ai/generative-ai/docs/multimodal/control-generated-output) + * guide for more details. */ class GenerationConfig private constructor( @@ -50,12 +87,21 @@ private constructor( * Mainly intended for Java interop. Kotlin consumers should use [generationConfig] for a more * idiomatic experience. * - * @property temperature The degree of randomness in token selection, typically between 0 and 1 - * @property topK The sum of probabilities to collect to during token selection - * @property topP How many tokens to select amongst the highest probabilities - * @property candidateCount The max *unique* responses to return - * @property maxOutputTokens The max tokens to generate per response - * @property stopSequences A list of strings to stop generation on occurrence of + * @property temperature See [GenerationConfig.temperature]. + * + * @property topK See [GenerationConfig.topK]. + * + * @property topP See [GenerationConfig.topP]. + * + * @property candidateCount See [GenerationConfig.candidateCount]. + * + * @property maxOutputTokens See [GenerationConfig.maxOutputTokens]. + * + * @property stopSequences See [GenerationConfig.stopSequences]. + * + * @property responseMimeType See [GenerationConfig.responseMimeType]. + * + * @property responseSchema See [GenerationConfig.responseSchema]. * @see [generationConfig] */ class Builder { From 48d460cc430589b3ebc8a668772ab351e1ff559b Mon Sep 17 00:00:00 2001 From: themiswang Date: Wed, 18 Sep 2024 10:40:26 -0400 Subject: [PATCH 3/4] version bump (#6270) --- firebase-crashlytics-ndk/CHANGELOG.md | 4 ++-- firebase-crashlytics-ndk/gradle.properties | 4 ++-- firebase-crashlytics/gradle.properties | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/firebase-crashlytics-ndk/CHANGELOG.md b/firebase-crashlytics-ndk/CHANGELOG.md index b8ece17d431..37e24ba237c 100644 --- a/firebase-crashlytics-ndk/CHANGELOG.md +++ b/firebase-crashlytics-ndk/CHANGELOG.md @@ -1,8 +1,8 @@ # Unreleased - +* [changed] Updated `firebase-crashlytics` dependency to v19.2.0 # 19.1.0 -* [changed] Updated `firebase-crashlytics` dependency to v19.0.4 +* [changed] Updated `firebase-crashlytics` dependency to v19.1.0 # 19.0.3 * [changed] Updated `firebase-crashlytics` dependency to v19.0.3 diff --git a/firebase-crashlytics-ndk/gradle.properties b/firebase-crashlytics-ndk/gradle.properties index 4deda310a06..d3f7052769e 100644 --- a/firebase-crashlytics-ndk/gradle.properties +++ b/firebase-crashlytics-ndk/gradle.properties @@ -1,2 +1,2 @@ -version=19.1.1 -latestReleasedVersion=19.1.0 \ No newline at end of file +version=19.2.0 +latestReleasedVersion=19.1.0 diff --git a/firebase-crashlytics/gradle.properties b/firebase-crashlytics/gradle.properties index 4deda310a06..d3f7052769e 100644 --- a/firebase-crashlytics/gradle.properties +++ b/firebase-crashlytics/gradle.properties @@ -1,2 +1,2 @@ -version=19.1.1 -latestReleasedVersion=19.1.0 \ No newline at end of file +version=19.2.0 +latestReleasedVersion=19.1.0 From d153670af9bf5837591d37635cf66ae3ac91271f Mon Sep 17 00:00:00 2001 From: Rodrigo Lazo Date: Wed, 18 Sep 2024 11:28:19 -0400 Subject: [PATCH 4/4] Add support for blockReasonMessage (#6268) b/367673161 --- .../google/firebase/vertexai/common/server/Types.kt | 1 + .../firebase/vertexai/internal/util/conversions.kt | 1 + .../google/firebase/vertexai/type/PromptFeedback.kt | 2 ++ .../firebase/vertexai/StreamingSnapshotTests.kt | 12 ++++++++++++ .../google/firebase/vertexai/UnarySnapshotTests.kt | 12 ++++++++++++ 5 files changed, 28 insertions(+) diff --git a/firebase-vertexai/src/main/kotlin/com/google/firebase/vertexai/common/server/Types.kt b/firebase-vertexai/src/main/kotlin/com/google/firebase/vertexai/common/server/Types.kt index a5c0b00e461..16f96796bbf 100644 --- a/firebase-vertexai/src/main/kotlin/com/google/firebase/vertexai/common/server/Types.kt +++ b/firebase-vertexai/src/main/kotlin/com/google/firebase/vertexai/common/server/Types.kt @@ -38,6 +38,7 @@ internal object FinishReasonSerializer : internal data class PromptFeedback( val blockReason: BlockReason? = null, val safetyRatings: List? = null, + val blockReasonMessage: String? = null, ) @Serializable(BlockReasonSerializer::class) diff --git a/firebase-vertexai/src/main/kotlin/com/google/firebase/vertexai/internal/util/conversions.kt b/firebase-vertexai/src/main/kotlin/com/google/firebase/vertexai/internal/util/conversions.kt index b16d5ffad36..0a67271b3a1 100644 --- a/firebase-vertexai/src/main/kotlin/com/google/firebase/vertexai/internal/util/conversions.kt +++ b/firebase-vertexai/src/main/kotlin/com/google/firebase/vertexai/internal/util/conversions.kt @@ -245,6 +245,7 @@ internal fun com.google.firebase.vertexai.common.server.PromptFeedback.toPublic( return com.google.firebase.vertexai.type.PromptFeedback( blockReason?.toPublic(), safetyRatings, + blockReasonMessage ) } diff --git a/firebase-vertexai/src/main/kotlin/com/google/firebase/vertexai/type/PromptFeedback.kt b/firebase-vertexai/src/main/kotlin/com/google/firebase/vertexai/type/PromptFeedback.kt index 7d9a0f5d438..f9d2a134628 100644 --- a/firebase-vertexai/src/main/kotlin/com/google/firebase/vertexai/type/PromptFeedback.kt +++ b/firebase-vertexai/src/main/kotlin/com/google/firebase/vertexai/type/PromptFeedback.kt @@ -21,10 +21,12 @@ package com.google.firebase.vertexai.type * * @param blockReason The reason that content was blocked, if at all. * @param safetyRatings A list of relevant [SafetyRating]. + * @param blockReasonMessage A message describing the reason that content was blocked, if any. */ class PromptFeedback( val blockReason: BlockReason?, val safetyRatings: List, + val blockReasonMessage: String? ) /** Describes why content was blocked. */ diff --git a/firebase-vertexai/src/test/java/com/google/firebase/vertexai/StreamingSnapshotTests.kt b/firebase-vertexai/src/test/java/com/google/firebase/vertexai/StreamingSnapshotTests.kt index 4dcfb5380cc..941f7647edd 100644 --- a/firebase-vertexai/src/test/java/com/google/firebase/vertexai/StreamingSnapshotTests.kt +++ b/firebase-vertexai/src/test/java/com/google/firebase/vertexai/StreamingSnapshotTests.kt @@ -123,6 +123,18 @@ internal class StreamingSnapshotTests { } } + @Test + fun `prompt blocked for safety with message`() = + goldenStreamingFile("streaming-failure-prompt-blocked-safety-with-message.txt") { + val responses = model.generateContentStream("prompt") + + withTimeout(testTimeout) { + val exception = shouldThrow { responses.collect() } + exception.response.promptFeedback?.blockReason shouldBe BlockReason.SAFETY + exception.response.promptFeedback?.blockReasonMessage shouldBe "Reasons" + } + } + @Test fun `empty content`() = goldenStreamingFile("streaming-failure-empty-content.txt") { diff --git a/firebase-vertexai/src/test/java/com/google/firebase/vertexai/UnarySnapshotTests.kt b/firebase-vertexai/src/test/java/com/google/firebase/vertexai/UnarySnapshotTests.kt index eeb45bba43e..04f47a0949b 100644 --- a/firebase-vertexai/src/test/java/com/google/firebase/vertexai/UnarySnapshotTests.kt +++ b/firebase-vertexai/src/test/java/com/google/firebase/vertexai/UnarySnapshotTests.kt @@ -167,6 +167,18 @@ internal class UnarySnapshotTests { } } + @Test + fun `prompt blocked for safety with message`() = + goldenUnaryFile("unary-failure-prompt-blocked-safety-with-message.json") { + withTimeout(testTimeout) { + shouldThrow { model.generateContent("prompt") } should + { + it.response.promptFeedback?.blockReason shouldBe BlockReason.SAFETY + it.response.promptFeedback?.blockReasonMessage shouldContain "Reasons" + } + } + } + @Test fun `empty content`() = goldenUnaryFile("unary-failure-empty-content.json") {