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

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.