Skip to content

Commit 8984a0f

Browse files
bendkbendk
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 team and the credentials management teams are responsible for threading issues like how to avoid blocking too many threads when we're waiting on the primary password dialog. Therefore, the code that affects this should be owned by us. In more practical terms, 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://phabricator.services.mozilla.com/D253666 What do others think? Is this a good idea? Do we need/want an ADR for this?
1 parent dea9dae commit 8984a0f

File tree

4 files changed

+49
-34
lines changed

4 files changed

+49
-34
lines changed

‎components/logins/android/build.gradle‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,12 @@ dependencies {
1919
api project(':init_rust_components')
2020
api project(':sync15')
2121

22+
implementation libs.kotlin.coroutines
2223
implementation libs.mozilla.glean
2324
implementation project(':init_rust_components')
2425

2526
testImplementation libs.mozilla.glean.forUnitTests
27+
testImplementation libs.kotlin.coroutines.test
2628
testImplementation libs.androidx.test.core
2729
testImplementation libs.androidx.work.testing
2830
testImplementation project(":syncmanager")

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

Lines changed: 34 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,9 @@
33
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
44

55
package mozilla.appservices.logins
6+
import kotlin.coroutines.CoroutineContext
7+
import kotlinx.coroutines.Dispatchers
8+
import kotlinx.coroutines.withContext
69

710
/**
811
* Import some private Glean types, so that we can use them in type declarations.
@@ -21,7 +24,11 @@ import org.mozilla.appservices.logins.GleanMetrics.LoginsStore as LoginsStoreMet
2124
* LoginStore.
2225
*/
2326

24-
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 {
2532
private var store: LoginStore
2633

2734
init {
@@ -30,94 +37,94 @@ class DatabaseLoginsStorage(dbPath: String, keyManager: KeyManager) : AutoClosea
3037
}
3138

3239
@Throws(LoginsApiException::class)
33-
fun reset() {
34-
this.store.reset()
40+
suspend fun reset(): Unit = withContext(coroutineContext) {
41+
store.reset()
3542
}
3643

3744
@Throws(LoginsApiException::class)
38-
fun wipeLocal() {
39-
this.store.wipeLocal()
45+
suspend fun wipeLocal(): Unit = withContext(coroutineContext) {
46+
store.wipeLocal()
4047
}
4148

4249
@Throws(LoginsApiException::class)
43-
fun delete(id: String): Boolean {
44-
return writeQueryCounters.measure {
50+
suspend fun delete(id: String): Boolean = withContext(coroutineContext) {
51+
writeQueryCounters.measure {
4552
store.delete(id)
4653
}
4754
}
4855

4956
@Throws(LoginsApiException::class)
50-
fun get(id: String): Login? {
51-
return readQueryCounters.measure {
57+
suspend fun get(id: String): Login? = withContext(coroutineContext) {
58+
readQueryCounters.measure {
5259
store.get(id)
5360
}
5461
}
5562

5663
@Throws(LoginsApiException::class)
57-
fun touch(id: String) {
64+
suspend fun touch(id: String): Unit = withContext(coroutineContext) {
5865
writeQueryCounters.measure {
5966
store.touch(id)
6067
}
6168
}
6269

6370
@Throws(LoginsApiException::class)
64-
fun isEmpty(): Boolean {
65-
return readQueryCounters.measure {
71+
suspend fun isEmpty(): Boolean = withContext(coroutineContext) {
72+
readQueryCounters.measure {
6673
store.isEmpty()
6774
}
6875
}
6976

7077
@Throws(LoginsApiException::class)
71-
fun list(): List<Login> {
72-
return readQueryCounters.measure {
78+
suspend fun list(): List<Login> = withContext(coroutineContext) {
79+
readQueryCounters.measure {
7380
store.list()
7481
}
7582
}
7683

7784
@Throws(LoginsApiException::class)
78-
fun hasLoginsByBaseDomain(baseDomain: String): Boolean {
79-
return readQueryCounters.measure {
85+
suspend fun hasLoginsByBaseDomain(baseDomain: String): Boolean = withContext(coroutineContext) {
86+
readQueryCounters.measure {
8087
store.hasLoginsByBaseDomain(baseDomain)
8188
}
8289
}
8390

8491
@Throws(LoginsApiException::class)
85-
fun getByBaseDomain(baseDomain: String): List<Login> {
86-
return readQueryCounters.measure {
92+
suspend fun getByBaseDomain(baseDomain: String): List<Login> = withContext(coroutineContext) {
93+
readQueryCounters.measure {
8794
store.getByBaseDomain(baseDomain)
8895
}
8996
}
9097

9198
@Throws(LoginsApiException::class)
92-
fun findLoginToUpdate(look: LoginEntry): Login? {
93-
return readQueryCounters.measure {
99+
suspend fun findLoginToUpdate(look: LoginEntry): Login? = withContext(coroutineContext) {
100+
readQueryCounters.measure {
94101
store.findLoginToUpdate(look)
95102
}
96103
}
97104

98105
@Throws(LoginsApiException::class)
99-
fun add(entry: LoginEntry): Login {
100-
return writeQueryCounters.measure {
106+
suspend fun add(entry: LoginEntry): Login = withContext(coroutineContext) {
107+
writeQueryCounters.measure {
101108
store.add(entry)
102109
}
103110
}
104111

105112
@Throws(LoginsApiException::class)
106-
fun update(id: String, entry: LoginEntry): Login {
107-
return writeQueryCounters.measure {
113+
suspend fun update(id: String, entry: LoginEntry): Login = withContext(coroutineContext) {
114+
writeQueryCounters.measure {
108115
store.update(id, entry)
109116
}
110117
}
111118

112119
@Throws(LoginsApiException::class)
113-
fun addOrUpdate(entry: LoginEntry): Login {
114-
return writeQueryCounters.measure {
120+
suspend fun addOrUpdate(entry: LoginEntry): Login = withContext(coroutineContext) {
121+
writeQueryCounters.measure {
115122
store.addOrUpdate(entry)
116123
}
117124
}
118125

119126
fun registerWithSyncManager() {
120-
return store.registerWithSyncManager()
127+
store.registerWithSyncManager()
121128
}
122129

123130
@Synchronized

‎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
@@ -4,14 +4,14 @@
44
package mozilla.appservices.logins
55

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

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

4747
store.add(
@@ -77,7 +77,7 @@ class DatabaseLoginsStorageTest {
7777
}
7878

7979
@Test
80-
fun testMetricsGathering() {
80+
fun testMetricsGathering() = runTest {
8181
val store = createTestStore()
8282

8383
assertNull(LoginsStoreMetrics.writeQueryCount.testGetValue())
@@ -132,7 +132,7 @@ class DatabaseLoginsStorageTest {
132132
}
133133

134134
@Test
135-
fun testTouch() {
135+
fun testTouch() = runTest {
136136
val store = getTestStore()
137137
val login = store.list()[0]
138138
// Wait 100ms so that touch is certain to change timeLastUsed.
@@ -145,13 +145,17 @@ class DatabaseLoginsStorageTest {
145145
assertEquals(login.timesUsed + 1, updatedLogin!!.timesUsed)
146146
assert(updatedLogin.timeLastUsed > login.timeLastUsed)
147147

148-
assertThrows(LoginsApiException.NoSuchRecord::class.java) { store.touch("abcdabcdabcd") }
148+
try {
149+
store.touch("abcdabcdabcd")
150+
} catch(e: LoginsApiException.NoSuchRecord) {
151+
// Expected error
152+
}
149153

150154
finishAndClose(store)
151155
}
152156

153157
@Test
154-
fun testDelete() {
158+
fun testDelete() = runTest {
155159
val store = getTestStore()
156160
val login = store.list()[0]
157161

@@ -165,7 +169,7 @@ class DatabaseLoginsStorageTest {
165169
}
166170

167171
@Test
168-
fun testWipeLocal() {
172+
fun testWipeLocal() = runTest {
169173
val test = getTestStore()
170174
val logins = test.list()
171175
assertEquals(2, logins.size)

‎gradle/libs.versions.toml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ protobuf-plugin = "0.9.5"
1414
# Kotlin
1515
kotlin-compiler = "2.1.21"
1616
kotlin-coroutines = "1.10.2"
17+
kotlin-coroutines-test = "1.10.2"
1718

1819
# Mozilla
1920
android-components = "139.0.20250417022706"
@@ -58,6 +59,7 @@ protobuf-compiler = { group = "com.google.protobuf", name = "protoc", version.re
5859
# Kotlin
5960
kotlin-gradle-plugin = { group = "org.jetbrains.kotlin", name = "kotlin-gradle-plugin", version.ref = "kotlin-compiler" }
6061
kotlin-coroutines = { group = "org.jetbrains.kotlinx", name = "kotlinx-coroutines-android", version.ref = "kotlin-coroutines" }
62+
kotlin-coroutines-test = { group = "org.jetbrains.kotlinx", name = "kotlinx-coroutines-test", version.ref = "kotlin-coroutines-test" }
6163

6264
# Mozilla
6365
mozilla-concept-fetch = { group = "org.mozilla.components", name = "concept-fetch", version.ref = "android-components" }

0 commit comments

Comments
 (0)