Skip to content

Commit d35e776

Browse files
committed
Make DatabaseLoginsStorage async
There hasn't been agreement on a grand vision for async Rust in app-services (mozilla#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: bendk/firefox@d038ba6 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?
1 parent 42c175c commit d35e776

File tree

4 files changed

+47
-36
lines changed

4 files changed

+47
-36
lines changed

components/logins/android/build.gradle

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,14 +46,14 @@ dependencies {
4646
api project(':sync15')
4747

4848
implementation project(':init_rust_components')
49-
49+
implementation libs.kotlinx.coroutines
5050
implementation libs.mozilla.glean
5151

5252
testImplementation project(":syncmanager")
53-
5453
testImplementation libs.androidx.test.core
5554
testImplementation libs.androidx.work.testing
5655
testImplementation libs.mozilla.glean.forUnitTests
56+
testImplementation libs.kotlinx.coroutines.test
5757
}
5858

5959
ext.configureUniFFIBindgen("logins")

components/logins/android/src/main/java/mozilla/appservices/logins/DatabaseLoginsStorage.kt

Lines changed: 33 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -12,17 +12,23 @@ package mozilla.appservices.logins
1212
* instantiate anything from these classes, and it's on us to fix any bustage
1313
* on version updates.
1414
*/
15-
15+
import kotlinx.coroutines.Dispatchers
16+
import kotlinx.coroutines.withContext
1617
import mozilla.telemetry.glean.private.CounterMetricType
1718
import mozilla.telemetry.glean.private.LabeledMetricType
19+
import kotlin.coroutines.CoroutineContext
1820
import org.mozilla.appservices.logins.GleanMetrics.LoginsStore as LoginsStoreMetrics
1921

2022
/**
2123
* An artifact of the uniffi conversion - a thin-ish wrapper around a
2224
* LoginStore.
2325
*/
2426

25-
class DatabaseLoginsStorage(dbPath: String, keyManager: KeyManager) : AutoCloseable {
27+
class DatabaseLoginsStorage(
28+
dbPath: String,
29+
keyManager: KeyManager,
30+
private val coroutineContext: CoroutineContext = Dispatchers.IO,
31+
) : AutoCloseable {
2632
private var store: LoginStore
2733

2834
init {
@@ -31,68 +37,68 @@ class DatabaseLoginsStorage(dbPath: String, keyManager: KeyManager) : AutoClosea
3137
}
3238

3339
@Throws(LoginsApiException::class)
34-
fun reset() {
35-
this.store.reset()
40+
suspend fun reset(): Unit = withContext(coroutineContext) {
41+
store.reset()
3642
}
3743

3844
@Throws(LoginsApiException::class)
39-
fun wipeLocal() {
40-
this.store.wipeLocal()
45+
suspend fun wipeLocal(): Unit = withContext(coroutineContext) {
46+
store.wipeLocal()
4147
}
4248

4349
@Throws(LoginsApiException::class)
44-
fun delete(id: String): Boolean {
45-
return store.delete(id)
50+
suspend fun delete(id: String): Boolean = withContext(coroutineContext) {
51+
store.delete(id)
4652
}
4753

4854
@Throws(LoginsApiException::class)
49-
fun get(id: String): Login? {
50-
return store.get(id)
55+
suspend fun get(id: String): Login? = withContext(coroutineContext) {
56+
store.get(id)
5157
}
5258

5359
@Throws(LoginsApiException::class)
54-
fun touch(id: String) {
60+
suspend fun touch(id: String): Unit = withContext(coroutineContext) {
5561
store.touch(id)
5662
}
5763

5864
@Throws(LoginsApiException::class)
59-
fun isEmpty(): Boolean {
60-
return store.isEmpty()
65+
suspend fun isEmpty(): Boolean = withContext(coroutineContext) {
66+
store.isEmpty()
6167
}
6268

6369
@Throws(LoginsApiException::class)
64-
fun list(): List<Login> {
65-
return store.list()
70+
suspend fun list(): List<Login> = withContext(coroutineContext) {
71+
store.list()
6672
}
6773

6874
@Throws(LoginsApiException::class)
69-
fun hasLoginsByBaseDomain(baseDomain: String): Boolean {
70-
return store.hasLoginsByBaseDomain(baseDomain)
75+
suspend fun hasLoginsByBaseDomain(baseDomain: String): Boolean = withContext(coroutineContext) {
76+
store.hasLoginsByBaseDomain(baseDomain)
7177
}
7278

7379
@Throws(LoginsApiException::class)
74-
fun getByBaseDomain(baseDomain: String): List<Login> {
75-
return store.getByBaseDomain(baseDomain)
80+
suspend fun getByBaseDomain(baseDomain: String): List<Login> = withContext(coroutineContext) {
81+
store.getByBaseDomain(baseDomain)
7682
}
7783

7884
@Throws(LoginsApiException::class)
79-
fun findLoginToUpdate(look: LoginEntry): Login? {
80-
return store.findLoginToUpdate(look)
85+
suspend fun findLoginToUpdate(look: LoginEntry): Login? = withContext(coroutineContext) {
86+
store.findLoginToUpdate(look)
8187
}
8288

8389
@Throws(LoginsApiException::class)
84-
fun add(entry: LoginEntry): Login {
85-
return store.add(entry)
90+
suspend fun add(entry: LoginEntry): Login = withContext(coroutineContext) {
91+
store.add(entry)
8692
}
8793

8894
@Throws(LoginsApiException::class)
89-
fun update(id: String, entry: LoginEntry): Login {
90-
return store.update(id, entry)
95+
suspend fun update(id: String, entry: LoginEntry): Login = withContext(coroutineContext) {
96+
store.update(id, entry)
9197
}
9298

9399
@Throws(LoginsApiException::class)
94-
fun addOrUpdate(entry: LoginEntry): Login {
95-
return store.addOrUpdate(entry)
100+
suspend fun addOrUpdate(entry: LoginEntry): Login = withContext(coroutineContext) {
101+
store.addOrUpdate(entry)
96102
}
97103

98104
fun registerWithSyncManager() {

components/logins/android/src/test/java/mozilla/appservices/logins/DatabaseLoginsStorageTest.kt

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,14 @@
55
package mozilla.appservices.logins
66

77
import androidx.test.core.app.ApplicationProvider
8+
import kotlinx.coroutines.test.runTest
89
import mozilla.appservices.RustComponentsInitializer
910
import mozilla.appservices.syncmanager.SyncManager
1011
import mozilla.telemetry.glean.testing.GleanTestRule
1112
import org.junit.Assert.assertEquals
1213
import org.junit.Assert.assertFalse
1314
import org.junit.Assert.assertNotNull
1415
import org.junit.Assert.assertNull
15-
import org.junit.Assert.assertThrows
1616
import org.junit.Assert.assertTrue
1717
import org.junit.Assert.fail
1818
import org.junit.Rule
@@ -41,7 +41,7 @@ class DatabaseLoginsStorageTest {
4141
return DatabaseLoginsStorage(dbPath = dbPath.absolutePath, keyManager = keyManager)
4242
}
4343

44-
private fun getTestStore(): DatabaseLoginsStorage {
44+
private suspend fun getTestStore(): DatabaseLoginsStorage {
4545
val store = createTestStore()
4646

4747
store.add(
@@ -76,8 +76,8 @@ class DatabaseLoginsStorageTest {
7676
// if this is all we need to do, then this helper should die!
7777
}
7878

79-
@Test
80-
fun testTouch() {
79+
@Test
80+
fun testTouch() = runTest {
8181
val store = getTestStore()
8282
val login = store.list()[0]
8383
// Wait 100ms so that touch is certain to change timeLastUsed.
@@ -90,13 +90,17 @@ class DatabaseLoginsStorageTest {
9090
assertEquals(login.timesUsed + 1, updatedLogin!!.timesUsed)
9191
assert(updatedLogin.timeLastUsed > login.timeLastUsed)
9292

93-
assertThrows(LoginsApiException.NoSuchRecord::class.java) { store.touch("abcdabcdabcd") }
93+
try {
94+
store.touch("abcdabcdabcd")
95+
} catch (e: LoginsApiException.NoSuchRecord) {
96+
// Expected error
97+
}
9498

9599
finishAndClose(store)
96100
}
97101

98102
@Test
99-
fun testDelete() {
103+
fun testDelete() = runTest {
100104
val store = getTestStore()
101105
val login = store.list()[0]
102106

@@ -110,7 +114,7 @@ class DatabaseLoginsStorageTest {
110114
}
111115

112116
@Test
113-
fun testWipeLocal() {
117+
fun testWipeLocal() = runTest {
114118
val test = getTestStore()
115119
val logins = test.list()
116120
assertEquals(2, logins.size)

gradle/libs.versions.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ protobuf-compiler = { group = "com.google.protobuf", name = "protoc", version.re
6060
# Kotlin
6161
kotlin-gradle-plugin = { group = "org.jetbrains.kotlin", name = "kotlin-gradle-plugin", version.ref = "kotlin" }
6262
kotlinx-coroutines = { group = "org.jetbrains.kotlinx", name = "kotlinx-coroutines-android", version.ref = "coroutines" }
63+
kotlinx-coroutines-test = { group = "org.jetbrains.kotlinx", name = "kotlinx-coroutines-test", version.ref = "coroutines" }
6364
kotlinx-serialization-json = { group = "org.jetbrains.kotlinx", name = "kotlinx-serialization-json", version.ref = "kotlinx-serialization" }
6465
kotlin-serialization = { group = "org.jetbrains.kotlin.plugin.serialization", name = "org.jetbrains.kotlin.plugin.serialization.gradle.plugin", version.ref = "kotlin" }
6566

0 commit comments

Comments
 (0)