From 686b62cb3f73ac2fbb37154dd5bb7a5ef4aa8a3d Mon Sep 17 00:00:00 2001 From: Ben Dean-Kawamura Date: Fri, 3 Jan 2025 15:57:24 -0500 Subject: [PATCH] Async Rust ADR --- docs/adr/0009-async-rust.md | 191 +++++++ .../0009/option-b-android-components.diff | 256 ++++++++++ .../adr/files/0009/option-b-app-services.diff | 467 ++++++++++++++++++ .../files/0009/option-b-firefox-desktop.diff | 130 +++++ docs/adr/files/0009/option-b-firefox-ios.diff | 109 ++++ .../0009/option-c-android-components.diff | 246 +++++++++ .../adr/files/0009/option-c-app-services.diff | 339 +++++++++++++ .../files/0009/option-c-firefox-android.diff | 92 ++++ 8 files changed, 1830 insertions(+) create mode 100644 docs/adr/0009-async-rust.md create mode 100644 docs/adr/files/0009/option-b-android-components.diff create mode 100644 docs/adr/files/0009/option-b-app-services.diff create mode 100644 docs/adr/files/0009/option-b-firefox-desktop.diff create mode 100644 docs/adr/files/0009/option-b-firefox-ios.diff create mode 100644 docs/adr/files/0009/option-c-android-components.diff create mode 100644 docs/adr/files/0009/option-c-app-services.diff create mode 100644 docs/adr/files/0009/option-c-firefox-android.diff diff --git a/docs/adr/0009-async-rust.md b/docs/adr/0009-async-rust.md new file mode 100644 index 0000000000..c5afddee9e --- /dev/null +++ b/docs/adr/0009-async-rust.md @@ -0,0 +1,191 @@ +# Using Async Rust + +* Status: draft +* Deciders: +* Date: ??? +* Previous discussion: https://github.com/mozilla/application-services/pull/5910 + +## Context and Problem Statement + +Our Rust components are currently written using synchronous Rust, however all current consumers wrap them in another class to present an async-style interface: + +* Firefox Android uses `CoroutineScope.runBlocking` to adapt methods to be `suspend` +* Firefox iOs mostly uses `DispatchQueue` and completion handlers, but the long-term plan is to transition to `async` functions. + Some components already do this, using `withCheckedThrowingContinuation` on top of `DispatchQueue` to define async functions. +* Firefox Desktop auto-generates C++ wrapper code to make them async using a [TOML config file](https://searchfox.org/mozilla-central/rev/cdfe21b20eacfaa6712dd9821d6383859ce386c6/toolkit/components/uniffi-bindgen-gecko-js/config.toml). + This was chosen because JS is single-threaded and doesn't provide a simple way to run blocking functions in a task queue. + One drawback from this choice is that Desktop has a very different async system compared to the mobile apps. + +With the new UniFFI async capabilities, it's possible to move the async code into Rust and avoid this wrapper layer. +This ADR discusses if we should do this, how we could implement it, and what our general approach to async should be. +To make things concrete, the Suggest component is used as an example of what would change if we moved to async Rust. + +### Scope + +This ADR covers the question of using a wrapper layer to implant async functionality. +It does not cover some related questions: + +* **Scheduling this work.** + If we decide to embrace async Rust, we do not need to commit to any particular deadline for switching to it. +* **Wrappers in general.** + [The previous PR](https://github.com/mozilla/application-services/pull/5910/) dedicated a separate ADR for this question, but we never came to a decision on this. + It seems that there's no single answer to the question, the general consensus was that we should evaluate things on a case-by-case basis and this ADR will just focus on the case of async. +* **Wrappers in consumer code.** + If we change things so that the current async wrapping layer is no longer needed, consumer engineers will still have a choice on if they want to keep their current wrapper layer or not. + This choice should be left to consumer engineers. + This ADR will touch on this, but not recommend anything. + +## Considered Options + +* **Option A: Do nothing.** + Keep the current system in place. +* **Option B: Async Rust using the current UniFFI.** + Make our component methods async by requiring the foreign code to pass in an interface that runs tasks in a worker queue and using +`oneshot::Channel` to communicate the results back. See below for how this would work in practice. +* **Option C: Async Rust with additional UniFFI support.** + Like `B`, but make `WorkerQueue` and `RustTask` built-in UniFFI types. + This would eliminate the need to define our own interfaces for these, instead UniFFI would allow foreign code to passing in `DispatchQueue`/`CoroutineScopes` to Rust and Rust could use those to run blocking tasks in a work queue. +* **Option D: Extend the uniffi-bindgen-gecko-js config system to Mobile.** + Extend the Gecko-JS system, where the config specifies that functions/methods should be wrapped as async, to also work for Kotlin/Swift. + +### Example code + +I (Ben) made some diffs to illustrate how the code would change for each option. +When doing that, I realized the wrapper layer was actually implementing important functionality for the component: + +* The Kotlin code extended our interrupt support to also interrupt pending `query()` calls. +* The Kotlin code also catches all errors and coverts them into regular returns (`query()` returns an empty list and `ingest()` returns false). +* The Swift code split async methods into 2 categories: low-priority calls like `ingest()` and high-priority calls like `query()` + +As part of the example changes, I moved this functionality to our Rust components. +This results in some extra code in the diffs, but I thought it was good to explore the messy details of this transition. +A important factor for deciding this ADR is where we want this functionality to live. + +#### Option B + - [app-services changes](./files/0009/option-b-app-services.diff) + - [android-components changes](./files/0009/option-b-android-components.diff) + - [Firefox iOS changes](./files/0009/option-b-firefox-ios.diff) + - [Firefox Desktop changes](./files/0009/option-b-firefox-desktop.diff) + +This option is possible to implement today, so I was able to test that the app-services and android-components changes actually compiled +I didn't do that for iOS and desktop, mostly because it's harder to perform a local build. +I think we can be confident that the actual changes would look similar to this. + +Summary of changes: +* Added the `WorkerQueue` and `RustTask` interfaces + * `RustTask` encapsulates a single task + * `WorkerQueue` is implemented by the foreign side and runs a `RustTask` in a work queue where it's okay to block. +* Use the above interfaces to wrap sync calls to be async, by running them in the `WorkerQueue` then sending the result via a `oneshot::Channel` that the + original async function was `await`ing. + * This is also a good place to do the error conversion/reporting. +* SuggestStoreBuilder gets a `build_async` method, which creates an `SuggestStoreAsync`. + It inputs 2 `WorkerQueue`s: one for ingest and one for everything else. +* Added supporting code so that `query()` can create it's `SqlInterruptScope` ahead of time, outside of the scheduled task. + That way `interrupt()` can also interrupt pending calls to `query()`. +* Updated the `ingest()` method to catch errors and return `false` rather than propagating them. + In general, I think this is a better API for consumers since there's nothing meaningful they can do with errors other than report them. + `query()` should probably also be made infallible. +* Removed the wrapper class from `android-components`, but kept the wrapper class in `firefox-ios`. + The goal here was to show the different options for consumer code. + +#### Option C + - [app-services changes](./files/0009/option-c-app-services.diff) + - [android-components changes](./files/0009/option-c-android-components.diff) + - [Firefox iOS changes](./files/0009/option-c-firefox-ios.diff) + - [Firefox Desktop changes](./files/0009/option-c-firefox-desktop.diff) + +This option assumes that UniFFI will provide something similar to `WorkerQueue`, so we don't need to define/implement that in app-services or the consumer repos. +This requires changes to UniFFI core, so none of this code works today. +However, I think we can be fairly confident that these changes will work since we have a long-standing [UniFFI PR](https://github.com/mozilla/uniffi-rs/pull/1837) that implements a similar feature -- in fact it's a more complex version. + +Summary of changes: essentially the same as `B`, but we don't need to define/implement the `WorkerQueue` and `RustTask` interfaces. + +#### Option D + +I'm not completely sure how this one would work in practice, but I assume that it would mean TOML configuration for Kotlin/Swift similar to the current Desktop configuration: + +``` +[suggest.async_wrappers] +# All functions/methods are wrapped to be async by default and must be `await`ed. +enable = true +# These are exceptions to the async wrapping. These functions must not be `await`ed. +main_thread = [ + "raw_suggestion_url_matches", + "SuggestStore.new", + "SuggestStore.interrupt", + "SuggestStoreBuilder.new", + "SuggestStoreBuilder.data_path", + "SuggestStoreBuilder.load_extension", + "SuggestStoreBuilder.remote_settings_bucket_name", + "SuggestStoreBuilder.remote_settings_server", + "SuggestStoreBuilder.build", +] +``` + +This would auto-generate async wrapper code. +For example, the Kotlin code would look something like this: + +``` +class SuggestStore { + + /** + * Queries the database for suggestions. + */ + suspend fun query(query: SuggestionQuery): List = + withContext(Dispatchers.IO) { + // ...current auto-generated code here + } +``` + +## Decision Outcome + +## Pros and Cons of the Options + +### (A) Do nothing + +* Good, because it requires the least amount of work +* Good, because it's proven to work +* Bad, because async is handled very differently in JS vs Kotlin+Swift +* Bad, because our component functionality implemented in wrapper code. + It needs to be duplicated or will be only available on one platform. + For example, the Kotlin functionality to interrupt pending `query()` calls would also be useful on Swift, but we never implemented that. +* Bad, because our component functionality is implemented in other repos, which makes it likely to go stale. + However, we could mitigate this moving that Kotlin/Swift wrapper code into app-services. +* Bad, because it makes our documentation system worse. For example, our docs for [query()](https://mozilla.github.io/application-services/kotlin/kotlin-components-docs/mozilla.appservices.suggest/-suggest-store/query.html) say it's a sync function, but it practice it's actually async. + +### (B) Async Rust using the current UniFFI + +* Good, because we'll have a common system for Async that works similarly all platforms +* Good, because our generated docs will match how the methods are used in practice. +* Good, because it encourages us to move async complexity into the core code. + This makes it available to all platforms and more likely to be maintained. +* Good because it opens the door for more efficient thread usage. + For example, we could make methods more fine-grained and only use the work queue for SQL operations, but not for network requests. +* Bad, because we're taking on risk by introducing the async UniFFI code to app-services. +* Bad, because our consumers need to define all a `WorkerQueue` implementations, which is a bit of a burden. + This feels especially bad on JS, where the whole concept of a work queue feels alien. +* Bad, because it makes it harder to provide bindings on new languages that don't support async, like C and C++. + Maybe we could bridge the gap with some sort of callback-based async system, but it's not clear how that would work. + +### (C) Async Rust with additional UniFFI support + +* Good/Bad/Risky for mostly the same reasons as (B). +* Good, because it removes the need for us and consumers to define `WorkerQueue` traits/impls. +* Good, because it can simplify the `WorkerQueue` code. + In particular, we can guarantee that the task is only executed once, which removes the need for `RustTaskContainer`. +* Bad, because we'll have to maintain the UniFFI worker queue code. + +### (D) Extend the uniffi-bindgen-gecko-js config system to Mobile + +* Good, because we'll have a common system for Async that works similarly all platforms +* Mostly good, because our generated docs will match how the methods are used in practice. + However, it could be weird to write docstrings for sync functions that are wrapped to be async. +* Good, because it's less Risky than B/C. + The system would just be auto-generating the kind of wrappers we already used. +* Bad, because it's hard for consumer engineers to configure. + For example, how can firefox-ios devs pick which QOS they want to use with a `DispatchQueue`? + They would probably have to specify it in the config file, which is less convent than passing it in from code. +* Bad, because it's not clear how we could handle complex cases like using both a low-priority and high-priority queue. + +## Decision Outcome +? diff --git a/docs/adr/files/0009/option-b-android-components.diff b/docs/adr/files/0009/option-b-android-components.diff new file mode 100644 index 0000000000..e17a695514 --- /dev/null +++ b/docs/adr/files/0009/option-b-android-components.diff @@ -0,0 +1,256 @@ +diff --git a/mobile/android/android-components/components/feature/fxsuggest/build.gradle b/mobile/android/android-components/components/feature/fxsuggest/build.gradle +index d5b717c0ef..28d9c82eb2 100644 +--- a/mobile/android/android-components/components/feature/fxsuggest/build.gradle ++++ b/mobile/android/android-components/components/feature/fxsuggest/build.gradle +@@ -42,6 +42,7 @@ + } + + dependencies { ++ api ComponentsDependencies.mozilla_appservices_suggest + api ComponentsDependencies.mozilla_remote_settings + + implementation project(':browser-state') +@@ -54,7 +55,6 @@ + + implementation ComponentsDependencies.androidx_work_runtime + implementation ComponentsDependencies.kotlin_coroutines +- implementation ComponentsDependencies.mozilla_appservices_suggest + + testImplementation project(':support-test') + +diff --git a/mobile/android/android-components/components/feature/fxsuggest/src/main/java/mozilla/components/feature/fxsuggest/FxSuggestStorage.kt b/mobile/android/android-components/components/feature/fxsuggest/src/main/java/mozilla/components/feature/fxsuggest/FxSuggestStorage.kt +index f4af344906..ddad66f290 100644 +--- a/mobile/android/android-components/components/feature/fxsuggest/src/main/java/mozilla/components/feature/fxsuggest/FxSuggestStorage.kt ++++ b/mobile/android/android-components/components/feature/fxsuggest/src/main/java/mozilla/components/feature/fxsuggest/FxSuggestStorage.kt +@@ -6,120 +6,47 @@ + + import android.content.Context + import androidx.annotation.VisibleForTesting ++import kotlinx.coroutines.launch ++import kotlinx.coroutines.CoroutineDispatcher + import kotlinx.coroutines.CoroutineScope + import kotlinx.coroutines.Dispatchers + import kotlinx.coroutines.cancelChildren + import kotlinx.coroutines.withContext + import mozilla.appservices.remotesettings.RemoteSettingsServer ++import mozilla.appservices.suggest.RustTask + import mozilla.appservices.suggest.SuggestApiException + import mozilla.appservices.suggest.SuggestIngestionConstraints + import mozilla.appservices.suggest.SuggestStore ++import mozilla.appservices.suggest.SuggestStoreAsync + import mozilla.appservices.suggest.SuggestStoreBuilder + import mozilla.appservices.suggest.Suggestion + import mozilla.appservices.suggest.SuggestionQuery ++import mozilla.appservices.suggest.WorkerQueue + import mozilla.components.support.base.log.logger.Logger + ++class WorkerQueueKt(val scope: CoroutineScope) : WorkerQueue { ++ override fun addTask(task: RustTask) { ++ scope.launch { ++ task.run() ++ } ++ } ++} ++ ++const val DATABASE_NAME = "suggest_data.sqlite" ++ + /** +- * A coroutine-aware wrapper around the synchronous [SuggestStore] interface. +- * ++ * Construct a new store + * @param context The Android application context. + * @param remoteSettingsServer The [RemoteSettingsServer] from which to ingest + * suggestions. + */ +-class FxSuggestStorage(context: Context, remoteSettingsServer: RemoteSettingsServer = RemoteSettingsServer.Prod) { +- // Lazily initializes the store on first use. `cacheDir` and using the `File` constructor +- // does I/O, so `store.value` should only be accessed from the read or write scope. +- @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) +- internal val store: Lazy = lazy { +- SuggestStoreBuilder() +- .dataPath(context.getDatabasePath(DATABASE_NAME).absolutePath) +- .remoteSettingsServer(remoteSettingsServer) +- .build() +- } +- +- // We expect almost all Suggest storage operations to be reads, with infrequent writes. The +- // I/O dispatcher supports both workloads, and using separate scopes lets us cancel reads +- // without affecting writes. +- private val readScope: CoroutineScope = CoroutineScope(Dispatchers.IO) +- private val writeScope: CoroutineScope = CoroutineScope(Dispatchers.IO) +- +- private val logger = Logger("FxSuggestStorage") +- +- /** +- * Queries the store for suggestions. +- * +- * @param query The input and suggestion types to match. +- * @return A list of matching suggestions. +- */ +- suspend fun query(query: SuggestionQuery): List = +- withContext(readScope.coroutineContext) { +- handleSuggestExceptions("query", emptyList()) { +- store.value.query(query) +- } +- } +- +- /** +- * Downloads and persists new Firefox Suggest search suggestions. +- * +- * @param constraints Optional limits on suggestions to ingest. +- * @return `true` if ingestion succeeded; `false` if ingestion failed and should be retried. +- */ +- suspend fun ingest(constraints: SuggestIngestionConstraints = SuggestIngestionConstraints()): Boolean = +- withContext(writeScope.coroutineContext) { +- handleSuggestExceptions("ingest", false) { +- store.value.ingest(constraints) +- true +- } +- } +- +- /** +- * Run startup ingestion +- * +- * This will run ingestion, only if there are currently no suggestions in the database. This is +- * used to initialize the database on first startup and also after Firefox updates that change +- * the schema (which often cause the suggestions table to be cleared). +- */ +- suspend fun runStartupIngestion() { +- logger.info("runStartupIngestion") +- ingest(SuggestIngestionConstraints(emptyOnly = true)) +- } +- +- /** +- * Interrupts any ongoing queries for suggestions. +- */ +- fun cancelReads() { +- if (store.isInitialized()) { +- store.value.interrupt() +- readScope.coroutineContext.cancelChildren() +- } +- } +- +- /** +- * Runs an [operation] with the given [name], ignoring and logging any non-fatal exceptions. +- * Returns either the result of the [operation], or the provided [default] value if the +- * [operation] throws an exception. +- * +- * @param name The name of the operation to run. +- * @param default The default value to return if the operation fails. +- * @param operation The operation to run. +- */ +- private inline fun handleSuggestExceptions( +- name: String, +- default: T, +- operation: () -> T, +- ): T { +- return try { +- operation() +- } catch (e: SuggestApiException) { +- logger.warn("Ignoring exception from `$name`", e) +- default +- } +- } +- +- internal companion object { +- /** +- * The database file name for permanent data. +- */ +- const val DATABASE_NAME = "suggest_data.sqlite" +- } ++public fun createSuggestStore( ++ context: Context, ++ remoteSettingsServer: RemoteSettingsServer = RemoteSettingsServer.Prod, ++ scope: CoroutineScope = CoroutineScope(Dispatchers.IO) ++): SuggestStoreAsync { ++ return SuggestStoreBuilder() ++ .dataPath(context.getDatabasePath(DATABASE_NAME).absolutePath) ++ .remoteSettingsServer(remoteSettingsServer) ++ .buildAsync(WorkerQueueKt(scope)) + } +diff --git a/mobile/android/android-components/components/feature/fxsuggest/src/main/java/mozilla/components/feature/fxsuggest/FxSuggestSuggestionProvider.kt b/mobile/android/android-components/components/feature/fxsuggest/src/main/java/mozilla/components/feature/fxsuggest/FxSuggestSuggestionProvider.kt +index eebe52baeb..92248d6583 100644 +--- a/mobile/android/android-components/components/feature/fxsuggest/src/main/java/mozilla/components/feature/fxsuggest/FxSuggestSuggestionProvider.kt ++++ b/mobile/android/android-components/components/feature/fxsuggest/src/main/java/mozilla/components/feature/fxsuggest/FxSuggestSuggestionProvider.kt +@@ -5,6 +5,7 @@ + package mozilla.components.feature.fxsuggest + + import android.content.res.Resources ++import mozilla.appservices.suggest.InterruptKind + import mozilla.appservices.suggest.Suggestion + import mozilla.appservices.suggest.SuggestionProvider + import mozilla.appservices.suggest.SuggestionQuery +@@ -76,7 +77,7 @@ + } + + override fun onInputCancelled() { +- GlobalFxSuggestDependencyProvider.requireStorage().cancelReads() ++ GlobalFxSuggestDependencyProvider.requireStorage().interrupt(InterruptKind.READ) + } + + private suspend fun List.into(): List = +diff --git a/mobile/android/android-components/components/feature/fxsuggest/src/main/java/mozilla/components/feature/fxsuggest/GlobalFxSuggestDependencyProvider.kt b/mobile/android/android-components/components/feature/fxsuggest/src/main/java/mozilla/components/feature/fxsuggest/GlobalFxSuggestDependencyProvider.kt +index faddb798f8..15f4277ac4 100644 +--- a/mobile/android/android-components/components/feature/fxsuggest/src/main/java/mozilla/components/feature/fxsuggest/GlobalFxSuggestDependencyProvider.kt ++++ b/mobile/android/android-components/components/feature/fxsuggest/src/main/java/mozilla/components/feature/fxsuggest/GlobalFxSuggestDependencyProvider.kt +@@ -4,11 +4,13 @@ + + package mozilla.components.feature.fxsuggest + ++import mozilla.appservices.suggest.SuggestStoreAsync ++ + /** + * Provides global access to the dependencies needed to access Firefox Suggest search suggestions. + */ + object GlobalFxSuggestDependencyProvider { +- internal var storage: FxSuggestStorage? = null ++ internal var storage: SuggestStoreAsync? = null + + /** + * Initializes this provider with a wrapped Suggest store. +@@ -18,11 +20,11 @@ + * + * @param storage The wrapped Suggest store. + */ +- fun initialize(storage: FxSuggestStorage) { ++ fun initialize(storage: SuggestStoreAsync) { + this.storage = storage + } + +- internal fun requireStorage(): FxSuggestStorage { ++ internal fun requireStorage(): SuggestStoreAsync { + return requireNotNull(storage) { + "`GlobalFxSuggestDependencyProvider.initialize` must be called before accessing `storage`" + } +diff --git a/mobile/android/android-components/samples/compose-browser/src/main/java/org/mozilla/samples/compose/browser/Components.kt b/mobile/android/android-components/samples/compose-browser/src/main/java/org/mozilla/samples/compose/browser/Components.kt +index 874b4081de..5802784cf3 100644 +--- a/mobile/android/android-components/samples/compose-browser/src/main/java/org/mozilla/samples/compose/browser/Components.kt ++++ b/mobile/android/android-components/samples/compose-browser/src/main/java/org/mozilla/samples/compose/browser/Components.kt +@@ -7,14 +7,15 @@ + import android.content.Context + import androidx.compose.runtime.Composable + import androidx.compose.ui.platform.LocalContext ++import mozilla.appservices.suggest.SuggestStoreAsync + import mozilla.components.browser.engine.gecko.GeckoEngine + import mozilla.components.browser.engine.gecko.fetch.GeckoViewFetchClient + import mozilla.components.browser.state.engine.EngineMiddleware + import mozilla.components.browser.state.store.BrowserStore + import mozilla.components.concept.engine.Engine + import mozilla.components.concept.fetch.Client ++import mozilla.components.feature.fxsuggest.createSuggestStore + import mozilla.components.feature.fxsuggest.FxSuggestIngestionScheduler +-import mozilla.components.feature.fxsuggest.FxSuggestStorage + import mozilla.components.feature.search.SearchUseCases + import mozilla.components.feature.search.middleware.SearchMiddleware + import mozilla.components.feature.search.region.RegionMiddleware +@@ -52,8 +53,8 @@ + + val locationService by lazy { LocationService.default() } + +- val fxSuggestStorage: FxSuggestStorage by lazy { +- FxSuggestStorage(context) ++ val fxSuggestStorage: SuggestStoreAsync by lazy { ++ createSuggestStore(context) + } + + val fxSuggestIngestionScheduler: FxSuggestIngestionScheduler by lazy { diff --git a/docs/adr/files/0009/option-b-app-services.diff b/docs/adr/files/0009/option-b-app-services.diff new file mode 100644 index 0000000000..ae598b73b6 --- /dev/null +++ b/docs/adr/files/0009/option-b-app-services.diff @@ -0,0 +1,467 @@ +diff --git a/Cargo.lock b/Cargo.lock +index c1171dad5d..b99fbfc825 100644 +--- a/Cargo.lock ++++ b/Cargo.lock +@@ -3431,6 +3431,12 @@ + ] + + [[package]] ++name = "oneshot" ++version = "0.1.8" ++source = "registry+https://github.com/rust-lang/crates.io-index" ++checksum = "e296cf87e61c9cfc1a61c3c63a0f7f286ed4554e0e22be84e8a38e1d264a2a29" ++ ++[[package]] + name = "oorandom" + version = "11.1.3" + source = "registry+https://github.com/rust-lang/crates.io-index" +@@ -4870,6 +4876,7 @@ + "interrupt-support", + "log", + "once_cell", ++ "oneshot", + "parking_lot", + "rc_crypto", + "remote_settings", +diff --git a/components/suggest/Cargo.toml b/components/suggest/Cargo.toml +index 241b332798..f68cb5ae64 100644 +--- a/components/suggest/Cargo.toml ++++ b/components/suggest/Cargo.toml +@@ -15,6 +15,7 @@ + interrupt-support = { path = "../support/interrupt" } + log = "0.4" + once_cell = "1.5" ++oneshot = "0.1.8" + parking_lot = ">=0.11,<=0.12" + remote_settings = { path = "../remote_settings" } + rusqlite = { version = "0.31.0", features = ["functions", "bundled", "load_extension"] } +diff --git a/components/suggest/android/build.gradle b/components/suggest/android/build.gradle +index 09056f3a91..70bc549387 100644 +--- a/components/suggest/android/build.gradle ++++ b/components/suggest/android/build.gradle +@@ -7,6 +7,8 @@ + + dependencies { + api project(":remotesettings") ++ ++ implementation libs.kotlinx.coroutines + } + + ext.configureUniFFIBindgen("suggest") +diff --git a/components/suggest/src/db.rs b/components/suggest/src/db.rs +index dcde6f7c85..fc1d22cf05 100644 +--- a/components/suggest/src/db.rs ++++ b/components/suggest/src/db.rs +@@ -113,18 +113,38 @@ + } + } + ++ pub fn begin_interrupt_scope(&self) -> Result { ++ Ok(self.interrupt_handle.begin_interrupt_scope()?) ++ } ++ + /// Accesses the Suggest database for reading. + pub fn read(&self, op: impl FnOnce(&SuggestDao) -> Result) -> Result { ++ self.read_with_scope(self.interrupt_handle.begin_interrupt_scope()?, op) ++ } ++ ++ /// Like [Self::read], but inputs a previously created `SqlInterruptScope` to use ++ pub fn read_with_scope( ++ &self, ++ scope: SqlInterruptScope, ++ op: impl FnOnce(&SuggestDao) -> Result, ++ ) -> Result { + let conn = self.conn.lock(); +- let scope = self.interrupt_handle.begin_interrupt_scope()?; + let dao = SuggestDao::new(&conn, &scope); + op(&dao) + } + + /// Accesses the Suggest database in a transaction for reading and writing. + pub fn write(&self, op: impl FnOnce(&mut SuggestDao) -> Result) -> Result { ++ self.write_with_scope(self.interrupt_handle.begin_interrupt_scope()?, op) ++ } ++ ++ /// Like [Self::write], but inputs a previously created `SqlInterruptScope` to use ++ pub fn write_with_scope( ++ &self, ++ scope: SqlInterruptScope, ++ op: impl FnOnce(&mut SuggestDao) -> Result, ++ ) -> Result { + let mut conn = self.conn.lock(); +- let scope = self.interrupt_handle.begin_interrupt_scope()?; + let tx = conn.transaction()?; + let mut dao = SuggestDao::new(&tx, &scope); + let result = op(&mut dao)?; +diff --git a/components/suggest/src/lib.rs b/components/suggest/src/lib.rs +index 689f39a57b..b9374e6daf 100644 +--- a/components/suggest/src/lib.rs ++++ b/components/suggest/src/lib.rs +@@ -18,6 +18,7 @@ + mod schema; + mod store; + mod suggestion; ++mod taskqueue; + #[cfg(test)] + mod testing; + pub mod util; +diff --git a/components/suggest/src/store.rs b/components/suggest/src/store.rs +index c61531908b..12afe1ee91 100644 +--- a/components/suggest/src/store.rs ++++ b/components/suggest/src/store.rs +@@ -10,6 +10,7 @@ + }; + + use error_support::{breadcrumb, handle_error}; ++use interrupt_support::SqlInterruptScope; + use once_cell::sync::OnceCell; + use parking_lot::Mutex; + use remote_settings::{self, RemoteSettingsConfig, RemoteSettingsServer}; +@@ -28,6 +29,7 @@ + SuggestAttachment, SuggestRecord, SuggestRecordId, SuggestRecordType, + }, + suggestion::AmpSuggestionType, ++ taskqueue::{run_in_background, WorkerQueue}, + QueryWithMetricsResult, Result, SuggestApiResult, Suggestion, SuggestionQuery, + }; + +@@ -116,6 +118,30 @@ + inner: SuggestStoreInner::new(data_path, extensions_to_load, client), + })) + } ++ ++ #[handle_error(Error)] ++ pub fn build_async( ++ &self, ++ worker_queue: Arc, ++ ) -> SuggestApiResult> { ++ let inner = self.0.lock(); ++ let extensions_to_load = inner.extensions_to_load.clone(); ++ let data_path = inner ++ .data_path ++ .clone() ++ .ok_or_else(|| Error::SuggestStoreBuilder("data_path not specified".to_owned()))?; ++ ++ let client = RemoteSettingsClient::new( ++ inner.remote_settings_server.clone(), ++ inner.remote_settings_bucket_name.clone(), ++ None, ++ )?; ++ ++ Ok(Arc::new(SuggestStoreAsync { ++ worker_queue, ++ inner: SuggestStoreInner::new(data_path, extensions_to_load, client), ++ })) ++ } + } + + /// What should be interrupted when [SuggestStore::interrupt] is called? +@@ -192,7 +218,10 @@ + /// Queries the database for suggestions. + #[handle_error(Error)] + pub fn query(&self, query: SuggestionQuery) -> SuggestApiResult> { +- Ok(self.inner.query(query)?.suggestions) ++ Ok(self ++ .inner ++ .query(query, self.inner.begin_reader_scope()?)? ++ .suggestions) + } + + /// Queries the database for suggestions. +@@ -201,7 +230,7 @@ + &self, + query: SuggestionQuery, + ) -> SuggestApiResult { +- self.inner.query(query) ++ self.inner.query(query, self.inner.begin_reader_scope()?) + } + + /// Dismiss a suggestion +@@ -312,6 +341,159 @@ + } + } + ++/// Suggest store where all the methods are async ++/// ++/// This duplicates the functionality from [SuggestStore], but exposes it as async methods. ++/// The plan is to migrate consumers over to this interface, then remove the old one. ++#[derive(uniffi::Object)] ++pub struct SuggestStoreAsync { ++ worker_queue: Arc, ++ inner: SuggestStoreInner, ++} ++ ++impl SuggestStoreAsync { ++ async fn wrap_method_call(self: Arc, f: F) -> SuggestApiResult ++ where ++ F: FnOnce(&SuggestStoreInner) -> Result, ++ F: Send + Sync + 'static, ++ T: Send + Sync + 'static, ++ { ++ run_in_background(self.worker_queue.clone(), move || f(&self.inner)) ++ .await ++ .map_err(error_support::convert_log_report_error) ++ } ++} ++#[uniffi::export] ++impl SuggestStoreAsync { ++ pub async fn query( ++ self: Arc, ++ query: SuggestionQuery, ++ ) -> SuggestApiResult> { ++ // Create the interrupt scope before `wrap_method_call`, this way we'll interrupts that ++ // happen after the query was scheduled, but before it was executed. ++ let scope = self ++ .inner ++ .begin_reader_scope() ++ .map_err(error_support::convert_log_report_error)?; ++ self.wrap_method_call(move |inner| Ok(inner.query(query, scope)?.suggestions)) ++ .await ++ } ++ ++ /// Queries the database for suggestions. ++ pub async fn query_with_metrics( ++ self: Arc, ++ query: SuggestionQuery, ++ ) -> SuggestApiResult { ++ let scope = self ++ .inner ++ .begin_reader_scope() ++ .map_err(error_support::convert_log_report_error)?; ++ self.wrap_method_call(move |inner| inner.query(query, scope)) ++ .await ++ } ++ ++ /// Dismiss a suggestion ++ /// ++ /// Dismissed suggestions will not be returned again ++ /// ++ /// In the case of AMP suggestions this should be the raw URL. ++ pub async fn dismiss_suggestion( ++ self: Arc, ++ suggestion_url: String, ++ ) -> SuggestApiResult<()> { ++ self.wrap_method_call(move |inner| inner.dismiss_suggestion(suggestion_url)) ++ .await ++ } ++ ++ /// Clear dismissed suggestions ++ pub async fn clear_dismissed_suggestions(self: Arc) -> SuggestApiResult<()> { ++ self.wrap_method_call(|inner| inner.clear_dismissed_suggestions()) ++ .await ++ } ++ ++ /// Interrupts any ongoing queries. ++ /// ++ /// This should be called when the user types new input into the address ++ /// bar, to ensure that they see fresh suggestions as they type. This ++ /// method does not interrupt any ongoing ingests. ++ /// ++ /// Note: this method is not async, since the goal is to preempt currently running async ++ /// operations. ++ #[uniffi::method(default(kind = None))] ++ pub fn interrupt(self: Arc, kind: Option) { ++ self.inner.interrupt(kind) ++ } ++ ++ /// Ingests new suggestions from Remote Settings. ++ /// ++ /// Returns `true` if the ingest succeeded, `false` if it needs to be tried again ++ #[uniffi::method(default(constraints = None))] ++ pub async fn ingest(self: Arc, constraints: Option) -> bool { ++ let constraints = constraints.unwrap_or_default(); ++ self.wrap_method_call(move |inner| inner.ingest(constraints)) ++ .await ++ // Use `is_ok` to squash errors and return a boolean instead. ++ // Note: errors are logged by the call to `convert_log_report_error` ++ .is_ok() ++ } ++ ++ /// Removes all content from the database. ++ pub async fn clear(self: Arc) -> SuggestApiResult<()> { ++ self.wrap_method_call(move |inner| inner.clear()).await ++ } ++ ++ /// Returns global Suggest configuration data. ++ pub async fn fetch_global_config(self: Arc) -> SuggestApiResult { ++ self.wrap_method_call(move |inner| inner.fetch_global_config()) ++ .await ++ } ++ ++ /// Returns per-provider Suggest configuration data. ++ pub async fn fetch_provider_config( ++ self: Arc, ++ provider: SuggestionProvider, ++ ) -> SuggestApiResult> { ++ self.wrap_method_call(move |inner| inner.fetch_provider_config(provider)) ++ .await ++ } ++ ++ /// Fetches geonames stored in the database. A geoname represents a ++ /// geographic place. ++ /// ++ /// `query` is a string that will be matched directly against geoname names. ++ /// It is not a query string in the usual Suggest sense. `match_name_prefix` ++ /// determines whether prefix matching is performed on names excluding ++ /// abbreviations and airport codes. When `true`, names that start with ++ /// `query` will match. When false, names that equal `query` will match. ++ /// ++ /// `geoname_type` restricts returned geonames to a [`GeonameType`]. ++ /// ++ /// `filter` restricts returned geonames to certain cities or regions. ++ /// Cities can be restricted to regions by including the regions in ++ /// `filter`, and regions can be restricted to those containing certain ++ /// cities by including the cities in `filter`. This is especially useful ++ /// since city and region names are not unique. `filter` is disjunctive: If ++ /// any item in `filter` matches a geoname, the geoname will be filtered in. ++ /// ++ /// The query can match a single geoname in more than one way. For example, ++ /// it can match both a full name and an abbreviation. The returned vec of ++ /// [`GeonameMatch`] values will include all matches for a geoname, one ++ /// match per `match_type` per geoname. In other words, a matched geoname ++ /// can map to more than one `GeonameMatch`. ++ pub async fn fetch_geonames( ++ self: Arc, ++ query: String, ++ match_name_prefix: bool, ++ geoname_type: Option, ++ filter: Option>, ++ ) -> SuggestApiResult> { ++ self.wrap_method_call(move |inner| { ++ inner.fetch_geonames(&query, match_name_prefix, geoname_type, filter) ++ }) ++ .await ++ } ++} ++ + /// Constraints limit which suggestions to ingest from Remote Settings. + #[derive(Clone, Default, Debug, uniffi::Record)] + pub struct SuggestIngestionConstraints { +@@ -399,15 +581,27 @@ + .get_or_try_init(|| SuggestStoreDbs::open(&self.data_path, &self.extensions_to_load)) + } + +- fn query(&self, query: SuggestionQuery) -> Result { ++ fn begin_reader_scope(&self) -> Result { ++ self.dbs()?.reader.begin_interrupt_scope() ++ } ++ ++ fn query( ++ &self, ++ query: SuggestionQuery, ++ scope: SqlInterruptScope, ++ ) -> Result { ++ // Before anything, check if the scope was interrupted. This handles the case where query ++ // scheduled by SuggestStoreAsync was interrupted. ++ scope.err_if_interrupted()?; + let mut metrics = SuggestQueryMetrics::default(); + let mut suggestions = vec![]; + + let unique_providers = query.providers.iter().collect::>(); + let reader = &self.dbs()?.reader; + for provider in unique_providers { ++ let scope = scope.clone(); + let new_suggestions = metrics.measure_query(provider.to_string(), || { +- reader.read(|dao| match provider { ++ reader.read_with_scope(scope, |dao| match provider { + SuggestionProvider::Amp => { + dao.fetch_amp_suggestions(&query, AmpSuggestionType::Desktop) + } +@@ -950,7 +1144,8 @@ + } + + pub fn fetch_suggestions(&self, query: SuggestionQuery) -> Vec { +- self.inner.query(query).unwrap().suggestions ++ let scope = self.inner.begin_reader_scope().unwrap(); ++ self.inner.query(query, scope).unwrap().suggestions + } + + pub fn fetch_global_config(&self) -> SuggestGlobalConfig { +diff --git a/components/suggest/src/taskqueue.rs b/components/suggest/src/taskqueue.rs +new file mode 100644 +index 0000000000..44231470dc +--- /dev/null ++++ b/components/suggest/src/taskqueue.rs +@@ -1,0 +1,69 @@ ++/* This Source Code Form is subject to the terms of the Mozilla Public ++ * License, v. 2.0. If a copy of the MPL was not distributed with this ++ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ ++ ++use std::{future, sync::Arc}; ++ ++use parking_lot::Mutex; ++ ++/// Worker queue scheduler trait ++/// ++/// This is implemented by foreign code to schedule blocking Rust tasks: ++/// - On Swift, it's implemented with a `DispatchQueue` ++/// - On Kotlin, it's implemented with a `CoroutineContext` ++/// - On Gecko-JS, it's implemented in Rust using the `moz_task` crate. ++#[uniffi::export(with_foreign)] ++pub trait WorkerQueue: Send + Sync { ++ fn add_task(&self, task: Arc); ++} ++ ++#[uniffi::export] ++pub trait RustTask: Send + Sync { ++ fn run(&self); ++} ++ ++/// Schedule a closure to run in the global worker queue. Returns the result of the closure ++/// asynchronously ++pub async fn run_in_background( ++ worker_queue: Arc, ++ task: impl FnOnce() -> T + Send + Sync + 'static, ++) -> T { ++ let (tx, rx) = oneshot::channel(); ++ ++ worker_queue.add_task(RustTaskContainer::new_arc(move || { ++ if let Err(e) = tx.send(task()) { ++ error_support::report_error!("suggest-oneshot-send", "{e}"); ++ } ++ })); ++ match rx.await { ++ Ok(v) => v, ++ Err(e) => { ++ error_support::report_error!("suggest-oneshot-recv", "{e}"); ++ // Not much we can do here other than await forever ++ future::pending().await ++ } ++ } ++} ++ ++/// Implements RustTask for any closure ++struct RustTaskContainer { ++ /// The one tricky part is that the task can only be run once, but the foreign language gets a ++ /// shared reference to it, so put it behind a Mutex + Option ++ task: Mutex>, ++} ++ ++impl RustTaskContainer { ++ fn new_arc(task: T) -> Arc { ++ Arc::new(Self { ++ task: Mutex::new(Some(task)), ++ }) ++ } ++} ++ ++impl RustTask for RustTaskContainer { ++ fn run(&self) { ++ if let Some(f) = self.task.lock().take() { ++ f() ++ } ++ } ++} +diff --git a/components/support/interrupt/src/sql.rs b/components/support/interrupt/src/sql.rs +index af97a6e1c6..5e8e1821e1 100644 +--- a/components/support/interrupt/src/sql.rs ++++ b/components/support/interrupt/src/sql.rs +@@ -75,7 +75,7 @@ + /// + /// This is used by the rust code to check if an operation should fail because it was interrupted. + /// It handles the case where we get interrupted outside of an SQL query. +-#[derive(Debug)] ++#[derive(Clone, Debug)] + pub struct SqlInterruptScope { + start_value: usize, + interrupt_counter: Arc, diff --git a/docs/adr/files/0009/option-b-firefox-desktop.diff b/docs/adr/files/0009/option-b-firefox-desktop.diff new file mode 100644 index 0000000000..012c7e3af4 --- /dev/null +++ b/docs/adr/files/0009/option-b-firefox-desktop.diff @@ -0,0 +1,130 @@ +diff --git a/browser/components/urlbar/private/SuggestBackendRust.sys.mjs b/browser/components/urlbar/private/SuggestBackendRust.sys.mjs +index 5d9633ab60..8563e112aa 100644 +--- a/browser/components/urlbar/private/SuggestBackendRust.sys.mjs ++++ b/browser/components/urlbar/private/SuggestBackendRust.sys.mjs +@@ -20,6 +20,7 @@ + SuggestionProvider: "resource://gre/modules/RustSuggest.sys.mjs", + SuggestionProviderConstraints: "resource://gre/modules/RustSuggest.sys.mjs", + SuggestionQuery: "resource://gre/modules/RustSuggest.sys.mjs", ++ makeWorkerQueue: "resource://gre/modules/RustUniffiWorkerQueue.sys.mjs", + TaskQueue: "resource:///modules/UrlbarUtils.sys.mjs", + UrlbarPrefs: "resource:///modules/UrlbarPrefs.sys.mjs", + Utils: "resource://services-settings/Utils.sys.mjs", +@@ -389,7 +390,7 @@ + .remoteSettingsServer(this.#remoteSettingsServer) + .remoteSettingsBucketName(this.#remoteSettingsBucketName); + try { +- this.#store = builder.build(); ++ this.#store = builder.buildAsync(makeWorkerQueue()); + } catch (error) { + this.logger.error("Error initializing SuggestStore", error); + return; +diff --git a/toolkit/components/uniffi-bindgen-gecko-js/components/Cargo.toml b/toolkit/components/uniffi-bindgen-gecko-js/components/Cargo.toml +index a2c12729f9..bf67358981 100644 +--- a/toolkit/components/uniffi-bindgen-gecko-js/components/Cargo.toml ++++ b/toolkit/components/uniffi-bindgen-gecko-js/components/Cargo.toml +@@ -21,7 +21,8 @@ + suggest = "0.1" + relevancy = "0.1" + webext-storage = "0.1" ++worker-queue = { path = "worker-queue/" } + + [features] + # Should we depend on xpcom crates? +-xpcom = [] ++xpcom = ["worker-queue/moz_task"] +diff --git a/toolkit/components/uniffi-bindgen-gecko-js/components/worker-queue/Cargo.toml b/toolkit/components/uniffi-bindgen-gecko-js/components/worker-queue/Cargo.toml +new file mode 100644 +index 0000000000..e1dddaeb51 +--- /dev/null ++++ b/toolkit/components/uniffi-bindgen-gecko-js/components/worker-queue/Cargo.toml +@@ -1,0 +1,18 @@ ++[package] ++name = "uniffi-worker-queue" ++version = "0.21.0" ++edition = "2021" ++license = "MPL-2.0" ++publish = false ++ ++[dependencies] ++# This needs to be optional, since we want to build this library to generate the bindings from, but ++# moz-task can only currently be built from inside `./mach build`. ++moz_task = { path = "../../../../../xpcom/rust/moz_task/", optional = true } ++# Needed for the WorkQueue trait, we should probably move that code into a shared app-services crate ++# if it's going to be used by multiple compnents ++suggest = "0.1" ++uniffi = { workspace = true } ++ ++[build-dependencies] ++uniffi = { workspace = true, features = ["build"] } +diff --git a/toolkit/components/uniffi-bindgen-gecko-js/components/worker-queue/src/lib.rs b/toolkit/components/uniffi-bindgen-gecko-js/components/worker-queue/src/lib.rs +new file mode 100644 +index 0000000000..6c194c479e +--- /dev/null ++++ b/toolkit/components/uniffi-bindgen-gecko-js/components/worker-queue/src/lib.rs +@@ -1,0 +1,38 @@ ++/* This Source Code Form is subject to the terms of the Mozilla Public ++ * License, v. 2.0. If a copy of the MPL was not distributed with this ++ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ ++ ++use std::sync::Arc; ++ ++use suggest::{WorkerQueue, RustTask}; ++ ++/// Factory function to create a worker queue for JS using moz_task ++#[uniffi::export] ++pub fn make_worker_queue() -> Arc { ++ Arc::new(MozTaskWorkerQueue) ++} ++ ++ ++/// ZST that implements WorkerQueue using moz_task ++struct MozTaskWorkerQueue; ++ ++impl WorkerQueue for MozTaskWorkerQueue { ++ // This version is what runs when we're linked into libgecko ++ #[cfg(feature = "moz_task")] ++ fn add_task(&self, task: Arc) { ++ if let Err(e) = moz_task::dispatch_background_task("UniFFI task", move || task.run()) { ++ log::error!("Failed to dispatch background task: {e}"); ++ } ++ ++ } ++ ++ // This is as stub that allows us to build a library for uniffi-bindgen to use, but without ++ // depending on gecko. Gecko can only be built inside of `./mach build` and we want to build ++ // the library using plain `cargo build`. ++ #[cfg(not(feature = "moz_task"))] ++ fn add_task(&self, _task: Arc) { ++ unimplemented!("moz_task not enabled"); ++ } ++} ++ ++uniffi::setup_scaffolding!("uniffi_worker_queue"); +diff --git a/toolkit/components/uniffi-bindgen-gecko-js/config.toml b/toolkit/components/uniffi-bindgen-gecko-js/config.toml +index 5035b0671c..168f42479a 100644 +--- a/toolkit/components/uniffi-bindgen-gecko-js/config.toml ++++ b/toolkit/components/uniffi-bindgen-gecko-js/config.toml +@@ -11,20 +11,8 @@ + + + [suggest.async_wrappers] +-# All functions/methods are wrapped to be async by default and must be `await`ed. +-enable = true +-# These are exceptions to the async wrapping. These functions must not be `await`ed. +-main_thread = [ +- "raw_suggestion_url_matches", +- "SuggestStore.new", +- "SuggestStore.interrupt", +- "SuggestStoreBuilder.new", +- "SuggestStoreBuilder.data_path", +- "SuggestStoreBuilder.load_extension", +- "SuggestStoreBuilder.remote_settings_bucket_name", +- "SuggestStoreBuilder.remote_settings_server", +- "SuggestStoreBuilder.build", +-] ++# Suggest implments async from Rust, so we don't need to enable the wrappers ++enable = false + + [relevancy.async_wrappers] + # All functions/methods are wrapped to be async by default and must be `await`ed. diff --git a/docs/adr/files/0009/option-b-firefox-ios.diff b/docs/adr/files/0009/option-b-firefox-ios.diff new file mode 100644 index 0000000000..a32a841366 --- /dev/null +++ b/docs/adr/files/0009/option-b-firefox-ios.diff @@ -0,0 +1,109 @@ +diff --git a/firefox-ios/Storage/Rust/RustFirefoxSuggest.swift b/firefox-ios/Storage/Rust/RustFirefoxSuggest.swift +index e12d428192..7be51fe12e 100644 +--- a/firefox-ios/Storage/Rust/RustFirefoxSuggest.swift ++++ b/firefox-ios/Storage/Rust/RustFirefoxSuggest.swift +@@ -4,7 +4,7 @@ + + import Foundation + +-import class MozillaAppServices.SuggestStore ++import class MozillaAppServices.SuggestStoreAsync + import class MozillaAppServices.SuggestStoreBuilder + import class MozillaAppServices.Viaduct + import enum MozillaAppServices.SuggestionProvider +@@ -30,15 +30,24 @@ + func interruptEverything() + } + ++private class WorkerQueueSwift : WorkerQueue { ++ private let queue: DispatchQueue ++ ++ init(label: String, qos: DispatchQos) { ++ self.queue = DispatchQueue(label, qos) ++ } ++ ++ func addTask(task: RustTask) { ++ self.queue.async { ++ task.run() ++ } ++ } ++} ++ + /// Wraps the synchronous Rust `SuggestStore` binding to execute + /// blocking operations on a dispatch queue. + public class RustFirefoxSuggest: RustFirefoxSuggestProtocol { +- private let store: SuggestStore +- +- // Using a pair of serial queues lets read and write operations run +- // without blocking one another. +- private let writerQueue = DispatchQueue(label: "RustFirefoxSuggest.writer") +- private let readerQueue = DispatchQueue(label: "RustFirefoxSuggest.reader") ++ private let store: SuggestStoreAsync + + public init(dataPath: String, cachePath: String, remoteSettingsConfig: RemoteSettingsConfig? = nil) throws { + var builder = SuggestStoreBuilder() +@@ -49,24 +58,21 @@ + builder = builder.remoteSettingsConfig(config: remoteSettingsConfig) + } + +- store = try builder.build() ++ store = try builder.buildAsync( ++ // Use .userInitiated QOS for most operations. Methods like `query` should be executed ++ // ASAP. ++ workerQueue: WorkerQueueSwift(label: "RustFirefoxSuggest.general", qos: .userInitiated), ++ // Use .utility QOS for ingest since latency is not important here. ++ // Also, using a separate queue allows `ingest()` and `query()` to run side-by-side. ++ ingestWorkerQueue: WorkerQueueSwift(label: "RustFirefoxSuggest.ingest", qos: .utility) ++ ) + } + + public func ingest() async throws { + // Ensure that the Rust networking stack has been initialized before + // downloading new suggestions. This is safe to call multiple times. + Viaduct.shared.useReqwestBackend() +- +- try await withCheckedThrowingContinuation { continuation in +- writerQueue.async(qos: .utility) { +- do { +- try self.store.ingest(constraints: SuggestIngestionConstraints()) +- continuation.resume() +- } catch { +- continuation.resume(throwing: error) +- } +- } +- } ++ return try await self.store.ingest(constraints: SuggestIngestionConstraints()) + } + + public func query( +@@ -74,24 +80,15 @@ + providers: [SuggestionProvider], + limit: Int32 + ) async throws -> [RustFirefoxSuggestion] { +- return try await withCheckedThrowingContinuation { continuation in +- readerQueue.async(qos: .userInitiated) { +- do { +- let suggestions = try self.store.query(query: SuggestionQuery( +- keyword: keyword, +- providers: providers, +- limit: limit +- )).compactMap(RustFirefoxSuggestion.init) +- continuation.resume(returning: suggestions) +- } catch { +- continuation.resume(throwing: error) +- } +- } +- } ++ return try await self.store.query(query: SuggestionQuery( ++ keyword: keyword, ++ providers: providers, ++ limit: limit ++ )).compactMap(RustFirefoxSuggestion.init) + } + + public func interruptReader() { +- store.interrupt() ++ store.interrupt(kind: .read) + } + + public func interruptEverything() { diff --git a/docs/adr/files/0009/option-c-android-components.diff b/docs/adr/files/0009/option-c-android-components.diff new file mode 100644 index 0000000000..281080c860 --- /dev/null +++ b/docs/adr/files/0009/option-c-android-components.diff @@ -0,0 +1,246 @@ +diff --git a/mobile/android/android-components/components/feature/fxsuggest/build.gradle b/mobile/android/android-components/components/feature/fxsuggest/build.gradle +index d5b717c0ef..28d9c82eb2 100644 +--- a/mobile/android/android-components/components/feature/fxsuggest/build.gradle ++++ b/mobile/android/android-components/components/feature/fxsuggest/build.gradle +@@ -42,6 +42,7 @@ + } + + dependencies { ++ api ComponentsDependencies.mozilla_appservices_suggest + api ComponentsDependencies.mozilla_remote_settings + + implementation project(':browser-state') +@@ -54,7 +55,6 @@ + + implementation ComponentsDependencies.androidx_work_runtime + implementation ComponentsDependencies.kotlin_coroutines +- implementation ComponentsDependencies.mozilla_appservices_suggest + + testImplementation project(':support-test') + +diff --git a/mobile/android/android-components/components/feature/fxsuggest/src/main/java/mozilla/components/feature/fxsuggest/FxSuggestStorage.kt b/mobile/android/android-components/components/feature/fxsuggest/src/main/java/mozilla/components/feature/fxsuggest/FxSuggestStorage.kt +index f4af344906..a1eb5eee69 100644 +--- a/mobile/android/android-components/components/feature/fxsuggest/src/main/java/mozilla/components/feature/fxsuggest/FxSuggestStorage.kt ++++ b/mobile/android/android-components/components/feature/fxsuggest/src/main/java/mozilla/components/feature/fxsuggest/FxSuggestStorage.kt +@@ -6,120 +6,36 @@ + + import android.content.Context + import androidx.annotation.VisibleForTesting ++import kotlinx.coroutines.launch ++import kotlinx.coroutines.CoroutineDispatcher + import kotlinx.coroutines.CoroutineScope + import kotlinx.coroutines.Dispatchers + import kotlinx.coroutines.cancelChildren +-import kotlinx.coroutines.withContext + import mozilla.appservices.remotesettings.RemoteSettingsServer + import mozilla.appservices.suggest.SuggestApiException + import mozilla.appservices.suggest.SuggestIngestionConstraints + import mozilla.appservices.suggest.SuggestStore ++import mozilla.appservices.suggest.SuggestStoreAsync + import mozilla.appservices.suggest.SuggestStoreBuilder + import mozilla.appservices.suggest.Suggestion + import mozilla.appservices.suggest.SuggestionQuery + import mozilla.components.support.base.log.logger.Logger + ++const val DATABASE_NAME = "suggest_data.sqlite" ++ + /** +- * A coroutine-aware wrapper around the synchronous [SuggestStore] interface. +- * ++ * Construct a new store + * @param context The Android application context. + * @param remoteSettingsServer The [RemoteSettingsServer] from which to ingest + * suggestions. + */ +-class FxSuggestStorage(context: Context, remoteSettingsServer: RemoteSettingsServer = RemoteSettingsServer.Prod) { +- // Lazily initializes the store on first use. `cacheDir` and using the `File` constructor +- // does I/O, so `store.value` should only be accessed from the read or write scope. +- @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) +- internal val store: Lazy = lazy { +- SuggestStoreBuilder() +- .dataPath(context.getDatabasePath(DATABASE_NAME).absolutePath) +- .remoteSettingsServer(remoteSettingsServer) +- .build() +- } +- +- // We expect almost all Suggest storage operations to be reads, with infrequent writes. The +- // I/O dispatcher supports both workloads, and using separate scopes lets us cancel reads +- // without affecting writes. +- private val readScope: CoroutineScope = CoroutineScope(Dispatchers.IO) +- private val writeScope: CoroutineScope = CoroutineScope(Dispatchers.IO) +- +- private val logger = Logger("FxSuggestStorage") +- +- /** +- * Queries the store for suggestions. +- * +- * @param query The input and suggestion types to match. +- * @return A list of matching suggestions. +- */ +- suspend fun query(query: SuggestionQuery): List = +- withContext(readScope.coroutineContext) { +- handleSuggestExceptions("query", emptyList()) { +- store.value.query(query) +- } +- } +- +- /** +- * Downloads and persists new Firefox Suggest search suggestions. +- * +- * @param constraints Optional limits on suggestions to ingest. +- * @return `true` if ingestion succeeded; `false` if ingestion failed and should be retried. +- */ +- suspend fun ingest(constraints: SuggestIngestionConstraints = SuggestIngestionConstraints()): Boolean = +- withContext(writeScope.coroutineContext) { +- handleSuggestExceptions("ingest", false) { +- store.value.ingest(constraints) +- true +- } +- } +- +- /** +- * Run startup ingestion +- * +- * This will run ingestion, only if there are currently no suggestions in the database. This is +- * used to initialize the database on first startup and also after Firefox updates that change +- * the schema (which often cause the suggestions table to be cleared). +- */ +- suspend fun runStartupIngestion() { +- logger.info("runStartupIngestion") +- ingest(SuggestIngestionConstraints(emptyOnly = true)) +- } +- +- /** +- * Interrupts any ongoing queries for suggestions. +- */ +- fun cancelReads() { +- if (store.isInitialized()) { +- store.value.interrupt() +- readScope.coroutineContext.cancelChildren() +- } +- } +- +- /** +- * Runs an [operation] with the given [name], ignoring and logging any non-fatal exceptions. +- * Returns either the result of the [operation], or the provided [default] value if the +- * [operation] throws an exception. +- * +- * @param name The name of the operation to run. +- * @param default The default value to return if the operation fails. +- * @param operation The operation to run. +- */ +- private inline fun handleSuggestExceptions( +- name: String, +- default: T, +- operation: () -> T, +- ): T { +- return try { +- operation() +- } catch (e: SuggestApiException) { +- logger.warn("Ignoring exception from `$name`", e) +- default +- } +- } +- +- internal companion object { +- /** +- * The database file name for permanent data. +- */ +- const val DATABASE_NAME = "suggest_data.sqlite" +- } ++public fun createSuggestStore( ++ context: Context, ++ remoteSettingsServer: RemoteSettingsServer = RemoteSettingsServer.Prod, ++ scope: CoroutineScope = CoroutineScope(Dispatchers.IO) ++): SuggestStoreAsync { ++ return SuggestStoreBuilder() ++ .dataPath(context.getDatabasePath(DATABASE_NAME).absolutePath) ++ .remoteSettingsServer(remoteSettingsServer) ++ .buildAsync(scope) + } +diff --git a/mobile/android/android-components/components/feature/fxsuggest/src/main/java/mozilla/components/feature/fxsuggest/FxSuggestSuggestionProvider.kt b/mobile/android/android-components/components/feature/fxsuggest/src/main/java/mozilla/components/feature/fxsuggest/FxSuggestSuggestionProvider.kt +index eebe52baeb..92248d6583 100644 +--- a/mobile/android/android-components/components/feature/fxsuggest/src/main/java/mozilla/components/feature/fxsuggest/FxSuggestSuggestionProvider.kt ++++ b/mobile/android/android-components/components/feature/fxsuggest/src/main/java/mozilla/components/feature/fxsuggest/FxSuggestSuggestionProvider.kt +@@ -5,6 +5,7 @@ + package mozilla.components.feature.fxsuggest + + import android.content.res.Resources ++import mozilla.appservices.suggest.InterruptKind + import mozilla.appservices.suggest.Suggestion + import mozilla.appservices.suggest.SuggestionProvider + import mozilla.appservices.suggest.SuggestionQuery +@@ -76,7 +77,7 @@ + } + + override fun onInputCancelled() { +- GlobalFxSuggestDependencyProvider.requireStorage().cancelReads() ++ GlobalFxSuggestDependencyProvider.requireStorage().interrupt(InterruptKind.READ) + } + + private suspend fun List.into(): List = +diff --git a/mobile/android/android-components/components/feature/fxsuggest/src/main/java/mozilla/components/feature/fxsuggest/GlobalFxSuggestDependencyProvider.kt b/mobile/android/android-components/components/feature/fxsuggest/src/main/java/mozilla/components/feature/fxsuggest/GlobalFxSuggestDependencyProvider.kt +index faddb798f8..15f4277ac4 100644 +--- a/mobile/android/android-components/components/feature/fxsuggest/src/main/java/mozilla/components/feature/fxsuggest/GlobalFxSuggestDependencyProvider.kt ++++ b/mobile/android/android-components/components/feature/fxsuggest/src/main/java/mozilla/components/feature/fxsuggest/GlobalFxSuggestDependencyProvider.kt +@@ -4,11 +4,13 @@ + + package mozilla.components.feature.fxsuggest + ++import mozilla.appservices.suggest.SuggestStoreAsync ++ + /** + * Provides global access to the dependencies needed to access Firefox Suggest search suggestions. + */ + object GlobalFxSuggestDependencyProvider { +- internal var storage: FxSuggestStorage? = null ++ internal var storage: SuggestStoreAsync? = null + + /** + * Initializes this provider with a wrapped Suggest store. +@@ -18,11 +20,11 @@ + * + * @param storage The wrapped Suggest store. + */ +- fun initialize(storage: FxSuggestStorage) { ++ fun initialize(storage: SuggestStoreAsync) { + this.storage = storage + } + +- internal fun requireStorage(): FxSuggestStorage { ++ internal fun requireStorage(): SuggestStoreAsync { + return requireNotNull(storage) { + "`GlobalFxSuggestDependencyProvider.initialize` must be called before accessing `storage`" + } +diff --git a/mobile/android/android-components/samples/compose-browser/src/main/java/org/mozilla/samples/compose/browser/Components.kt b/mobile/android/android-components/samples/compose-browser/src/main/java/org/mozilla/samples/compose/browser/Components.kt +index 874b4081de..5802784cf3 100644 +--- a/mobile/android/android-components/samples/compose-browser/src/main/java/org/mozilla/samples/compose/browser/Components.kt ++++ b/mobile/android/android-components/samples/compose-browser/src/main/java/org/mozilla/samples/compose/browser/Components.kt +@@ -7,14 +7,15 @@ + import android.content.Context + import androidx.compose.runtime.Composable + import androidx.compose.ui.platform.LocalContext ++import mozilla.appservices.suggest.SuggestStoreAsync + import mozilla.components.browser.engine.gecko.GeckoEngine + import mozilla.components.browser.engine.gecko.fetch.GeckoViewFetchClient + import mozilla.components.browser.state.engine.EngineMiddleware + import mozilla.components.browser.state.store.BrowserStore + import mozilla.components.concept.engine.Engine + import mozilla.components.concept.fetch.Client ++import mozilla.components.feature.fxsuggest.createSuggestStore + import mozilla.components.feature.fxsuggest.FxSuggestIngestionScheduler +-import mozilla.components.feature.fxsuggest.FxSuggestStorage + import mozilla.components.feature.search.SearchUseCases + import mozilla.components.feature.search.middleware.SearchMiddleware + import mozilla.components.feature.search.region.RegionMiddleware +@@ -52,8 +53,8 @@ + + val locationService by lazy { LocationService.default() } + +- val fxSuggestStorage: FxSuggestStorage by lazy { +- FxSuggestStorage(context) ++ val fxSuggestStorage: SuggestStoreAsync by lazy { ++ createSuggestStore(context) + } + + val fxSuggestIngestionScheduler: FxSuggestIngestionScheduler by lazy { diff --git a/docs/adr/files/0009/option-c-app-services.diff b/docs/adr/files/0009/option-c-app-services.diff new file mode 100644 index 0000000000..eb4e38b464 --- /dev/null +++ b/docs/adr/files/0009/option-c-app-services.diff @@ -0,0 +1,339 @@ +diff --git a/components/suggest/android/build.gradle b/components/suggest/android/build.gradle +index 09056f3a91..70bc549387 100644 +--- a/components/suggest/android/build.gradle ++++ b/components/suggest/android/build.gradle +@@ -7,6 +7,8 @@ + + dependencies { + api project(":remotesettings") ++ ++ implementation libs.kotlinx.coroutines + } + + ext.configureUniFFIBindgen("suggest") +diff --git a/components/suggest/src/db.rs b/components/suggest/src/db.rs +index dcde6f7c85..fc1d22cf05 100644 +--- a/components/suggest/src/db.rs ++++ b/components/suggest/src/db.rs +@@ -113,18 +113,38 @@ + } + } + ++ pub fn begin_interrupt_scope(&self) -> Result { ++ Ok(self.interrupt_handle.begin_interrupt_scope()?) ++ } ++ + /// Accesses the Suggest database for reading. + pub fn read(&self, op: impl FnOnce(&SuggestDao) -> Result) -> Result { ++ self.read_with_scope(self.interrupt_handle.begin_interrupt_scope()?, op) ++ } ++ ++ /// Like [Self::read], but inputs a previously created `SqlInterruptScope` to use ++ pub fn read_with_scope( ++ &self, ++ scope: SqlInterruptScope, ++ op: impl FnOnce(&SuggestDao) -> Result, ++ ) -> Result { + let conn = self.conn.lock(); +- let scope = self.interrupt_handle.begin_interrupt_scope()?; + let dao = SuggestDao::new(&conn, &scope); + op(&dao) + } + + /// Accesses the Suggest database in a transaction for reading and writing. + pub fn write(&self, op: impl FnOnce(&mut SuggestDao) -> Result) -> Result { ++ self.write_with_scope(self.interrupt_handle.begin_interrupt_scope()?, op) ++ } ++ ++ /// Like [Self::write], but inputs a previously created `SqlInterruptScope` to use ++ pub fn write_with_scope( ++ &self, ++ scope: SqlInterruptScope, ++ op: impl FnOnce(&mut SuggestDao) -> Result, ++ ) -> Result { + let mut conn = self.conn.lock(); +- let scope = self.interrupt_handle.begin_interrupt_scope()?; + let tx = conn.transaction()?; + let mut dao = SuggestDao::new(&tx, &scope); + let result = op(&mut dao)?; +diff --git a/components/suggest/src/store.rs b/components/suggest/src/store.rs +index c61531908b..e83a228293 100644 +--- a/components/suggest/src/store.rs ++++ b/components/suggest/src/store.rs +@@ -10,9 +10,11 @@ + }; + + use error_support::{breadcrumb, handle_error}; ++use interrupt_support::SqlInterruptScope; + use once_cell::sync::OnceCell; + use parking_lot::Mutex; + use remote_settings::{self, RemoteSettingsConfig, RemoteSettingsServer}; ++use uniffi::{run_in_background, WorkerQueue}; + + use serde::de::DeserializeOwned; + +@@ -116,6 +118,30 @@ + inner: SuggestStoreInner::new(data_path, extensions_to_load, client), + })) + } ++ ++ #[handle_error(Error)] ++ pub fn build_async( ++ &self, ++ worker_queue: Arc, ++ ) -> SuggestApiResult> { ++ let inner = self.0.lock(); ++ let extensions_to_load = inner.extensions_to_load.clone(); ++ let data_path = inner ++ .data_path ++ .clone() ++ .ok_or_else(|| Error::SuggestStoreBuilder("data_path not specified".to_owned()))?; ++ ++ let client = RemoteSettingsClient::new( ++ inner.remote_settings_server.clone(), ++ inner.remote_settings_bucket_name.clone(), ++ None, ++ )?; ++ ++ Ok(Arc::new(SuggestStoreAsync { ++ worker_queue, ++ inner: SuggestStoreInner::new(data_path, extensions_to_load, client), ++ })) ++ } + } + + /// What should be interrupted when [SuggestStore::interrupt] is called? +@@ -192,7 +218,10 @@ + /// Queries the database for suggestions. + #[handle_error(Error)] + pub fn query(&self, query: SuggestionQuery) -> SuggestApiResult> { +- Ok(self.inner.query(query)?.suggestions) ++ Ok(self ++ .inner ++ .query(query, self.inner.begin_reader_scope()?)? ++ .suggestions) + } + + /// Queries the database for suggestions. +@@ -201,7 +230,7 @@ + &self, + query: SuggestionQuery, + ) -> SuggestApiResult { +- self.inner.query(query) ++ self.inner.query(query, self.inner.begin_reader_scope()?) + } + + /// Dismiss a suggestion +@@ -312,6 +341,159 @@ + } + } + ++/// Suggest store where all the methods are async ++/// ++/// This duplicates the functionality from [SuggestStore], but exposes it as async methods. ++/// The plan is to migrate consumers over to this interface, then remove the old one. ++#[derive(uniffi::Object)] ++pub struct SuggestStoreAsync { ++ worker_queue: Arc, ++ inner: SuggestStoreInner, ++} ++ ++impl SuggestStoreAsync { ++ async fn wrap_method_call(self: Arc, f: F) -> SuggestApiResult ++ where ++ F: FnOnce(&SuggestStoreInner) -> Result, ++ F: Send + Sync + 'static, ++ T: Send + Sync + 'static, ++ { ++ run_in_background(self.worker_queue.clone(), move || f(&self.inner)) ++ .await ++ .map_err(error_support::convert_log_report_error) ++ } ++} ++#[uniffi::export] ++impl SuggestStoreAsync { ++ pub async fn query( ++ self: Arc, ++ query: SuggestionQuery, ++ ) -> SuggestApiResult> { ++ // Create the interrupt scope before `wrap_method_call`, this way we'll interrupts that ++ // happen after the query was scheduled, but before it was executed. ++ let scope = self ++ .inner ++ .begin_reader_scope() ++ .map_err(error_support::convert_log_report_error)?; ++ self.wrap_method_call(move |inner| Ok(inner.query(query, scope)?.suggestions)) ++ .await ++ } ++ ++ /// Queries the database for suggestions. ++ pub async fn query_with_metrics( ++ self: Arc, ++ query: SuggestionQuery, ++ ) -> SuggestApiResult { ++ let scope = self ++ .inner ++ .begin_reader_scope() ++ .map_err(error_support::convert_log_report_error)?; ++ self.wrap_method_call(move |inner| inner.query(query, scope)) ++ .await ++ } ++ ++ /// Dismiss a suggestion ++ /// ++ /// Dismissed suggestions will not be returned again ++ /// ++ /// In the case of AMP suggestions this should be the raw URL. ++ pub async fn dismiss_suggestion( ++ self: Arc, ++ suggestion_url: String, ++ ) -> SuggestApiResult<()> { ++ self.wrap_method_call(move |inner| inner.dismiss_suggestion(suggestion_url)) ++ .await ++ } ++ ++ /// Clear dismissed suggestions ++ pub async fn clear_dismissed_suggestions(self: Arc) -> SuggestApiResult<()> { ++ self.wrap_method_call(|inner| inner.clear_dismissed_suggestions()) ++ .await ++ } ++ ++ /// Interrupts any ongoing queries. ++ /// ++ /// This should be called when the user types new input into the address ++ /// bar, to ensure that they see fresh suggestions as they type. This ++ /// method does not interrupt any ongoing ingests. ++ /// ++ /// Note: this method is not async, since the goal is to preempt currently running async ++ /// operations. ++ #[uniffi::method(default(kind = None))] ++ pub fn interrupt(self: Arc, kind: Option) { ++ self.inner.interrupt(kind) ++ } ++ ++ /// Ingests new suggestions from Remote Settings. ++ /// ++ /// Returns `true` if the ingest succeeded, `false` if it needs to be tried again ++ #[uniffi::method(default(constraints = None))] ++ pub async fn ingest(self: Arc, constraints: Option) -> bool { ++ let constraints = constraints.unwrap_or_default(); ++ self.wrap_method_call(move |inner| inner.ingest(constraints)) ++ .await ++ // Use `is_ok` to squash errors and return a boolean instead. ++ // Note: errors are logged by the call to `convert_log_report_error` ++ .is_ok() ++ } ++ ++ /// Removes all content from the database. ++ pub async fn clear(self: Arc) -> SuggestApiResult<()> { ++ self.wrap_method_call(move |inner| inner.clear()).await ++ } ++ ++ /// Returns global Suggest configuration data. ++ pub async fn fetch_global_config(self: Arc) -> SuggestApiResult { ++ self.wrap_method_call(move |inner| inner.fetch_global_config()) ++ .await ++ } ++ ++ /// Returns per-provider Suggest configuration data. ++ pub async fn fetch_provider_config( ++ self: Arc, ++ provider: SuggestionProvider, ++ ) -> SuggestApiResult> { ++ self.wrap_method_call(move |inner| inner.fetch_provider_config(provider)) ++ .await ++ } ++ ++ /// Fetches geonames stored in the database. A geoname represents a ++ /// geographic place. ++ /// ++ /// `query` is a string that will be matched directly against geoname names. ++ /// It is not a query string in the usual Suggest sense. `match_name_prefix` ++ /// determines whether prefix matching is performed on names excluding ++ /// abbreviations and airport codes. When `true`, names that start with ++ /// `query` will match. When false, names that equal `query` will match. ++ /// ++ /// `geoname_type` restricts returned geonames to a [`GeonameType`]. ++ /// ++ /// `filter` restricts returned geonames to certain cities or regions. ++ /// Cities can be restricted to regions by including the regions in ++ /// `filter`, and regions can be restricted to those containing certain ++ /// cities by including the cities in `filter`. This is especially useful ++ /// since city and region names are not unique. `filter` is disjunctive: If ++ /// any item in `filter` matches a geoname, the geoname will be filtered in. ++ /// ++ /// The query can match a single geoname in more than one way. For example, ++ /// it can match both a full name and an abbreviation. The returned vec of ++ /// [`GeonameMatch`] values will include all matches for a geoname, one ++ /// match per `match_type` per geoname. In other words, a matched geoname ++ /// can map to more than one `GeonameMatch`. ++ pub async fn fetch_geonames( ++ self: Arc, ++ query: String, ++ match_name_prefix: bool, ++ geoname_type: Option, ++ filter: Option>, ++ ) -> SuggestApiResult> { ++ self.wrap_method_call(move |inner| { ++ inner.fetch_geonames(&query, match_name_prefix, geoname_type, filter) ++ }) ++ .await ++ } ++} ++ + /// Constraints limit which suggestions to ingest from Remote Settings. + #[derive(Clone, Default, Debug, uniffi::Record)] + pub struct SuggestIngestionConstraints { +@@ -399,15 +581,27 @@ + .get_or_try_init(|| SuggestStoreDbs::open(&self.data_path, &self.extensions_to_load)) + } + +- fn query(&self, query: SuggestionQuery) -> Result { ++ fn begin_reader_scope(&self) -> Result { ++ self.dbs()?.reader.begin_interrupt_scope() ++ } ++ ++ fn query( ++ &self, ++ query: SuggestionQuery, ++ scope: SqlInterruptScope, ++ ) -> Result { ++ // Before anything, check if the scope was interrupted. This handles the case where query ++ // scheduled by SuggestStoreAsync was interrupted. ++ scope.err_if_interrupted()?; + let mut metrics = SuggestQueryMetrics::default(); + let mut suggestions = vec![]; + + let unique_providers = query.providers.iter().collect::>(); + let reader = &self.dbs()?.reader; + for provider in unique_providers { ++ let scope = scope.clone(); + let new_suggestions = metrics.measure_query(provider.to_string(), || { +- reader.read(|dao| match provider { ++ reader.read_with_scope(scope, |dao| match provider { + SuggestionProvider::Amp => { + dao.fetch_amp_suggestions(&query, AmpSuggestionType::Desktop) + } +@@ -950,7 +1144,8 @@ + } + + pub fn fetch_suggestions(&self, query: SuggestionQuery) -> Vec { +- self.inner.query(query).unwrap().suggestions ++ let scope = self.inner.begin_reader_scope().unwrap(); ++ self.inner.query(query, scope).unwrap().suggestions + } + + pub fn fetch_global_config(&self) -> SuggestGlobalConfig { +diff --git a/components/support/interrupt/src/sql.rs b/components/support/interrupt/src/sql.rs +index af97a6e1c6..5e8e1821e1 100644 +--- a/components/support/interrupt/src/sql.rs ++++ b/components/support/interrupt/src/sql.rs +@@ -75,7 +75,7 @@ + /// + /// This is used by the rust code to check if an operation should fail because it was interrupted. + /// It handles the case where we get interrupted outside of an SQL query. +-#[derive(Debug)] ++#[derive(Clone, Debug)] + pub struct SqlInterruptScope { + start_value: usize, + interrupt_counter: Arc, diff --git a/docs/adr/files/0009/option-c-firefox-android.diff b/docs/adr/files/0009/option-c-firefox-android.diff new file mode 100644 index 0000000000..4d726b2511 --- /dev/null +++ b/docs/adr/files/0009/option-c-firefox-android.diff @@ -0,0 +1,92 @@ +diff --git a/firefox-ios/Storage/Rust/RustFirefoxSuggest.swift b/firefox-ios/Storage/Rust/RustFirefoxSuggest.swift +index e12d428192..607e2aca20 100644 +--- a/firefox-ios/Storage/Rust/RustFirefoxSuggest.swift ++++ b/firefox-ios/Storage/Rust/RustFirefoxSuggest.swift +@@ -4,7 +4,7 @@ + + import Foundation + +-import class MozillaAppServices.SuggestStore ++import class MozillaAppServices.SuggestStoreAsync + import class MozillaAppServices.SuggestStoreBuilder + import class MozillaAppServices.Viaduct + import enum MozillaAppServices.SuggestionProvider +@@ -33,12 +33,7 @@ + /// Wraps the synchronous Rust `SuggestStore` binding to execute + /// blocking operations on a dispatch queue. + public class RustFirefoxSuggest: RustFirefoxSuggestProtocol { +- private let store: SuggestStore +- +- // Using a pair of serial queues lets read and write operations run +- // without blocking one another. +- private let writerQueue = DispatchQueue(label: "RustFirefoxSuggest.writer") +- private let readerQueue = DispatchQueue(label: "RustFirefoxSuggest.reader") ++ private let store: SuggestStoreAsync + + public init(dataPath: String, cachePath: String, remoteSettingsConfig: RemoteSettingsConfig? = nil) throws { + var builder = SuggestStoreBuilder() +@@ -49,24 +44,21 @@ + builder = builder.remoteSettingsConfig(config: remoteSettingsConfig) + } + +- store = try builder.build() ++ store = try builder.buildAsync( ++ // Use .userInitiated QOS for most operations. Methods like `query` should be executed ++ // ASAP. ++ workerQueue: DispatchQueue(label: "RustFirefoxSuggest.general", qos: .userInitiated), ++ // Use .utility QOS for ingest since latency is not important here. ++ // Also, using a separate queue allows `ingest()` and `query()` to run side-by-side. ++ ingestWorkerQueue: DispatchQueue(label: "RustFirefoxSuggest.ingest", qos: .utility) ++ ) + } + + public func ingest() async throws { + // Ensure that the Rust networking stack has been initialized before + // downloading new suggestions. This is safe to call multiple times. + Viaduct.shared.useReqwestBackend() +- +- try await withCheckedThrowingContinuation { continuation in +- writerQueue.async(qos: .utility) { +- do { +- try self.store.ingest(constraints: SuggestIngestionConstraints()) +- continuation.resume() +- } catch { +- continuation.resume(throwing: error) +- } +- } +- } ++ return try await self.store.ingest(constraints: SuggestIngestionConstraints()) + } + + public func query( +@@ -74,24 +66,15 @@ + providers: [SuggestionProvider], + limit: Int32 + ) async throws -> [RustFirefoxSuggestion] { +- return try await withCheckedThrowingContinuation { continuation in +- readerQueue.async(qos: .userInitiated) { +- do { +- let suggestions = try self.store.query(query: SuggestionQuery( +- keyword: keyword, +- providers: providers, +- limit: limit +- )).compactMap(RustFirefoxSuggestion.init) +- continuation.resume(returning: suggestions) +- } catch { +- continuation.resume(throwing: error) +- } +- } +- } ++ return try await self.store.query(query: SuggestionQuery( ++ keyword: keyword, ++ providers: providers, ++ limit: limit ++ )).compactMap(RustFirefoxSuggestion.init) + } + + public func interruptReader() { +- store.interrupt() ++ store.interrupt(kind: .read) + } + + public func interruptEverything() {