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 10, 2023
1 parent 8aa85ff commit 7017508
Show file tree
Hide file tree
Showing 2 changed files with 169 additions and 0 deletions.
58 changes: 58 additions & 0 deletions docs/adr/0008-wrapper-code.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
# 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.

So... should we?

### 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 reject wrapper code, we do not need to commit to any particular timeline for actually removing it.

## Decision Drivers

## Considered Options

* **(A) Embrace wrapper code and continue to use it**
* **(B) Reject wrapper code and remove it**
* **(C) Use wrapper code for async wrapping only**

## Decision Outcome

## Pros and Cons of the Options

### (A) Embrace wrapper code and continue to use it

* Good, because wrapper code allows us to wrap sync Rust code in async wrapper functions and this is a low-risk approach to async.
* Good, because wrapper code allows us to mitigate breaking changes in the Rust code.
* Bad, because there are better ways to handle breaking changes, like Rust feature flags.
We could introduce breaking changes, and large changes in general, behind a feature flag.
We would wait to enable the feature flag on the megazord until consumer application was ready.
* Good, because it allows us to present APIs using idiomatic, native, types.
For example, UniFFI callback interfaces may meet our requirements, but the Swift `NotificationCenter` may provide a more ergonomic API.

### (B) Reject wrapper code and remove it

* Good, because it simplifies our documentation strategy.
There is active work in UniFFI to autogenerate 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.
* 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) Use wrapper code for async wrapping only

* We must chose this one over (B) if we decide to avoid async Rust in `ADR-0009`
* Good, because a "vanilla" API may be easier to integrate with multiple consumer apps.
* Good, because it somewhat simplifies our documentation strategy, although not as much as (B).
111 changes: 111 additions & 0 deletions docs/adr/0009-async-rust.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
# 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.

So... should we?

### 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.

This seems to be working okay although it feels slightly weird.
It's not completely clear that it would scale with more complex components.

### 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.run_blocking(|| self.db.wipe_local()).await
}
```

We would need to merge this code, which is currently planed 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 `run_blocking()` 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 `run_blocking()` 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) Embrace Async Rust**
* **(B) Avoid Async Rust**

## Decision Outcome

## Pros and Cons of the Options

### (A) Embrace 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.
* Good because it allows us to be more effecient 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) Avoid Async Rust

This is basically just the inverse of the last section.

0 comments on commit 7017508

Please sign in to comment.