Adding twin ADRS on wrapper code and async Rust

[bendk]

These are independant but related decisions, so two ADRs in one pull request is a good way to handle them.

Pull Request checklist

• Breaking changes: This PR follows our breaking change policy
  • This PR follows the breaking change policy:
    • This PR has no breaking API changes, or
    • There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
• Quality: This PR builds and tests run cleanly
  • Note:
    • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
    • If this pull request includes a breaking change, consider cutting a new release after merging.
• Tests: This PR includes thorough tests or an explanation of why it does not
• Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
  • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
• Dependencies: This PR follows our dependency management guidelines
  • Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8aa85ff) 36.68% compared to head (ae48436) 36.68%.
Report is 4 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5910   +/-   ##
=======================================
  Coverage   36.68%   36.68%           
=======================================
  Files         348      348           
  Lines       33663    33663           
=======================================
  Hits        12349    12349           
  Misses      21314    21314           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tarikeshaq]

Thank you so much for taking this on and writing this out @bendk - I wrote down my initial thoughts going through this, but feel free to challenge my comments.

My own position on those two is: (but I'm curios to hear what others think as well!)

• For wrappers, I believe it's a case-by-case basis. And occasionally adding wrapper to make our API more idiomatic shouldn't be discouraged. That said, I do think it'll be educational to evaluate what are the existing cases that couldn't be satisfied with UniFFI, if the answer is "close to none", that would change my position
• For Async Rust, it's too early to commit to embracing it but I would love to see a small component running using Async Rust as support.
  • It's an expensive commitment, so being diligent with quantifying the wins would go a long way
  • Maybe the decision could be along the lines of "Lets experiment with Async Rust, starting with the XX component, then re-evaluate". That way we commit to trying it, and commit to re-visiting the large question of embracing it.

[tarikeshaq]

Yeah there is a tiny extra clarification here that might make the conversation just a little bit clearer - we are talking about wrapper code that lives in A-S, and not in the consuming apps

[bendk]

android-components kind of confuses things, but we don't need to mention that here. Is there a better way to write this sentence?

[linabutler]

I was also wondering about that—at first, I thought you were including the wrappers in Android Components and Firefox for iOS, but after seeing @tarikeshaq's comment and reading further, I realized you were just talking about the wrappers that live in Application Services.

We use the term "hand-written Swift code" and "hand-written Kotlin code" in our documentation for adding a new component, so I wonder if using that term here would help with understanding?

This could definitely use some more wordsmithing, but maybe something like this?

Application-services components currently consist of a Rust core, and UniFFI-generated bindings for Swift and Kotlin. Additionally, some of our components have hand-written Swift and Kotlin code, containing wrappers and extensions for the UniFFI bindings.

[linabutler]

(I called out "extensions" separately from "wrappers" to hint at cases like #5346 (comment), but they both fall under the umbrella of "hand-written foreign code").

[tarikeshaq]

For me, this is the biggest benefit and it's directly related to async as well. I like to think of our components as SDKs that we expose to both Kotlin and Swift, and thus should be exposed using idiomatic code. As nice as it is to only work in Rust and expose everything through UniFFI - we start to hit limitations where language specific features are the more "idiomatic" option, but are too specific for UniFFI to support (like the notification center example you provided)

[tarikeshaq]

I don't fully agree with this one - I agree the active work in UniFFI will truly improve the documentation strategy, but I'm not sure that removing wrapper code makes the documentation story any better. Both javadocs and swift docs have very rich documentation systems that would be hard to fully conform to (found this great blog post about swift's documentation)

I still believe what we're doing in UniFFI is many times better than the status quo, where the generated code has no doc comments at all, but I doubt that story alone makes the documentation story better than hand-written docs in swift

[bendk]

I'll try to add this as a new bullet. Maybe it's most precise to say that auto-generating the docs would simplify things, but we might end up with worse docs if we don't hand-write the wrapper doc-strings.

[bendk]

Added the bullet below, but I should say that I'm a bit skeptical of the hand-written approach. IMO, a much bigger issue than specifics with javadoc/swiftdoc is the risk of documentation drift as we make changes. If we could make it work, I'd prefer using rustdoc for everything and rename the types (UInt32/UInt instead of u32). Yes it won't look as idiomatic, but rustdoc is still pretty good and it's much harder for the docs to get out-of-sync with each other and with the code.

[tarikeshaq]

+1 that if we embrace async, having async callback support will be critical for things like network support, I'd almost want to see how that system works in practice before making a final call here.

[linabutler]

Agreed! This one might also need some support from Android Components; IIRC, it currently wraps GeckoView's async networking client in a blocking interface. It would be really compelling (and a fair bit of work! 😅) to have networking be one of our first uses of async callbacks, though!

[tarikeshaq]

This is the largest Bad I see here, the cost of sticking to our existing, non-async Rust is cheap (no work!) but it might be expensive engineering time spent to make all the async bits work (async callback interfaces, migrating away from sync calls and the breaking changes that follow, async viaduct, etc)

There is also the cost of having to handle possible inconsistencies we never had to handle before (if we use async mutexes that drop locks on awaits, allowing different tasks to interleave in ways we never had to deal with before)

This almost makes me want to take the position of: "It's too costly and risky to embrace because of uncertainty - however if we can quantify possible wins, let's try it on a small surface and realize those wins to give us the confidence we're doing the right thing"

[jonalmeida]

There is also the cost of having to handle possible inconsistencies we never had to handle before (if we use async mutexes that drop locks on awaits, allowing different tasks to interleave in ways we never had to deal with before)

I think you are describing my concern here from the embedder perspective: we have our platform async system (Coroutines) and we will have the native rust async system which have to both sync (heh) together correctly because the race bugs from this would be painful to debug.

[bendk]

Agreed that this is the biggest concern.

FWIW, UniFFI uses the platform async system to schedule the Rust work, so races don't seem that much more likely to me. If there's a bug, I would expect either hangs if the task gets lost somehow or just regular exceptions / crashes.

[tarikeshaq]

To provide an example for this, I've experimented with having the fxa_client compile to WASM and the dependency on NSS was a problem, I've opted to web apis exposed through the JS runtime the wasm is running in, and those web apis often return promises, that have to be wrapped in Futures.

The above makes it so it's difficult to expose our fxa_client to web applications without exposing an async Rust API.

[bendk]

For Async Rust, it's too early to commit to embracing it but I would love to see a small component running using Async Rust as support.

I wouldn't want to move faster than that either. I think calling the strategy "embracing Async Rust" is too much, it should be something more like "timidly waving at Async Rust from 100ft away".

[bendk]

Thanks for all the feedback here. I just pushed an update with a bunch of changes, the main ones are:

• Significantly re-framed the wrapper questions. Instead of "do we like wrappers or not?", it's now "how much effort are we willing to put into removing wrappers?"
• Added an option (C) for the async question

[bendk]

I spent a lot of time in H2 investigating ways to make UniFFI work with async Rust, but I have to say this one seems pretty attractive.

[linabutler]

These both look great; thank you so much for taking the time to write them up!

[linabutler]

I was also wondering about that—at first, I thought you were including the wrappers in Android Components and Firefox for iOS, but after seeing @tarikeshaq's comment and reading further, I realized you were just talking about the wrappers that live in Application Services.

We use the term "hand-written Swift code" and "hand-written Kotlin code" in our documentation for adding a new component, so I wonder if using that term here would help with understanding?

This could definitely use some more wordsmithing, but maybe something like this?

Application-services components currently consist of a Rust core, and UniFFI-generated bindings for Swift and Kotlin. Additionally, some of our components have hand-written Swift and Kotlin code, containing wrappers and extensions for the UniFFI bindings.

[linabutler]

(I called out "extensions" separately from "wrappers" to hint at cases like #5346 (comment), but they both fall under the umbrella of "hand-written foreign code").

[linabutler]

This is a great point, and I think this kind of wrapping is best done in our consuming applications (Android Components and Firefox for iOS) than in Application Services. Those wrappers in Android Components / Firefox for Android, and Firefox for iOS, can bridge the UniFFI interfaces to the the libraries that the consuming apps use.

We have one example of that in the Suggest component: our UniFFI binding returns a Vec<u8> for favicon data, which is decoded into an android.graphics.Bitmap on Android, and a UIImage on iOS. These are Android and iOS-specific types—not part of the Kotlin or Swift core language—so it makes a bit more sense for that logic to live in Fenix and Firefox for iOS, than in a wrapper in Application Services. (External types aren't quite suitable there, either, for a handful of reasons).

Background scheduling is another example: the WorkManager library in Android works completely differently than BGTaskScheduler on iOS, and the consuming application might not even want to call into the Rust component in the background at all. A "vanilla" API would make that integration easier.

[mhammond]

Not wanting to distract from the main point here, but it would be super interesting to know how uniffi "CustomTypes" work here - eg, we could probably use something like Image in the UDL, which is just an alias for Vec<u8>, and Android and iOS each has special code that knows how to turn that vec into the local type.

[linabutler]

At first, I was wondering if this was a bit inconsistent with your "Wrapper code" ADR, because that ADR (seems to?) explicitly exclude wrappers outside of Application Services.

But thinking about it a bit more, the way you've written this makes sense to me! All our wrappers—wherever they live—do two things:

1. Wrap the synchronous Rust component interface in an asynchronous foreign interface.
2. Bridge the Rust interface to platform-specific Android and iOS interfaces (WorkManager, BGTaskScheduler, UIKit, etc.)

We can lift (1) into Application Services, and still keep the wrappers in our consuming applications for (2).

[linabutler]

Agreed! This one might also need some support from Android Components; IIRC, it currently wraps GeckoView's async networking client in a blocking interface. It would be really compelling (and a fair bit of work! 😅) to have networking be one of our first uses of async callbacks, though!

[linabutler]

🙋🏼‍♀️ I volunteer remote_settings as a tribute another test component!

• It has a small API surface.
• It could be a good "launch customer" for async networking.
• It uses a lock to keep track of some client state, and races there shouldn't be catastrophic.
• It doesn't do any persistence (e.g., to SQLite) directly.
• It has a small Android Components wrapper, that won't be too disruptive to migrate.

[bendk]

First off: Thanks for offering this! Secondly: maybe we should be thinking about this component-by-component.

Async makes sense for remote_settings so that's a natural place to start exploring.

Async seems to make the least sense for our syncable-storage components. I wonder if we should just lean into saying the non-async API is the "real" API. We could still have an async wrapper class, but this would be more of an add-on. In practice, this would mean that when publish documentation, we would create detailed documentation for the sync API while the async wrapper would just have some text like "all methods work the same as LoginsStore, but are wrapped with withContext to present an async API". I'm think that might work fine, although I wonder if that would make the IDE tooltips worse.

[lougeniaC64]

I agree with other comments that suggest we need to analyze our current wrapper code to see how much of it we can remove and that the result will inform our future strategy. I for one am assuming that we won't be able to remove all of it so we will end up at option C. We also might want to have a one-to-one wrapper to consumer strategy so that we can put the more platform-specific, opinionated code there. But in order to reduce our API surface I think we should do what we can to reduce the wrapper code we have.

[mhammond]

Sorry for the delay and sorry that I only got to the first ADR - I'll look at the second soon!

[mhammond]

I don't think that's true for wrappers in our repo - eg, places seems most concerned with capturing telemetry. From memory the wrappers in a-c are mostly like this, but it does raise the question: if uniffi gave us the capability of presenting the async interface, would this actually allow us to remove the wrappers, or just thin them out?

Conversely though, these telemetry wrappers exist because of some weird architectural considerations - ie, it's not our desire to have things set up this way and if our goal was to remove all wrappers, we'd just do something different here?

[bendk]

When I wrote this up, I was incorrectly picturing the a-c wrappers as living in a-s. This raises a question that I think the ADRs should be more explicit about: who owns the wrappers in a-c and firefox-ios? In practice, it's been us and if that's true I think we should try to move them into our repo -- or at least split out some part and move that in.

I think this would clarify/improve the relationship between our teams. One thing I noticed doing the FxA work is that I had lots of opinions on how the a-c code should be organized. But once I figured out what part of the API should move to a-s, I was willing and happy to let those opinions go and let the firefox-android team own those decisions.

Going back to the ADR: what kind of API should we own and export to our consumers? Sync? Async? Sync with an async wrapper?

[mhammond]

This is a concern to me (ie, that stuff will hand around). It's also interesting to think about who our "consumers" are in the medium term. It seems unlikely they will include Android or desktop, so really this is all about iOS.

That said though, it might be interesting to experiment with - or maybe to look back at some of the breaking changes we did make and see whether this approach would actually have worked in practice.

I wonder if another alternative would be to see if we can make the components more modular - eg, just allow iOS to stay on an "old" version of places even though they take a newer (say) tabs. This will have its own challenges though with crate versions - eg, if we update sqlite - but this might be managable (eg, I think the sqlite example would be managable by rearranging some dependencies, and for other crates it might occasionally mean some duplicate crates lower in the depdendency graph, but that might be ok)

[mhammond]

I don't quite understand the implications of this, but if "only using interfaces that UniFFI could support" implies "our swift code consumers can't use the 'NotificationCentre` then it's probably a non-starter?

[bendk]

I think it would be more like "our swift consumers need to do the work to hook up a callback interface to the NotificationCenter themselves". Definitely a drawback but not totally out of the question. I do prefer C over B at this point though.

[mhammond]

Not wanting to distract from the main point here, but it would be super interesting to know how uniffi "CustomTypes" work here - eg, we could probably use something like Image in the UDL, which is just an alias for Vec<u8>, and Android and iOS each has special code that knows how to turn that vec into the local type.

[mhammond]

I think I'd be leaning towards an option described as something like "as few wrappers as possible, but no less" - ie, a perfect world has no wrappers, but this world is going to force some on us. Deciding "no wrappers" would be a fool's errand and not survive reality.

I guess this is closest to (C) Is there some feature uniffi is missing to enable this without the "bad" parts? It almost gets back to the "decorators" idea we had all that time ago - ie, something more like "augmentation" than "wrappers"?