Skip to content

Commit

Permalink
Adding twin ADRS on wrapper code and async Rust
Browse files Browse the repository at this point in the history
These are independant but related decisions, so two ADRs in one pull
request is a good way to handle them.
  • Loading branch information
bendk committed Nov 13, 2023
1 parent 64033af commit d221ffd
Show file tree
Hide file tree
Showing 2 changed files with 241 additions and 0 deletions.
90 changes: 90 additions & 0 deletions docs/adr/0008-wrapper-code.md
Original file line number Diff line number Diff line change
@@ -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.
151 changes: 151 additions & 0 deletions docs/adr/0009-async-rust.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
# 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.
* 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.

0 comments on commit d221ffd

Please sign in to comment.