Skip to content

Commit d221ffd

Browse files
bendkbendk
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 64033af commit d221ffd

2 files changed

Lines changed: 241 additions & 0 deletions

File tree

‎docs/adr/0008-wrapper-code.md‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
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+
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.
14+
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.
15+
16+
### Possible changes
17+
18+
This section explores some possibilities for removing wrapper code.
19+
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.
20+
21+
#### Async
22+
23+
One of the main reasons for our current wrapper code is to wrap our sync API and present an async interface.
24+
ADR-0009 lays out a couple of alternatives to this.
25+
26+
#### Feature flags for breaking changes
27+
28+
Another main reason for our current wrapper code is to mitigate breaking API changes.
29+
The wrapper layer allows us to make breaking changes at the Rust level, but keep the same API at the wrapper layer.
30+
31+
An alternative to this would be to use Rust feature flags to manage breaking changes.
32+
Any breaking change, or large change in general, would be behind a feature flag.
33+
We would wait to enable the feature flag on the megazord until consumer application was ready to take the change.
34+
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.
35+
This would simplify the consumer PRs since they could run CI normally.
36+
37+
Some potential uses of of features flags would be:
38+
39+
- **Cosmetic changes**. If we want to rename a field/function name, we could put that rename behind a feature flag.
40+
- **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.
41+
42+
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.
43+
44+
#### UniFFI-supported interfaces
45+
46+
One last reason for wrapper code is to present idiomatic interfaces to our Rust code -- especially for callback interfaces.
47+
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.
48+
If we wanted to remove all wrapper code we would need to commit to only using interfaces that UniFFI could support.
49+
50+
## Decision Drivers
51+
52+
## Considered Options
53+
54+
* **(A) Keep using our current strategy**
55+
Don't invest time into removing wrapper code.
56+
57+
* **(B) Remove all wrapper code**
58+
Invest time into removing wrapper code with the intention of removing all of it.
59+
60+
* **(C) Remove most wrapper code, keep an additive wrapper layer**
61+
Invest time into removing wrapper code, but keep some wrapper code.
62+
In particular, keep a wrapper layer that adds new API surfaces rather than replaces existing ones.
63+
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`.
64+
65+
## Decision Outcome
66+
67+
## Pros and Cons of the Options
68+
69+
### (A) Keep using our current strategy
70+
71+
* Good, because we can spend our time on other improvements
72+
* Good, because there's no chance of wasting time on implementing solutions that may not work out in practice.
73+
74+
### (B) Remove all wrapper code
75+
* Good, because it simplifies our documentation strategy.
76+
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).
77+
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.
78+
If there are wrappers, then this probably isn't going to work.
79+
In general, wrappers mean we are have multiple public APIs which makes documentation harder.
80+
* Bad, because it may lead to worse documentation.
81+
Hand-written documents can be better than auto-generated ones and especially since they can specifically target javadoc and swiftdoc.
82+
* Good, because a "vanilla" API may be easier to integrate with multiple consumer apps.
83+
`NotificationCenter` might be the preferred choice for firefox-ios, but another Swift app may want to use a different system.
84+
By only using UniFFI-supported types we can be fairly sure that our code will work with any system.
85+
86+
### (C) Remove most wrapper code, keep an additive wrapper layer
87+
* Good, because most documentation can be auto-generated and some can be hand-written.
88+
The hand-written documentation would be the language-specific parts, which probably need to be written by hand.
89+
* Bad because it may lead to worse documentation, for the same reasons an (B).
90+
* Good, because consumer apps can choose to use the wrapper interfaces or not.

‎docs/adr/0009-async-rust.md‎

Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,151 @@
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+
It also seems possible to auto-generate the async wrapper with UniFFI.
15+
16+
What should our async strategy be?
17+
18+
### Scope
19+
20+
This ADR discusses what our general policy on wrapper code should be.
21+
It does not cover how we should plan our work.
22+
If we decide to embrace async Rust, we do not need to commit to any particular timeline for actually switching to it.
23+
24+
### Desktop and gecko-js
25+
26+
On desktop, we can't write async wrappers because it's not possible in Javascript.
27+
Instead we use a strategy where every function is automatically wrapped as async in the C++ layer.
28+
Using a config file, it's possible to opt-out of this strategy for particular functions/methods.
29+
30+
### Android-components
31+
32+
In Kotlin, the async wrapper layer currently lives in `android-components`.
33+
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`.
34+
35+
## How it would work
36+
37+
### SQLite queries
38+
39+
One of the reasons our code currently blocks is to run SQLite queries.
40+
https://github.com/mozilla/uniffi-rs/pull/1837 has a system to run blocking code inside an async function.
41+
It would basically mean replacing code like this:
42+
43+
```kotlin
44+
override suspend fun wipeLocal() = withContext(coroutineContext) {
45+
conn.getStorage().wipeLocal()
46+
}
47+
```
48+
49+
with code like this:
50+
```rust
51+
52+
async fn wipe_local() {
53+
self.queue.execute(|| self.db.wipe_local()).await
54+
}
55+
```
56+
57+
We would need to merge #1837, which is currently planned for the end of 2023.
58+
59+
### Locks
60+
61+
Another reason our code blocks is to wait on a `Mutex` or `RWLock`.
62+
There are a few ways we could handle this:
63+
64+
* The simplest is to continue using regular locks, inside a `execute()` call, which would be very similar to our current system.
65+
* We could also consider switching to `async_lock` and reversing the order: lock first, then make a `execute()` call.
66+
This may be more efficient since the async task would suspend while waiting for the lock rather than blocking a thread
67+
* We could also ditch locks and use [actors and channels](https://ryhl.io/blog/actors-with-tokio/) to protect our resources.
68+
It's probably not worth rewriting our current components to do this, but this approach might be useful for new components.
69+
70+
### Network requests
71+
72+
The last reason we block is for network requests.
73+
To support that we would probably need some sort of "async viaduct" that would allow consumer applications to choose either:
74+
- Use async functions from the `reqwest` library.
75+
This matches what we currently do for `firefox-ios`.
76+
- Use the foreign language's network stack via an async callback interface.
77+
This matches what we currently do for `firefox-android`.
78+
This would require implemnenting https://github.com/mozilla/uniffi-rs/issues/1729, which is currently planed for the end of 2023.
79+
80+
## Decision Drivers
81+
82+
## Considered Options
83+
84+
* **(A) Experiment with async Rust**
85+
86+
* Pick a small component like `tabs` or `push` and use it to test our async Rust.
87+
* Use async Rust for new components.
88+
* Consider slowly switching existing components to use async Rust.
89+
90+
* **(B) Keep hand-written Async wrappers**
91+
92+
Don't change the status quo.
93+
94+
* **(C) Auto-generate Async wrappers**
95+
96+
We could also make the `gecko-js` model the official model and switch other languages to use it as well.
97+
For example, we could support something like this in `uniffi.toml`:
98+
99+
```toml
100+
[[bindings.async_wrappers]]
101+
# Class to wrap, methods wrapped with an async version
102+
wrapped = "LoginStore"
103+
# Name of the wrapper class.
104+
# UniFFI would generate async wrapper methods that worked exactly like the current hand-written code.
105+
# For most languages, the wrapper class constructors would input an extra parameter to handle the async wrapping (for example `CoroutineContext` or `DispatchQueue`).
106+
wrapper = "LoginStoreAsync"
107+
# methods to skip wrapping and keep sync (optional)
108+
sync_methods = [...]
109+
```
110+
111+
We could also support async wrappers for callback interfaces.
112+
These would allow the foreign code to implement an sync callback interface using async code
113+
The Rust code would block while waiting for the result.
114+
115+
## Decision Outcome
116+
117+
## Pros and Cons of the Options
118+
119+
### (A) Experiment with async Rust
120+
121+
* Good, if we decide to avoid wrappers in `ADR-0008` because it allows us to remove the async wrappers.
122+
* Bad, because there's a risk that the UniFFI async code will cause issues and our current async strategy is working okay.
123+
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.
124+
* Good because it allows us to be more efficient with our thread usage.
125+
When an async task is waiting on a lock or network request, it can suspend itself and release the thread for other async work.
126+
Currently, we need to block a thread while we are waiting for this.
127+
However, it's not clear this would meaningfully affect our consumers since we don't run that many blocking operations.
128+
We would be saving maybe 1-2 threads at certain points.
129+
* Good, because it makes it easier to integrate with new languages that expect async.
130+
For example, WASM integration libraries usually returns `Future` objects to Rust which can only be evaluated in an async context.
131+
Note: this is a separate issue from UniFFI adding WASM support.
132+
If we switched our component code to using async Rust, it's possible that we could use `wasm-bindgen` instead.
133+
* Bad, because it makes it harder to provide bindings on new languages that don't support async, like C and C++.
134+
Maybe we could bridge the gap with some sort of callback-based async system, but it's not clear how that would work.
135+
136+
### (B) Keep hand-written Async wrappers
137+
138+
* Good, this is the status quo, and doesn't require any work
139+
140+
### (C) Auto-generate Async wrappers
141+
142+
* Good, if we decide to avoid wrappers in `ADR-0008` because it allows us to remove the hand-written async wrappers.
143+
* Good, because we could copy over auto-generated documentation and simply add `async` or `suspend` to the function signature.
144+
* Good, because it's less risky than (A)
145+
* Bad, because we would continue to have inefficiencies in our threading strategy.
146+
* Good, because this is a more flexible async strategy.
147+
We could use async wrappers to integrate with languages like WASM and not use them to integrate with languages like C and C++.
148+
However, it's not clear at all how this would work in practice.
149+
* Bad, because it's less flexible than (A).
150+
For example, with (A) it would be for the main API interface to return another interface with async methods from Rust code.
151+
That wouldn't be possible with this system, although it's not clear how big of an issue that would be.

0 commit comments

Comments
 (0)