Ads-Client: refactor shutdown procedure - #7584
Merged
Merged
Conversation
thesuzerain
commented
Sep 4, 2026
| } | ||
|
|
||
| #[test] | ||
| fn test_shutdown_telemetry_basic() { |
Collaborator
Author
There was a problem hiding this comment.
These tests are not identical to the ones before, notice the use of MozAdsClientBuilder instead of an internal function to create an AdsClient. This is because I've moved some of the shutdown logic outside of AdsClient to not have to get a lock on the entire AdsClient. I think the logic of these tests should be functionally identical, but worth noting.
thesuzerain
commented
Sep 4, 2026
| // Log the error, but continue with shutdown. | ||
| error!("Failed to shutdown the ads client: {:?}", err); | ||
| } | ||
| self.shutdown_references.shutdown(); |
Collaborator
Author
There was a problem hiding this comment.
Note the significant difference here: we are no longer shutting down the sqlite database 'safely', as it doesn't seem to be needed to do to fix this crash. We will revisit this in another refactor.
Almaju
approved these changes
Sep 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes two issues related to the uniffi crash error:
Builderwas holding on to a reference to the telemetry. So if you closed the app too quickly, the JS side would not have collected the Builder and destroyed the secondary telemetry reference.AdsClient. This means that when we fixed the async issue inm-c(thatshutdownis behaving fire and forget rather than awaiting for it), it can now hang if timing is unlucky. This refactors the shutdown procedure to only await on the telemetry call to the JS layer (the minimum we can lock on).shutdownof theHttpCache, in favor of future refactors.Pull Request checklist
[ci full]to the PR title.