From 1941c10f45dc15dea599809b1d2f0674531f476f Mon Sep 17 00:00:00 2001 From: Ben Dean-Kawamura Date: Thu, 9 Nov 2023 10:31:29 -0500 Subject: [PATCH] Adding twin ADRS on wrapper code and async Rust These are independant but related decisions, so two ADRs in one pull request is a good way to handle them. --- docs/adr/0008-wrapper-code.md | 90 +++++++++++++++++++++ docs/adr/0009-async-rust.md | 148 ++++++++++++++++++++++++++++++++++ 2 files changed, 238 insertions(+) create mode 100644 docs/adr/0008-wrapper-code.md create mode 100644 docs/adr/0009-async-rust.md diff --git a/docs/adr/0008-wrapper-code.md b/docs/adr/0008-wrapper-code.md new file mode 100644 index 0000000000..a6a1e6664f --- /dev/null +++ b/docs/adr/0008-wrapper-code.md @@ -0,0 +1,90 @@ +# Wrapper code + +* Status: draft +* Deciders: +* Date: ??? + +## Context and Problem Statement + +Application-services components currently consist of a Rust core that gets wrapped by Swift and Kotlin wrappers. +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. diff --git a/docs/adr/0009-async-rust.md b/docs/adr/0009-async-rust.md new file mode 100644 index 0000000000..14cb85445b --- /dev/null +++ b/docs/adr/0009-async-rust.md @@ -0,0 +1,148 @@ +# Using Async Rust + +* Status: draft +* Deciders: +* Date: ??? + +## Context and Problem Statement + +Our Rust components are currently written as 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 an 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.