From 5f6bc63616e4438d2c0386f4e34a440136ebfe97 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 8 Nov 2024 20:06:25 +0000 Subject: [PATCH 1/2] dataconnect: GrpcMetadataIntegrationTest.kt: Fix race condition waiting for auth/appcheck to be ready (#6446) --- .../GrpcMetadataIntegrationTest.kt | 11 ++ .../DataConnectCredentialsTokenManager.kt | 8 ++ .../core/FirebaseDataConnectImpl.kt | 88 ++++++++---- .../core/FirebaseDataConnectImplUnitTest.kt | 128 +++++++++++++----- .../dataconnect/testutil/CleanupsRule.kt | 95 +++++++++++++ 5 files changed, 271 insertions(+), 59 deletions(-) create mode 100644 firebase-dataconnect/testutil/src/main/kotlin/com/google/firebase/dataconnect/testutil/CleanupsRule.kt diff --git a/firebase-dataconnect/src/androidTest/kotlin/com/google/firebase/dataconnect/GrpcMetadataIntegrationTest.kt b/firebase-dataconnect/src/androidTest/kotlin/com/google/firebase/dataconnect/GrpcMetadataIntegrationTest.kt index 6ad17b40106..8ff7715ddb9 100644 --- a/firebase-dataconnect/src/androidTest/kotlin/com/google/firebase/dataconnect/GrpcMetadataIntegrationTest.kt +++ b/firebase-dataconnect/src/androidTest/kotlin/com/google/firebase/dataconnect/GrpcMetadataIntegrationTest.kt @@ -23,6 +23,7 @@ import com.google.android.gms.tasks.Tasks import com.google.firebase.appcheck.AppCheckProvider import com.google.firebase.appcheck.AppCheckProviderFactory import com.google.firebase.appcheck.FirebaseAppCheck +import com.google.firebase.dataconnect.core.FirebaseDataConnectInternal import com.google.firebase.dataconnect.generated.GeneratedConnector import com.google.firebase.dataconnect.generated.GeneratedMutation import com.google.firebase.dataconnect.generated.GeneratedQuery @@ -137,6 +138,7 @@ class GrpcMetadataIntegrationTest : DataConnectIntegrationTestBase() { fun executeQueryShouldNotSendAuthMetadataWhenNotLoggedIn() = runTest { val grpcServer = inProcessDataConnectGrpcServer.newInstance() val dataConnect = dataConnectFactory.newInstance(grpcServer) + (dataConnect as FirebaseDataConnectInternal).awaitAuthReady() val queryRef = dataConnect.query("qryfyk7yfppfe", Unit, serializer(), serializer()) val metadatasJob = async { grpcServer.metadatas.first() } @@ -149,6 +151,7 @@ class GrpcMetadataIntegrationTest : DataConnectIntegrationTestBase() { fun executeMutationShouldNotSendAuthMetadataWhenNotLoggedIn() = runTest { val grpcServer = inProcessDataConnectGrpcServer.newInstance() val dataConnect = dataConnectFactory.newInstance(grpcServer) + (dataConnect as FirebaseDataConnectInternal).awaitAuthReady() val mutationRef = dataConnect.mutation("mutckjpte9v9j", Unit, serializer(), serializer()) val metadatasJob = async { grpcServer.metadatas.first() } @@ -162,6 +165,7 @@ class GrpcMetadataIntegrationTest : DataConnectIntegrationTestBase() { fun executeQueryShouldSendAuthMetadataWhenLoggedIn() = runTest { val grpcServer = inProcessDataConnectGrpcServer.newInstance() val dataConnect = dataConnectFactory.newInstance(grpcServer) + (dataConnect as FirebaseDataConnectInternal).awaitAuthReady() val queryRef = dataConnect.query("qryyarwrxe2fv", Unit, serializer(), serializer()) val metadatasJob = async { grpcServer.metadatas.first() } firebaseAuthSignIn(dataConnect) @@ -175,6 +179,7 @@ class GrpcMetadataIntegrationTest : DataConnectIntegrationTestBase() { fun executeMutationShouldSendAuthMetadataWhenLoggedIn() = runTest { val grpcServer = inProcessDataConnectGrpcServer.newInstance() val dataConnect = dataConnectFactory.newInstance(grpcServer) + (dataConnect as FirebaseDataConnectInternal).awaitAuthReady() val mutationRef = dataConnect.mutation("mutayn7as5k7d", Unit, serializer(), serializer()) val metadatasJob = async { grpcServer.metadatas.first() } @@ -189,6 +194,7 @@ class GrpcMetadataIntegrationTest : DataConnectIntegrationTestBase() { fun executeQueryShouldNotSendAuthMetadataAfterLogout() = runTest { val grpcServer = inProcessDataConnectGrpcServer.newInstance() val dataConnect = dataConnectFactory.newInstance(grpcServer) + (dataConnect as FirebaseDataConnectInternal).awaitAuthReady() val queryRef = dataConnect.query("qryyarwrxe2fv", Unit, serializer(), serializer()) val metadatasJob1 = async { grpcServer.metadatas.first() } val metadatasJob2 = async { grpcServer.metadatas.take(2).last() } @@ -206,6 +212,7 @@ class GrpcMetadataIntegrationTest : DataConnectIntegrationTestBase() { fun executeMutationShouldNotSendAuthMetadataAfterLogout() = runTest { val grpcServer = inProcessDataConnectGrpcServer.newInstance() val dataConnect = dataConnectFactory.newInstance(grpcServer) + (dataConnect as FirebaseDataConnectInternal).awaitAuthReady() val mutationRef = dataConnect.mutation("mutvw945ag3vv", Unit, serializer(), serializer()) val metadatasJob1 = async { grpcServer.metadatas.first() } @@ -226,6 +233,7 @@ class GrpcMetadataIntegrationTest : DataConnectIntegrationTestBase() { // appcheck token is sent at all. val grpcServer = inProcessDataConnectGrpcServer.newInstance() val dataConnect = dataConnectFactory.newInstance(grpcServer) + (dataConnect as FirebaseDataConnectInternal).awaitAppCheckReady() val queryRef = dataConnect.query("qrybbeekpkkck", Unit, serializer(), serializer()) val metadatasJob = async { grpcServer.metadatas.first() } @@ -240,6 +248,7 @@ class GrpcMetadataIntegrationTest : DataConnectIntegrationTestBase() { // appcheck token is sent at all. val grpcServer = inProcessDataConnectGrpcServer.newInstance() val dataConnect = dataConnectFactory.newInstance(grpcServer) + (dataConnect as FirebaseDataConnectInternal).awaitAppCheckReady() val mutationRef = dataConnect.mutation("mutbs7hhxk39c", Unit, serializer(), serializer()) val metadatasJob = async { grpcServer.metadatas.first() } @@ -253,6 +262,7 @@ class GrpcMetadataIntegrationTest : DataConnectIntegrationTestBase() { fun executeQueryShouldSendAppCheckMetadataWhenAppCheckIsEnabled() = runTest { val grpcServer = inProcessDataConnectGrpcServer.newInstance() val dataConnect = dataConnectFactory.newInstance(grpcServer) + (dataConnect as FirebaseDataConnectInternal).awaitAppCheckReady() val queryRef = dataConnect.query("qryyarwrxe2fv", Unit, serializer(), serializer()) val metadatasJob = async { grpcServer.metadatas.first() } val appCheck = FirebaseAppCheck.getInstance(dataConnect.app) @@ -267,6 +277,7 @@ class GrpcMetadataIntegrationTest : DataConnectIntegrationTestBase() { fun executeMutationShouldSendAppCheckMetadataWhenAppCheckIsEnabled() = runTest { val grpcServer = inProcessDataConnectGrpcServer.newInstance() val dataConnect = dataConnectFactory.newInstance(grpcServer) + (dataConnect as FirebaseDataConnectInternal).awaitAppCheckReady() val mutationRef = dataConnect.mutation("mutz4hzqzpgb4", Unit, serializer(), serializer()) val metadatasJob = async { grpcServer.metadatas.first() } diff --git a/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/DataConnectCredentialsTokenManager.kt b/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/DataConnectCredentialsTokenManager.kt index 74c80472e52..f859a4fd35a 100644 --- a/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/DataConnectCredentialsTokenManager.kt +++ b/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/DataConnectCredentialsTokenManager.kt @@ -45,6 +45,9 @@ import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.async import kotlinx.coroutines.cancel import kotlinx.coroutines.ensureActive +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.launch import kotlinx.coroutines.yield @@ -58,6 +61,9 @@ internal sealed class DataConnectCredentialsTokenManager( val instanceId: String get() = logger.nameWithId + private val _providerAvailable = MutableStateFlow(false) + val providerAvailable: StateFlow = _providerAvailable.asStateFlow() + @Suppress("LeakingThis") private val weakThis = WeakReference(this) private val coroutineScope = @@ -448,6 +454,8 @@ internal sealed class DataConnectCredentialsTokenManager( break } } + + _providerAvailable.value = true } /** diff --git a/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/FirebaseDataConnectImpl.kt b/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/FirebaseDataConnectImpl.kt index 564cdf3b003..b04b6aa6ead 100644 --- a/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/FirebaseDataConnectImpl.kt +++ b/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/FirebaseDataConnectImpl.kt @@ -54,6 +54,8 @@ import kotlinx.coroutines.async import kotlinx.coroutines.cancel import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.collect +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock @@ -72,6 +74,9 @@ internal interface FirebaseDataConnectInternal : FirebaseDataConnect { val lazyGrpcClient: SuspendingLazy val lazyQueryManager: SuspendingLazy + + suspend fun awaitAuthReady() + suspend fun awaitAppCheckReady() } internal class FirebaseDataConnectImpl( @@ -107,11 +112,18 @@ internal class FirebaseDataConnectImpl( SupervisorJob() + nonBlockingDispatcher + CoroutineName(instanceId) + - CoroutineExceptionHandler { _, throwable -> - logger.warn(throwable) { "uncaught exception from a coroutine" } + CoroutineExceptionHandler { context, throwable -> + logger.warn(throwable) { + val coroutineName = context[CoroutineName]?.name + "WARNING: uncaught exception from coroutine named \"$coroutineName\" " + + "(error code jszxcbe37k)" + } } ) + private val authProviderAvailable = MutableStateFlow(false) + private val appCheckProviderAvailable = MutableStateFlow(false) + // Protects `closed`, `grpcClient`, `emulatorSettings`, and `queryManager`. private val mutex = Mutex() @@ -121,29 +133,49 @@ internal class FirebaseDataConnectImpl( // All accesses to this variable _must_ have locked `mutex`. private var closed = false - private val lazyDataConnectAuth = - SuspendingLazy(mutex) { - if (closed) throw IllegalStateException("FirebaseDataConnect instance has been closed") - DataConnectAuth( - deferredAuthProvider = deferredAuthProvider, - parentCoroutineScope = coroutineScope, - blockingDispatcher = blockingDispatcher, - logger = Logger("DataConnectAuth").apply { debug { "created by $instanceId" } }, - ) - .apply { initialize() } + private val dataConnectAuth: DataConnectAuth = + DataConnectAuth( + deferredAuthProvider = deferredAuthProvider, + parentCoroutineScope = coroutineScope, + blockingDispatcher = blockingDispatcher, + logger = Logger("DataConnectAuth").apply { debug { "created by $instanceId" } }, + ) + + override suspend fun awaitAuthReady() { + authProviderAvailable.first { it } + } + + init { + coroutineScope.launch(CoroutineName("DataConnectAuth initializer for $instanceId")) { + dataConnectAuth.initialize() + dataConnectAuth.providerAvailable.collect { isProviderAvailable -> + logger.debug { "authProviderAvailable=$isProviderAvailable" } + authProviderAvailable.value = isProviderAvailable + } } + } - private val lazyDataConnectAppCheck = - SuspendingLazy(mutex) { - if (closed) throw IllegalStateException("FirebaseDataConnect instance has been closed") - DataConnectAppCheck( - deferredAppCheckTokenProvider = deferredAppCheckProvider, - parentCoroutineScope = coroutineScope, - blockingDispatcher = blockingDispatcher, - logger = Logger("DataConnectAppCheck").apply { debug { "created by $instanceId" } }, - ) - .apply { initialize() } + private val dataConnectAppCheck: DataConnectAppCheck = + DataConnectAppCheck( + deferredAppCheckTokenProvider = deferredAppCheckProvider, + parentCoroutineScope = coroutineScope, + blockingDispatcher = blockingDispatcher, + logger = Logger("DataConnectAppCheck").apply { debug { "created by $instanceId" } }, + ) + + override suspend fun awaitAppCheckReady() { + appCheckProviderAvailable.first { it } + } + + init { + coroutineScope.launch(CoroutineName("DataConnectAppCheck initializer for $instanceId")) { + dataConnectAppCheck.initialize() + dataConnectAppCheck.providerAvailable.collect { isProviderAvailable -> + logger.debug { "appCheckProviderAvailable=$isProviderAvailable" } + appCheckProviderAvailable.value = isProviderAvailable + } } + } private val lazyGrpcRPCs = SuspendingLazy(mutex) { @@ -181,8 +213,8 @@ internal class FirebaseDataConnectImpl( val grpcMetadata = DataConnectGrpcMetadata.forSystemVersions( firebaseApp = app, - dataConnectAuth = lazyDataConnectAuth.getLocked(), - dataConnectAppCheck = lazyDataConnectAppCheck.getLocked(), + dataConnectAuth = dataConnectAuth, + dataConnectAppCheck = dataConnectAppCheck, connectorLocation = config.location, parentLogger = logger, ) @@ -210,8 +242,8 @@ internal class FirebaseDataConnectImpl( projectId = projectId, connector = config, grpcRPCs = lazyGrpcRPCs.getLocked(), - dataConnectAuth = lazyDataConnectAuth.getLocked(), - dataConnectAppCheck = lazyDataConnectAppCheck.getLocked(), + dataConnectAuth = dataConnectAuth, + dataConnectAppCheck = dataConnectAppCheck, logger = Logger("DataConnectGrpcClient").apply { debug { "created by $instanceId" } }, ) } @@ -397,8 +429,8 @@ internal class FirebaseDataConnectImpl( // Close Auth and AppCheck synchronously to avoid race conditions with auth callbacks. // Since close() is re-entrant, this is safe even if they have already been closed. - lazyDataConnectAuth.initializedValueOrNull?.close() - lazyDataConnectAppCheck.initializedValueOrNull?.close() + dataConnectAuth.close() + dataConnectAppCheck.close() // Start the job to asynchronously close the gRPC client. while (true) { diff --git a/firebase-dataconnect/src/test/kotlin/com/google/firebase/dataconnect/core/FirebaseDataConnectImplUnitTest.kt b/firebase-dataconnect/src/test/kotlin/com/google/firebase/dataconnect/core/FirebaseDataConnectImplUnitTest.kt index c903d6ef9a2..b2d43fd36ff 100644 --- a/firebase-dataconnect/src/test/kotlin/com/google/firebase/dataconnect/core/FirebaseDataConnectImplUnitTest.kt +++ b/firebase-dataconnect/src/test/kotlin/com/google/firebase/dataconnect/core/FirebaseDataConnectImplUnitTest.kt @@ -16,11 +16,19 @@ package com.google.firebase.dataconnect.core import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.google.firebase.appcheck.interop.InteropAppCheckTokenProvider +import com.google.firebase.auth.internal.InternalAuthProvider import com.google.firebase.dataconnect.FirebaseDataConnect.CallerSdkType +import com.google.firebase.dataconnect.testutil.CleanupsRule import com.google.firebase.dataconnect.testutil.DataConnectLogLevelRule +import com.google.firebase.dataconnect.testutil.DelayedDeferred import com.google.firebase.dataconnect.testutil.FirebaseAppUnitTestingRule +import com.google.firebase.dataconnect.testutil.RandomSeedTestRule +import com.google.firebase.dataconnect.testutil.UnavailableDeferred +import com.google.firebase.dataconnect.testutil.delayIgnoringTestScheduler import com.google.firebase.dataconnect.testutil.property.arbitrary.dataConnect import io.kotest.assertions.assertSoftly +import io.kotest.assertions.withClue import io.kotest.matchers.nulls.shouldBeNull import io.kotest.matchers.shouldBe import io.kotest.matchers.types.shouldBeSameInstanceAs @@ -28,13 +36,16 @@ import io.kotest.property.Arb import io.kotest.property.RandomSource import io.kotest.property.arbitrary.enum import io.kotest.property.arbitrary.next -import io.kotest.property.arbitrary.string import io.mockk.mockk +import kotlin.time.Duration.Companion.milliseconds +import kotlin.time.Duration.Companion.seconds +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.asExecutor +import kotlinx.coroutines.launch import kotlinx.coroutines.test.runTest import kotlinx.serialization.DeserializationStrategy import kotlinx.serialization.SerializationStrategy import kotlinx.serialization.modules.SerializersModule -import org.junit.After import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith @@ -44,6 +55,11 @@ class FirebaseDataConnectImplUnitTest { @get:Rule val dataConnectLogLevelRule = DataConnectLogLevelRule() + @get:Rule val cleanups = CleanupsRule() + + @get:Rule val randomSeedTestRule = RandomSeedTestRule() + private val rs: RandomSource by randomSeedTestRule.rs + @get:Rule val firebaseAppFactory = FirebaseAppUnitTestingRule( @@ -52,33 +68,11 @@ class FirebaseDataConnectImplUnitTest { projectIdKey = "a988y548hz", ) - private val rs = RandomSource.default() - private val dataConnect: FirebaseDataConnectImpl by lazy { - val app = firebaseAppFactory.newInstance() - - FirebaseDataConnectImpl( - context = app.applicationContext, - app = app, - projectId = app.options.projectId!!, - config = Arb.dataConnect.connectorConfig().next(rs), - blockingExecutor = mockk(relaxed = true), - nonBlockingExecutor = mockk(relaxed = true), - deferredAuthProvider = mockk(relaxed = true), - deferredAppCheckProvider = mockk(relaxed = true), - creator = mockk(relaxed = true), - settings = Arb.dataConnect.dataConnectSettings().next(rs), - ) - } - - @After - fun closeDataConnect() { - dataConnect.close() - } - @Test fun `query() with no options set should use null for each option`() = runTest { + val dataConnect = newDataConnect() val operationName = Arb.dataConnect.operationName().next(rs) - val variables = TestVariables(Arb.string(size = 8).next(rs)) + val variables: TestVariables = mockk() val dataDeserializer: DeserializationStrategy = mockk() val variablesSerializer: SerializationStrategy = mockk() @@ -103,8 +97,9 @@ class FirebaseDataConnectImplUnitTest { @Test fun `query() with all options specified should use the given options`() = runTest { + val dataConnect = newDataConnect() val operationName = Arb.dataConnect.operationName().next(rs) - val variables = TestVariables(Arb.string(size = 8).next(rs)) + val variables: TestVariables = mockk() val dataDeserializer: DeserializationStrategy = mockk() val variablesSerializer: SerializationStrategy = mockk() val callerSdkType = Arb.enum().next() @@ -136,8 +131,9 @@ class FirebaseDataConnectImplUnitTest { @Test fun `mutation() with no options set should use null for each option`() = runTest { + val dataConnect = newDataConnect() val operationName = Arb.dataConnect.operationName().next(rs) - val variables = TestVariables(Arb.string(size = 8).next(rs)) + val variables: TestVariables = mockk() val dataDeserializer: DeserializationStrategy = mockk() val variablesSerializer: SerializationStrategy = mockk() @@ -162,8 +158,9 @@ class FirebaseDataConnectImplUnitTest { @Test fun `mutation() with all options specified should use the given options`() = runTest { + val dataConnect = newDataConnect() val operationName = Arb.dataConnect.operationName().next(rs) - val variables = TestVariables(Arb.string(size = 8).next(rs)) + val variables: TestVariables = mockk() val dataDeserializer: DeserializationStrategy = mockk() val variablesSerializer: SerializationStrategy = mockk() val callerSdkType = Arb.enum().next() @@ -193,7 +190,76 @@ class FirebaseDataConnectImplUnitTest { } } - private data class TestVariables(val foo: String) + @Test + fun `awaitAuthReady() should return once the provider is available`() { + val deferredAuthProvider = DelayedDeferred(mockk(relaxed = true)) + val dataConnect = + newDataConnect( + deferredAuthProvider = deferredAuthProvider, + deferredAppCheckProvider = UnavailableDeferred(), + ) + awaitReadyTest("awaitAuthReady", deferredAuthProvider) { dataConnect.awaitAuthReady() } + } + + @Test + fun `awaitAppCheckReady() should return once the provider is available`() { + val deferredAppCheckProvider = + DelayedDeferred(mockk(relaxed = true)) + val dataConnect = + newDataConnect( + deferredAuthProvider = UnavailableDeferred(), + deferredAppCheckProvider = deferredAppCheckProvider, + ) + awaitReadyTest("awaitAppCheckReady", deferredAppCheckProvider) { + dataConnect.awaitAppCheckReady() + } + } + + private fun awaitReadyTest( + functionName: String, + delayedDeferred: DelayedDeferred<*>, + block: suspend () -> Unit + ) = + runTest(timeout = 60.seconds) { + val job = launch { block() } + + repeat(5) { + delayIgnoringTestScheduler(100.milliseconds) + withClue("job.isCompleted iteration $it") { job.isCompleted shouldBe false } + } + + delayedDeferred.makeAvailable() + + backgroundScope.launch { + delayIgnoringTestScheduler(20.seconds) + throw Exception("timeout waiting for $functionName() to return") + } + + job.join() + } + + private fun newDataConnect( + deferredAuthProvider: com.google.firebase.inject.Deferred = + mockk(relaxed = true), + deferredAppCheckProvider: com.google.firebase.inject.Deferred = + mockk(relaxed = true), + ): FirebaseDataConnectImpl { + val app = firebaseAppFactory.newInstance() + return FirebaseDataConnectImpl( + context = app.applicationContext, + app = app, + projectId = app.options.projectId!!, + config = Arb.dataConnect.connectorConfig().next(rs), + blockingExecutor = Dispatchers.IO.asExecutor(), + nonBlockingExecutor = Dispatchers.Default.asExecutor(), + deferredAuthProvider = deferredAuthProvider, + deferredAppCheckProvider = deferredAppCheckProvider, + creator = mockk(relaxed = true), + settings = Arb.dataConnect.dataConnectSettings().next(rs), + ) + .also { cleanups.register("close FirebaseDataConnectImpl") { it.close() } } + } - private data class TestData(val bar: String) + private interface TestVariables + private interface TestData } diff --git a/firebase-dataconnect/testutil/src/main/kotlin/com/google/firebase/dataconnect/testutil/CleanupsRule.kt b/firebase-dataconnect/testutil/src/main/kotlin/com/google/firebase/dataconnect/testutil/CleanupsRule.kt new file mode 100644 index 00000000000..3da6891a9e5 --- /dev/null +++ b/firebase-dataconnect/testutil/src/main/kotlin/com/google/firebase/dataconnect/testutil/CleanupsRule.kt @@ -0,0 +1,95 @@ +/* + * 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.dataconnect.testutil + +import android.util.Log +import io.kotest.common.runBlocking +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock +import org.junit.rules.ExternalResource + +/** + * A JUnit test rule that allows tests to register "cleanups" to be run on test completion. If any + * one cleanup throws an exception then the exception will be logged and the rest of the cleanups + * will be executed, finally throwing the exception of the first failed cleanup. Cleanups will be + * executed in the opposite order in which they are registered. Instances are thread-safe and + * cleanups may be registered concurrently from multiple threads. + */ +class CleanupsRule : ExternalResource() { + + private val mutex = Mutex() + private var beforeCalled = false + private var afterCalled = false + private val cleanups = mutableListOf() + + /** + * Registers a cleanup. This function potentially blocks briefly while acquiring the lock on the + * list of cleanups. Throws an exception if called before [before] or after [after] has started. + */ + fun register(name: String? = null, cleanup: suspend () -> Unit) = runBlocking { + registerSuspending(name, cleanup) + } + + /** + * Registers a cleanup. This function potentially suspends while acquiring the lock on the list of + * cleanups. Throws an exception if called before [before] or after [after] has started. + */ + suspend fun registerSuspending(name: String? = null, cleanup: suspend () -> Unit) { + mutex.withLock { + check(beforeCalled) { + "cleanups can not be registered until before() is called " + "(error code 3yrwbehmvk)" + } + + check(!afterCalled) { + "cleanups can not be registered after after() has started " + "(error code dw92ms797f)" + } + + cleanups.add(Cleanup(name, cleanup)) + } + } + + override fun before(): Unit = runBlocking { + mutex.withLock { + check(!beforeCalled) { "before() has already been called (error code hg4a8ab5ve)" } + beforeCalled = true + } + } + + override fun after(): Unit = runBlocking { + mutex.withLock { + check(!afterCalled) { "after() has already been called (error code brewwkxs6g)" } + afterCalled = true + } + + var firstException: Throwable? = null + + while (true) { + val cleanup = mutex.withLock { cleanups.removeLastOrNull() } ?: break + + val result = cleanup.runCatching { action() } + result.onFailure { + Log.e("CleanupsRule", "cleanup ${cleanup.name} failed: $it", it) + if (firstException === null) { + firstException = it + } + } + } + + firstException?.let { throw it } + } + + private data class Cleanup(val name: String?, val action: suspend () -> Unit) +} From f20340a13fbe17d0a2ad3ee23a09d3a6570a68e4 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 8 Nov 2024 21:37:04 +0000 Subject: [PATCH 2/2] dataconnect: TestFirebaseAppFactory.kt: work around IllegalStateException in tests by adding a delay before calling FirebaseApp.delete() (#6447) --- .../testutil/TestFirebaseAppFactory.kt | 14 ++++++- .../DataConnectCredentialsTokenManager.kt | 22 ++--------- .../core/DataConnectAuthUnitTest.kt | 37 ------------------- 3 files changed, 16 insertions(+), 57 deletions(-) diff --git a/firebase-dataconnect/androidTestutil/src/main/kotlin/com/google/firebase/dataconnect/testutil/TestFirebaseAppFactory.kt b/firebase-dataconnect/androidTestutil/src/main/kotlin/com/google/firebase/dataconnect/testutil/TestFirebaseAppFactory.kt index b291450927f..fd56cacfa24 100644 --- a/firebase-dataconnect/androidTestutil/src/main/kotlin/com/google/firebase/dataconnect/testutil/TestFirebaseAppFactory.kt +++ b/firebase-dataconnect/androidTestutil/src/main/kotlin/com/google/firebase/dataconnect/testutil/TestFirebaseAppFactory.kt @@ -22,6 +22,12 @@ import com.google.firebase.app import com.google.firebase.initialize import com.google.firebase.util.nextAlphanumericString import kotlin.random.Random +import kotlin.time.Duration.Companion.seconds +import kotlinx.coroutines.DelicateCoroutinesApi +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.GlobalScope +import kotlinx.coroutines.delay +import kotlinx.coroutines.launch /** * A JUnit test rule that creates instances of [FirebaseApp] for use during testing, and closes them @@ -37,6 +43,12 @@ class TestFirebaseAppFactory : FactoryTestRule() { ) override fun destroyInstance(instance: FirebaseApp) { - instance.delete() + // Work around app crash due to IllegalStateException from FirebaseAuth if `delete()` is called + // very quickly after `FirebaseApp.getInstance()`. See b/378116261 for details. + @OptIn(DelicateCoroutinesApi::class) + GlobalScope.launch(Dispatchers.IO) { + delay(1.seconds) + instance.delete() + } } } diff --git a/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/DataConnectCredentialsTokenManager.kt b/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/DataConnectCredentialsTokenManager.kt index f859a4fd35a..61b0f9bd769 100644 --- a/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/DataConnectCredentialsTokenManager.kt +++ b/firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/core/DataConnectCredentialsTokenManager.kt @@ -236,9 +236,7 @@ internal sealed class DataConnectCredentialsTokenManager( if (state.compareAndSet(oldState, State.Closed)) { providerListenerPair?.run { - provider?.let { provider -> - runIgnoringFirebaseAppDeleted { removeTokenListener(provider, tokenListener) } - } + provider?.let { provider -> removeTokenListener(provider, tokenListener) } } return } @@ -422,7 +420,7 @@ internal sealed class DataConnectCredentialsTokenManager( @DeferredApi private fun onProviderAvailable(newProvider: T, tokenListener: L) { logger.debug { "onProviderAvailable(newProvider=$newProvider)" } - runIgnoringFirebaseAppDeleted { addTokenListener(newProvider, tokenListener) } + addTokenListener(newProvider, tokenListener) while (true) { val oldState = state.get() @@ -437,7 +435,7 @@ internal sealed class DataConnectCredentialsTokenManager( "onProviderAvailable(newProvider=$newProvider)" + " unregistering token listener that was just added" } - runIgnoringFirebaseAppDeleted { removeTokenListener(newProvider, tokenListener) } + removeTokenListener(newProvider, tokenListener) break } is State.Ready -> @@ -486,20 +484,6 @@ internal sealed class DataConnectCredentialsTokenManager( private class GetTokenCancelledException(cause: Throwable) : DataConnectException("getToken() was cancelled, likely by close()", cause) - // Work around a race condition where addIdTokenListener() and removeIdTokenListener() throw if - // the FirebaseApp is deleted during or before its invocation. - private fun runIgnoringFirebaseAppDeleted(block: () -> Unit) { - try { - block() - } catch (e: IllegalStateException) { - if (e.message == "FirebaseApp was deleted") { - logger.warn(e) { "ignoring exception: $e" } - } else { - throw e - } - } - } - protected data class GetTokenResult(val token: String?) private companion object { diff --git a/firebase-dataconnect/src/test/kotlin/com/google/firebase/dataconnect/core/DataConnectAuthUnitTest.kt b/firebase-dataconnect/src/test/kotlin/com/google/firebase/dataconnect/core/DataConnectAuthUnitTest.kt index 81bf0b7fcd9..b45a943a1f4 100644 --- a/firebase-dataconnect/src/test/kotlin/com/google/firebase/dataconnect/core/DataConnectAuthUnitTest.kt +++ b/firebase-dataconnect/src/test/kotlin/com/google/firebase/dataconnect/core/DataConnectAuthUnitTest.kt @@ -552,43 +552,6 @@ class DataConnectAuthUnitTest { ) } - @Test - fun `addIdTokenListener() throwing IllegalStateException due to FirebaseApp deleted should be ignored`() = - runTest { - every { mockInternalAuthProvider.addIdTokenListener(any()) } throws - firebaseAppDeletedException - coEvery { mockInternalAuthProvider.getAccessToken(any()) } returns taskForToken(accessToken) - val dataConnectAuth = newDataConnectAuth() - dataConnectAuth.initialize() - advanceUntilIdle() - - eventually(`check every 100 milliseconds for 2 seconds`) { - mockLogger.shouldHaveLoggedExactlyOneMessageContaining( - "ignoring exception: $firebaseAppDeletedException" - ) - } - val result = dataConnectAuth.getToken(requestId) - withClue("result=$result") { result shouldBe accessToken } - } - - @Test - fun `removeIdTokenListener() throwing IllegalStateException due to FirebaseApp deleted should be ignored`() = - runTest { - every { mockInternalAuthProvider.removeIdTokenListener(any()) } throws - firebaseAppDeletedException - val dataConnectAuth = newDataConnectAuth() - dataConnectAuth.initialize() - advanceUntilIdle() - - dataConnectAuth.close() - - eventually(`check every 100 milliseconds for 2 seconds`) { - mockLogger.shouldHaveLoggedExactlyOneMessageContaining( - "ignoring exception: $firebaseAppDeletedException" - ) - } - } - private fun TestScope.newDataConnectAuth( deferredInternalAuthProvider: DeferredInternalAuthProvider = ImmediateDeferred(mockInternalAuthProvider),