Adding twin ADRS on wrapper code and async Rust

docs/adr/0008-wrapper-code.md

@@ -0,0 +1,91 @@
+# Wrapper code
+
+* Status: draft
+* Deciders: 
+* Date: ???
+
+## Context and Problem Statement
+
+Application-services components currently consist of a Rust core, and UniFFI-generated bindings for Swift and Kotlin.
+Additionally, some of our components have hand-written Swift and Kotlin code, containing wrappers and extensions for the UniFFI bindings.
+In the past, these wrappers were strictly necessary because of deficiencies in our FFI strategy.
+However, UniFFI is reaching the point where it can support all our requirements and it's possible to remove the wrapper code.
+
+We should decide how much effort, if any, we want to invest into getting rid of the wrapper code now that it's technically possible.
+We should also decide if there are places where wrapper code makes sense and we don't want to replace it, even ignoring the time investment.
+
+### Possible changes
+
+This section explores some possibilities for removing wrapper code.
+Deciding on any particular possibility listed here is out of scope for the ADR, the decision is if we should invest our time investigating some of these.
+
+#### Async
+
+One of the main reasons for our current wrapper code is to wrap our sync API and present an async interface.
+ADR-0009 lays out a couple of alternatives to this.
+
+#### Feature flags for breaking changes
+
+Another main reason for our current wrapper code is to mitigate breaking API changes.
+The wrapper layer allows us to make breaking changes at the Rust level, but keep the same API at the wrapper layer.
+
+An alternative to this would be to use Rust feature flags to manage breaking changes.
+Any breaking change, or large change in general, would be behind a feature flag.
+We would wait to enable the feature flag on the megazord until consumer application was ready to take the change.
+Maybe we could have a transition period where we built two megazords, for example `megazord` and `megazord-with-logins-local-encrytion` and the consumer app could pick between the two.
+This would simplify the consumer PRs since they could run CI normally.
+
+Some potential uses of of features flags would be:
+
+- **Cosmetic changes**. If we want to rename a field/function name, we could put that rename behind a feature flag.
+- **Architectural changes**. For bigger changes, like the logins local encryption rework, we could maintain two pieces of code at once. For example, copy db.rs to db/old.rs and db/new.rs. Create db/mod.rs which runs `use old::*` or `use new::*` depending on the feature flag. Then we do our work in db/new.rs.
+
+Maintaining many feature flags at once would require significant effort, so we should aim to get our consumers to use the new feature flag soon after our work is complete.
+
+#### UniFFI-supported interfaces
+
+One last reason for wrapper code is to present idiomatic interfaces to our Rust code -- especially for callback interfaces.
+For example, it's possible to define a UniFFI callback interface for notifications, but the FxA wrapper code uses the `NotificationCenter` on Swift which is not supported by UniFFI.
+If we wanted to remove all wrapper code we would need to commit to only using interfaces that UniFFI could support.
+
+## Decision Drivers
+
+## Considered Options
+
+* **(A) Keep using our current strategy**
+Don't invest time into removing wrapper code.
+
+* **(B) Remove all wrapper code**
+Invest time into removing wrapper code with the intention of removing all of it.
+
+* **(C) Remove most wrapper code, keep an additive wrapper layer**
+Invest time into removing wrapper code, but keep some wrapper code.
+In particular, keep a wrapper layer that adds new API surfaces rather than replaces existing ones.
+For example, we could define an `FxaEventListener` interface with UniFFI, then add a `IosEventListener` class that implemented that interface by forwarding the messages to the `NotificationCenter`.
+
+## Decision Outcome
+
+## Pros and Cons of the Options
+
+### (A) Keep using our current strategy
+
+* Good, because we can spend our time on other improvements
+* Good, because there's no chance of wasting time on implementing solutions that may not work out in practice.
+
+### (B) Remove all wrapper code
+* Good, because it simplifies our documentation strategy.
+ There is active work in UniFFI to auto-generate the bindings documentation from the Rust docstrings (https://github.com/mozilla/uniffi-rs/pull/1498, https://github.com/mozilla/uniffi-rs/pull/1493).
+ If there are no wrappers, then we could potentially use this to auto-generate a high-level documentation site and/or docstrings in the generated bindings code.
+ If there are wrappers, then this probably isn't going to work.
+ In general, wrappers mean we are have multiple public APIs which makes documentation harder.
+* Bad, because it may lead to worse documentation.
+ Hand-written documents can be better than auto-generated ones and especially since they can specifically target javadoc and swiftdoc.
+* Good, because a "vanilla" API may be easier to integrate with multiple consumer apps.
+ `NotificationCenter` might be the preferred choice for firefox-ios, but another Swift app may want to use a different system.
+ By only using UniFFI-supported types we can be fairly sure that our code will work with any system.
+
+### (C) Remove most wrapper code, keep an additive wrapper layer
+* Good, because most documentation can be auto-generated and some can be hand-written.
+ The hand-written documentation would be the language-specific parts, which probably need to be written by hand.
+* Bad because it may lead to worse documentation, for the same reasons an (B).
+* Good, because consumer apps can choose to use the wrapper interfaces or not.

docs/adr/0009-async-rust.md

@@ -0,0 +1,151 @@
+# Using Async Rust
+
+* Status: draft
+* Deciders: 
+* Date: ???
+
+## Context and Problem Statement
+
+Our Rust components are currently written using synchronous Rust.
+The components are then wrapped in Kotlin to present an async interface.
+Swift also wraps them to present an async-style interface, although it currently uses `DispatchQueue` and completion handlers rather than `async` functions.
+
+UniFFI has been adding async capabilities in the last year and it seems possible to switch to using async Rust and not having a hand-written async wrapper.
+It also seems possible to auto-generate the async wrapper with UniFFI.
+
+What should our async strategy be?
+
+### Scope
+
+This ADR discusses what our general policy on wrapper code should be.
+It does not cover how we should plan our work.
+If we decide to embrace async Rust, we do not need to commit to any particular timeline for actually switching to it.
+
+### Desktop and gecko-js
+
+On desktop, we can't write async wrappers because it's not possible in Javascript.
+Instead we use a strategy where every function is automatically wrapped as async in the C++ layer.
+Using a config file, it's possible to opt-out of this strategy for particular functions/methods.
+
+### Android-components
+
+In Kotlin, the async wrapper layer currently lives in `android-components`.
+For the purposes of this ADR, it doesn't really matter, and this ADR will not make a distinction between wrapper code in our repo and `android-components`.
+
+## How it would work
+
+### SQLite queries
+
+One of the reasons our code currently blocks is to run SQLite queries.
+https://github.com/mozilla/uniffi-rs/pull/1837 has a system to run blocking code inside an async function.
+It would basically mean replacing code like this:
+
+```kotlin
+ override suspend fun wipeLocal() = withContext(coroutineContext) {
+ conn.getStorage().wipeLocal()
+ }
+```
+
+with code like this:
+```rust
+
+ async fn wipe_local() {
+ self.queue.execute(|| self.db.wipe_local()).await
+ }
+```
+
+We would need to merge #1837, which is currently planned for the end of 2023.
+
+### Locks
+
+Another reason our code blocks is to wait on a `Mutex` or `RWLock`.
+There are a few ways we could handle this:
+
+* The simplest is to continue using regular locks, inside a `execute()` call, which would be very similar to our current system.
+* We could also consider switching to `async_lock` and reversing the order: lock first, then make a `execute()` call.
+ This may be more efficient since the async task would suspend while waiting for the lock rather than blocking a thread
+* We could also ditch locks and use [actors and channels](https://ryhl.io/blog/actors-with-tokio/) to protect our resources.
+ It's probably not worth rewriting our current components to do this, but this approach might be useful for new components.
+
+### Network requests
+
+The last reason we block is for network requests.
+To support that we would probably need some sort of "async viaduct" that would allow consumer applications to choose either:
+- Use async functions from the `reqwest` library.
+ This matches what we currently do for `firefox-ios`.
+- Use the foreign language's network stack via an async callback interface.
+ This matches what we currently do for `firefox-android`.
+ This would require implemnenting https://github.com/mozilla/uniffi-rs/issues/1729, which is currently planed for the end of 2023.
+
+## Decision Drivers
+
+## Considered Options
+
+* **(A) Experiment with async Rust**
+
+* Pick a small component like `tabs` or `push` and use it to test our async Rust.
+* Use async Rust for new components.
+* Consider slowly switching existing components to use async Rust.
+
+* **(B) Keep hand-written Async wrappers**
+
+Don't change the status quo.
+
+* **(C) Auto-generate Async wrappers**
+
+We could also make the `gecko-js` model the official model and switch other languages to use it as well.
+For example, we could support something like this in `uniffi.toml`:
+
+```toml
+[[bindings.async_wrappers]]
+# Class to wrap, methods wrapped with an async version
+wrapped = "LoginStore"
+# Name of the wrapper class.
+# UniFFI would generate async wrapper methods that worked exactly like the current hand-written code.
+# For most languages, the wrapper class constructors would input an extra parameter to handle the async wrapping (for example `CoroutineContext` or `DispatchQueue`).
+wrapper = "LoginStoreAsync"
+# methods to skip wrapping and keep sync (optional)
+sync_methods = [...]
+```
+
+We could also support async wrappers for callback interfaces.
+These would allow the foreign code to implement an sync callback interface using async code
+The Rust code would block while waiting for the result.
+
+## Decision Outcome
+
+## Pros and Cons of the Options
+
+### (A) Experiment with async Rust
+
+* Good, if we decide to avoid wrappers in `ADR-0008` because it allows us to remove the async wrappers.
+* Bad, because there's a risk that the UniFFI async code will cause issues and our current async strategy is working okay.
+ Even if we pick a small component to experiment with, it would be bad if that component crashes or stops responding because of async issues.
+* Good because it allows us to be more efficient with our thread usage.
+ When an async task is waiting on a lock or network request, it can suspend itself and release the thread for other async work.
+ Currently, we need to block a thread while we are waiting for this.
+ However, it's not clear this would meaningfully affect our consumers since we don't run that many blocking operations.
+ We would be saving maybe 1-2 threads at certain points.
+* Good, because it makes it easier to integrate with new languages that expect async.
+ For example, WASM integration libraries usually returns `Future` objects to Rust which can only be evaluated in an async context.
+ Note: this is a separate issue from UniFFI adding WASM support.
+ If we switched our component code to using async Rust, it's possible that we could use `wasm-bindgen` instead.
+* 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.
+
+### (B) Keep hand-written Async wrappers
+
+* Good, this is the status quo, and doesn't require any work
+
+### (C) Auto-generate Async wrappers
+
+* Good, if we decide to avoid wrappers in `ADR-0008` because it allows us to remove the hand-written async wrappers.
+* Good, because we could copy over auto-generated documentation and simply add `async` or `suspend` to the function signature.
+* Good, because it's less risky than (A)
+* Bad, because we would continue to have inefficiencies in our threading strategy.
+* Good, because this is a more flexible async strategy.
+ We could use async wrappers to integrate with languages like WASM and not use them to integrate with languages like C and C++.
+ However, it's not clear at all how this would work in practice.
+* Bad, because it's less flexible than (A).
+ For example, with (A) it would be for the main API interface to return another interface with async methods from Rust code.
+ That wouldn't be possible with this system, although it's not clear how big of an issue that would be.