Make DatabaseLoginsStorage async

[bendk]

There hasn't been agreement on a grand vision for async Rust in app-services (#6549). I think part of the reason is that ADR is discussing too many things. This is my attempt to take a small change from that ADR and see if we agree.

This change takes the async wrapping code that's currently in android-components and moves it to our app-services wrapper. This improves the API boundary IMO. The SYNC team and the credentials management teams are responsible for threading issues. For example, we're the one's thinking through how to avoid blocking too many threads when we're waiting on the primary password dialog. Since we're responsible for threading issues, we should own the code

On a practical level: I have some ideas on how to improve our threading strategy and it's simpler to think about those changes if all the code is in one repo.

Here's the corresponding android-components change: bendk/firefox@d038ba6

If we agree on this, then I think we can do something similar for iOS.

What do others think? Is this a good idea? Do we need/want an ADR for this?

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 diligence applied in selecting them.

[lougeniaC64]

The SYNC sync and the credentials management teams are responsible for threading issues. For example, we're the one's thinking through how to avoid blocking too many threads when we're waiting on the primary password dialog. Since we're responsible for threading issues, we should own the code

A couple questions . . . what is the SYNC sync? I'm assuming that's us but I just want to make sure I didn't miss a jargon drop. Also, are we responsible for threading issues? I'm going to, perhaps anally, push back a little here.

I see that you've converted the Kotlin logins store functions to suspend which is what I would expect. Since these functions will be non-blocking would we need to own threading issues? My concern is less about what happens on the Android side and far more what happens on the iOS side, particularly with the less than ideal DispatchQueue usage we have at present. We might bite off more than we can chew by taking full ownership.

That aside these changes make sense to me, though I defer to Mark and Sammy as my Android knowledge is meager.

[mhammond]

A couple questions . . . what is the SYNC sync? I'm assuming that's us but I just want to make sure I didn't miss a jargon drop.

I think Ben meant to write "sync team" there. So I think I agree with you both - with our "sync team" hat on, I don't think we own the threading model for anything other than our components. But with our "rust components" hat on, I agree that we should have a holistic view and approach to threading so the credential management team and the sync team don't need to think too much about it and would need to try hard to screw things up.

Sorry I missed this - I notice it's now over 6 months old - should I look at it in detail?

[lougeniaC64]

But with our "rust components" hat on, I agree that we should have a holistic view and approach to threading so the credential management team and the sync team don't need to think too much about it and would need to try hard to screw things up.

Ah yes, the rust components team . . . assuming this is a pattern we'd implement in all of our components and any new components moving forward this makes sense.

[bendk]

Yup, that should read "Sync team". Sorry for the confusion. I'll try to dust this one off and rebase it sometime next week.

Since these functions will be non-blocking would we need to own threading issues?

Yes I think we would, but we also kind of do now IMO. I just had to fix a deadlock on desktop because of threading issues, specifically weird issues that happen because of the way the main thread is special cased there. It would be great not to have to do this, but my feeling is that we're going to be on the hook one way or another and this is a pro-active step to help that work by marking functions as async or sync. This communicates how they're meant to be used and also we get some help from the compiler. Maybe if RemoteSettingsClient::get_records was async I would have been a bit more careful around holding that lock and we could have avoided the issue.

My concern is less about what happens on the Android side and far more what happens on the iOS side, particularly with the less than ideal DispatchQueue usage we have at present.

Yeah, I'm also concerned about that part -- the iOS side definitely feels riskier. Maybe this could help us standardize how we use DispatchQueue a bit and prevent errors in that code though.

Sorry I missed this - I notice it's now over 6 months old - should I look at it in detail?

That'd be great, thanks. I am planning to rebase so maybe you want to wait for that. That said, I don't think any of the basic ideas have changed in that time though so maybe you don't need to wait.

with our "sync team" hat on, I don't think we own the threading model for anything other than our components. But with our "rust components" hat on, I agree that we should have a holistic view and approach to threading so the credential management team and the sync team don't need to think too much about it and would need to try hard to screw things up.

This is great point that I need to think about more, but I like your basic setup: We're the rust components team and we should be recommending an approach to threading that helps teams like credential management produce correct code.

[lougeniaC64]

Maybe this could help us standardize how we use DispatchQueue a bit and prevent errors in that code though.

There's definitely some cleanup we could do but I think the iOS team is working on removing DispatchQueue use altogether. I'm not sure when that work is happening, what they're moving to, or how that would play with our approach but we can cross that bridge when we get to it.

[bendk]

Maybe this could help us standardize how we use DispatchQueue a bit and prevent errors in that code though.

There's definitely some cleanup we could do but I think the iOS team is working on removing DispatchQueue use altogether. I'm not sure when that work is happening, what they're moving to, or how that would play with our approach but we can cross that bridge when we get to it.

This would be great to talk to them about. I think one legitimate use for Dispatch queue is for blocking functions like our ones that run SQLite queries.

[bendk]

Rebased this against the current main.

[mhammond]

the ownership issue is interesting. There was talk in the past that the app wants to own the context - ie, that they may want to make their own context rather than use Dispatchers.IO - or maybe even just customize it - eg, the docs for that include:

// 100 threads for MySQL connection
val myMysqlDbDispatcher = Dispatchers.IO.limitedParallelism(100)
// 60 threads for MongoDB connection
val myMongoDbDispatcher = Dispatchers.IO.limitedParallelism(60)

(which is, um, interesting - 160 threads? Why not! :) The docs do qualify this, but still!)

and it's not clear each component really has enough context to make that decision? I agree that it's our job to help, eg, avoid blocking too many threads for the master-password dialog, but less clear that it's our call how many threads there are in total. Might that ultimately be dynamic based on the system info? So really I think that we do need to involve more of the browser teams here - the same basic question also exists on desktop, so it's really not just a mobile thing.

But if we assume everyone agrees this is a good idea, what I don't really understand is what we do for components without hand-written wrappers. Would we force hand-written wrappers just for this purpose, or would we try and have uniffi learn how to generate them? Neither of those options sounds great. Does this also imply we should have hand-written wrappers for JS? That might actually be mildly preferable to the current config.toml story, which I don't think is great. But also sounds tedious.

This also basically assumes "option a" from that ADR, right? Any other option would end up undoing this? So landing this would leave us in a position where we also move other components to this model, or have a situation where things are inconsistent pending movement on the ADR. That doesn't seem ideal, especially as I kinda like option a the worst there (although maybe it wins for pragmatic reasons).

Ultimately, this patch is quite low stakes, and shuffles things around in ways that don't fundamentally change anything for this component. OTOH though, I'm not sure we'd want to move every component to this model while we try and work out wtf we are doing about async rust. So I'm not against it, but I also fail to see how it moves the needle in any way that's important, especially assuming a mono-repo and assuming the status quo that the actual consumers of this are just firefox and focus. It's languished so long that it might just be a good artifact for that ADR?

[bendk]

the ownership issue is interesting. There was talk in the past that the app wants to own the context - ie, that they may want to make their own context rather than use Dispatchers.IO - or maybe even just customize it - eg, the docs for that include:
...
hat it's our job to help, eg, avoid blocking too many threads for the master-password dialog, but less clear that it's our call how many threads there are in total. Might that ultimately be dynamic based on the system info? So really I think that we do need to involve more of the browser teams here - the same basic question also exists on desktop, so it's really not just a mobile thing.

I agree and I think the code reflects that. The application passes a CoroutineContext to the DatabaseLoginsStorage constructor and Dispatchers.IO is just the default value. I think it's a good balance balance: we tell consumers that these methods are meant to be async, but we let them provide the context that they run in.

But if we assume everyone agrees this is a good idea, what I don't really understand is what we do for components without hand-written wrappers. Would we force hand-written wrappers just for this purpose, or would we try and have uniffi learn how to generate them? Neither of those options sounds great. Does this also imply we should have hand-written wrappers for JS? That might actually be mildly preferable to the current config.toml story, which I don't think is great. But also sounds tedious.

My feeling is that we teach UniFFI how to generate the code for us, but it wouldn't be generating this exact code. I'm picturing something like mozilla/uniffi-rs#1837, which works differently internally but has essentially the same public API: consumers pass in a CoroutineContext / DisptachQueue / Executor for us to run our blocking functions in. How do you feel about this option?

I think we could do something similar in JS, although I think we would need to involve the C++ layer. I haven't thought this one all the way though.

This also basically assumes "option a" from that ADR, right? Any other option would end up undoing this?

I my mind it's pushing us towards option C. Code-wise, I agree it's option A. However, the idea was to move the wrappers into our repo so that we're more free to refactor towards option C without needing to coordinate with other teams.

... I also fail to see how it moves the needle in any way that's important, especially assuming a mono-repo and assuming the status quo that the actual consumers of this are just firefox and focus

Good point about the mono-repo. I feel like that's going to fix some of the ownership questions in a broader way and kind of makes the previous point moot. It feels like a bad idea to try to merge this before that move.

[this] might just be a good artifact for that ADR?

I think you might be right. Maybe this could be a separate section or a separate ADR altogether. It seems like a good way to engage with the mobile engineers. They will probably have opinions on what API they want to use, what kinds of control they want over the async context, if we should use a DispatchQueue on swift, etc.