Skip to content

Refactors locks in AdsClient to avoid requirements of universal locks - #7627

Open
thesuzerain wants to merge 8 commits into
mainfrom
Moves-locks-for-ads-client
Open

thesuzerain wants to merge 8 commits into
mainfrom
Moves-locks-for-ads-client

Conversation

@thesuzerain

@thesuzerain thesuzerain commented Sep 25, 2026 •

Copy link
Copy Markdown
Collaborator

This restructure allows us to avoid having to lock the entire AdsClient, but only restricts locks to what we specifically need- in this case, HttpCache.

This allows:

  • stopping an HTTP query from locking the entire ads client.
  • In turn, this lets us move logic out of the FFI layer that we need to be able to resolve 'instantly' (the shutdown logic, or any future fire-and-forget requests to the background worker)

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.

}

#[derive(Clone)]
pub struct WrappedHttpCache(Option<Arc<Mutex<Option<HttpCache>>>>);

@thesuzerain thesuzerain Sep 25, 2026 •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This impl is a bit over the top and something I may not keep, I just didn't like the Option<...<Option<...>> and the nested structures it required to get locks. This lets us treat the double option as a single option. I think this is uglier actually. I'm considering a couple options before I PR this:

  • Arc<Mutex<Option<HttpCache>>> and get a lock every time even if HttpCache is unset.
  • Option<Arc<Mutex<Option<HttpCache>>>> and keep the nested Option handling.

Either way this code snippet is not the critical part of this refactor. However, we do need to pass this mutex 'up and out' to the AdsClient test shutdown_does_not_require_http_client_lock

@thesuzerain
thesuzerain marked this pull request as ready for review September 25, 2026 19:36
@thesuzerain
thesuzerain requested a review from a team as a code owner September 25, 2026 19:36

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant