Skip to content

Async Rust ADR #6549

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Async Rust ADR #6549

wants to merge 1 commit into from

Conversation

@bendk
Copy link
Contributor

@bendk bendk commented Jan 7, 2025 •
edited
Loading

This is my new version of #5910, which is more focused and more concrete. Maybe we're finally ready to actually make a decision here 🤞

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.

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

@bendk bendk mentioned this pull request Jan 7, 2025
Closed
5 tasks
@bendk bendk added the discuss label Jan 7, 2025
bendk
bendk commented Jan 7, 2025
View reviewed changes
docs/adr/files/0009/option-b-android-components.diff
@@ -0,0 +1,256 @@
diff --git a/mobile/android/android-components/components/feature/fxsuggest/build.gradle b/mobile/android/android-components/components/feature/fxsuggest/build.gradle
index d5b717c0ef..28d9c82eb2 100644
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure the best way to communicate these changes, but I went with checking in diffs because I wanted to make sure they never expired. I can put these into a github PR or phab patch if people want, maybe those would look nicer.

@bendk bendk force-pushed the push-stwkonvvqqlk branch from 686b62c to de5b7c8 Compare January 23, 2025 14:19
@bendk
Copy link
Contributor Author

bendk commented Jan 23, 2025

I updated this after our meeting yesterday:

  • Added a list of issues this is trying to address to the problem statement section
  • Changed option A from "Do nothing" to "Address as many issues as we can without moving to async Rust".

With this change, I could get behind option A. I think we could solve the worst issue, which is how the functionality is split across different repos/languages and easy to forget about when you're writing Rust code. The impact on the docs is unfortunate, but not really a deal-breaker.

That said, I still like option C the best. I like that it makes the code we write match more closely with how consumers are using it and it opens up new possibilities for the future. I also don't see much downside/risk personally.

bendk added a commit to bendk/application-services that referenced this pull request Jun 13, 2025
@bendk
Make DatabaseLoginsStorage async
8984a0f 
There hasn't been agreement on a grand vision for async Rust in
app-services (mozilla#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 like how to avoid
blocking too many threads when we're waiting on the primary password
dialog.  Therefore, the code that affects this should be owned by us. In
more practical terms, 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: https://phabricator.services.mozilla.com/D253666

What do others think?  Is this a good idea?  Do we need/want an ADR for this?
bendk added a commit to bendk/application-services that referenced this pull request Jun 13, 2025
@bendk
Make DatabaseLoginsStorage async
f84bf7a 
There hasn't been agreement on a grand vision for async Rust in
app-services (mozilla#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 like how to avoid
blocking too many threads when we're waiting on the primary password
dialog.  Therefore, the code that affects this should be owned by us. In
more practical terms, 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: https://phabricator.services.mozilla.com/D253666

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?
bendk added a commit to bendk/application-services that referenced this pull request Jun 13, 2025
@bendk
Make DatabaseLoginsStorage async
f794d5f 
There hasn't been agreement on a grand vision for async Rust in
app-services (mozilla#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 like how to avoid
blocking too many threads when we're waiting on the primary password
dialog.  Therefore, the code that affects this should be owned by us. In
more practical terms, 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?
bendk added a commit to bendk/application-services that referenced this pull request Jun 13, 2025
@bendk
Make DatabaseLoginsStorage async
051d80f 
There hasn't been agreement on a grand vision for async Rust in
app-services (mozilla#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?
@bendk bendk mentioned this pull request Jun 13, 2025
Open
5 tasks
bendk added a commit to bendk/application-services that referenced this pull request Jun 13, 2025
@bendk
Make DatabaseLoginsStorage async
8a3152c 
There hasn't been agreement on a grand vision for async Rust in
app-services (mozilla#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?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

discuss

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@bendk