From 8a3152c1ccb87e87dc70a153b9b5013422e4cbde Mon Sep 17 00:00:00 2001 From: Ben Dean-Kawamura Date: Thu, 12 Jun 2025 16:51:43 -0400 Subject: [PATCH] Make `DatabaseLoginsStorage` async There hasn't been agreement on a grand vision for async Rust in app-services (#6549). I think part of the reason is that ADR is discussing too many things. This is my attempt to take a small change from that ADR and see if we agree. This change takes the async wrapping code that's currently in android-components and moves it to our app-services wrapper. This improves the API boundary IMO. The SYNC sync and the credentials management teams are responsible for threading issues. For example, we're the one's thinking through how to avoid blocking too many threads when we're waiting on the primary password dialog. Since we're responsible for threading issues, we should own the code On a practical level: I have some ideas on how to improve our threading strategy and it's simpler to think about those changes if all the code is in one repo. Here's the corresponding android-components change: https://github.com/bendk/firefox/commit/d038ba6fd1ec1b39f76e0720e594916d61a4600a If we agree on this, then I think we can do something similar for iOS. What do others think? Is this a good idea? Do we need/want an ADR for this? --- components/logins/android/build.gradle | 2 + .../logins/DatabaseLoginsStorage.kt | 70 +++++++++---------- .../logins/DatabaseLoginsStorageTest.kt | 18 +++-- gradle/libs.versions.toml | 2 + 4 files changed, 49 insertions(+), 43 deletions(-) diff --git a/components/logins/android/build.gradle b/components/logins/android/build.gradle index 850925d5e5..254b884fdc 100644 --- a/components/logins/android/build.gradle +++ b/components/logins/android/build.gradle @@ -19,10 +19,12 @@ dependencies { api project(':init_rust_components') api project(':sync15') + implementation libs.kotlin.coroutines implementation libs.mozilla.glean implementation project(':init_rust_components') testImplementation libs.mozilla.glean.forUnitTests + testImplementation libs.kotlin.coroutines.test testImplementation libs.androidx.test.core testImplementation libs.androidx.work.testing testImplementation project(":syncmanager") diff --git a/components/logins/android/src/main/java/mozilla/appservices/logins/DatabaseLoginsStorage.kt b/components/logins/android/src/main/java/mozilla/appservices/logins/DatabaseLoginsStorage.kt index ca3dd32217..cfb33c4051 100644 --- a/components/logins/android/src/main/java/mozilla/appservices/logins/DatabaseLoginsStorage.kt +++ b/components/logins/android/src/main/java/mozilla/appservices/logins/DatabaseLoginsStorage.kt @@ -3,17 +3,11 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ package mozilla.appservices.logins - -/** - * Import some private Glean types, so that we can use them in type declarations. - * - * I do not like importing these private classes, but I do like the nice generic - * code they allow me to write! By agreement with the Glean team, we must not - * instantiate anything from these classes, and it's on us to fix any bustage - * on version updates. - */ +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.withContext import mozilla.telemetry.glean.private.CounterMetricType import mozilla.telemetry.glean.private.LabeledMetricType +import kotlin.coroutines.CoroutineContext import org.mozilla.appservices.logins.GleanMetrics.LoginsStore as LoginsStoreMetrics /** @@ -21,7 +15,11 @@ import org.mozilla.appservices.logins.GleanMetrics.LoginsStore as LoginsStoreMet * LoginStore. */ -class DatabaseLoginsStorage(dbPath: String, keyManager: KeyManager) : AutoCloseable { +class DatabaseLoginsStorage( + dbPath: String, + keyManager: KeyManager, + private val coroutineContext: CoroutineContext = Dispatchers.IO, +) : AutoCloseable { private var store: LoginStore init { @@ -30,94 +28,94 @@ class DatabaseLoginsStorage(dbPath: String, keyManager: KeyManager) : AutoClosea } @Throws(LoginsApiException::class) - fun reset() { - this.store.reset() + suspend fun reset(): Unit = withContext(coroutineContext) { + store.reset() } @Throws(LoginsApiException::class) - fun wipeLocal() { - this.store.wipeLocal() + suspend fun wipeLocal(): Unit = withContext(coroutineContext) { + store.wipeLocal() } @Throws(LoginsApiException::class) - fun delete(id: String): Boolean { - return writeQueryCounters.measure { + suspend fun delete(id: String): Boolean = withContext(coroutineContext) { + writeQueryCounters.measure { store.delete(id) } } @Throws(LoginsApiException::class) - fun get(id: String): Login? { - return readQueryCounters.measure { + suspend fun get(id: String): Login? = withContext(coroutineContext) { + readQueryCounters.measure { store.get(id) } } @Throws(LoginsApiException::class) - fun touch(id: String) { + suspend fun touch(id: String): Unit = withContext(coroutineContext) { writeQueryCounters.measure { store.touch(id) } } @Throws(LoginsApiException::class) - fun isEmpty(): Boolean { - return readQueryCounters.measure { + suspend fun isEmpty(): Boolean = withContext(coroutineContext) { + readQueryCounters.measure { store.isEmpty() } } @Throws(LoginsApiException::class) - fun list(): List { - return readQueryCounters.measure { + suspend fun list(): List = withContext(coroutineContext) { + readQueryCounters.measure { store.list() } } @Throws(LoginsApiException::class) - fun hasLoginsByBaseDomain(baseDomain: String): Boolean { - return readQueryCounters.measure { + suspend fun hasLoginsByBaseDomain(baseDomain: String): Boolean = withContext(coroutineContext) { + readQueryCounters.measure { store.hasLoginsByBaseDomain(baseDomain) } } @Throws(LoginsApiException::class) - fun getByBaseDomain(baseDomain: String): List { - return readQueryCounters.measure { + suspend fun getByBaseDomain(baseDomain: String): List = withContext(coroutineContext) { + readQueryCounters.measure { store.getByBaseDomain(baseDomain) } } @Throws(LoginsApiException::class) - fun findLoginToUpdate(look: LoginEntry): Login? { - return readQueryCounters.measure { + suspend fun findLoginToUpdate(look: LoginEntry): Login? = withContext(coroutineContext) { + readQueryCounters.measure { store.findLoginToUpdate(look) } } @Throws(LoginsApiException::class) - fun add(entry: LoginEntry): Login { - return writeQueryCounters.measure { + suspend fun add(entry: LoginEntry): Login = withContext(coroutineContext) { + writeQueryCounters.measure { store.add(entry) } } @Throws(LoginsApiException::class) - fun update(id: String, entry: LoginEntry): Login { - return writeQueryCounters.measure { + suspend fun update(id: String, entry: LoginEntry): Login = withContext(coroutineContext) { + writeQueryCounters.measure { store.update(id, entry) } } @Throws(LoginsApiException::class) - fun addOrUpdate(entry: LoginEntry): Login { - return writeQueryCounters.measure { + suspend fun addOrUpdate(entry: LoginEntry): Login = withContext(coroutineContext) { + writeQueryCounters.measure { store.addOrUpdate(entry) } } fun registerWithSyncManager() { - return store.registerWithSyncManager() + store.registerWithSyncManager() } @Synchronized diff --git a/components/logins/android/src/test/java/mozilla/appservices/logins/DatabaseLoginsStorageTest.kt b/components/logins/android/src/test/java/mozilla/appservices/logins/DatabaseLoginsStorageTest.kt index e19e7d6cfa..341ebce208 100644 --- a/components/logins/android/src/test/java/mozilla/appservices/logins/DatabaseLoginsStorageTest.kt +++ b/components/logins/android/src/test/java/mozilla/appservices/logins/DatabaseLoginsStorageTest.kt @@ -4,6 +4,7 @@ package mozilla.appservices.logins import androidx.test.core.app.ApplicationProvider +import kotlinx.coroutines.test.runTest import mozilla.appservices.RustComponentsInitializer import mozilla.appservices.syncmanager.SyncManager import mozilla.telemetry.glean.testing.GleanTestRule @@ -11,7 +12,6 @@ import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertNotNull import org.junit.Assert.assertNull -import org.junit.Assert.assertThrows import org.junit.Assert.assertTrue import org.junit.Assert.fail import org.junit.Rule @@ -41,7 +41,7 @@ class DatabaseLoginsStorageTest { return DatabaseLoginsStorage(dbPath = dbPath.absolutePath, keyManager = keyManager) } - protected fun getTestStore(): DatabaseLoginsStorage { + protected suspend fun getTestStore(): DatabaseLoginsStorage { val store = createTestStore() store.add( @@ -77,7 +77,7 @@ class DatabaseLoginsStorageTest { } @Test - fun testMetricsGathering() { + fun testMetricsGathering() = runTest { val store = createTestStore() assertNull(LoginsStoreMetrics.writeQueryCount.testGetValue()) @@ -132,7 +132,7 @@ class DatabaseLoginsStorageTest { } @Test - fun testTouch() { + fun testTouch() = runTest { val store = getTestStore() val login = store.list()[0] // Wait 100ms so that touch is certain to change timeLastUsed. @@ -145,13 +145,17 @@ class DatabaseLoginsStorageTest { assertEquals(login.timesUsed + 1, updatedLogin!!.timesUsed) assert(updatedLogin.timeLastUsed > login.timeLastUsed) - assertThrows(LoginsApiException.NoSuchRecord::class.java) { store.touch("abcdabcdabcd") } + try { + store.touch("abcdabcdabcd") + } catch (e: LoginsApiException.NoSuchRecord) { + // Expected error + } finishAndClose(store) } @Test - fun testDelete() { + fun testDelete() = runTest { val store = getTestStore() val login = store.list()[0] @@ -165,7 +169,7 @@ class DatabaseLoginsStorageTest { } @Test - fun testWipeLocal() { + fun testWipeLocal() = runTest { val test = getTestStore() val logins = test.list() assertEquals(2, logins.size) diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 191368d152..6fa24dffa7 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -14,6 +14,7 @@ protobuf-plugin = "0.9.5" # Kotlin kotlin-compiler = "2.1.21" kotlin-coroutines = "1.10.2" +kotlin-coroutines-test = "1.10.2" # Mozilla android-components = "139.0.20250417022706" @@ -58,6 +59,7 @@ protobuf-compiler = { group = "com.google.protobuf", name = "protoc", version.re # Kotlin kotlin-gradle-plugin = { group = "org.jetbrains.kotlin", name = "kotlin-gradle-plugin", version.ref = "kotlin-compiler" } kotlin-coroutines = { group = "org.jetbrains.kotlinx", name = "kotlinx-coroutines-android", version.ref = "kotlin-coroutines" } +kotlin-coroutines-test = { group = "org.jetbrains.kotlinx", name = "kotlinx-coroutines-test", version.ref = "kotlin-coroutines-test" } # Mozilla mozilla-concept-fetch = { group = "org.mozilla.components", name = "concept-fetch", version.ref = "android-components" }