Skip to content

Commit ae48436

Browse files
committed
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.
1 parent 8aa85ff commit ae48436

2 files changed

Lines changed: 164 additions & 0 deletions

File tree

docs/adr/0008-wrapper-code.md

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
# Wrapper code
2+
3+
* Status: draft
4+
* Deciders:
5+
* Date: ???
6+
7+
## Context and Problem Statement
8+
9+
Application-services components currently consist of a Rust core that gets wrapped by Swift and Kotlin wrappers.
10+
In the past, these wrappers were strictly necessary because of deficiencies in our FFI strategy.
11+
However, UniFFI is reaching the point where it can support all our requirements and it's possible to remove the wrapper code.
12+
13+
So... should we?
14+
15+
### Scope
16+
17+
This ADR discusses what our general policy on wrapper code should be.
18+
It does not cover how we should plan our work.
19+
If we decide to reject wrapper code, we do not need to commit to any particular timeline for actually removing it.
20+
21+
## Decision Drivers
22+
23+
## Considered Options
24+
25+
* **(A) Embrace wrapper code and continue to use it**
26+
* **(B) Reject wrapper code and remove it**
27+
* **(C) Use wrapper code for async wrapping only**
28+
29+
## Decision Outcome
30+
31+
## Pros and Cons of the Options
32+
33+
### (A) Embrace wrapper code and continue to use it
34+
35+
* Good, because wrapper code allows us to wrap sync Rust code in async wrapper functions and this is a low-risk approach to async.
36+
* Good, because wrapper code allows us to mitigate breaking changes in the Rust code.
37+
* Bad, because there are better ways to handle breaking changes, like Rust feature flags.
38+
We could introduce breaking changes, and large changes in general, behind a feature flag.
39+
We would wait to enable the feature flag on the megazord until consumer application was ready.
40+
* Good, because it allows us to present APIs using idiomatic, native, types.
41+
For example, UniFFI callback interfaces may meet our requirements, but the Swift `NotificationCenter` may provide a more ergonomic API.
42+
43+
### (B) Reject wrapper code and remove it
44+
45+
* Good, because it simplifies our documentation strategy.
46+
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).
47+
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.
48+
If there are wrappers, then this probably isn't going to work.
49+
In general, wrappers mean we are have multiple public APIs which makes documentation harder.
50+
* Good, because a "vanilla" API may be easier to integrate with multiple consumer apps.
51+
`NotificationCenter` might be the preferred choice for firefox-ios, but another Swift app may want to use a different system.
52+
By only using UniFFI-supported types we can be fairly sure that our code will work with any system.
53+
54+
### (C) Use wrapper code for async wrapping only
55+
56+
* We must chose this one over (B) if we decide to avoid async Rust in `ADR-0009`
57+
* Good, because a "vanilla" API may be easier to integrate with multiple consumer apps.
58+
* Good, because it somewhat simplifies our documentation strategy, although not as much as (B).

docs/adr/0009-async-rust.md

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
# Using Async Rust
2+
3+
* Status: draft
4+
* Deciders:
5+
* Date: ???
6+
7+
## Context and Problem Statement
8+
9+
Our Rust components are currently written as using synchronous Rust.
10+
The components are then wrapped in Kotlin to present an async interface.
11+
Swift also wraps them to present an async-style interface, although it currently uses `DispatchQueue` and completion handlers rather than `async` functions.
12+
13+
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.
14+
15+
So... should we?
16+
17+
### Scope
18+
19+
This ADR discusses what our general policy on wrapper code should be.
20+
It does not cover how we should plan our work.
21+
If we decide to embrace async Rust, we do not need to commit to any particular timeline for actually switching to it.
22+
23+
### Desktop and gecko-js
24+
25+
On desktop, we can't write async wrappers because it's not possible in Javascript.
26+
Instead we use a strategy where every function is automatically wrapped as async in the C++ layer.
27+
Using a config file, it's possible to opt-out of this strategy for particular functions/methods.
28+
29+
This seems to be working okay although it feels slightly weird.
30+
It's not completely clear that it would scale with more complex components.
31+
32+
### Android-components
33+
34+
In Kotlin, the async wrapper layer currently lives in `android-components`.
35+
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`.
36+
37+
## How it would work
38+
39+
### SQLite queries
40+
41+
One of the reasons our code currently blocks is to run SQLite queries.
42+
https://github.com/mozilla/uniffi-rs/pull/1837 has a system to run blocking code inside an async function.
43+
It would basically mean replacing code like this:
44+
45+
```kotlin
46+
override suspend fun wipeLocal() = withContext(coroutineContext) {
47+
conn.getStorage().wipeLocal()
48+
}
49+
```
50+
51+
with code like this:
52+
```rust
53+
54+
async fn wipe_local() {
55+
self.queue.run_blocking(|| self.db.wipe_local()).await
56+
}
57+
```
58+
59+
We would need to merge this code, which is currently planed for the end of 2023.
60+
61+
### Locks
62+
63+
Another reason our code blocks is to wait on a `Mutex` or `RWLock`.
64+
To handle this, we could switch from using `parking_lot` to `async_lock`.
65+
This could be achieved with the current UniFFI, no additional changes are needed.
66+
67+
### Network requests
68+
69+
The last reason we block is for network requests.
70+
To support that we would probably need some sort of "async viaduct" that would allow consumer applications to choose either:
71+
- Use async functions from the `reqwest` library.
72+
This matches what we currently do for `firefox-ios`.
73+
- Use the foreign language's network stack via an async callback interface.
74+
This matches what we currently do for `firefox-android`.
75+
This would require implemnenting https://github.com/mozilla/uniffi-rs/issues/1729, which is currently planed for the end of 2023.
76+
77+
## Decision Drivers
78+
79+
## Considered Options
80+
81+
* **(A) Embrace Async Rust**
82+
* **(B) Avoid Async Rust**
83+
84+
## Decision Outcome
85+
86+
## Pros and Cons of the Options
87+
88+
### (A) Embrace Async Rust
89+
90+
* Good, if we decide to avoid wrappers in `ADR-0008` because it allows us to remove the async wrappers.
91+
* Bad, because there's a risk that the UniFFI async code will cause issues and our current async strategy is working okay.
92+
* Good because it allows us to be more effecient with our thread usage.
93+
When an async task is waiting on a lock or network request, it can suspend itself and release the thread for other async work.
94+
Currently, we need to block a thread while we are waiting for this.
95+
However, it's not clear this would meaningfully affect our consumers since we don't run that many blocking operations.
96+
We would be saving maybe 1-2 threads at certain points.
97+
* Good, because it makes it easier to integrate with new languages that expect async.
98+
For example, WASM integration libraries usually returns `Future` objects to Rust which can only be evaluated in an async context.
99+
Note: this is a separate issue from UniFFI adding WASM support.
100+
If we switched our component code to using async Rust, it's possible that we could use `wasm-bindgen` instead.
101+
* Bad, because it makes it harder to provide bindings on new languages that don't support async, like C and C++.
102+
Maybe we could bridge the gap with some sort of callback-based async system, but it's not clear how that would work.
103+
104+
### (B) Avoid Async Rust
105+
106+
This is basically just the inverse of the last section.

0 commit comments

Comments
 (0)