From 34415fc817aceb976f3def56a351e034882f52cc Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Thu, 10 Sep 2026 12:27:08 -0700 Subject: [PATCH 01/25] feat: Adds the background worker --- .../integration-tests/tests/mars_async.rs | 318 ++++++++++++++++++ components/ads-client/src/ads_store.rs | 63 +++- components/ads-client/src/client.rs | 29 +- components/ads-client/src/client/error.rs | 34 +- components/ads-client/src/ffi.rs | 7 +- components/ads-client/src/lib.rs | 133 +++++++- components/ads-client/src/worker.rs | 63 ++++ components/ads-client/src/worker/command.rs | 190 +++++++++++ 8 files changed, 823 insertions(+), 14 deletions(-) create mode 100644 components/ads-client/integration-tests/tests/mars_async.rs create mode 100644 components/ads-client/src/worker.rs create mode 100644 components/ads-client/src/worker/command.rs diff --git a/components/ads-client/integration-tests/tests/mars_async.rs b/components/ads-client/integration-tests/tests/mars_async.rs new file mode 100644 index 00000000000..4ae8102d3ed --- /dev/null +++ b/components/ads-client/integration-tests/tests/mars_async.rs @@ -0,0 +1,318 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public +* License, v. 2.0. If a copy of the MPL was not distributed with this +* file, You can obtain one at http://mozilla.org/MPL/2.0/. +*/ + +use ads_client::MozAdsIABContent; +use ads_client::MozAdsIABContentTaxonomy; +use ads_client::MozAdsPlacementRequestWithCount; +use ads_client::ads_store::PlacementId; +use ads_client::{ + MozAdsClient, MozAdsClientBuilder, MozAdsEnvironment, MozAdsPlacementRequest, + MozAdsRequestOptions, MozAdsTile, +}; +use std::sync::Arc; + +pub const TEST_TIMEOUT_DURATION: std::time::Duration = std::time::Duration::from_secs(10); + +fn init_backend() { + viaduct_hyper::viaduct_init_backend_hyper(); +} + +fn prod_client() -> ads_client::MozAdsClient { + Arc::new(MozAdsClientBuilder::new()) + .environment(MozAdsEnvironment::Prod) + .build() +} + +#[test] +#[ignore = "integration test: run manually with -- --ignored"] +fn test_contract_image_prod_async() { + init_backend(); + + // Prefetch + let placement_id = PlacementId::new("mock_billboard_1"); + let client = prod_client(); + let result = client.prefetch_ads( + vec![MozAdsPlacementRequest { + iab_content: None, + placement_id: placement_id.clone().into(), + }], + vec![], + vec![], + None, + ); + + assert!( + result.is_ok(), + "Image ad dispatch request failed: {:?}", + result.err() + ); + + // Ping (waits for queue to clear) + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION)); + assert!(ping.is_ok(), "Ping failed: {:?}", ping.err()); + + // Query + let result = client.query_image_ads(placement_id); + assert!( + result.is_ok(), + "Querying for ads failed: {:?}", + result.err() + ); + let placements = result.unwrap(); + + assert!(placements.is_some()); +} + +#[test] +#[ignore = "integration test: run manually with -- --ignored"] +fn test_contract_image_with_categories_prod_async() { + init_backend(); + + // Prefetch + let placement_id = PlacementId::new("mock_billboard_1"); + let client = prod_client(); + let result = client.prefetch_ads( + vec![MozAdsPlacementRequest { + iab_content: Some(MozAdsIABContent { + category_ids: vec!["338".to_string()], + taxonomy: MozAdsIABContentTaxonomy::IAB3_0, + }), + placement_id: placement_id.clone().into(), + }], + vec![], + vec![], + Some(MozAdsRequestOptions { + flags: std::collections::HashMap::from([("contextual_placement".to_string(), true)]), + ..Default::default() + }), + ); + + assert!( + result.is_ok(), + "Image ad dispatch request failed: {:?}", + result.err() + ); + + // Ping (waits for queue to clear) + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION)); + assert!(ping.is_ok(), "Ping failed: {:?}", ping.err()); + + // Query + let result = client.query_image_ads(placement_id); + assert!( + result.is_ok(), + "Querying for ads failed: {:?}", + result.err() + ); + + let placements = result.unwrap(); + assert!(placements.is_some()); + let ad = placements.unwrap(); + assert!(!ad.url.is_empty(), "destination url should be populated"); + assert!(!ad.image_url.is_empty(), "image url should be populated"); +} + +#[test] +#[ignore = "integration test: run manually with -- --ignored"] +fn test_contract_spoc_prod_async() { + init_backend(); + + // Prefetch + let placement_id = PlacementId::new("mock_spoc_1"); + let client = prod_client(); + let result = client.prefetch_ads( + vec![], + vec![MozAdsPlacementRequestWithCount { + count: 3, + iab_content: None, + placement_id: placement_id.clone().into(), + }], + vec![], + None, + ); + + assert!( + result.is_ok(), + "Spoc ad dispatch request failed: {:?}", + result.err() + ); + + // Ping (waits for queue to clear) + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION)); + assert!(ping.is_ok(), "Ping failed: {:?}", ping.err()); + + // Query + let result = client.query_spoc_ads(placement_id); + assert!( + result.is_ok(), + "Querying for ads failed: {:?}", + result.err() + ); + let placements = result.unwrap(); + assert!(placements.is_some()); + assert!(placements.unwrap().len() == 3); +} + +#[test] +#[ignore = "integration test: run manually with -- --ignored"] +fn test_contract_tile_prod_async() { + init_backend(); + + // Prefetch + let placement_id = PlacementId::new("mock_tile_1"); + let client = prod_client(); + let result = client.prefetch_ads( + vec![], + vec![], + vec![MozAdsPlacementRequest { + iab_content: None, + placement_id: placement_id.clone().into(), + }], + None, + ); + + assert!( + result.is_ok(), + "Tile ad dispatch request failed: {:?}", + result.err() + ); + + // Ping (waits for queue to clear) + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION)); + assert!(ping.is_ok(), "Ping failed: {:?}", ping.err()); + + // Query + let result = client.query_tile_ads(placement_id); + assert!( + result.is_ok(), + "Querying for ads failed: {:?}", + result.err() + ); + let placements = result.unwrap(); + + assert!(placements.is_some()); +} + +#[test] +#[ignore = "integration test: run manually with -- --ignored"] +fn test_contract_tile_ohttp_prod_async() { + init_backend(); + viaduct::ohttp::configure_ohttp_channel( + "ads-client".to_string(), + viaduct::ohttp::OhttpConfig { + relay_url: "https://mozilla-ohttp.fastly-edge.com/".to_string(), + gateway_host: "prod.ohttp-gateway.prod.webservices.mozgcp.net".to_string(), + }, + ) + .expect("OHTTP channel configuration should succeed"); + + // Prefetch + let placement_id = PlacementId::new("mock_tile_1"); + let client = prod_client(); + let result = client.prefetch_ads( + vec![], + vec![], + vec![MozAdsPlacementRequest { + iab_content: None, + placement_id: placement_id.clone().into(), + }], + Some(MozAdsRequestOptions { + ohttp: true, + ..Default::default() + }), + ); + + assert!( + result.is_ok(), + "Tile ad dispatch request failed: {:?}", + result.err() + ); + + // Ping (waits for queue to clear) + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION)); + assert!(ping.is_ok(), "Ping failed: {:?}", ping.err()); + + // Query + let result = client.query_tile_ads(placement_id); + assert!( + result.is_ok(), + "Querying for ads failed: {:?}", + result.err() + ); + let placements = result.unwrap(); + + assert!( + placements.is_some(), + "OHTTP response should contain mock_tile_1" + ); +} + +#[test] +#[ignore = "integration test: run manually with -- --ignored"] +fn test_contract_multi_ad_type_prod_async() { + init_backend(); + + // Prefetch + let placement_image_id = PlacementId::new("mock_billboard_1"); + let placement_spoc_id = PlacementId::new("mock_spoc_1"); + let placement_tile_id = PlacementId::new("mock_tile_1"); + let client = prod_client(); + let result = client.prefetch_ads( + vec![MozAdsPlacementRequest { + iab_content: None, + placement_id: placement_image_id.clone().into(), + }], + vec![MozAdsPlacementRequestWithCount { + count: 4, + iab_content: None, + placement_id: placement_spoc_id.clone().into(), + }], + vec![MozAdsPlacementRequest { + iab_content: None, + placement_id: placement_tile_id.clone().into(), + }], + None, + ); + + assert!( + result.is_ok(), + "Image ad dispatch request failed: {:?}", + result.err() + ); + + // Ping (waits for queue to clear) + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION)); + assert!(ping.is_ok(), "Ping failed: {:?}", ping.err()); + + // Query + let result = client.query_image_ads(placement_image_id); + assert!( + result.is_ok(), + "Querying for image ads failed: {:?}", + result.err() + ); + let placements = result.unwrap(); + assert!(placements.is_some()); + + let result = client.query_spoc_ads(placement_spoc_id); + assert!( + result.is_ok(), + "Querying for spoc ads failed: {:?}", + result.err() + ); + let placements = result.unwrap(); + assert!(placements.is_some()); + assert!(placements.unwrap().len() == 4); + + let result = client.query_tile_ads(placement_tile_id); + assert!( + result.is_ok(), + "Querying for ads failed: {:?}", + result.err() + ); + let placements = result.unwrap(); + + assert!(placements.is_some()); +} \ No newline at end of file diff --git a/components/ads-client/src/ads_store.rs b/components/ads-client/src/ads_store.rs index eb8c377d94d..00b7861adf6 100644 --- a/components/ads-client/src/ads_store.rs +++ b/components/ads-client/src/ads_store.rs @@ -7,9 +7,12 @@ use serde::{Deserialize, Serialize}; use crate::{ ads_store::{builder::AdsStoreBuilder, store::AdsStoreHolder}, common::bytesize::ByteSize, - mars::ad_response::{AdImage, AdSpoc, AdTile}, + mars::{ + ad_response::{AdImage, AdSpoc, AdTile}, + error::FetchAdsError, + }, }; -use std::path::Path; +use std::{collections::HashMap, path::Path}; /// Identification of placement sent and returned from MARS (eg: `mock_spoc_1`) #[derive(Debug, Hash, PartialEq, Eq, Clone)] @@ -19,9 +22,6 @@ impl PlacementId { pub fn new(s: &str) -> PlacementId { PlacementId(s.to_string()) } - pub fn into_inner(self) -> String { - self.0 - } } impl AsRef for PlacementId { @@ -30,13 +30,52 @@ impl AsRef for PlacementId { } } +impl From for PlacementId { + fn from(value: String) -> Self { + PlacementId(value) + } +} + +impl From for String { + fn from(value: PlacementId) -> Self { + value.0 + } +} + #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] pub enum StorableAd { Image(AdImage), - Spoc(AdSpoc), + Spoc(Vec), Tile(AdTile), } +impl StorableAd { + // TODO: Should these be result? + pub fn into_image(self) -> Option { + if let StorableAd::Image(image) = self { + Some(image) + } else { + None + } + } + + pub fn into_spocs(self) -> Option> { + if let StorableAd::Spoc(spocs) = self { + Some(spocs) + } else { + None + } + } + + pub fn into_tile(self) -> Option { + if let StorableAd::Tile(tile) = self { + Some(tile) + } else { + None + } + } +} + pub struct AdsStore { holder: AdsStoreHolder, #[allow(dead_code)] @@ -61,6 +100,18 @@ impl AdsStore { self.holder.invalidate_ad_by_id(placement_id)?; Ok(()) } + + pub fn lookup(&self, placement_id: &PlacementId) -> Result, FetchAdsError> { + self.holder.lookup(placement_id) + } + + pub fn store_ads(&self, ads: HashMap) -> Result<(), FetchAdsError> { + // TODO: The query can be rewritten to have multiple ads inserted in one + for (placement_id, ad) in ads { + self.holder.store_ad(&placement_id, ad)?; + } + Ok(()) + } } #[cfg(test)] diff --git a/components/ads-client/src/client.rs b/components/ads-client/src/client.rs index 99290582119..739892f6fc5 100644 --- a/components/ads-client/src/client.rs +++ b/components/ads-client/src/client.rs @@ -7,12 +7,12 @@ use std::collections::HashMap; use std::sync::Arc; use std::time::Duration; -use crate::ads_store::AdsStore; +use crate::ads_store::{AdsStore, PlacementId, StorableAd}; use crate::common::bytesize::ByteSize; use crate::http_cache::{CachePolicy, HttpCache}; use crate::mars::ad_request::{AdPlacementRequest, AdRequestFlags}; use crate::mars::ad_response::{AdImage, AdResponse, AdResponseValue, AdSpoc, AdTile}; -use crate::mars::error::{RecordClickError, RecordImpressionError, ReportAdError}; +use crate::mars::error::{FetchAdsError, RecordClickError, RecordImpressionError, ReportAdError}; use crate::mars::{MARSClient, ReportReason}; use crate::shutdown::{AdsStoreShutdown, ShutdownReferences}; use crate::telemetry::Telemetry; @@ -116,6 +116,31 @@ where self.client.clear_cache() } + pub fn cache_ads( + &mut self, + ads: HashMap, + ) -> Result<(), FetchAdsError> { + let ads_store = self.ads_store.lock(); + if let Some(ads_store) = ads_store.as_ref() { + ads_store.store_ads(ads)?; + } + // TODO: Should we error if no ads store? + Ok(()) + } + + pub fn get_cached_ad( + &self, + placement_id: &PlacementId, + ) -> Result, FetchAdsError> { + let ads_store = self.ads_store.lock(); + if let Some(ads_store) = ads_store.as_ref() { + Ok(ads_store.lookup(placement_id)?) + } else { + // TODO: Should we error if no ads store? + Ok(None) + } + } + pub fn get_context_id(&self) -> context_id::ApiResult { self.context_id_provider.context_id() } diff --git a/components/ads-client/src/client/error.rs b/components/ads-client/src/client/error.rs index 2542939493f..913a36792c9 100644 --- a/components/ads-client/src/client/error.rs +++ b/components/ads-client/src/client/error.rs @@ -3,10 +3,18 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -use crate::mars::error::{FetchAdsError, RecordClickError, RecordImpressionError, ReportAdError}; +use std::sync::mpsc::{RecvTimeoutError, TrySendError}; + +use crate::{ + mars::error::{FetchAdsError, RecordClickError, RecordImpressionError, ReportAdError}, + worker::command, +}; #[derive(Debug, thiserror::Error)] pub enum ComponentError { + #[error("Error requesting ads from worker: {0}")] + BackgroundWorker(#[from] BackgroundWorkerError), + #[error("Error recording a click for a placement: {0}")] RecordClick(#[from] RecordClickError), @@ -28,3 +36,27 @@ pub enum RequestAdsError { #[error("Error requesting ads from MARS: {0}")] FetchAds(#[from] FetchAdsError), } + +#[derive(Debug, thiserror::Error)] +pub enum BackgroundWorkerError { + #[error("Error sending pong back from background worker")] + PongFailure(Box>), + + #[error("Error requesting new ads from the background worker: worker closed")] + WorkerClosed, + + #[error("Error requesting new ads from the background worker: worker full")] + WorkerFull, + + #[error("Worker timed out waiting for response: {0}")] + WorkerTimedOut(#[from] RecvTimeoutError), +} + +impl From> for BackgroundWorkerError { + fn from(value: TrySendError) -> Self { + match value { + TrySendError::Disconnected(_) => BackgroundWorkerError::WorkerClosed, + TrySendError::Full(_) => BackgroundWorkerError::WorkerFull, + } + } +} diff --git a/components/ads-client/src/ffi.rs b/components/ads-client/src/ffi.rs index 4cc40fa8a82..c17584a1238 100644 --- a/components/ads-client/src/ffi.rs +++ b/components/ads-client/src/ffi.rs @@ -23,7 +23,7 @@ use crate::mars::ad_response::{ use crate::mars::Environment; use crate::mars::ReportReason; use crate::AdsClientUrl; -use crate::MozAdsClient; +use crate::{worker, MozAdsClient}; use parking_lot::Mutex; use std::collections::HashMap; @@ -147,9 +147,12 @@ impl MozAdsClientBuilder { }; let client = AdsClient::new(client_config); let shutdown_references = client.shutdown_references(); + let inner = Arc::new(Mutex::new(client)); + let worker = worker::AdsClientWorkerWrapper::new(inner.clone()); MozAdsClient { - inner: Mutex::new(client), + inner, shutdown_references, + worker, } } diff --git a/components/ads-client/src/lib.rs b/components/ads-client/src/lib.rs index a599362b5c1..63832b8bfd4 100644 --- a/components/ads-client/src/lib.rs +++ b/components/ads-client/src/lib.rs @@ -3,7 +3,11 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -use std::collections::HashMap; +use std::{ + collections::HashMap, + sync::{mpsc, Arc}, + time::Duration, +}; use client::error::ComponentError; use error_support::handle_error; @@ -22,10 +26,17 @@ pub mod http_cache; mod mars; pub mod shutdown; pub mod telemetry; +pub mod worker; pub use ffi::*; -use crate::{ffi::telemetry::MozAdsTelemetryWrapper, shutdown::ShutdownReferences}; +use crate::{ + ads_store::{PlacementId, StorableAd}, + client::error::{BackgroundWorkerError, RequestAdsError}, + ffi::telemetry::MozAdsTelemetryWrapper, + shutdown::ShutdownReferences, + worker::{command::DispatchCommand, AdsClientWorkerWrapper}, +}; #[cfg(test)] mod test_utils; @@ -37,11 +48,14 @@ uniffi::custom_type!(AdsClientUrl, String, { try_lift: |val| Ok(AdsClientUrl::parse(&val)?), lower: |obj| obj.as_str().to_string(), }); +uniffi::custom_type!(PlacementId, String); +pub type MozAdsClientInner = Arc>>; #[derive(uniffi::Object)] pub struct MozAdsClient { - inner: Mutex>, + inner: MozAdsClientInner, shutdown_references: ShutdownReferences, + worker: AdsClientWorkerWrapper, } #[uniffi::export] @@ -179,4 +193,117 @@ impl MozAdsClient { .map_err(ComponentError::RequestAds)?; Ok(response.into_iter().map(|(k, v)| (k, v.into())).collect()) } + + // TODO: Can we make this one request? + #[handle_error(ComponentError)] + #[uniffi::method(default(image_ad_requests = [], spoc_ad_requests = [], tile_ad_requests = [], options = None))] + pub fn prefetch_ads( + &self, + image_ad_requests: Vec, + spoc_ad_requests: Vec, + tile_ad_requests: Vec, + options: Option, + ) -> AdsClientApiResult<()> { + let options = options.unwrap_or_default(); + let flags = AdRequestFlags::from(&options); + let ohttp = options.ohttp; + let blocks = options.blocks.clone(); + let cache_policy: CachePolicy = options.into(); + + // Dispatch image requests + if !image_ad_requests.is_empty() { + self.worker.dispatch(DispatchCommand::RequestImageAds { + image_ad_requests, + ohttp, + cache_policy, + flags: flags.clone(), + blocks: blocks.clone(), + })?; + } + // Dispatch spoc requests + if !spoc_ad_requests.is_empty() { + self.worker.dispatch(DispatchCommand::RequestSpocAds { + spoc_ad_requests, + ohttp, + cache_policy, + flags: flags.clone(), + blocks: blocks.clone(), + })?; + } + + // Dispatch tiles requests + if !tile_ad_requests.is_empty() { + self.worker.dispatch(DispatchCommand::RequestTileAds { + tile_ad_requests, + ohttp, + cache_policy, + flags: flags.clone(), + blocks: blocks.clone(), + })?; + } + + Ok(()) + } + + #[handle_error(ComponentError)] + #[uniffi::method()] + pub fn query_image_ads( + &self, + placement_id: PlacementId, + ) -> AdsClientApiResult> { + let inner = self.inner.lock(); + let image_ad: Option = inner + .get_cached_ad(&placement_id) + .map_err(RequestAdsError::from)?; + Ok(image_ad + .and_then(|ad| ad.into_image()) + .map(|ad| ad.clone().into())) + } + + #[handle_error(ComponentError)] + #[uniffi::method()] + pub fn query_spoc_ads( + &self, + placement_id: PlacementId, + ) -> AdsClientApiResult>> { + let inner = self.inner.lock(); + let spoc_ads: Option = inner + .get_cached_ad(&placement_id) + .map_err(RequestAdsError::from)?; + Ok(spoc_ads + .and_then(|ad| ad.into_spocs()) + .map(|ad| ad.clone().into_iter().map(|x| x.into()).collect())) + } + + #[handle_error(ComponentError)] + #[uniffi::method()] + pub fn query_tile_ads( + &self, + placement_id: PlacementId, + ) -> AdsClientApiResult> { + let inner = self.inner.lock(); + let tile_ad: Option = inner + .get_cached_ad(&placement_id) + .map_err(RequestAdsError::from)?; + Ok(tile_ad + .and_then(|ad| ad.into_tile()) + .map(|ad| ad.clone().into())) + } + + // Pings the background worker and waits for a response back, for use in tests. + // Because the background worker is synchronous, this returns if the worker is empty, + // making it useful for integration tests to wait until all tasks have completed. + #[handle_error(ComponentError)] + pub fn ping_background_worker(&self, timeout: Option) -> AdsClientApiResult<()> { + let (tx, rx) = mpsc::sync_channel(0); + self.worker.dispatch(DispatchCommand::Ping(tx))?; + + if let Some(timeout) = timeout { + rx.recv_timeout(timeout) + .map_err(BackgroundWorkerError::from)?; + } else { + rx.recv().map_err(|_| BackgroundWorkerError::WorkerClosed)?; + } + Ok(()) + } } diff --git a/components/ads-client/src/worker.rs b/components/ads-client/src/worker.rs new file mode 100644 index 00000000000..0538d7e99b9 --- /dev/null +++ b/components/ads-client/src/worker.rs @@ -0,0 +1,63 @@ +use crate::{ + client::error::{BackgroundWorkerError, ComponentError}, + worker::command::DispatchCommand, + MozAdsClientInner, +}; +use std::{ + sync::mpsc::{self, Receiver, SyncSender}, + thread::JoinHandle, +}; + +pub mod command; + +pub const ADS_CLIENT_WORKER_CHANNEL_BUFFER_SIZE: usize = 1000; +pub const ADS_CLIENT_WORKER_THREAD_NAME: &str = "ads-client.worker"; + +pub struct AdsClientWorkerWrapper { + _worker_thread: Option>, + worker_dispatch: Option>, +} + +impl AdsClientWorkerWrapper { + pub fn new(inner: MozAdsClientInner) -> AdsClientWorkerWrapper { + let (worker_dispatch, worker_thread) = Option::unzip(build_worker_thread(inner.clone())); + AdsClientWorkerWrapper { + _worker_thread: worker_thread, + worker_dispatch, + } + } + + pub fn dispatch(&self, command: DispatchCommand) -> Result<(), ComponentError> { + if let Some(worker_dispatch) = &self.worker_dispatch { + worker_dispatch + .try_send(command) + .map_err(BackgroundWorkerError::from)?; + + Ok(()) + } else { + Err(BackgroundWorkerError::WorkerClosed.into()) + } + } +} + +// Spawn worker thread from a reference to the client, returning a synchronous channel transmitter to the thread, and its JoinHandle. +// Returns None if thread fails to build. +pub fn build_worker_thread( + inner_client: MozAdsClientInner, +) -> Option<(SyncSender, JoinHandle<()>)> { + let (tx, rx) = mpsc::sync_channel(ADS_CLIENT_WORKER_CHANNEL_BUFFER_SIZE); + let worker_thread_handle = std::thread::Builder::new() + .name(ADS_CLIENT_WORKER_THREAD_NAME.to_string()) + .spawn(move || crate::worker::worker(inner_client, rx)).inspect_err(|err| { + error_support::error!("Failed to create ads-client worker thread `{ADS_CLIENT_WORKER_THREAD_NAME}` with: {err}") + }).ok()?; + Some((tx, worker_thread_handle)) +} + +fn worker(inner_client: MozAdsClientInner, rx: Receiver) { + // Synchronously run tasks in the order they are passed in this separate channel. + while let Ok(command) = rx.recv() { + // Error is naturally logged through `handle_error` conversion macro. + let _ = command.run_command(&inner_client); + } +} diff --git a/components/ads-client/src/worker/command.rs b/components/ads-client/src/worker/command.rs new file mode 100644 index 00000000000..a1bb1ebba4a --- /dev/null +++ b/components/ads-client/src/worker/command.rs @@ -0,0 +1,190 @@ +use std::{collections::HashMap, sync::mpsc::SyncSender}; + +use error_support::handle_error; +use url::Url; + +use crate::{ + ads_store::StorableAd, + client::error::{BackgroundWorkerError, ComponentError, RequestAdsError}, + http_cache::CachePolicy, + mars::{ad_request::AdPlacementRequest, ReportReason}, + AdsClientApiResult, MozAdsClientInner, MozAdsPlacementRequest, MozAdsPlacementRequestWithCount, +}; + +// Command dispatch enum for passing different instructions to the background worker thread. +// `RequestImageAds`, `RequestSpocAds`, `RequestTileAds` are prefetch mechanisms that query and load data into the local cache. +// `RecordClick`, `RecordImpression`, `ReportAd` are fire and forget mechanisms that do not load data. +// `Ping` is a synchronous command for internal use that triggers its inner channel when command resolves (eg: when the queue is empty). +pub enum DispatchCommand { + RequestImageAds { + image_ad_requests: Vec, + cache_policy: CachePolicy, + ohttp: bool, + flags: HashMap, + blocks: Vec, + }, + RequestSpocAds { + spoc_ad_requests: Vec, + cache_policy: CachePolicy, + ohttp: bool, + flags: HashMap, + blocks: Vec, + }, + RequestTileAds { + tile_ad_requests: Vec, + cache_policy: CachePolicy, + ohttp: bool, + flags: HashMap, + blocks: Vec, + }, + RecordClick { + url: Url, + ohttp: bool, + }, + RecordImpression { + url: Url, + ohttp: bool, + }, + ReportAd { + url: Url, + reason: ReportReason, + ohttp: bool, + }, + Ping(SyncSender<()>), +} + +impl DispatchCommand { + // Runs a dispatched command synchronously in it's thread. + // The dispatched command calls the corresponding `AdsClient` synchronous method, meaning that behavior between the two is shared. + // This includes telemetry calls, meaning that for a successful `RecordClick`, all of the following will get logged: + // - CommandDispatchedOperationEvent::RecordClick (on dispatch) + // - ClientOperationEvent::RecordClick (on `AdsClient` method success) + // - CommandProcessedOperationEvent::RecordClick (on process) + #[handle_error(ComponentError)] + pub fn run_command(self, ads_client_inner: &MozAdsClientInner) -> AdsClientApiResult<()> { + match self { + DispatchCommand::RequestImageAds { + image_ad_requests, + cache_policy, + flags, + ohttp, + blocks, + } => { + let mut inner = ads_client_inner.lock(); + if !image_ad_requests.is_empty() { + let image_ad_requests: Vec = + image_ad_requests.iter().map(|r| r.into()).collect(); + let image_response = inner + .request_image_ads( + image_ad_requests, + flags, + Some(cache_policy), + ohttp, + blocks, + ) + .map_err(ComponentError::RequestAds)?; + // TODO: Bulk insert + inner + .cache_ads( + image_response + .into_iter() + .map(|(k, v)| (k.into(), StorableAd::Image(v))) + .collect(), + ) + .map_err(RequestAdsError::from)?; + } + Ok(()) + } + // TODO: Can we modify these to be one call? + DispatchCommand::RequestSpocAds { + spoc_ad_requests, + cache_policy, + flags, + ohttp, + blocks, + } => { + let mut inner = ads_client_inner.lock(); + if !spoc_ad_requests.is_empty() { + let spoc_ad_requests: Vec = + spoc_ad_requests.iter().map(|r| r.into()).collect(); + let spoc_response = inner + .request_spoc_ads( + spoc_ad_requests, + flags, + Some(cache_policy), + ohttp, + blocks, + ) + .map_err(ComponentError::RequestAds)?; + inner + .cache_ads( + spoc_response + .into_iter() + .map(|(k, v)| (k.into(), StorableAd::Spoc(v))) + .collect(), + ) + .map_err(RequestAdsError::from)?; + } + Ok(()) + } + DispatchCommand::RequestTileAds { + tile_ad_requests, + cache_policy, + flags, + ohttp, + blocks, + } => { + let mut inner = ads_client_inner.lock(); + if !tile_ad_requests.is_empty() { + let tile_ad_requests: Vec = + tile_ad_requests.iter().map(|r| r.into()).collect(); + let tile_response = inner + .request_tile_ads( + tile_ad_requests, + flags, + Some(cache_policy), + ohttp, + blocks, + ) + .map_err(ComponentError::RequestAds)?; + inner + .cache_ads( + tile_response + .into_iter() + .map(|(k, v)| (k.into(), StorableAd::Tile(v))) + .collect(), + ) + .map_err(RequestAdsError::from)?; + } + Ok(()) + } + DispatchCommand::RecordClick { url, ohttp } => { + let inner = ads_client_inner.lock(); + inner + .record_click(url, ohttp) + .map_err(ComponentError::RecordClick)?; + Ok(()) + } + DispatchCommand::RecordImpression { url, ohttp } => { + let inner = ads_client_inner.lock(); + inner + .record_impression(url, ohttp) + .map_err(ComponentError::RecordImpression)?; + Ok(()) + } + DispatchCommand::ReportAd { url, ohttp, reason } => { + let inner = ads_client_inner.lock(); + inner + .report_ad(url, reason, ohttp) + .map_err(ComponentError::ReportAd)?; + Ok(()) + } + DispatchCommand::Ping(sender) => { + sender + .try_send(()) + .map_err(|err| BackgroundWorkerError::PongFailure(Box::new(err)))?; + Ok(()) + } + } + } +} From 791f0a6d2c5edb551fb1a7d9d2e68e95f27b5888 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Thu, 10 Sep 2026 13:11:47 -0700 Subject: [PATCH 02/25] fix: removes the FFI layer --- .../integration-tests/tests/mars_async.rs | 318 ------------------ components/ads-client/src/lib.rs | 100 +----- 2 files changed, 1 insertion(+), 417 deletions(-) delete mode 100644 components/ads-client/integration-tests/tests/mars_async.rs diff --git a/components/ads-client/integration-tests/tests/mars_async.rs b/components/ads-client/integration-tests/tests/mars_async.rs deleted file mode 100644 index 4ae8102d3ed..00000000000 --- a/components/ads-client/integration-tests/tests/mars_async.rs +++ /dev/null @@ -1,318 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public -* License, v. 2.0. If a copy of the MPL was not distributed with this -* file, You can obtain one at http://mozilla.org/MPL/2.0/. -*/ - -use ads_client::MozAdsIABContent; -use ads_client::MozAdsIABContentTaxonomy; -use ads_client::MozAdsPlacementRequestWithCount; -use ads_client::ads_store::PlacementId; -use ads_client::{ - MozAdsClient, MozAdsClientBuilder, MozAdsEnvironment, MozAdsPlacementRequest, - MozAdsRequestOptions, MozAdsTile, -}; -use std::sync::Arc; - -pub const TEST_TIMEOUT_DURATION: std::time::Duration = std::time::Duration::from_secs(10); - -fn init_backend() { - viaduct_hyper::viaduct_init_backend_hyper(); -} - -fn prod_client() -> ads_client::MozAdsClient { - Arc::new(MozAdsClientBuilder::new()) - .environment(MozAdsEnvironment::Prod) - .build() -} - -#[test] -#[ignore = "integration test: run manually with -- --ignored"] -fn test_contract_image_prod_async() { - init_backend(); - - // Prefetch - let placement_id = PlacementId::new("mock_billboard_1"); - let client = prod_client(); - let result = client.prefetch_ads( - vec![MozAdsPlacementRequest { - iab_content: None, - placement_id: placement_id.clone().into(), - }], - vec![], - vec![], - None, - ); - - assert!( - result.is_ok(), - "Image ad dispatch request failed: {:?}", - result.err() - ); - - // Ping (waits for queue to clear) - let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION)); - assert!(ping.is_ok(), "Ping failed: {:?}", ping.err()); - - // Query - let result = client.query_image_ads(placement_id); - assert!( - result.is_ok(), - "Querying for ads failed: {:?}", - result.err() - ); - let placements = result.unwrap(); - - assert!(placements.is_some()); -} - -#[test] -#[ignore = "integration test: run manually with -- --ignored"] -fn test_contract_image_with_categories_prod_async() { - init_backend(); - - // Prefetch - let placement_id = PlacementId::new("mock_billboard_1"); - let client = prod_client(); - let result = client.prefetch_ads( - vec![MozAdsPlacementRequest { - iab_content: Some(MozAdsIABContent { - category_ids: vec!["338".to_string()], - taxonomy: MozAdsIABContentTaxonomy::IAB3_0, - }), - placement_id: placement_id.clone().into(), - }], - vec![], - vec![], - Some(MozAdsRequestOptions { - flags: std::collections::HashMap::from([("contextual_placement".to_string(), true)]), - ..Default::default() - }), - ); - - assert!( - result.is_ok(), - "Image ad dispatch request failed: {:?}", - result.err() - ); - - // Ping (waits for queue to clear) - let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION)); - assert!(ping.is_ok(), "Ping failed: {:?}", ping.err()); - - // Query - let result = client.query_image_ads(placement_id); - assert!( - result.is_ok(), - "Querying for ads failed: {:?}", - result.err() - ); - - let placements = result.unwrap(); - assert!(placements.is_some()); - let ad = placements.unwrap(); - assert!(!ad.url.is_empty(), "destination url should be populated"); - assert!(!ad.image_url.is_empty(), "image url should be populated"); -} - -#[test] -#[ignore = "integration test: run manually with -- --ignored"] -fn test_contract_spoc_prod_async() { - init_backend(); - - // Prefetch - let placement_id = PlacementId::new("mock_spoc_1"); - let client = prod_client(); - let result = client.prefetch_ads( - vec![], - vec![MozAdsPlacementRequestWithCount { - count: 3, - iab_content: None, - placement_id: placement_id.clone().into(), - }], - vec![], - None, - ); - - assert!( - result.is_ok(), - "Spoc ad dispatch request failed: {:?}", - result.err() - ); - - // Ping (waits for queue to clear) - let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION)); - assert!(ping.is_ok(), "Ping failed: {:?}", ping.err()); - - // Query - let result = client.query_spoc_ads(placement_id); - assert!( - result.is_ok(), - "Querying for ads failed: {:?}", - result.err() - ); - let placements = result.unwrap(); - assert!(placements.is_some()); - assert!(placements.unwrap().len() == 3); -} - -#[test] -#[ignore = "integration test: run manually with -- --ignored"] -fn test_contract_tile_prod_async() { - init_backend(); - - // Prefetch - let placement_id = PlacementId::new("mock_tile_1"); - let client = prod_client(); - let result = client.prefetch_ads( - vec![], - vec![], - vec![MozAdsPlacementRequest { - iab_content: None, - placement_id: placement_id.clone().into(), - }], - None, - ); - - assert!( - result.is_ok(), - "Tile ad dispatch request failed: {:?}", - result.err() - ); - - // Ping (waits for queue to clear) - let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION)); - assert!(ping.is_ok(), "Ping failed: {:?}", ping.err()); - - // Query - let result = client.query_tile_ads(placement_id); - assert!( - result.is_ok(), - "Querying for ads failed: {:?}", - result.err() - ); - let placements = result.unwrap(); - - assert!(placements.is_some()); -} - -#[test] -#[ignore = "integration test: run manually with -- --ignored"] -fn test_contract_tile_ohttp_prod_async() { - init_backend(); - viaduct::ohttp::configure_ohttp_channel( - "ads-client".to_string(), - viaduct::ohttp::OhttpConfig { - relay_url: "https://mozilla-ohttp.fastly-edge.com/".to_string(), - gateway_host: "prod.ohttp-gateway.prod.webservices.mozgcp.net".to_string(), - }, - ) - .expect("OHTTP channel configuration should succeed"); - - // Prefetch - let placement_id = PlacementId::new("mock_tile_1"); - let client = prod_client(); - let result = client.prefetch_ads( - vec![], - vec![], - vec![MozAdsPlacementRequest { - iab_content: None, - placement_id: placement_id.clone().into(), - }], - Some(MozAdsRequestOptions { - ohttp: true, - ..Default::default() - }), - ); - - assert!( - result.is_ok(), - "Tile ad dispatch request failed: {:?}", - result.err() - ); - - // Ping (waits for queue to clear) - let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION)); - assert!(ping.is_ok(), "Ping failed: {:?}", ping.err()); - - // Query - let result = client.query_tile_ads(placement_id); - assert!( - result.is_ok(), - "Querying for ads failed: {:?}", - result.err() - ); - let placements = result.unwrap(); - - assert!( - placements.is_some(), - "OHTTP response should contain mock_tile_1" - ); -} - -#[test] -#[ignore = "integration test: run manually with -- --ignored"] -fn test_contract_multi_ad_type_prod_async() { - init_backend(); - - // Prefetch - let placement_image_id = PlacementId::new("mock_billboard_1"); - let placement_spoc_id = PlacementId::new("mock_spoc_1"); - let placement_tile_id = PlacementId::new("mock_tile_1"); - let client = prod_client(); - let result = client.prefetch_ads( - vec![MozAdsPlacementRequest { - iab_content: None, - placement_id: placement_image_id.clone().into(), - }], - vec![MozAdsPlacementRequestWithCount { - count: 4, - iab_content: None, - placement_id: placement_spoc_id.clone().into(), - }], - vec![MozAdsPlacementRequest { - iab_content: None, - placement_id: placement_tile_id.clone().into(), - }], - None, - ); - - assert!( - result.is_ok(), - "Image ad dispatch request failed: {:?}", - result.err() - ); - - // Ping (waits for queue to clear) - let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION)); - assert!(ping.is_ok(), "Ping failed: {:?}", ping.err()); - - // Query - let result = client.query_image_ads(placement_image_id); - assert!( - result.is_ok(), - "Querying for image ads failed: {:?}", - result.err() - ); - let placements = result.unwrap(); - assert!(placements.is_some()); - - let result = client.query_spoc_ads(placement_spoc_id); - assert!( - result.is_ok(), - "Querying for spoc ads failed: {:?}", - result.err() - ); - let placements = result.unwrap(); - assert!(placements.is_some()); - assert!(placements.unwrap().len() == 4); - - let result = client.query_tile_ads(placement_tile_id); - assert!( - result.is_ok(), - "Querying for ads failed: {:?}", - result.err() - ); - let placements = result.unwrap(); - - assert!(placements.is_some()); -} \ No newline at end of file diff --git a/components/ads-client/src/lib.rs b/components/ads-client/src/lib.rs index 63832b8bfd4..f5a0dc71078 100644 --- a/components/ads-client/src/lib.rs +++ b/components/ads-client/src/lib.rs @@ -31,8 +31,7 @@ pub mod worker; pub use ffi::*; use crate::{ - ads_store::{PlacementId, StorableAd}, - client::error::{BackgroundWorkerError, RequestAdsError}, + client::error::BackgroundWorkerError, ffi::telemetry::MozAdsTelemetryWrapper, shutdown::ShutdownReferences, worker::{command::DispatchCommand, AdsClientWorkerWrapper}, @@ -48,7 +47,6 @@ uniffi::custom_type!(AdsClientUrl, String, { try_lift: |val| Ok(AdsClientUrl::parse(&val)?), lower: |obj| obj.as_str().to_string(), }); -uniffi::custom_type!(PlacementId, String); pub type MozAdsClientInner = Arc>>; #[derive(uniffi::Object)] @@ -194,102 +192,6 @@ impl MozAdsClient { Ok(response.into_iter().map(|(k, v)| (k, v.into())).collect()) } - // TODO: Can we make this one request? - #[handle_error(ComponentError)] - #[uniffi::method(default(image_ad_requests = [], spoc_ad_requests = [], tile_ad_requests = [], options = None))] - pub fn prefetch_ads( - &self, - image_ad_requests: Vec, - spoc_ad_requests: Vec, - tile_ad_requests: Vec, - options: Option, - ) -> AdsClientApiResult<()> { - let options = options.unwrap_or_default(); - let flags = AdRequestFlags::from(&options); - let ohttp = options.ohttp; - let blocks = options.blocks.clone(); - let cache_policy: CachePolicy = options.into(); - - // Dispatch image requests - if !image_ad_requests.is_empty() { - self.worker.dispatch(DispatchCommand::RequestImageAds { - image_ad_requests, - ohttp, - cache_policy, - flags: flags.clone(), - blocks: blocks.clone(), - })?; - } - // Dispatch spoc requests - if !spoc_ad_requests.is_empty() { - self.worker.dispatch(DispatchCommand::RequestSpocAds { - spoc_ad_requests, - ohttp, - cache_policy, - flags: flags.clone(), - blocks: blocks.clone(), - })?; - } - - // Dispatch tiles requests - if !tile_ad_requests.is_empty() { - self.worker.dispatch(DispatchCommand::RequestTileAds { - tile_ad_requests, - ohttp, - cache_policy, - flags: flags.clone(), - blocks: blocks.clone(), - })?; - } - - Ok(()) - } - - #[handle_error(ComponentError)] - #[uniffi::method()] - pub fn query_image_ads( - &self, - placement_id: PlacementId, - ) -> AdsClientApiResult> { - let inner = self.inner.lock(); - let image_ad: Option = inner - .get_cached_ad(&placement_id) - .map_err(RequestAdsError::from)?; - Ok(image_ad - .and_then(|ad| ad.into_image()) - .map(|ad| ad.clone().into())) - } - - #[handle_error(ComponentError)] - #[uniffi::method()] - pub fn query_spoc_ads( - &self, - placement_id: PlacementId, - ) -> AdsClientApiResult>> { - let inner = self.inner.lock(); - let spoc_ads: Option = inner - .get_cached_ad(&placement_id) - .map_err(RequestAdsError::from)?; - Ok(spoc_ads - .and_then(|ad| ad.into_spocs()) - .map(|ad| ad.clone().into_iter().map(|x| x.into()).collect())) - } - - #[handle_error(ComponentError)] - #[uniffi::method()] - pub fn query_tile_ads( - &self, - placement_id: PlacementId, - ) -> AdsClientApiResult> { - let inner = self.inner.lock(); - let tile_ad: Option = inner - .get_cached_ad(&placement_id) - .map_err(RequestAdsError::from)?; - Ok(tile_ad - .and_then(|ad| ad.into_tile()) - .map(|ad| ad.clone().into())) - } - // Pings the background worker and waits for a response back, for use in tests. // Because the background worker is synchronous, this returns if the worker is empty, // making it useful for integration tests to wait until all tasks have completed. From ef7a00ad5725f4ddb5afd5deacb2db77732499ba Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Thu, 10 Sep 2026 15:32:09 -0700 Subject: [PATCH 03/25] feat: fixes, updates --- components/ads-client/src/ads_store.rs | 1 - components/ads-client/src/ffi.rs | 7 +++- components/ads-client/src/worker.rs | 7 ++++ components/ads-client/src/worker/command.rs | 38 +-------------------- 4 files changed, 14 insertions(+), 39 deletions(-) diff --git a/components/ads-client/src/ads_store.rs b/components/ads-client/src/ads_store.rs index 00b7861adf6..bf72b36982d 100644 --- a/components/ads-client/src/ads_store.rs +++ b/components/ads-client/src/ads_store.rs @@ -106,7 +106,6 @@ impl AdsStore { } pub fn store_ads(&self, ads: HashMap) -> Result<(), FetchAdsError> { - // TODO: The query can be rewritten to have multiple ads inserted in one for (placement_id, ad) in ads { self.holder.store_ad(&placement_id, ad)?; } diff --git a/components/ads-client/src/ffi.rs b/components/ads-client/src/ffi.rs index c17584a1238..5789b0c6c63 100644 --- a/components/ads-client/src/ffi.rs +++ b/components/ads-client/src/ffi.rs @@ -134,6 +134,7 @@ impl MozAdsClientBuilder { .take() .map(MozAdsTelemetryWrapper::new) .unwrap_or_else(MozAdsTelemetryWrapper::noop); + let store_set = inner.store_config.is_some(); let client_config = AdsClientConfig { cache_config: inner.cache_config.clone().map(Into::into), context_id_provider: inner @@ -148,7 +149,11 @@ impl MozAdsClientBuilder { let client = AdsClient::new(client_config); let shutdown_references = client.shutdown_references(); let inner = Arc::new(Mutex::new(client)); - let worker = worker::AdsClientWorkerWrapper::new(inner.clone()); + let worker = if store_set { + worker::AdsClientWorkerWrapper::new(inner.clone()) + } else { + worker::AdsClientWorkerWrapper::new_empty() + }; MozAdsClient { inner, shutdown_references, diff --git a/components/ads-client/src/worker.rs b/components/ads-client/src/worker.rs index 0538d7e99b9..712cec06bf4 100644 --- a/components/ads-client/src/worker.rs +++ b/components/ads-client/src/worker.rs @@ -27,6 +27,13 @@ impl AdsClientWorkerWrapper { } } + pub fn new_empty() -> AdsClientWorkerWrapper { + AdsClientWorkerWrapper { + _worker_thread: None, + worker_dispatch: None, + } + } + pub fn dispatch(&self, command: DispatchCommand) -> Result<(), ComponentError> { if let Some(worker_dispatch) = &self.worker_dispatch { worker_dispatch diff --git a/components/ads-client/src/worker/command.rs b/components/ads-client/src/worker/command.rs index a1bb1ebba4a..de7a12ca53a 100644 --- a/components/ads-client/src/worker/command.rs +++ b/components/ads-client/src/worker/command.rs @@ -1,13 +1,12 @@ use std::{collections::HashMap, sync::mpsc::SyncSender}; use error_support::handle_error; -use url::Url; use crate::{ ads_store::StorableAd, client::error::{BackgroundWorkerError, ComponentError, RequestAdsError}, http_cache::CachePolicy, - mars::{ad_request::AdPlacementRequest, ReportReason}, + mars::ad_request::AdPlacementRequest, AdsClientApiResult, MozAdsClientInner, MozAdsPlacementRequest, MozAdsPlacementRequestWithCount, }; @@ -37,19 +36,6 @@ pub enum DispatchCommand { flags: HashMap, blocks: Vec, }, - RecordClick { - url: Url, - ohttp: bool, - }, - RecordImpression { - url: Url, - ohttp: bool, - }, - ReportAd { - url: Url, - reason: ReportReason, - ohttp: bool, - }, Ping(SyncSender<()>), } @@ -83,7 +69,6 @@ impl DispatchCommand { blocks, ) .map_err(ComponentError::RequestAds)?; - // TODO: Bulk insert inner .cache_ads( image_response @@ -158,27 +143,6 @@ impl DispatchCommand { } Ok(()) } - DispatchCommand::RecordClick { url, ohttp } => { - let inner = ads_client_inner.lock(); - inner - .record_click(url, ohttp) - .map_err(ComponentError::RecordClick)?; - Ok(()) - } - DispatchCommand::RecordImpression { url, ohttp } => { - let inner = ads_client_inner.lock(); - inner - .record_impression(url, ohttp) - .map_err(ComponentError::RecordImpression)?; - Ok(()) - } - DispatchCommand::ReportAd { url, ohttp, reason } => { - let inner = ads_client_inner.lock(); - inner - .report_ad(url, reason, ohttp) - .map_err(ComponentError::ReportAd)?; - Ok(()) - } DispatchCommand::Ping(sender) => { sender .try_send(()) From b187cd4958c400f3c9a1f71d33f55b8ca63587c3 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Thu, 10 Sep 2026 15:35:41 -0700 Subject: [PATCH 04/25] fix: removed ping --- components/ads-client/src/lib.rs | 17 ----------------- components/ads-client/src/worker/command.rs | 12 +----------- 2 files changed, 1 insertion(+), 28 deletions(-) diff --git a/components/ads-client/src/lib.rs b/components/ads-client/src/lib.rs index f5a0dc71078..3a1e746b2ca 100644 --- a/components/ads-client/src/lib.rs +++ b/components/ads-client/src/lib.rs @@ -191,21 +191,4 @@ impl MozAdsClient { .map_err(ComponentError::RequestAds)?; Ok(response.into_iter().map(|(k, v)| (k, v.into())).collect()) } - - // Pings the background worker and waits for a response back, for use in tests. - // Because the background worker is synchronous, this returns if the worker is empty, - // making it useful for integration tests to wait until all tasks have completed. - #[handle_error(ComponentError)] - pub fn ping_background_worker(&self, timeout: Option) -> AdsClientApiResult<()> { - let (tx, rx) = mpsc::sync_channel(0); - self.worker.dispatch(DispatchCommand::Ping(tx))?; - - if let Some(timeout) = timeout { - rx.recv_timeout(timeout) - .map_err(BackgroundWorkerError::from)?; - } else { - rx.recv().map_err(|_| BackgroundWorkerError::WorkerClosed)?; - } - Ok(()) - } } diff --git a/components/ads-client/src/worker/command.rs b/components/ads-client/src/worker/command.rs index de7a12ca53a..de196407c3b 100644 --- a/components/ads-client/src/worker/command.rs +++ b/components/ads-client/src/worker/command.rs @@ -4,7 +4,7 @@ use error_support::handle_error; use crate::{ ads_store::StorableAd, - client::error::{BackgroundWorkerError, ComponentError, RequestAdsError}, + client::error::{ComponentError, RequestAdsError}, http_cache::CachePolicy, mars::ad_request::AdPlacementRequest, AdsClientApiResult, MozAdsClientInner, MozAdsPlacementRequest, MozAdsPlacementRequestWithCount, @@ -12,8 +12,6 @@ use crate::{ // Command dispatch enum for passing different instructions to the background worker thread. // `RequestImageAds`, `RequestSpocAds`, `RequestTileAds` are prefetch mechanisms that query and load data into the local cache. -// `RecordClick`, `RecordImpression`, `ReportAd` are fire and forget mechanisms that do not load data. -// `Ping` is a synchronous command for internal use that triggers its inner channel when command resolves (eg: when the queue is empty). pub enum DispatchCommand { RequestImageAds { image_ad_requests: Vec, @@ -36,7 +34,6 @@ pub enum DispatchCommand { flags: HashMap, blocks: Vec, }, - Ping(SyncSender<()>), } impl DispatchCommand { @@ -80,7 +77,6 @@ impl DispatchCommand { } Ok(()) } - // TODO: Can we modify these to be one call? DispatchCommand::RequestSpocAds { spoc_ad_requests, cache_policy, @@ -143,12 +139,6 @@ impl DispatchCommand { } Ok(()) } - DispatchCommand::Ping(sender) => { - sender - .try_send(()) - .map_err(|err| BackgroundWorkerError::PongFailure(Box::new(err)))?; - Ok(()) - } } } } From 0e4c7d53dbd43a863fc680b8b19e1e9e7a91796e Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Thu, 10 Sep 2026 16:00:33 -0700 Subject: [PATCH 05/25] fix: pong removal --- components/ads-client/src/client/error.rs | 3 --- components/ads-client/src/ffi.rs | 2 +- components/ads-client/src/lib.rs | 8 +++----- components/ads-client/src/worker/command.rs | 6 +----- 4 files changed, 5 insertions(+), 14 deletions(-) diff --git a/components/ads-client/src/client/error.rs b/components/ads-client/src/client/error.rs index 913a36792c9..cf5371653a6 100644 --- a/components/ads-client/src/client/error.rs +++ b/components/ads-client/src/client/error.rs @@ -39,9 +39,6 @@ pub enum RequestAdsError { #[derive(Debug, thiserror::Error)] pub enum BackgroundWorkerError { - #[error("Error sending pong back from background worker")] - PongFailure(Box>), - #[error("Error requesting new ads from the background worker: worker closed")] WorkerClosed, diff --git a/components/ads-client/src/ffi.rs b/components/ads-client/src/ffi.rs index 5789b0c6c63..49b462515a4 100644 --- a/components/ads-client/src/ffi.rs +++ b/components/ads-client/src/ffi.rs @@ -157,7 +157,7 @@ impl MozAdsClientBuilder { MozAdsClient { inner, shutdown_references, - worker, + _worker: worker } } diff --git a/components/ads-client/src/lib.rs b/components/ads-client/src/lib.rs index 3a1e746b2ca..7d387d82ff9 100644 --- a/components/ads-client/src/lib.rs +++ b/components/ads-client/src/lib.rs @@ -5,8 +5,7 @@ use std::{ collections::HashMap, - sync::{mpsc, Arc}, - time::Duration, + sync::Arc, }; use client::error::ComponentError; @@ -31,10 +30,9 @@ pub mod worker; pub use ffi::*; use crate::{ - client::error::BackgroundWorkerError, ffi::telemetry::MozAdsTelemetryWrapper, shutdown::ShutdownReferences, - worker::{command::DispatchCommand, AdsClientWorkerWrapper}, + worker::AdsClientWorkerWrapper, }; #[cfg(test)] @@ -53,7 +51,7 @@ pub type MozAdsClientInner = Arc>>; pub struct MozAdsClient { inner: MozAdsClientInner, shutdown_references: ShutdownReferences, - worker: AdsClientWorkerWrapper, + _worker: AdsClientWorkerWrapper, } #[uniffi::export] diff --git a/components/ads-client/src/worker/command.rs b/components/ads-client/src/worker/command.rs index de196407c3b..a443fe2ce6b 100644 --- a/components/ads-client/src/worker/command.rs +++ b/components/ads-client/src/worker/command.rs @@ -1,4 +1,4 @@ -use std::{collections::HashMap, sync::mpsc::SyncSender}; +use std::collections::HashMap; use error_support::handle_error; @@ -39,10 +39,6 @@ pub enum DispatchCommand { impl DispatchCommand { // Runs a dispatched command synchronously in it's thread. // The dispatched command calls the corresponding `AdsClient` synchronous method, meaning that behavior between the two is shared. - // This includes telemetry calls, meaning that for a successful `RecordClick`, all of the following will get logged: - // - CommandDispatchedOperationEvent::RecordClick (on dispatch) - // - ClientOperationEvent::RecordClick (on `AdsClient` method success) - // - CommandProcessedOperationEvent::RecordClick (on process) #[handle_error(ComponentError)] pub fn run_command(self, ads_client_inner: &MozAdsClientInner) -> AdsClientApiResult<()> { match self { From 59fa8318a23e86f1872e7e9ac3d2c3079e10ac81 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Thu, 10 Sep 2026 16:36:04 -0700 Subject: [PATCH 06/25] fix: adds shutdown --- components/ads-client/src/ads_store.rs | 27 ----------------------- components/ads-client/src/client.rs | 8 +++---- components/ads-client/src/client/error.rs | 12 +++++----- components/ads-client/src/ffi.rs | 2 +- components/ads-client/src/lib.rs | 8 ++----- components/ads-client/src/mars/error.rs | 3 +++ components/ads-client/src/worker.rs | 2 +- 7 files changed, 17 insertions(+), 45 deletions(-) diff --git a/components/ads-client/src/ads_store.rs b/components/ads-client/src/ads_store.rs index bf72b36982d..d8db0b7a5d1 100644 --- a/components/ads-client/src/ads_store.rs +++ b/components/ads-client/src/ads_store.rs @@ -49,33 +49,6 @@ pub enum StorableAd { Tile(AdTile), } -impl StorableAd { - // TODO: Should these be result? - pub fn into_image(self) -> Option { - if let StorableAd::Image(image) = self { - Some(image) - } else { - None - } - } - - pub fn into_spocs(self) -> Option> { - if let StorableAd::Spoc(spocs) = self { - Some(spocs) - } else { - None - } - } - - pub fn into_tile(self) -> Option { - if let StorableAd::Tile(tile) = self { - Some(tile) - } else { - None - } - } -} - pub struct AdsStore { holder: AdsStoreHolder, #[allow(dead_code)] diff --git a/components/ads-client/src/client.rs b/components/ads-client/src/client.rs index 739892f6fc5..afad1f43e13 100644 --- a/components/ads-client/src/client.rs +++ b/components/ads-client/src/client.rs @@ -123,9 +123,10 @@ where let ads_store = self.ads_store.lock(); if let Some(ads_store) = ads_store.as_ref() { ads_store.store_ads(ads)?; + Ok(()) + } else { + Err(FetchAdsError::SqliteShutdown) } - // TODO: Should we error if no ads store? - Ok(()) } pub fn get_cached_ad( @@ -136,8 +137,7 @@ where if let Some(ads_store) = ads_store.as_ref() { Ok(ads_store.lookup(placement_id)?) } else { - // TODO: Should we error if no ads store? - Ok(None) + Err(FetchAdsError::SqliteShutdown) } } diff --git a/components/ads-client/src/client/error.rs b/components/ads-client/src/client/error.rs index cf5371653a6..ea1b96f24a7 100644 --- a/components/ads-client/src/client/error.rs +++ b/components/ads-client/src/client/error.rs @@ -40,20 +40,20 @@ pub enum RequestAdsError { #[derive(Debug, thiserror::Error)] pub enum BackgroundWorkerError { #[error("Error requesting new ads from the background worker: worker closed")] - WorkerClosed, + Closed, #[error("Error requesting new ads from the background worker: worker full")] - WorkerFull, + Full, - #[error("Worker timed out waiting for response: {0}")] - WorkerTimedOut(#[from] RecvTimeoutError), + #[error("Background worker timed out waiting for response: {0}")] + TimedOut(#[from] RecvTimeoutError), } impl From> for BackgroundWorkerError { fn from(value: TrySendError) -> Self { match value { - TrySendError::Disconnected(_) => BackgroundWorkerError::WorkerClosed, - TrySendError::Full(_) => BackgroundWorkerError::WorkerFull, + TrySendError::Disconnected(_) => BackgroundWorkerError::Closed, + TrySendError::Full(_) => BackgroundWorkerError::Full, } } } diff --git a/components/ads-client/src/ffi.rs b/components/ads-client/src/ffi.rs index 49b462515a4..bfec99326e9 100644 --- a/components/ads-client/src/ffi.rs +++ b/components/ads-client/src/ffi.rs @@ -157,7 +157,7 @@ impl MozAdsClientBuilder { MozAdsClient { inner, shutdown_references, - _worker: worker + _worker: worker, } } diff --git a/components/ads-client/src/lib.rs b/components/ads-client/src/lib.rs index 7d387d82ff9..11dc7789d2b 100644 --- a/components/ads-client/src/lib.rs +++ b/components/ads-client/src/lib.rs @@ -3,10 +3,7 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -use std::{ - collections::HashMap, - sync::Arc, -}; +use std::{collections::HashMap, sync::Arc}; use client::error::ComponentError; use error_support::handle_error; @@ -30,8 +27,7 @@ pub mod worker; pub use ffi::*; use crate::{ - ffi::telemetry::MozAdsTelemetryWrapper, - shutdown::ShutdownReferences, + ffi::telemetry::MozAdsTelemetryWrapper, shutdown::ShutdownReferences, worker::AdsClientWorkerWrapper, }; diff --git a/components/ads-client/src/mars/error.rs b/components/ads-client/src/mars/error.rs index ce87833924c..22f9f0573dd 100644 --- a/components/ads-client/src/mars/error.rs +++ b/components/ads-client/src/mars/error.rs @@ -46,6 +46,9 @@ pub enum FetchAdsError { #[error("Internal database error: {0}")] Sqlite(#[from] rusqlite::Error), + #[error("Internal database error: database shut down or uninitialized")] + SqliteShutdown, + #[error("Error sending request: {0}")] Request(#[from] viaduct::ViaductError), diff --git a/components/ads-client/src/worker.rs b/components/ads-client/src/worker.rs index 712cec06bf4..6af30746715 100644 --- a/components/ads-client/src/worker.rs +++ b/components/ads-client/src/worker.rs @@ -42,7 +42,7 @@ impl AdsClientWorkerWrapper { Ok(()) } else { - Err(BackgroundWorkerError::WorkerClosed.into()) + Err(BackgroundWorkerError::Closed.into()) } } } From 93f7d7dbf8e696195f50b2532317ad424fc1f0b7 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Thu, 10 Sep 2026 17:29:17 -0700 Subject: [PATCH 07/25] fix: buffer --- components/ads-client/src/ffi.rs | 7 ++++++- components/ads-client/src/worker.rs | 17 +++++++++++++---- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/components/ads-client/src/ffi.rs b/components/ads-client/src/ffi.rs index bfec99326e9..924991ab3a3 100644 --- a/components/ads-client/src/ffi.rs +++ b/components/ads-client/src/ffi.rs @@ -135,6 +135,10 @@ impl MozAdsClientBuilder { .map(MozAdsTelemetryWrapper::new) .unwrap_or_else(MozAdsTelemetryWrapper::noop); let store_set = inner.store_config.is_some(); + let worker_buffer_size = inner + .store_config + .as_ref() + .and_then(|x| x.worker_buffer_size); let client_config = AdsClientConfig { cache_config: inner.cache_config.clone().map(Into::into), context_id_provider: inner @@ -150,7 +154,7 @@ impl MozAdsClientBuilder { let shutdown_references = client.shutdown_references(); let inner = Arc::new(Mutex::new(client)); let worker = if store_set { - worker::AdsClientWorkerWrapper::new(inner.clone()) + worker::AdsClientWorkerWrapper::new(inner.clone(), worker_buffer_size) } else { worker::AdsClientWorkerWrapper::new_empty() }; @@ -219,6 +223,7 @@ pub struct MozAdsCacheConfig { #[derive(Clone, uniffi::Record)] pub struct MozAdsStoreConfig { pub db_path: String, + pub worker_buffer_size: Option, } #[derive(Debug, PartialEq, uniffi::Record)] diff --git a/components/ads-client/src/worker.rs b/components/ads-client/src/worker.rs index 6af30746715..d01ef1e2065 100644 --- a/components/ads-client/src/worker.rs +++ b/components/ads-client/src/worker.rs @@ -10,7 +10,8 @@ use std::{ pub mod command; -pub const ADS_CLIENT_WORKER_CHANNEL_BUFFER_SIZE: usize = 1000; +// This is a somewhat arbitrary default value that is overridable. +pub const ADS_CLIENT_WORKER_CHANNEL_BUFFER_SIZE_DEFAULT: usize = 10000; pub const ADS_CLIENT_WORKER_THREAD_NAME: &str = "ads-client.worker"; pub struct AdsClientWorkerWrapper { @@ -19,8 +20,13 @@ pub struct AdsClientWorkerWrapper { } impl AdsClientWorkerWrapper { - pub fn new(inner: MozAdsClientInner) -> AdsClientWorkerWrapper { - let (worker_dispatch, worker_thread) = Option::unzip(build_worker_thread(inner.clone())); + pub fn new( + inner: MozAdsClientInner, + worker_buffer_size: Option, + ) -> AdsClientWorkerWrapper { + let worker_buffer_size = worker_buffer_size.and_then(|x| usize::try_from(x).ok()); + let (worker_dispatch, worker_thread) = + Option::unzip(build_worker_thread(inner.clone(), worker_buffer_size)); AdsClientWorkerWrapper { _worker_thread: worker_thread, worker_dispatch, @@ -51,8 +57,11 @@ impl AdsClientWorkerWrapper { // Returns None if thread fails to build. pub fn build_worker_thread( inner_client: MozAdsClientInner, + max_channel_size: Option, ) -> Option<(SyncSender, JoinHandle<()>)> { - let (tx, rx) = mpsc::sync_channel(ADS_CLIENT_WORKER_CHANNEL_BUFFER_SIZE); + let (tx, rx) = mpsc::sync_channel( + max_channel_size.unwrap_or(ADS_CLIENT_WORKER_CHANNEL_BUFFER_SIZE_DEFAULT), + ); let worker_thread_handle = std::thread::Builder::new() .name(ADS_CLIENT_WORKER_THREAD_NAME.to_string()) .spawn(move || crate::worker::worker(inner_client, rx)).inspect_err(|err| { From 231d4cddae3ae900eb1bee4d631b11166fc901b0 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Thu, 10 Sep 2026 17:40:20 -0700 Subject: [PATCH 08/25] fix: changelog --- CHANGELOG.md | 6 ++++++ components/ads-client/src/worker.rs | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4850659024f..6e6aba952b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ [Full Changelog](In progress) +## ✨ What's Changed ✨ + +### Ads-Client + +- Adds a background worker to allow for future fire-and-forget logic. + # v157.0 (_2026-09-10_) ## ✨ What's Changed ✨ diff --git a/components/ads-client/src/worker.rs b/components/ads-client/src/worker.rs index d01ef1e2065..039c84e2290 100644 --- a/components/ads-client/src/worker.rs +++ b/components/ads-client/src/worker.rs @@ -10,7 +10,7 @@ use std::{ pub mod command; -// This is a somewhat arbitrary default value that is overridable. +// This is a somewhat arbitrary default value that is overridable. pub const ADS_CLIENT_WORKER_CHANNEL_BUFFER_SIZE_DEFAULT: usize = 10000; pub const ADS_CLIENT_WORKER_THREAD_NAME: &str = "ads-client.worker"; From df0fcac0f578a72a83f7b0dfd5ac849d63bf5100 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Fri, 11 Sep 2026 17:45:26 -0700 Subject: [PATCH 09/25] fix: Revisions --- CHANGELOG.md | 6 --- components/ads-client/src/ads_store.rs | 26 +++++++++++ components/ads-client/src/client.rs | 50 ++++++++++++++++++--- components/ads-client/src/ffi.rs | 4 +- components/ads-client/src/lib.rs | 5 +-- components/ads-client/src/worker.rs | 15 +++---- components/ads-client/src/worker/command.rs | 6 +-- 7 files changed, 82 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ca50530a873..77ae367bc02 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,12 +2,6 @@ [Full Changelog](In progress) -## ✨ What's Changed ✨ - -### Ads-Client - -- Adds a background worker to allow for future fire-and-forget logic. - ### Glean - Updated to v70.0.0 ([#7598](https://github.com/mozilla/application-services/pull/7598)) diff --git a/components/ads-client/src/ads_store.rs b/components/ads-client/src/ads_store.rs index d8db0b7a5d1..17ecf9fa891 100644 --- a/components/ads-client/src/ads_store.rs +++ b/components/ads-client/src/ads_store.rs @@ -49,6 +49,32 @@ pub enum StorableAd { Tile(AdTile), } +impl StorableAd { + pub fn into_image(self) -> Option { + if let StorableAd::Image(image) = self { + Some(image) + } else { + None + } + } + + pub fn into_spocs(self) -> Option> { + if let StorableAd::Spoc(spocs) = self { + Some(spocs) + } else { + None + } + } + + pub fn into_tile(self) -> Option { + if let StorableAd::Tile(tile) = self { + Some(tile) + } else { + None + } + } +} + pub struct AdsStore { holder: AdsStoreHolder, #[allow(dead_code)] diff --git a/components/ads-client/src/client.rs b/components/ads-client/src/client.rs index afad1f43e13..94a9d91d31f 100644 --- a/components/ads-client/src/client.rs +++ b/components/ads-client/src/client.rs @@ -116,7 +116,7 @@ where self.client.clear_cache() } - pub fn cache_ads( + pub fn store_ads( &mut self, ads: HashMap, ) -> Result<(), FetchAdsError> { @@ -129,15 +129,51 @@ where } } - pub fn get_cached_ad( - &self, - placement_id: &PlacementId, - ) -> Result, FetchAdsError> { + pub fn get_stored_ad_images(&self, placement_id: &PlacementId) -> Option { let ads_store = self.ads_store.lock(); if let Some(ads_store) = ads_store.as_ref() { - Ok(ads_store.lookup(placement_id)?) + match ads_store.lookup(placement_id) { + Ok(ad) => ad.and_then(|ad| ad.into_image()), + Err(_) => { + // TODO: Telemetry should return an error here (eg: some internal sqlite error) + None + } + } } else { - Err(FetchAdsError::SqliteShutdown) + // TODO: Telemetry should be added here for the database being shut down. + None + } + } + + pub fn get_stored_ad_spocs(&self, placement_id: &PlacementId) -> Option> { + let ads_store = self.ads_store.lock(); + if let Some(ads_store) = ads_store.as_ref() { + match ads_store.lookup(placement_id) { + Ok(ad) => ad.and_then(|ad| ad.into_spocs()), + Err(_) => { + // TODO: Telemetry should return an error here (eg: some internal sqlite error) + None + } + } + } else { + // TODO: Telemetry should be added here for the database being shut down. + None + } + } + + pub fn get_stored_ad_tile(&self, placement_id: &PlacementId) -> Option { + let ads_store = self.ads_store.lock(); + if let Some(ads_store) = ads_store.as_ref() { + match ads_store.lookup(placement_id) { + Ok(ad) => ad.and_then(|ad| ad.into_tile()), + Err(_) => { + // TODO: Telemetry should return an error here (eg: some internal sqlite error) + None + } + } + } else { + // TODO: Telemetry should be added here for the database being shut down. + None } } diff --git a/components/ads-client/src/ffi.rs b/components/ads-client/src/ffi.rs index 924991ab3a3..8e8120069b8 100644 --- a/components/ads-client/src/ffi.rs +++ b/components/ads-client/src/ffi.rs @@ -154,9 +154,9 @@ impl MozAdsClientBuilder { let shutdown_references = client.shutdown_references(); let inner = Arc::new(Mutex::new(client)); let worker = if store_set { - worker::AdsClientWorkerWrapper::new(inner.clone(), worker_buffer_size) + worker::BackgroundWorker::new(inner.clone(), worker_buffer_size) } else { - worker::AdsClientWorkerWrapper::new_empty() + worker::BackgroundWorker::new_empty() }; MozAdsClient { inner, diff --git a/components/ads-client/src/lib.rs b/components/ads-client/src/lib.rs index 11dc7789d2b..a1141d63d63 100644 --- a/components/ads-client/src/lib.rs +++ b/components/ads-client/src/lib.rs @@ -27,8 +27,7 @@ pub mod worker; pub use ffi::*; use crate::{ - ffi::telemetry::MozAdsTelemetryWrapper, shutdown::ShutdownReferences, - worker::AdsClientWorkerWrapper, + ffi::telemetry::MozAdsTelemetryWrapper, shutdown::ShutdownReferences, worker::BackgroundWorker, }; #[cfg(test)] @@ -47,7 +46,7 @@ pub type MozAdsClientInner = Arc>>; pub struct MozAdsClient { inner: MozAdsClientInner, shutdown_references: ShutdownReferences, - _worker: AdsClientWorkerWrapper, + _worker: BackgroundWorker, } #[uniffi::export] diff --git a/components/ads-client/src/worker.rs b/components/ads-client/src/worker.rs index 039c84e2290..1de19b25d4f 100644 --- a/components/ads-client/src/worker.rs +++ b/components/ads-client/src/worker.rs @@ -14,27 +14,24 @@ pub mod command; pub const ADS_CLIENT_WORKER_CHANNEL_BUFFER_SIZE_DEFAULT: usize = 10000; pub const ADS_CLIENT_WORKER_THREAD_NAME: &str = "ads-client.worker"; -pub struct AdsClientWorkerWrapper { +pub struct BackgroundWorker { _worker_thread: Option>, worker_dispatch: Option>, } -impl AdsClientWorkerWrapper { - pub fn new( - inner: MozAdsClientInner, - worker_buffer_size: Option, - ) -> AdsClientWorkerWrapper { +impl BackgroundWorker { + pub fn new(inner: MozAdsClientInner, worker_buffer_size: Option) -> BackgroundWorker { let worker_buffer_size = worker_buffer_size.and_then(|x| usize::try_from(x).ok()); let (worker_dispatch, worker_thread) = Option::unzip(build_worker_thread(inner.clone(), worker_buffer_size)); - AdsClientWorkerWrapper { + BackgroundWorker { _worker_thread: worker_thread, worker_dispatch, } } - pub fn new_empty() -> AdsClientWorkerWrapper { - AdsClientWorkerWrapper { + pub fn new_empty() -> BackgroundWorker { + BackgroundWorker { _worker_thread: None, worker_dispatch: None, } diff --git a/components/ads-client/src/worker/command.rs b/components/ads-client/src/worker/command.rs index a443fe2ce6b..33783a2d6dd 100644 --- a/components/ads-client/src/worker/command.rs +++ b/components/ads-client/src/worker/command.rs @@ -63,7 +63,7 @@ impl DispatchCommand { ) .map_err(ComponentError::RequestAds)?; inner - .cache_ads( + .store_ads( image_response .into_iter() .map(|(k, v)| (k.into(), StorableAd::Image(v))) @@ -94,7 +94,7 @@ impl DispatchCommand { ) .map_err(ComponentError::RequestAds)?; inner - .cache_ads( + .store_ads( spoc_response .into_iter() .map(|(k, v)| (k.into(), StorableAd::Spoc(v))) @@ -125,7 +125,7 @@ impl DispatchCommand { ) .map_err(ComponentError::RequestAds)?; inner - .cache_ads( + .store_ads( tile_response .into_iter() .map(|(k, v)| (k.into(), StorableAd::Tile(v))) From 141f1d50c1b7cce11dd48f5beb0573bbe95b9509 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Tue, 15 Sep 2026 15:10:29 -0700 Subject: [PATCH 10/25] fix: updated with cfg --- components/ads-client/src/client.rs | 8 +++++++- components/ads-client/src/client/error.rs | 12 ++++++------ components/ads-client/src/ffi.rs | 8 +++++++- components/ads-client/src/lib.rs | 8 +++++--- 4 files changed, 25 insertions(+), 11 deletions(-) diff --git a/components/ads-client/src/client.rs b/components/ads-client/src/client.rs index da09ade8fe3..8c453fedbe3 100644 --- a/components/ads-client/src/client.rs +++ b/components/ads-client/src/client.rs @@ -9,7 +9,9 @@ use crate::common::bytesize::ByteSize; use crate::http_cache::{CachePolicy, HttpCache}; use crate::mars::ad_request::{AdPlacementRequest, AdRequestFlags}; use crate::mars::ad_response::{AdImage, AdResponse, AdResponseValue, AdSpoc, AdTile}; -use crate::mars::error::{FetchAdsError, RecordClickError, RecordImpressionError, ReportAdError}; +#[cfg(feature = "stateful")] +use crate::mars::error::FetchAdsError; +use crate::mars::error::{RecordClickError, RecordImpressionError, ReportAdError}; use crate::mars::{MARSClient, ReportReason}; #[cfg(feature = "stateful")] use crate::shutdown::AdsStoreShutdown; @@ -111,6 +113,7 @@ where self.client.clear_cache() } + #[cfg(feature = "stateful")] pub fn store_ads( &mut self, ads: HashMap, @@ -124,6 +127,7 @@ where } } + #[cfg(feature = "stateful")] pub fn get_stored_ad_images(&self, placement_id: &PlacementId) -> Option { let ads_store = self.ads_store.lock(); if let Some(ads_store) = ads_store.as_ref() { @@ -140,6 +144,7 @@ where } } + #[cfg(feature = "stateful")] pub fn get_stored_ad_spocs(&self, placement_id: &PlacementId) -> Option> { let ads_store = self.ads_store.lock(); if let Some(ads_store) = ads_store.as_ref() { @@ -156,6 +161,7 @@ where } } + #[cfg(feature = "stateful")] pub fn get_stored_ad_tile(&self, placement_id: &PlacementId) -> Option { let ads_store = self.ads_store.lock(); if let Some(ads_store) = ads_store.as_ref() { diff --git a/components/ads-client/src/client/error.rs b/components/ads-client/src/client/error.rs index ea1b96f24a7..9067f1bf9c5 100644 --- a/components/ads-client/src/client/error.rs +++ b/components/ads-client/src/client/error.rs @@ -3,15 +3,13 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -use std::sync::mpsc::{RecvTimeoutError, TrySendError}; - -use crate::{ - mars::error::{FetchAdsError, RecordClickError, RecordImpressionError, ReportAdError}, - worker::command, -}; +use crate::mars::error::{FetchAdsError, RecordClickError, RecordImpressionError, ReportAdError}; +#[cfg(feature = "stateful")] +use crate::worker::command; #[derive(Debug, thiserror::Error)] pub enum ComponentError { + #[cfg(feature = "stateful")] #[error("Error requesting ads from worker: {0}")] BackgroundWorker(#[from] BackgroundWorkerError), @@ -37,6 +35,7 @@ pub enum RequestAdsError { FetchAds(#[from] FetchAdsError), } +#[cfg(feature = "stateful")] #[derive(Debug, thiserror::Error)] pub enum BackgroundWorkerError { #[error("Error requesting new ads from the background worker: worker closed")] @@ -49,6 +48,7 @@ pub enum BackgroundWorkerError { TimedOut(#[from] RecvTimeoutError), } +#[cfg(feature = "stateful")] impl From> for BackgroundWorkerError { fn from(value: TrySendError) -> Self { match value { diff --git a/components/ads-client/src/ffi.rs b/components/ads-client/src/ffi.rs index d956be28bbb..e75e1ff5744 100644 --- a/components/ads-client/src/ffi.rs +++ b/components/ads-client/src/ffi.rs @@ -20,8 +20,10 @@ use crate::mars::ad_response::{ }; use crate::mars::Environment; use crate::mars::ReportReason; +#[cfg(feature = "stateful")] +use crate::worker; use crate::AdsClientUrl; -use crate::{worker, MozAdsClient}; +use crate::MozAdsClient; use parking_lot::Mutex; use std::collections::HashMap; use std::sync::Arc; @@ -107,7 +109,9 @@ impl MozAdsClientBuilder { .take() .map(MozAdsTelemetryWrapper::new) .unwrap_or_else(MozAdsTelemetryWrapper::noop); + #[cfg(feature = "stateful")] let store_set = inner.store_config.is_some(); + #[cfg(feature = "stateful")] let worker_buffer_size = inner .store_config .as_ref() @@ -122,6 +126,7 @@ impl MozAdsClientBuilder { let client = AdsClient::new(client_config); let shutdown_references = client.shutdown_references(); let inner = Arc::new(Mutex::new(client)); + #[cfg(feature = "stateful")] let worker = if store_set { worker::BackgroundWorker::new(inner.clone(), worker_buffer_size) } else { @@ -130,6 +135,7 @@ impl MozAdsClientBuilder { MozAdsClient { inner, shutdown_references, + #[cfg(feature = "stateful")] _worker: worker, } } diff --git a/components/ads-client/src/lib.rs b/components/ads-client/src/lib.rs index 733a7705385..587fd3616a5 100644 --- a/components/ads-client/src/lib.rs +++ b/components/ads-client/src/lib.rs @@ -23,13 +23,14 @@ pub mod http_cache; mod mars; pub mod shutdown; pub mod telemetry; +#[cfg(feature = "stateful")] pub mod worker; pub use ffi::*; -use crate::{ - ffi::telemetry::MozAdsTelemetryWrapper, shutdown::ShutdownReferences, worker::BackgroundWorker, -}; +#[cfg(feature = "stateful")] +use crate::worker::BackgroundWorker; +use crate::{ffi::telemetry::MozAdsTelemetryWrapper, shutdown::ShutdownReferences}; #[cfg(test)] mod test_utils; @@ -47,6 +48,7 @@ pub type MozAdsClientInner = Arc>>; pub struct MozAdsClient { inner: MozAdsClientInner, shutdown_references: ShutdownReferences, + #[cfg(feature = "stateful")] _worker: BackgroundWorker, } From 46b0abe29c414a07755cf2d2648f21812f9c63b8 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Tue, 15 Sep 2026 15:13:49 -0700 Subject: [PATCH 11/25] fix: fmt clippy --- components/ads-client/src/client/error.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/components/ads-client/src/client/error.rs b/components/ads-client/src/client/error.rs index 9067f1bf9c5..e6e648ce9c1 100644 --- a/components/ads-client/src/client/error.rs +++ b/components/ads-client/src/client/error.rs @@ -3,6 +3,9 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ +#[cfg(feature = "stateful")] +use std::sync::mpsc::{RecvTimeoutError, TrySendError}; + use crate::mars::error::{FetchAdsError, RecordClickError, RecordImpressionError, ReportAdError}; #[cfg(feature = "stateful")] use crate::worker::command; From d02d8a136cf2fa5298172ad39b92cd6e4a9ae6b3 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Thu, 17 Sep 2026 10:09:17 -0700 Subject: [PATCH 12/25] fix: reorganizes background worker part --- components/ads-client/src/worker.rs | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/components/ads-client/src/worker.rs b/components/ads-client/src/worker.rs index 1de19b25d4f..e679d572cae 100644 --- a/components/ads-client/src/worker.rs +++ b/components/ads-client/src/worker.rs @@ -20,13 +20,27 @@ pub struct BackgroundWorker { } impl BackgroundWorker { - pub fn new(inner: MozAdsClientInner, worker_buffer_size: Option) -> BackgroundWorker { + pub fn new( + inner_client: MozAdsClientInner, + worker_buffer_size: Option, + ) -> BackgroundWorker { let worker_buffer_size = worker_buffer_size.and_then(|x| usize::try_from(x).ok()); - let (worker_dispatch, worker_thread) = - Option::unzip(build_worker_thread(inner.clone(), worker_buffer_size)); + + let (tx, rx) = mpsc::sync_channel( + worker_buffer_size.unwrap_or(ADS_CLIENT_WORKER_CHANNEL_BUFFER_SIZE_DEFAULT), + ); + let Some(worker_thread) = std::thread::Builder::new() + .name(ADS_CLIENT_WORKER_THREAD_NAME.to_string()) + .spawn(move || crate::worker::worker(inner_client, rx)).inspect_err(|err| { + error_support::error!("Failed to create ads-client worker thread `{ADS_CLIENT_WORKER_THREAD_NAME}` with: {err}") + }).ok() + else { + return BackgroundWorker { _worker_thread: None, worker_dispatch: None } + }; + BackgroundWorker { - _worker_thread: worker_thread, - worker_dispatch, + _worker_thread: Some(worker_thread), + worker_dispatch: Some(tx), } } From 1084aa51e22fc5cdd5ce2f411a612b09287f17d0 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Fri, 25 Sep 2026 11:08:06 -0700 Subject: [PATCH 13/25] fix: update to shrink PR size and only use RequestQueue --- components/ads-client/src/client/error.rs | 21 --- components/ads-client/src/ffi.rs | 7 +- components/ads-client/src/lib.rs | 1 - components/ads-client/src/request.rs | 22 +-- components/ads-client/src/worker.rs | 128 ++++++++++++------ components/ads-client/src/worker/command.rs | 140 -------------------- 6 files changed, 88 insertions(+), 231 deletions(-) delete mode 100644 components/ads-client/src/worker/command.rs diff --git a/components/ads-client/src/client/error.rs b/components/ads-client/src/client/error.rs index e6e648ce9c1..220be09eca0 100644 --- a/components/ads-client/src/client/error.rs +++ b/components/ads-client/src/client/error.rs @@ -3,12 +3,7 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -#[cfg(feature = "stateful")] -use std::sync::mpsc::{RecvTimeoutError, TrySendError}; - use crate::mars::error::{FetchAdsError, RecordClickError, RecordImpressionError, ReportAdError}; -#[cfg(feature = "stateful")] -use crate::worker::command; #[derive(Debug, thiserror::Error)] pub enum ComponentError { @@ -43,20 +38,4 @@ pub enum RequestAdsError { pub enum BackgroundWorkerError { #[error("Error requesting new ads from the background worker: worker closed")] Closed, - - #[error("Error requesting new ads from the background worker: worker full")] - Full, - - #[error("Background worker timed out waiting for response: {0}")] - TimedOut(#[from] RecvTimeoutError), -} - -#[cfg(feature = "stateful")] -impl From> for BackgroundWorkerError { - fn from(value: TrySendError) -> Self { - match value { - TrySendError::Disconnected(_) => BackgroundWorkerError::Closed, - TrySendError::Full(_) => BackgroundWorkerError::Full, - } - } } diff --git a/components/ads-client/src/ffi.rs b/components/ads-client/src/ffi.rs index 8d9774df2b0..6a1a3523216 100644 --- a/components/ads-client/src/ffi.rs +++ b/components/ads-client/src/ffi.rs @@ -110,11 +110,6 @@ impl MozAdsClientBuilder { .unwrap_or_else(MozAdsTelemetryWrapper::noop); #[cfg(feature = "stateful")] let store_set = inner.store_config.is_some(); - #[cfg(feature = "stateful")] - let worker_buffer_size = inner - .store_config - .as_ref() - .and_then(|x| x.worker_buffer_size); let client_config = AdsClientConfig { cache_config: inner.cache_config.clone().map(Into::into), environment: inner.environment.clone().unwrap_or_default().into(), @@ -127,7 +122,7 @@ impl MozAdsClientBuilder { let inner = Arc::new(Mutex::new(client)); #[cfg(feature = "stateful")] let worker = if store_set { - worker::BackgroundWorker::new(inner.clone(), worker_buffer_size) + worker::BackgroundWorker::new(inner.clone()) } else { worker::BackgroundWorker::new_empty() }; diff --git a/components/ads-client/src/lib.rs b/components/ads-client/src/lib.rs index fad9cfb71c6..cc5d4eb7402 100644 --- a/components/ads-client/src/lib.rs +++ b/components/ads-client/src/lib.rs @@ -34,7 +34,6 @@ pub use ffi::*; #[cfg(feature = "stateful")] use crate::worker::BackgroundWorker; -use crate::{ffi::telemetry::MozAdsTelemetryWrapper, shutdown::ShutdownReferences}; #[cfg(test)] mod test_utils; diff --git a/components/ads-client/src/request.rs b/components/ads-client/src/request.rs index a63c15a0282..342fb42278e 100644 --- a/components/ads-client/src/request.rs +++ b/components/ads-client/src/request.rs @@ -3,8 +3,9 @@ use url::Url; use crate::{ ads_store::PlacementId, mars::{ad_request::AdPlacementRequest, ReportReason}, + worker::DispatchRequest, }; -use std::collections::{HashMap, HashSet, VecDeque}; +use std::collections::{HashMap, VecDeque}; pub const MAXIMUM_ADS_BATCH_COUNT: usize = 100; @@ -66,25 +67,6 @@ pub enum QueuedRequest { ReportAd { url: Url, reason: ReportReason }, } -// Request dispatch enum for passing different instructions to the background worker thread. -// `RequestAds` are prefetch mechanisms that query and load data into the local cache. -#[derive(Clone, Debug, PartialEq)] -pub enum DispatchRequest { - RequestAds { - ad_requests: HashSet, - }, - RecordClick { - url: Url, - }, - RecordImpression { - url: Url, - }, - ReportAd { - url: Url, - reason: ReportReason, - }, -} - impl From for DispatchRequest { fn from(value: QueuedRequest) -> Self { match value { diff --git a/components/ads-client/src/worker.rs b/components/ads-client/src/worker.rs index e679d572cae..cc312367965 100644 --- a/components/ads-client/src/worker.rs +++ b/components/ads-client/src/worker.rs @@ -1,62 +1,80 @@ +use error_support::handle_error; +use parking_lot::Mutex; +use url::Url; + use crate::{ client::error::{BackgroundWorkerError, ComponentError}, - worker::command::DispatchCommand, - MozAdsClientInner, + mars::{ad_request::AdPlacementRequest, ReportReason}, + request::{QueuedRequest, RequestQueue}, + AdsClientApiResult, MozAdsClientInner, }; use std::{ - sync::mpsc::{self, Receiver, SyncSender}, + collections::HashSet, + sync::{ + mpsc::{self, SyncSender}, + Arc, + }, thread::JoinHandle, }; -pub mod command; - // This is a somewhat arbitrary default value that is overridable. pub const ADS_CLIENT_WORKER_CHANNEL_BUFFER_SIZE_DEFAULT: usize = 10000; pub const ADS_CLIENT_WORKER_THREAD_NAME: &str = "ads-client.worker"; pub struct BackgroundWorker { _worker_thread: Option>, - worker_dispatch: Option>, + worker_request_queue: Option>>, } impl BackgroundWorker { - pub fn new( - inner_client: MozAdsClientInner, - worker_buffer_size: Option, - ) -> BackgroundWorker { - let worker_buffer_size = worker_buffer_size.and_then(|x| usize::try_from(x).ok()); + pub fn new(inner_client: MozAdsClientInner) -> BackgroundWorker { + let request_queue = Arc::new(Mutex::new(RequestQueue::new())); + let worker_request_queue = request_queue.clone(); - let (tx, rx) = mpsc::sync_channel( - worker_buffer_size.unwrap_or(ADS_CLIENT_WORKER_CHANNEL_BUFFER_SIZE_DEFAULT), - ); let Some(worker_thread) = std::thread::Builder::new() .name(ADS_CLIENT_WORKER_THREAD_NAME.to_string()) - .spawn(move || crate::worker::worker(inner_client, rx)).inspect_err(|err| { + .spawn(move || crate::worker::worker(inner_client, worker_request_queue.clone())).inspect_err(|err| { error_support::error!("Failed to create ads-client worker thread `{ADS_CLIENT_WORKER_THREAD_NAME}` with: {err}") }).ok() else { - return BackgroundWorker { _worker_thread: None, worker_dispatch: None } + return BackgroundWorker { _worker_thread: None, worker_request_queue: None } }; BackgroundWorker { _worker_thread: Some(worker_thread), - worker_dispatch: Some(tx), + worker_request_queue: Some(request_queue), } } pub fn new_empty() -> BackgroundWorker { BackgroundWorker { _worker_thread: None, - worker_dispatch: None, + worker_request_queue: None, } } - pub fn dispatch(&self, command: DispatchCommand) -> Result<(), ComponentError> { - if let Some(worker_dispatch) = &self.worker_dispatch { - worker_dispatch - .try_send(command) - .map_err(BackgroundWorkerError::from)?; + pub fn dispatch_ads_request( + &self, + ads_requests: Vec, + ) -> Result<(), ComponentError> { + if let Some(worker_dispatch) = &self.worker_request_queue { + let mut queue = worker_dispatch.lock(); + for req in ads_requests { + queue.push_ad_request(req); + } + Ok(()) + } else { + Err(BackgroundWorkerError::Closed.into()) + } + } + pub fn dispatch_queued_request( + &self, + queued_request: QueuedRequest, + ) -> Result<(), ComponentError> { + if let Some(worker_dispatch) = &self.worker_request_queue { + let mut queue = worker_dispatch.lock(); + queue.push_queued_request(queued_request); Ok(()) } else { Err(BackgroundWorkerError::Closed.into()) @@ -64,27 +82,51 @@ impl BackgroundWorker { } } -// Spawn worker thread from a reference to the client, returning a synchronous channel transmitter to the thread, and its JoinHandle. -// Returns None if thread fails to build. -pub fn build_worker_thread( - inner_client: MozAdsClientInner, - max_channel_size: Option, -) -> Option<(SyncSender, JoinHandle<()>)> { - let (tx, rx) = mpsc::sync_channel( - max_channel_size.unwrap_or(ADS_CLIENT_WORKER_CHANNEL_BUFFER_SIZE_DEFAULT), - ); - let worker_thread_handle = std::thread::Builder::new() - .name(ADS_CLIENT_WORKER_THREAD_NAME.to_string()) - .spawn(move || crate::worker::worker(inner_client, rx)).inspect_err(|err| { - error_support::error!("Failed to create ads-client worker thread `{ADS_CLIENT_WORKER_THREAD_NAME}` with: {err}") - }).ok()?; - Some((tx, worker_thread_handle)) +// Endless worker for background thread that synchronously run tasks in the order provided by the RequestQueue. +fn worker(inner_client: MozAdsClientInner, request_queue: Arc>) { + loop { + // Get lock and extract next request. Lock is not held over the duration of the request but immediately dropped. + let next_request = { + let mut queue = request_queue.lock(); + queue.next() + }; + + if let Some(request) = next_request { + // Error is already logged through `handle_error` conversion macro. + let _ = request.handle_request(&inner_client); + } else { + // TODO: delay here? + } + } } -fn worker(inner_client: MozAdsClientInner, rx: Receiver) { - // Synchronously run tasks in the order they are passed in this separate channel. - while let Ok(command) = rx.recv() { - // Error is naturally logged through `handle_error` conversion macro. - let _ = command.run_command(&inner_client); +// Request dispatch enum for passing different instructions to the background worker thread. +// `RequestAds` are prefetch mechanisms that query and load data into the local cache. +#[derive(Clone, Debug, PartialEq)] +pub enum DispatchRequest { + RequestAds { + ad_requests: HashSet, + }, + RecordClick { + url: Url, + }, + RecordImpression { + url: Url, + }, + ReportAd { + url: Url, + reason: ReportReason, + }, +} + +impl DispatchRequest { + // Runs a dispatched command synchronously in it's thread. + // The dispatched command calls the corresponding `AdsClient` synchronous method, meaning that behavior between the two is shared. + #[handle_error(ComponentError)] + pub fn handle_request(self, _ads_client_inner: &MozAdsClientInner) -> AdsClientApiResult<()> { + // TODO: Add 'running a request' logic to here. + error_support::error!("Running a request is currently not set up yet: {self:?}"); + + Ok(()) } } diff --git a/components/ads-client/src/worker/command.rs b/components/ads-client/src/worker/command.rs deleted file mode 100644 index 33783a2d6dd..00000000000 --- a/components/ads-client/src/worker/command.rs +++ /dev/null @@ -1,140 +0,0 @@ -use std::collections::HashMap; - -use error_support::handle_error; - -use crate::{ - ads_store::StorableAd, - client::error::{ComponentError, RequestAdsError}, - http_cache::CachePolicy, - mars::ad_request::AdPlacementRequest, - AdsClientApiResult, MozAdsClientInner, MozAdsPlacementRequest, MozAdsPlacementRequestWithCount, -}; - -// Command dispatch enum for passing different instructions to the background worker thread. -// `RequestImageAds`, `RequestSpocAds`, `RequestTileAds` are prefetch mechanisms that query and load data into the local cache. -pub enum DispatchCommand { - RequestImageAds { - image_ad_requests: Vec, - cache_policy: CachePolicy, - ohttp: bool, - flags: HashMap, - blocks: Vec, - }, - RequestSpocAds { - spoc_ad_requests: Vec, - cache_policy: CachePolicy, - ohttp: bool, - flags: HashMap, - blocks: Vec, - }, - RequestTileAds { - tile_ad_requests: Vec, - cache_policy: CachePolicy, - ohttp: bool, - flags: HashMap, - blocks: Vec, - }, -} - -impl DispatchCommand { - // Runs a dispatched command synchronously in it's thread. - // The dispatched command calls the corresponding `AdsClient` synchronous method, meaning that behavior between the two is shared. - #[handle_error(ComponentError)] - pub fn run_command(self, ads_client_inner: &MozAdsClientInner) -> AdsClientApiResult<()> { - match self { - DispatchCommand::RequestImageAds { - image_ad_requests, - cache_policy, - flags, - ohttp, - blocks, - } => { - let mut inner = ads_client_inner.lock(); - if !image_ad_requests.is_empty() { - let image_ad_requests: Vec = - image_ad_requests.iter().map(|r| r.into()).collect(); - let image_response = inner - .request_image_ads( - image_ad_requests, - flags, - Some(cache_policy), - ohttp, - blocks, - ) - .map_err(ComponentError::RequestAds)?; - inner - .store_ads( - image_response - .into_iter() - .map(|(k, v)| (k.into(), StorableAd::Image(v))) - .collect(), - ) - .map_err(RequestAdsError::from)?; - } - Ok(()) - } - DispatchCommand::RequestSpocAds { - spoc_ad_requests, - cache_policy, - flags, - ohttp, - blocks, - } => { - let mut inner = ads_client_inner.lock(); - if !spoc_ad_requests.is_empty() { - let spoc_ad_requests: Vec = - spoc_ad_requests.iter().map(|r| r.into()).collect(); - let spoc_response = inner - .request_spoc_ads( - spoc_ad_requests, - flags, - Some(cache_policy), - ohttp, - blocks, - ) - .map_err(ComponentError::RequestAds)?; - inner - .store_ads( - spoc_response - .into_iter() - .map(|(k, v)| (k.into(), StorableAd::Spoc(v))) - .collect(), - ) - .map_err(RequestAdsError::from)?; - } - Ok(()) - } - DispatchCommand::RequestTileAds { - tile_ad_requests, - cache_policy, - flags, - ohttp, - blocks, - } => { - let mut inner = ads_client_inner.lock(); - if !tile_ad_requests.is_empty() { - let tile_ad_requests: Vec = - tile_ad_requests.iter().map(|r| r.into()).collect(); - let tile_response = inner - .request_tile_ads( - tile_ad_requests, - flags, - Some(cache_policy), - ohttp, - blocks, - ) - .map_err(ComponentError::RequestAds)?; - inner - .store_ads( - tile_response - .into_iter() - .map(|(k, v)| (k.into(), StorableAd::Tile(v))) - .collect(), - ) - .map_err(RequestAdsError::from)?; - } - Ok(()) - } - } - } -} From 8ca855016410066770edf9dea614c6b8c1be5425 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Fri, 25 Sep 2026 11:15:23 -0700 Subject: [PATCH 14/25] fix: Some cleanup --- components/ads-client/src/ads_store.rs | 18 +----- components/ads-client/src/client.rs | 69 +---------------------- components/ads-client/src/client/error.rs | 2 +- components/ads-client/src/ffi.rs | 1 - components/ads-client/src/mars/error.rs | 3 - components/ads-client/src/worker.rs | 11 +--- 6 files changed, 5 insertions(+), 99 deletions(-) diff --git a/components/ads-client/src/ads_store.rs b/components/ads-client/src/ads_store.rs index 106ace44d42..42002db976a 100644 --- a/components/ads-client/src/ads_store.rs +++ b/components/ads-client/src/ads_store.rs @@ -7,12 +7,9 @@ use serde::{Deserialize, Serialize}; use crate::{ ads_store::{builder::AdsStoreBuilder, store::AdsStoreHolder}, common::bytesize::ByteSize, - mars::{ - ad_response::{AdImage, AdSpoc, AdTile}, - error::FetchAdsError, - }, + mars::ad_response::{AdImage, AdSpoc, AdTile}, }; -use std::{collections::HashMap, path::Path}; +use std::path::Path; /// Identification of placement sent and returned from MARS (eg: `mock_spoc_1`) #[derive(Debug, Hash, PartialEq, Eq, Clone)] @@ -104,17 +101,6 @@ impl AdsStore { pub fn is_memory(&self) -> bool { self.is_memory } - - pub fn lookup(&self, placement_id: &PlacementId) -> Result, FetchAdsError> { - self.holder.lookup(placement_id) - } - - pub fn store_ads(&self, ads: HashMap) -> Result<(), FetchAdsError> { - for (placement_id, ad) in ads { - self.holder.store_ad(&placement_id, ad)?; - } - Ok(()) - } } #[cfg(test)] diff --git a/components/ads-client/src/client.rs b/components/ads-client/src/client.rs index b0a631f9e96..1ad02325536 100644 --- a/components/ads-client/src/client.rs +++ b/components/ads-client/src/client.rs @@ -4,13 +4,11 @@ */ #[cfg(feature = "stateful")] -use crate::ads_store::{AdsStore, PlacementId, StorableAd}; +use crate::ads_store::AdsStore; use crate::common::bytesize::ByteSize; use crate::http_cache::{CachePolicy, HttpCache}; use crate::mars::ad_request::{AdPlacementRequest, AdRequestFlags}; use crate::mars::ad_response::{AdImage, AdResponse, AdResponseValue, AdSpoc, AdTile}; -#[cfg(feature = "stateful")] -use crate::mars::error::FetchAdsError; use crate::mars::error::{RecordClickError, RecordImpressionError, ReportAdError}; use crate::mars::{MARSClient, ReportReason}; #[cfg(feature = "stateful")] @@ -112,71 +110,6 @@ where self.client.clear_cache() } - #[cfg(feature = "stateful")] - pub fn store_ads( - &mut self, - ads: HashMap, - ) -> Result<(), FetchAdsError> { - let ads_store = self.ads_store.lock(); - if let Some(ads_store) = ads_store.as_ref() { - ads_store.store_ads(ads)?; - Ok(()) - } else { - Err(FetchAdsError::SqliteShutdown) - } - } - - #[cfg(feature = "stateful")] - pub fn get_stored_ad_images(&self, placement_id: &PlacementId) -> Option { - let ads_store = self.ads_store.lock(); - if let Some(ads_store) = ads_store.as_ref() { - match ads_store.lookup(placement_id) { - Ok(ad) => ad.and_then(|ad| ad.into_image()), - Err(_) => { - // TODO: Telemetry should return an error here (eg: some internal sqlite error) - None - } - } - } else { - // TODO: Telemetry should be added here for the database being shut down. - None - } - } - - #[cfg(feature = "stateful")] - pub fn get_stored_ad_spocs(&self, placement_id: &PlacementId) -> Option> { - let ads_store = self.ads_store.lock(); - if let Some(ads_store) = ads_store.as_ref() { - match ads_store.lookup(placement_id) { - Ok(ad) => ad.and_then(|ad| ad.into_spocs()), - Err(_) => { - // TODO: Telemetry should return an error here (eg: some internal sqlite error) - None - } - } - } else { - // TODO: Telemetry should be added here for the database being shut down. - None - } - } - - #[cfg(feature = "stateful")] - pub fn get_stored_ad_tile(&self, placement_id: &PlacementId) -> Option { - let ads_store = self.ads_store.lock(); - if let Some(ads_store) = ads_store.as_ref() { - match ads_store.lookup(placement_id) { - Ok(ad) => ad.and_then(|ad| ad.into_tile()), - Err(_) => { - // TODO: Telemetry should return an error here (eg: some internal sqlite error) - None - } - } - } else { - // TODO: Telemetry should be added here for the database being shut down. - None - } - } - pub fn get_context_id(&self) -> context_id::ApiResult { self.context_id_component.request(DEFAULT_ROTATION_DAYS) } diff --git a/components/ads-client/src/client/error.rs b/components/ads-client/src/client/error.rs index 220be09eca0..d8b275f781f 100644 --- a/components/ads-client/src/client/error.rs +++ b/components/ads-client/src/client/error.rs @@ -36,6 +36,6 @@ pub enum RequestAdsError { #[cfg(feature = "stateful")] #[derive(Debug, thiserror::Error)] pub enum BackgroundWorkerError { - #[error("Error requesting new ads from the background worker: worker closed")] + #[error("Background worker is closed")] Closed, } diff --git a/components/ads-client/src/ffi.rs b/components/ads-client/src/ffi.rs index 6a1a3523216..a321379d783 100644 --- a/components/ads-client/src/ffi.rs +++ b/components/ads-client/src/ffi.rs @@ -184,7 +184,6 @@ pub struct MozAdsCacheConfig { #[derive(Clone, uniffi::Record)] pub struct MozAdsStoreConfig { pub db_path: String, - pub worker_buffer_size: Option, } #[derive(Debug, PartialEq, uniffi::Record)] diff --git a/components/ads-client/src/mars/error.rs b/components/ads-client/src/mars/error.rs index 22f9f0573dd..ce87833924c 100644 --- a/components/ads-client/src/mars/error.rs +++ b/components/ads-client/src/mars/error.rs @@ -46,9 +46,6 @@ pub enum FetchAdsError { #[error("Internal database error: {0}")] Sqlite(#[from] rusqlite::Error), - #[error("Internal database error: database shut down or uninitialized")] - SqliteShutdown, - #[error("Error sending request: {0}")] Request(#[from] viaduct::ViaductError), diff --git a/components/ads-client/src/worker.rs b/components/ads-client/src/worker.rs index cc312367965..3b0eff44004 100644 --- a/components/ads-client/src/worker.rs +++ b/components/ads-client/src/worker.rs @@ -8,17 +8,8 @@ use crate::{ request::{QueuedRequest, RequestQueue}, AdsClientApiResult, MozAdsClientInner, }; -use std::{ - collections::HashSet, - sync::{ - mpsc::{self, SyncSender}, - Arc, - }, - thread::JoinHandle, -}; +use std::{collections::HashSet, sync::Arc, thread::JoinHandle}; -// This is a somewhat arbitrary default value that is overridable. -pub const ADS_CLIENT_WORKER_CHANNEL_BUFFER_SIZE_DEFAULT: usize = 10000; pub const ADS_CLIENT_WORKER_THREAD_NAME: &str = "ads-client.worker"; pub struct BackgroundWorker { From e4cecc8e9510b23aa1304eaf5d9756a7891041d8 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Fri, 25 Sep 2026 11:49:34 -0700 Subject: [PATCH 15/25] fix: some comments, removed some unnecessary things --- components/ads-client/src/ads_store.rs | 26 -------------------------- components/ads-client/src/worker.rs | 16 ++++++++++++++-- 2 files changed, 14 insertions(+), 28 deletions(-) diff --git a/components/ads-client/src/ads_store.rs b/components/ads-client/src/ads_store.rs index 42002db976a..98e54af9e8d 100644 --- a/components/ads-client/src/ads_store.rs +++ b/components/ads-client/src/ads_store.rs @@ -46,32 +46,6 @@ pub enum StorableAd { Tile(AdTile), } -impl StorableAd { - pub fn into_image(self) -> Option { - if let StorableAd::Image(image) = self { - Some(image) - } else { - None - } - } - - pub fn into_spocs(self) -> Option> { - if let StorableAd::Spoc(spocs) = self { - Some(spocs) - } else { - None - } - } - - pub fn into_tile(self) -> Option { - if let StorableAd::Tile(tile) = self { - Some(tile) - } else { - None - } - } -} - pub struct AdsStore { holder: AdsStoreHolder, #[allow(dead_code)] diff --git a/components/ads-client/src/worker.rs b/components/ads-client/src/worker.rs index 3b0eff44004..4434fdff3cd 100644 --- a/components/ads-client/src/worker.rs +++ b/components/ads-client/src/worker.rs @@ -8,10 +8,14 @@ use crate::{ request::{QueuedRequest, RequestQueue}, AdsClientApiResult, MozAdsClientInner, }; -use std::{collections::HashSet, sync::Arc, thread::JoinHandle}; +use std::{collections::HashSet, sync::Arc, thread::JoinHandle, time::Duration}; pub const ADS_CLIENT_WORKER_THREAD_NAME: &str = "ads-client.worker"; +// Thread sleep delay to wait for new requests to queue before resolving them. +// This must be a very short amount of time to ensure ads are requested near-immediately on startup. +pub const ADS_CLIENT_WORKER_SLEEP_LENGTH: Duration = Duration::from_millis(2); + pub struct BackgroundWorker { _worker_thread: Option>, worker_request_queue: Option>>, @@ -22,6 +26,8 @@ impl BackgroundWorker { let request_queue = Arc::new(Mutex::new(RequestQueue::new())); let worker_request_queue = request_queue.clone(); + // Attempt to spawn a background worker thread to handle requests. + // If it fails, all fields of `BackgroundWorker` will be populated with None, and an error will be logged. let Some(worker_thread) = std::thread::Builder::new() .name(ADS_CLIENT_WORKER_THREAD_NAME.to_string()) .spawn(move || crate::worker::worker(inner_client, worker_request_queue.clone())).inspect_err(|err| { @@ -44,6 +50,8 @@ impl BackgroundWorker { } } + // Add to the background worker's queue an ads request. + // These are prioritized and will be batched before sending. pub fn dispatch_ads_request( &self, ads_requests: Vec, @@ -59,6 +67,9 @@ impl BackgroundWorker { } } + // Add to the background worker's queue a QueuedRequest (a non-ads request). + // (eg: RecordClick, RecordImpression) + // These are queued to send after any ads requests. pub fn dispatch_queued_request( &self, queued_request: QueuedRequest, @@ -86,7 +97,8 @@ fn worker(inner_client: MozAdsClientInner, request_queue: Arc Date: Thu, 1 Oct 2026 14:36:43 -0400 Subject: [PATCH 16/25] fix: revisions --- components/ads-client/src/lib.rs | 5 ++--- components/ads-client/src/worker.rs | 26 +++++++++++++++----------- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/components/ads-client/src/lib.rs b/components/ads-client/src/lib.rs index 69d9a0f8778..033ecccb5c8 100644 --- a/components/ads-client/src/lib.rs +++ b/components/ads-client/src/lib.rs @@ -46,11 +46,9 @@ uniffi::custom_type!(AdsClientUrl, String, { try_lift: |val| Ok(AdsClientUrl::parse(&val)?), lower: |obj| obj.as_str().to_string(), }); - -pub type MozAdsClientInner = Arc>>; #[derive(uniffi::Object)] pub struct MozAdsClient { - inner: MozAdsClientInner, + inner: Arc>>, shutdown_references: ShutdownReferences, #[cfg(feature = "stateful")] _worker: BackgroundWorker, @@ -74,6 +72,7 @@ impl MozAdsClient { #[uniffi::method()] pub fn shutdown(&self) -> AdsClientApiResult<()> { if let Err(e) = self.shutdown_references.shutdown() { + // TODO: Replace this log with telemetry. error_support::error!("Could not successfully shutdown ads-client: {e}"); } Ok(()) diff --git a/components/ads-client/src/worker.rs b/components/ads-client/src/worker.rs index 4434fdff3cd..af0bbc09611 100644 --- a/components/ads-client/src/worker.rs +++ b/components/ads-client/src/worker.rs @@ -1,12 +1,10 @@ -use error_support::handle_error; use parking_lot::Mutex; use url::Url; use crate::{ - client::error::{BackgroundWorkerError, ComponentError}, - mars::{ad_request::AdPlacementRequest, ReportReason}, - request::{QueuedRequest, RequestQueue}, - AdsClientApiResult, MozAdsClientInner, + client::{ + AdsClient, error::{BackgroundWorkerError, ComponentError}, + }, mars::{ReportReason, ad_request::AdPlacementRequest}, request::{QueuedRequest, RequestQueue}, telemetry::Telemetry, }; use std::{collections::HashSet, sync::Arc, thread::JoinHandle, time::Duration}; @@ -22,7 +20,7 @@ pub struct BackgroundWorker { } impl BackgroundWorker { - pub fn new(inner_client: MozAdsClientInner) -> BackgroundWorker { + pub fn new(inner_client: Arc>>) -> BackgroundWorker { let request_queue = Arc::new(Mutex::new(RequestQueue::new())); let worker_request_queue = request_queue.clone(); @@ -31,6 +29,7 @@ impl BackgroundWorker { let Some(worker_thread) = std::thread::Builder::new() .name(ADS_CLIENT_WORKER_THREAD_NAME.to_string()) .spawn(move || crate::worker::worker(inner_client, worker_request_queue.clone())).inspect_err(|err| { + // TODO: Replace this log with telemetry. error_support::error!("Failed to create ads-client worker thread `{ADS_CLIENT_WORKER_THREAD_NAME}` with: {err}") }).ok() else { @@ -85,7 +84,10 @@ impl BackgroundWorker { } // Endless worker for background thread that synchronously run tasks in the order provided by the RequestQueue. -fn worker(inner_client: MozAdsClientInner, request_queue: Arc>) { +fn worker( + inner_client: Arc>>, + request_queue: Arc>, +) { loop { // Get lock and extract next request. Lock is not held over the duration of the request but immediately dropped. let next_request = { @@ -94,8 +96,8 @@ fn worker(inner_client: MozAdsClientInner, request_queue: Arc AdsClientApiResult<()> { + pub fn handle_request( + self, + _ads_client_inner: Arc>>, + ) -> Result<(), ComponentError> { // TODO: Add 'running a request' logic to here. error_support::error!("Running a request is currently not set up yet: {self:?}"); From e8c8d916e2b94bff8a449d96d0b354993b88c3e7 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Thu, 1 Oct 2026 14:56:59 -0400 Subject: [PATCH 17/25] fix: fmt --- components/ads-client/src/worker.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/components/ads-client/src/worker.rs b/components/ads-client/src/worker.rs index af0bbc09611..2a5be2a0c21 100644 --- a/components/ads-client/src/worker.rs +++ b/components/ads-client/src/worker.rs @@ -3,8 +3,12 @@ use url::Url; use crate::{ client::{ - AdsClient, error::{BackgroundWorkerError, ComponentError}, - }, mars::{ReportReason, ad_request::AdPlacementRequest}, request::{QueuedRequest, RequestQueue}, telemetry::Telemetry, + error::{BackgroundWorkerError, ComponentError}, + AdsClient, + }, + mars::{ad_request::AdPlacementRequest, ReportReason}, + request::{QueuedRequest, RequestQueue}, + telemetry::Telemetry, }; use std::{collections::HashSet, sync::Arc, thread::JoinHandle, time::Duration}; @@ -20,7 +24,9 @@ pub struct BackgroundWorker { } impl BackgroundWorker { - pub fn new(inner_client: Arc>>) -> BackgroundWorker { + pub fn new( + inner_client: Arc>>, + ) -> BackgroundWorker { let request_queue = Arc::new(Mutex::new(RequestQueue::new())); let worker_request_queue = request_queue.clone(); @@ -84,7 +90,7 @@ impl BackgroundWorker { } // Endless worker for background thread that synchronously run tasks in the order provided by the RequestQueue. -fn worker( +fn worker( inner_client: Arc>>, request_queue: Arc>, ) { From 05a8beb371d49719be17ddf80c45ce5fbe5407dc Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Fri, 9 Oct 2026 17:44:10 -0700 Subject: [PATCH 18/25] fix: Merge conflict resolution --- components/ads-client/src/ffi.rs | 4 +--- components/ads-client/src/lib.rs | 3 +-- components/ads-client/src/request.rs | 19 ------------------- components/ads-client/src/worker.rs | 12 ++++++------ 4 files changed, 8 insertions(+), 30 deletions(-) diff --git a/components/ads-client/src/ffi.rs b/components/ads-client/src/ffi.rs index fbe5a48ae43..20b6e6a6cf9 100644 --- a/components/ads-client/src/ffi.rs +++ b/components/ads-client/src/ffi.rs @@ -116,8 +116,7 @@ impl MozAdsClientBuilder { telemetry: telemetry.clone(), }; let client = AdsClient::new(client_config); - let shutdown_references = client.shutdown_references(); - let inner = Arc::new(Mutex::new(client)); + let inner = Arc::new(client); #[cfg(feature = "stateful")] let worker = if store_set { worker::BackgroundWorker::new(inner.clone()) @@ -126,7 +125,6 @@ impl MozAdsClientBuilder { }; MozAdsClient { inner, - shutdown_references, #[cfg(feature = "stateful")] _worker: worker, } diff --git a/components/ads-client/src/lib.rs b/components/ads-client/src/lib.rs index 28d147fef8e..17b5363fa56 100644 --- a/components/ads-client/src/lib.rs +++ b/components/ads-client/src/lib.rs @@ -46,8 +46,7 @@ uniffi::custom_type!(AdsClientUrl, String, { }); #[derive(uniffi::Object)] pub struct MozAdsClient { - inner: Arc>>, - shutdown_references: ShutdownReferences, + inner: Arc>, #[cfg(feature = "stateful")] _worker: BackgroundWorker, } diff --git a/components/ads-client/src/request.rs b/components/ads-client/src/request.rs index 4eaf7b3b70a..0f4d6597fea 100644 --- a/components/ads-client/src/request.rs +++ b/components/ads-client/src/request.rs @@ -67,25 +67,6 @@ pub enum QueuedRequest { ReportAd { reason: ReportReason, url: Url }, } -// Request dispatch enum for passing different instructions to the background worker thread. -// `RequestAds` are prefetch mechanisms that query and load data into the local cache. -#[derive(Clone, Debug, PartialEq)] -pub enum DispatchRequest { - RecordClick { - url: Url, - }, - RecordImpression { - url: Url, - }, - ReportAd { - reason: ReportReason, - url: Url, - }, - RequestAds { - ad_requests: HashSet, - }, -} - impl From for DispatchRequest { fn from(value: QueuedRequest) -> Self { match value { diff --git a/components/ads-client/src/worker.rs b/components/ads-client/src/worker.rs index 2a5be2a0c21..8f103f3dc72 100644 --- a/components/ads-client/src/worker.rs +++ b/components/ads-client/src/worker.rs @@ -25,7 +25,7 @@ pub struct BackgroundWorker { impl BackgroundWorker { pub fn new( - inner_client: Arc>>, + inner_client: Arc>, ) -> BackgroundWorker { let request_queue = Arc::new(Mutex::new(RequestQueue::new())); let worker_request_queue = request_queue.clone(); @@ -91,7 +91,7 @@ impl BackgroundWorker { // Endless worker for background thread that synchronously run tasks in the order provided by the RequestQueue. fn worker( - inner_client: Arc>>, + inner_client: Arc>, request_queue: Arc>, ) { loop { @@ -115,9 +115,6 @@ fn worker( // `RequestAds` are prefetch mechanisms that query and load data into the local cache. #[derive(Clone, Debug, PartialEq)] pub enum DispatchRequest { - RequestAds { - ad_requests: HashSet, - }, RecordClick { url: Url, }, @@ -128,6 +125,9 @@ pub enum DispatchRequest { url: Url, reason: ReportReason, }, + RequestAds { + ad_requests: HashSet, + }, } impl DispatchRequest { @@ -135,7 +135,7 @@ impl DispatchRequest { // The dispatched command calls the corresponding `AdsClient` synchronous method, meaning that behavior between the two is shared. pub fn handle_request( self, - _ads_client_inner: Arc>>, + _ads_client_inner: Arc>, ) -> Result<(), ComponentError> { // TODO: Add 'running a request' logic to here. error_support::error!("Running a request is currently not set up yet: {self:?}"); From ad9fb2cf973df76fa3b8ed5caa692788b07272db Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Tue, 13 Oct 2026 10:01:32 -0700 Subject: [PATCH 19/25] fix: Background worker ping interval is 100ms by default, customizable --- components/ads-client/src/client/config.rs | 1 + components/ads-client/src/ffi.rs | 10 +++++++++- components/ads-client/src/worker.rs | 11 ++++++++--- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/components/ads-client/src/client/config.rs b/components/ads-client/src/client/config.rs index 234d2a58e46..bb1d915e838 100644 --- a/components/ads-client/src/client/config.rs +++ b/components/ads-client/src/client/config.rs @@ -28,4 +28,5 @@ pub struct AdsCacheConfig { #[derive(Clone, Debug)] pub struct AdsStoreConfig { pub db_path: String, + pub batch_interval_ms: Option, } diff --git a/components/ads-client/src/ffi.rs b/components/ads-client/src/ffi.rs index 20b6e6a6cf9..f679fdb2e5e 100644 --- a/components/ads-client/src/ffi.rs +++ b/components/ads-client/src/ffi.rs @@ -115,11 +115,16 @@ impl MozAdsClientBuilder { store_config: inner.store_config.clone().map(Into::into), telemetry: telemetry.clone(), }; + #[cfg(feature = "stateful")] + let worker_batch_interval_ms = client_config + .store_config + .as_ref() + .and_then(|config| config.batch_interval_ms); let client = AdsClient::new(client_config); let inner = Arc::new(client); #[cfg(feature = "stateful")] let worker = if store_set { - worker::BackgroundWorker::new(inner.clone()) + worker::BackgroundWorker::new(inner.clone(), worker_batch_interval_ms) } else { worker::BackgroundWorker::new_empty() }; @@ -182,6 +187,8 @@ pub struct MozAdsCacheConfig { #[derive(Clone, uniffi::Record)] pub struct MozAdsStoreConfig { pub db_path: String, + /// Interval rate for batching commands used by the background worker. + pub batch_interval_ms: Option, } #[derive(Debug, PartialEq, uniffi::Record)] @@ -457,6 +464,7 @@ impl From for AdsStoreConfig { fn from(config: MozAdsStoreConfig) -> Self { Self { db_path: config.db_path, + batch_interval_ms: config.batch_interval_ms, } } } diff --git a/components/ads-client/src/worker.rs b/components/ads-client/src/worker.rs index 8f103f3dc72..2eca8223043 100644 --- a/components/ads-client/src/worker.rs +++ b/components/ads-client/src/worker.rs @@ -16,7 +16,7 @@ pub const ADS_CLIENT_WORKER_THREAD_NAME: &str = "ads-client.worker"; // Thread sleep delay to wait for new requests to queue before resolving them. // This must be a very short amount of time to ensure ads are requested near-immediately on startup. -pub const ADS_CLIENT_WORKER_SLEEP_LENGTH: Duration = Duration::from_millis(2); +pub const ADS_CLIENT_WORKER_SLEEP_LENGTH_DEFAULT: Duration = Duration::from_millis(100); pub struct BackgroundWorker { _worker_thread: Option>, @@ -26,6 +26,7 @@ pub struct BackgroundWorker { impl BackgroundWorker { pub fn new( inner_client: Arc>, + batch_interval_ms: Option, ) -> BackgroundWorker { let request_queue = Arc::new(Mutex::new(RequestQueue::new())); let worker_request_queue = request_queue.clone(); @@ -34,7 +35,7 @@ impl BackgroundWorker { // If it fails, all fields of `BackgroundWorker` will be populated with None, and an error will be logged. let Some(worker_thread) = std::thread::Builder::new() .name(ADS_CLIENT_WORKER_THREAD_NAME.to_string()) - .spawn(move || crate::worker::worker(inner_client, worker_request_queue.clone())).inspect_err(|err| { + .spawn(move || crate::worker::worker(inner_client, worker_request_queue.clone(), batch_interval_ms)).inspect_err(|err| { // TODO: Replace this log with telemetry. error_support::error!("Failed to create ads-client worker thread `{ADS_CLIENT_WORKER_THREAD_NAME}` with: {err}") }).ok() @@ -93,7 +94,11 @@ impl BackgroundWorker { fn worker( inner_client: Arc>, request_queue: Arc>, + batch_interval_ms: Option, ) { + let sleep_duration = batch_interval_ms + .map(|t| Duration::from_millis(t)) + .unwrap_or(ADS_CLIENT_WORKER_SLEEP_LENGTH_DEFAULT); loop { // Get lock and extract next request. Lock is not held over the duration of the request but immediately dropped. let next_request = { @@ -106,7 +111,7 @@ fn worker( let _ = request.handle_request(inner_client.clone()); } else { // If the queue is empty, we have a very short pause with no locks to allow it to be written to. - std::thread::sleep(ADS_CLIENT_WORKER_SLEEP_LENGTH); + std::thread::sleep(sleep_duration); } } } From d2863fcad0d7262e3e57b3b065ffbe820a30a5c2 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Wed, 14 Oct 2026 14:10:41 -0700 Subject: [PATCH 20/25] feat: Encapsulates locks in RequestQueue, moves 'up' defaulting logic --- components/ads-client/src/client/config.rs | 5 +- components/ads-client/src/ffi.rs | 15 +++-- components/ads-client/src/request.rs | 68 +++++++++++++++------- components/ads-client/src/worker.rs | 27 +++------ 4 files changed, 70 insertions(+), 45 deletions(-) diff --git a/components/ads-client/src/client/config.rs b/components/ads-client/src/client/config.rs index bb1d915e838..28e6674976c 100644 --- a/components/ads-client/src/client/config.rs +++ b/components/ads-client/src/client/config.rs @@ -3,6 +3,9 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ +#[cfg(feature = "stateful")] +use std::time::Duration; + use crate::mars::Environment; use crate::telemetry::Telemetry; @@ -28,5 +31,5 @@ pub struct AdsCacheConfig { #[derive(Clone, Debug)] pub struct AdsStoreConfig { pub db_path: String, - pub batch_interval_ms: Option, + pub batch_interval_duration: Option, } diff --git a/components/ads-client/src/ffi.rs b/components/ads-client/src/ffi.rs index f679fdb2e5e..e8a78b764dc 100644 --- a/components/ads-client/src/ffi.rs +++ b/components/ads-client/src/ffi.rs @@ -19,6 +19,8 @@ use crate::mars::Environment; use crate::mars::ReportReason; #[cfg(feature = "stateful")] use crate::worker; +#[cfg(feature = "stateful")] +use crate::worker::ADS_CLIENT_WORKER_SLEEP_LENGTH_DEFAULT; use crate::AdsClientUrl; use crate::MozAdsClient; use parking_lot::Mutex; @@ -26,6 +28,7 @@ use std::collections::HashMap; use std::sync::Arc; #[cfg(test)] use std::sync::Weak; +use std::time::Duration; pub use error::{AdsClientApiResult, MozAdsClientApiError}; pub use telemetry::MozAdsTelemetry; @@ -116,15 +119,17 @@ impl MozAdsClientBuilder { telemetry: telemetry.clone(), }; #[cfg(feature = "stateful")] - let worker_batch_interval_ms = client_config + let worker_sleep_duration = client_config .store_config .as_ref() - .and_then(|config| config.batch_interval_ms); + .and_then(|config| config.batch_interval_duration) + .unwrap_or(ADS_CLIENT_WORKER_SLEEP_LENGTH_DEFAULT); + let client = AdsClient::new(client_config); let inner = Arc::new(client); #[cfg(feature = "stateful")] let worker = if store_set { - worker::BackgroundWorker::new(inner.clone(), worker_batch_interval_ms) + worker::BackgroundWorker::new(inner.clone(), worker_sleep_duration) } else { worker::BackgroundWorker::new_empty() }; @@ -188,7 +193,7 @@ pub struct MozAdsCacheConfig { pub struct MozAdsStoreConfig { pub db_path: String, /// Interval rate for batching commands used by the background worker. - pub batch_interval_ms: Option, + pub batch_interval_duration: Option, } #[derive(Debug, PartialEq, uniffi::Record)] @@ -464,7 +469,7 @@ impl From for AdsStoreConfig { fn from(config: MozAdsStoreConfig) -> Self { Self { db_path: config.db_path, - batch_interval_ms: config.batch_interval_ms, + batch_interval_duration: config.batch_interval_duration, } } } diff --git a/components/ads-client/src/request.rs b/components/ads-client/src/request.rs index 0f4d6597fea..499bfd5af69 100644 --- a/components/ads-client/src/request.rs +++ b/components/ads-client/src/request.rs @@ -1,3 +1,4 @@ +use parking_lot::Mutex; use url::Url; use crate::{ @@ -5,13 +6,21 @@ use crate::{ mars::{ad_request::AdPlacementRequest, ReportReason}, worker::DispatchRequest, }; -use std::collections::{HashMap, VecDeque}; +use std::{ + collections::{HashMap, VecDeque}, + sync::Arc, +}; pub const MAXIMUM_ADS_BATCH_COUNT: usize = 100; // Structure for storing and queuing and storing `DispatchRequest`s sent to background worker. // AdPlacementRequest will be merged together, allowing fewer network requests to be sent. +#[derive(Clone)] pub struct RequestQueue { + inner: Arc>, +} + +struct RequestQueueInner { queued_ads: HashMap, // The ordered queue of non-AdPlacementRequest commands to iterate through. @@ -20,42 +29,51 @@ pub struct RequestQueue { impl RequestQueue { pub fn new() -> RequestQueue { - RequestQueue { + let inner = RequestQueueInner { queued_ads: HashMap::new(), request_queue: VecDeque::new(), + }; + RequestQueue { + inner: Arc::new(Mutex::new(inner)), } } - pub fn clear(&mut self) { - self.request_queue = VecDeque::new(); - self.queued_ads = HashMap::new(); + pub fn clear(&self) { + let mut inner = self.inner.lock(); + inner.request_queue = VecDeque::new(); + inner.queued_ads = HashMap::new(); } // Pops the next available DispatchRequest to be run by the worker. // This will prioritize any ad_requests, batched, followed by any queued QueuedRequests (eg: RecordClick, etc) - pub fn next(&mut self) -> Option { + pub fn next(&self) -> Option { + let mut inner = self.inner.lock(); + // First, if any ad requests are queued, batch the first `MAXIMUM_ADS_BATCH_COUNT` and resolve those. - let num_ads = self.queued_ads.len().min(MAXIMUM_ADS_BATCH_COUNT); - let mut ad_requests: Vec<_> = self.queued_ads.drain().collect(); + let num_ads = inner.queued_ads.len().min(MAXIMUM_ADS_BATCH_COUNT); + let mut ad_requests: Vec<_> = inner.queued_ads.drain().collect(); if ad_requests.len() > 0 { let requeue_requests = ad_requests.split_off(num_ads); - self.queued_ads = requeue_requests.into_iter().collect(); + inner.queued_ads = requeue_requests.into_iter().collect(); return Some(DispatchRequest::RequestAds { ad_requests: ad_requests.into_iter().map(|(_, v)| v).collect(), }); } // Otherwise, pop the next queue-able command. - self.request_queue.pop_front().map(|c| c.into()) + inner.request_queue.pop_front().map(|c| c.into()) } - pub fn push_ad_request(&mut self, ad_request: AdPlacementRequest) { - self.queued_ads + pub fn push_ad_request(&self, ad_request: AdPlacementRequest) { + let mut inner = self.inner.lock(); + inner + .queued_ads .insert(ad_request.placement.clone().into(), ad_request); } - pub fn push_queued_request(&mut self, queued_command: QueuedRequest) { - self.request_queue.push_back(queued_command); + pub fn push_queued_request(&self, queued_command: QueuedRequest) { + let mut inner = self.inner.lock(); + inner.request_queue.push_back(queued_command); } } @@ -122,7 +140,7 @@ mod tests { fn identical_command_comes_out() { let request = example_request_ads(0); let command = example_request_ads_dispatch(&[0]); - let mut queue = RequestQueue::new(); + let queue = RequestQueue::new(); queue.push_ad_request(request.clone()); let retrieved_dispatch = queue.next().expect("Request should exist in queue"); @@ -134,7 +152,7 @@ mod tests { let request_0 = example_request_ads(0); let request_1 = example_request_ads(1); let dispatch = example_request_ads_dispatch(&[0, 1]); - let mut queue = RequestQueue::new(); + let queue = RequestQueue::new(); // Having two "placement_0" and one "placement_1" should result in returning a single DispatchRequest with one "placement_0" and one "placement_1" queue.push_ad_request(request_0.clone()); @@ -142,16 +160,21 @@ mod tests { queue.push_ad_request(request_1.clone()); let retrieved_dispatch = queue.next().expect("Request should exist in queue"); + assert!(queue.next().is_none()); - assert!(queue.queued_ads.is_empty()); - assert!(queue.request_queue.is_empty()); + + { + let inner = queue.inner.lock(); + assert!(inner.queued_ads.is_empty()); + assert!(inner.request_queue.is_empty()); + } assert_eq!(dispatch, retrieved_dispatch); } #[test] fn split_off_too_many_ads() { - let mut queue = RequestQueue::new(); + let queue = RequestQueue::new(); // Queue enough of this command to go one-over the limit. for i in 0..(MAXIMUM_ADS_BATCH_COUNT + 1) { @@ -165,8 +188,11 @@ mod tests { queue.next().expect("Second command should exist in queue"); assert!(queue.next().is_none()); - assert!(queue.queued_ads.is_empty()); - assert!(queue.request_queue.is_empty()); + { + let inner = queue.inner.lock(); + assert!(inner.queued_ads.is_empty()); + assert!(inner.request_queue.is_empty()); + } // We do not check for a precise match with sample data here, because HashMap<..>s are unsorted, so which placement gets pushed to the next one is random. // It is also irrelevant to the MARS request. diff --git a/components/ads-client/src/worker.rs b/components/ads-client/src/worker.rs index 2eca8223043..b515c6f398e 100644 --- a/components/ads-client/src/worker.rs +++ b/components/ads-client/src/worker.rs @@ -1,4 +1,3 @@ -use parking_lot::Mutex; use url::Url; use crate::{ @@ -20,22 +19,22 @@ pub const ADS_CLIENT_WORKER_SLEEP_LENGTH_DEFAULT: Duration = Duration::from_mill pub struct BackgroundWorker { _worker_thread: Option>, - worker_request_queue: Option>>, + worker_request_queue: Option, } impl BackgroundWorker { pub fn new( inner_client: Arc>, - batch_interval_ms: Option, + worker_sleep_duration: Duration, ) -> BackgroundWorker { - let request_queue = Arc::new(Mutex::new(RequestQueue::new())); + let request_queue = RequestQueue::new(); let worker_request_queue = request_queue.clone(); // Attempt to spawn a background worker thread to handle requests. // If it fails, all fields of `BackgroundWorker` will be populated with None, and an error will be logged. let Some(worker_thread) = std::thread::Builder::new() .name(ADS_CLIENT_WORKER_THREAD_NAME.to_string()) - .spawn(move || crate::worker::worker(inner_client, worker_request_queue.clone(), batch_interval_ms)).inspect_err(|err| { + .spawn(move || crate::worker::worker(inner_client, worker_request_queue.clone(), worker_sleep_duration)).inspect_err(|err| { // TODO: Replace this log with telemetry. error_support::error!("Failed to create ads-client worker thread `{ADS_CLIENT_WORKER_THREAD_NAME}` with: {err}") }).ok() @@ -63,9 +62,8 @@ impl BackgroundWorker { ads_requests: Vec, ) -> Result<(), ComponentError> { if let Some(worker_dispatch) = &self.worker_request_queue { - let mut queue = worker_dispatch.lock(); for req in ads_requests { - queue.push_ad_request(req); + worker_dispatch.push_ad_request(req); } Ok(()) } else { @@ -81,8 +79,7 @@ impl BackgroundWorker { queued_request: QueuedRequest, ) -> Result<(), ComponentError> { if let Some(worker_dispatch) = &self.worker_request_queue { - let mut queue = worker_dispatch.lock(); - queue.push_queued_request(queued_request); + worker_dispatch.push_queued_request(queued_request); Ok(()) } else { Err(BackgroundWorkerError::Closed.into()) @@ -93,18 +90,12 @@ impl BackgroundWorker { // Endless worker for background thread that synchronously run tasks in the order provided by the RequestQueue. fn worker( inner_client: Arc>, - request_queue: Arc>, - batch_interval_ms: Option, + request_queue: RequestQueue, + sleep_duration: Duration, ) { - let sleep_duration = batch_interval_ms - .map(|t| Duration::from_millis(t)) - .unwrap_or(ADS_CLIENT_WORKER_SLEEP_LENGTH_DEFAULT); loop { // Get lock and extract next request. Lock is not held over the duration of the request but immediately dropped. - let next_request = { - let mut queue = request_queue.lock(); - queue.next() - }; + let next_request = request_queue.next(); if let Some(request) = next_request { // TODO: Request should be logged via telemetry here. From 0272b6be87ce30cb7e1113c342621accaf23fbbe Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Fri, 16 Oct 2026 12:41:51 -0700 Subject: [PATCH 21/25] feat: refactor --- components/ads-client/src/client.rs | 147 ++++++++++++++++++++- components/ads-client/src/client/config.rs | 7 + components/ads-client/src/ffi.rs | 60 +++++---- components/ads-client/src/lib.rs | 5 - components/ads-client/src/worker.rs | 19 +-- 5 files changed, 192 insertions(+), 46 deletions(-) diff --git a/components/ads-client/src/client.rs b/components/ads-client/src/client.rs index e777ff38bbc..b74d06bb649 100644 --- a/components/ads-client/src/client.rs +++ b/components/ads-client/src/client.rs @@ -13,12 +13,13 @@ use crate::mars::ad_response::{AdResponse, AdResponseValue}; use crate::mars::error::{RecordClickError, RecordImpressionError, ReportAdError}; use crate::mars::{MARSClient, ReportReason}; use crate::telemetry::{self, ClientOperation}; +#[cfg(feature = "stateful")] +use crate::worker::{self, BackgroundWorker}; use config::AdsClientConfig; use context_id::{ContextIDComponent, DefaultContextIdCallback}; use error::RequestAdsError; use sql_support::open_database; use std::collections::HashMap; -#[cfg(feature = "stateful")] use std::sync::Arc; use std::time::Duration; use url::Url; @@ -31,7 +32,18 @@ const DEFAULT_TTL_SECONDS: u64 = 300; const DEFAULT_MAX_CACHE_SIZE_MIB: u64 = 10; const DEFAULT_ROTATION_DAYS: u8 = 3; +// Thread sleep delay to wait for new requests to queue before resolving them. +// This must be a very short amount of time to ensure ads are requested near-immediately on startup. +#[cfg(feature = "stateful")] +pub const ADS_CLIENT_WORKER_SLEEP_LENGTH_DEFAULT: Duration = Duration::from_millis(100); + pub struct AdsClient { + inner: Arc, + #[cfg(feature = "stateful")] + _worker: BackgroundWorker, +} + +pub struct AdsClientInner { #[cfg(feature = "stateful")] ads_store: Arc>, client: MARSClient, @@ -39,6 +51,127 @@ pub struct AdsClient { } impl AdsClient { + pub fn new(client_config: AdsClientConfig) -> Self { + #[cfg(feature = "stateful")] + let store_exists = client_config.store_config.is_some(); + #[cfg(feature = "stateful")] + let worker_sleep_duration = client_config + .worker_config + .as_ref() + .and_then(|x| x.batch_interval_duration) + .unwrap_or(ADS_CLIENT_WORKER_SLEEP_LENGTH_DEFAULT); + let inner = Arc::new(AdsClientInner::new(client_config)); + + #[cfg(feature = "stateful")] + let worker = if store_exists { + worker::BackgroundWorker::new(inner.clone(), worker_sleep_duration) + } else { + worker::BackgroundWorker::new_empty() + }; + + AdsClient { + inner, + #[cfg(feature = "stateful")] + _worker: worker, + } + } + + pub fn clear_cache(&self) -> Result<(), open_database::Error> { + self.inner.clear_cache() + } + + #[cfg(test)] + pub fn get_context_id(&self) -> context_id::ApiResult { + self.inner.get_context_id() + } + + pub fn record_click(&self, click_url: Url, ohttp: bool) -> Result<(), RecordClickError> { + self.inner.record_click(click_url, ohttp) + } + + pub fn record_impression( + &self, + impression_url: Url, + ohttp: bool, + ) -> Result<(), RecordImpressionError> { + self.inner.record_impression(impression_url, ohttp) + } + + pub fn report_ad( + &self, + report_url: Url, + reason: ReportReason, + ohttp: bool, + ) -> Result<(), ReportAdError> { + self.inner.report_ad(report_url, reason, ohttp) + } + + pub fn request_image_ads( + &self, + ad_placement_requests: Vec, + flags: AdRequestFlags, + options: Option, + ohttp: bool, + blocks: Vec, + ) -> Result, RequestAdsError> { + self.inner + .request_image_ads(ad_placement_requests, flags, options, ohttp, blocks) + } + + pub fn request_spoc_ads( + &self, + ad_placement_requests: Vec, + flags: AdRequestFlags, + options: Option, + ohttp: bool, + blocks: Vec, + ) -> Result>, RequestAdsError> { + self.inner + .request_spoc_ads(ad_placement_requests, flags, options, ohttp, blocks) + } + + pub fn request_tile_ads( + &self, + ad_placement_requests: Vec, + flags: AdRequestFlags, + options: Option, + ohttp: bool, + blocks: Vec, + relevance: AdRequestRelevance, + ) -> Result, RequestAdsError> { + self.inner.request_tile_ads( + ad_placement_requests, + flags, + options, + ohttp, + blocks, + relevance, + ) + } + + pub fn shutdown(&self) { + self.inner.shutdown(); + } + + #[cfg(test)] + fn request_ads( + &self, + placements: Vec, + flags: AdRequestFlags, + options: Option, + ohttp: bool, + blocks: Vec, + relevance: AdRequestRelevance, + ) -> Result, RequestAdsError> + where + A: AdResponseValue, + { + self.inner + .request_ads(placements, flags, options, ohttp, blocks, relevance) + } +} + +impl AdsClientInner { pub fn new(client_config: AdsClientConfig) -> Self { let context_id_component = ContextIDComponent::new( &Uuid::new_v4().to_string(), @@ -319,7 +452,7 @@ mod tests { use super::*; fn new_with_mars_client(client: MARSClient) -> AdsClient { - AdsClient { + let inner = Arc::new(AdsClientInner { #[cfg(feature = "stateful")] ads_store: Arc::new(Some( AdsStoreBuilder::new("test_store.db") @@ -333,6 +466,10 @@ mod tests { false, Box::new(DefaultContextIdCallback), ), + }); + AdsClient { + inner: inner.clone(), + _worker: BackgroundWorker::new(inner.clone(), ADS_CLIENT_WORKER_SLEEP_LENGTH_DEFAULT), } } @@ -343,6 +480,8 @@ mod tests { environment: Environment::Test, #[cfg(feature = "stateful")] store_config: None, + #[cfg(feature = "stateful")] + worker_config: None, }; let client = AdsClient::new(config); let context_id = client.get_context_id().unwrap(); @@ -434,6 +573,8 @@ mod tests { environment: Environment::Test, #[cfg(feature = "stateful")] store_config: None, + #[cfg(feature = "stateful")] + worker_config: None, }; let client = AdsClient::new(config); @@ -472,6 +613,8 @@ mod tests { environment: Environment::Test, #[cfg(feature = "stateful")] store_config: None, + #[cfg(feature = "stateful")] + worker_config: None, }; let client = AdsClient::new(config); diff --git a/components/ads-client/src/client/config.rs b/components/ads-client/src/client/config.rs index 0e970e3240d..8afcd989ca7 100644 --- a/components/ads-client/src/client/config.rs +++ b/components/ads-client/src/client/config.rs @@ -13,6 +13,8 @@ pub struct AdsClientConfig { pub environment: Environment, #[cfg(feature = "stateful")] pub store_config: Option, + #[cfg(feature = "stateful")] + pub worker_config: Option, } #[derive(Clone, Debug)] @@ -26,5 +28,10 @@ pub struct AdsCacheConfig { #[derive(Clone, Debug)] pub struct AdsStoreConfig { pub db_path: String, +} + +#[cfg(feature = "stateful")] +#[derive(Clone, Debug)] +pub struct AdsWorkerConfig { pub batch_interval_duration: Option, } diff --git a/components/ads-client/src/ffi.rs b/components/ads-client/src/ffi.rs index 938e86aae2f..2c679a9fca0 100644 --- a/components/ads-client/src/ffi.rs +++ b/components/ads-client/src/ffi.rs @@ -6,9 +6,9 @@ pub mod error; pub mod telemetry; use crate::ads::{AdCallbacks, AdImage, AdSpoc, AdTile, SpocFrequencyCaps, SpocRanking}; -#[cfg(feature = "stateful")] -use crate::client::config::AdsStoreConfig; use crate::client::config::{AdsCacheConfig, AdsClientConfig}; +#[cfg(feature = "stateful")] +use crate::client::config::{AdsStoreConfig, AdsWorkerConfig}; use crate::client::AdsClient; use crate::http_cache::CachePolicy; use crate::mars::ad_request::{ @@ -16,10 +16,6 @@ use crate::mars::ad_request::{ }; use crate::mars::Environment; use crate::mars::ReportReason; -#[cfg(feature = "stateful")] -use crate::worker; -#[cfg(feature = "stateful")] -use crate::worker::ADS_CLIENT_WORKER_SLEEP_LENGTH_DEFAULT; use crate::AdsClientUrl; use crate::MozAdsClient; use parking_lot::Mutex; @@ -86,8 +82,11 @@ pub struct MozAdsClientBuilder(Mutex); struct MozAdsClientBuilderInner { cache_config: Option, environment: Option, + #[cfg(feature = "stateful")] store_config: Option, telemetry: Option>, + #[cfg(feature = "stateful")] + worker_config: Option, } impl Default for MozAdsClientBuilder { @@ -109,34 +108,18 @@ impl MozAdsClientBuilder { if let Some(callback) = inner.telemetry.take() { telemetry::install(callback); } - #[cfg(feature = "stateful")] - let store_set = inner.store_config.is_some(); let client_config = AdsClientConfig { cache_config: inner.cache_config.clone().map(Into::into), environment: inner.environment.clone().unwrap_or_default().into(), #[cfg(feature = "stateful")] store_config: inner.store_config.clone().map(Into::into), + #[cfg(feature = "stateful")] + worker_config: inner.worker_config.clone().map(Into::into), }; - #[cfg(feature = "stateful")] - let worker_sleep_duration = client_config - .store_config - .as_ref() - .and_then(|config| config.batch_interval_duration) - .unwrap_or(ADS_CLIENT_WORKER_SLEEP_LENGTH_DEFAULT); let client = AdsClient::new(client_config); let inner = Arc::new(client); - #[cfg(feature = "stateful")] - let worker = if store_set { - worker::BackgroundWorker::new(inner.clone(), worker_sleep_duration) - } else { - worker::BackgroundWorker::new_empty() - }; - MozAdsClient { - inner, - #[cfg(feature = "stateful")] - _worker: worker, - } + MozAdsClient { inner } } pub fn cache_config(self: Arc, cache_config: MozAdsCacheConfig) -> Arc { @@ -149,13 +132,24 @@ impl MozAdsClientBuilder { self } + pub fn telemetry(self: Arc, telemetry: Box) -> Arc { + self.0.lock().telemetry = Some(Arc::from(telemetry)); + self + } +} + +#[cfg(feature = "stateful")] +#[uniffi::export] +impl MozAdsClientBuilder { + #[cfg(feature = "stateful")] pub fn store_config(self: Arc, store_config: MozAdsStoreConfig) -> Arc { self.0.lock().store_config = Some(store_config); self } - pub fn telemetry(self: Arc, telemetry: Box) -> Arc { - self.0.lock().telemetry = Some(Arc::from(telemetry)); + #[cfg(feature = "stateful")] + pub fn worker_config(self: Arc, worker_config: MozAdsWorkerConfig) -> Arc { + self.0.lock().worker_config = Some(worker_config); self } } @@ -191,6 +185,10 @@ pub struct MozAdsCacheConfig { #[derive(Clone, uniffi::Record)] pub struct MozAdsStoreConfig { pub db_path: String, +} + +#[derive(Clone, uniffi::Record)] +pub struct MozAdsWorkerConfig { /// Interval rate for batching commands used by the background worker. pub batch_interval_duration: Option, } @@ -468,6 +466,14 @@ impl From for AdsStoreConfig { fn from(config: MozAdsStoreConfig) -> Self { Self { db_path: config.db_path, + } + } +} + +#[cfg(feature = "stateful")] +impl From for AdsWorkerConfig { + fn from(config: MozAdsWorkerConfig) -> Self { + Self { batch_interval_duration: config.batch_interval_duration, } } diff --git a/components/ads-client/src/lib.rs b/components/ads-client/src/lib.rs index 43454cfba72..a8578e74570 100644 --- a/components/ads-client/src/lib.rs +++ b/components/ads-client/src/lib.rs @@ -30,9 +30,6 @@ pub mod worker; pub use ffi::*; -#[cfg(feature = "stateful")] -use crate::worker::BackgroundWorker; - #[cfg(test)] mod test_utils; @@ -46,8 +43,6 @@ uniffi::custom_type!(AdsClientUrl, String, { #[derive(uniffi::Object)] pub struct MozAdsClient { inner: Arc, - #[cfg(feature = "stateful")] - _worker: BackgroundWorker, } #[uniffi::export] diff --git a/components/ads-client/src/worker.rs b/components/ads-client/src/worker.rs index b515c6f398e..8325061c72c 100644 --- a/components/ads-client/src/worker.rs +++ b/components/ads-client/src/worker.rs @@ -3,28 +3,23 @@ use url::Url; use crate::{ client::{ error::{BackgroundWorkerError, ComponentError}, - AdsClient, + AdsClientInner, }, mars::{ad_request::AdPlacementRequest, ReportReason}, request::{QueuedRequest, RequestQueue}, - telemetry::Telemetry, }; use std::{collections::HashSet, sync::Arc, thread::JoinHandle, time::Duration}; pub const ADS_CLIENT_WORKER_THREAD_NAME: &str = "ads-client.worker"; -// Thread sleep delay to wait for new requests to queue before resolving them. -// This must be a very short amount of time to ensure ads are requested near-immediately on startup. -pub const ADS_CLIENT_WORKER_SLEEP_LENGTH_DEFAULT: Duration = Duration::from_millis(100); - pub struct BackgroundWorker { _worker_thread: Option>, worker_request_queue: Option, } impl BackgroundWorker { - pub fn new( - inner_client: Arc>, + pub fn new( + inner_client: Arc, worker_sleep_duration: Duration, ) -> BackgroundWorker { let request_queue = RequestQueue::new(); @@ -88,8 +83,8 @@ impl BackgroundWorker { } // Endless worker for background thread that synchronously run tasks in the order provided by the RequestQueue. -fn worker( - inner_client: Arc>, +fn worker( + inner_client: Arc, request_queue: RequestQueue, sleep_duration: Duration, ) { @@ -129,9 +124,9 @@ pub enum DispatchRequest { impl DispatchRequest { // Runs a dispatched command synchronously in it's thread. // The dispatched command calls the corresponding `AdsClient` synchronous method, meaning that behavior between the two is shared. - pub fn handle_request( + pub fn handle_request( self, - _ads_client_inner: Arc>, + _ads_client_inner: Arc, ) -> Result<(), ComponentError> { // TODO: Add 'running a request' logic to here. error_support::error!("Running a request is currently not set up yet: {self:?}"); From 6c3c609c8c5af61fa30ce17504dea56dea302d62 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Fri, 16 Oct 2026 13:02:17 -0700 Subject: [PATCH 22/25] fix: some renaming --- components/ads-client/src/worker.rs | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/components/ads-client/src/worker.rs b/components/ads-client/src/worker.rs index 8325061c72c..37a82fbb74d 100644 --- a/components/ads-client/src/worker.rs +++ b/components/ads-client/src/worker.rs @@ -13,8 +13,11 @@ use std::{collections::HashSet, sync::Arc, thread::JoinHandle, time::Duration}; pub const ADS_CLIENT_WORKER_THREAD_NAME: &str = "ads-client.worker"; pub struct BackgroundWorker { - _worker_thread: Option>, - worker_request_queue: Option, + // Worker thread is not needed to be accessed, but the handle needs to be held by the structure. + // If None, no worker thread is present, and attempts to write to the request queue will be rejected + // (because no consumer of the request queue will exist) + thread_handle: Option>, + request_queue: RequestQueue, } impl BackgroundWorker { @@ -34,19 +37,19 @@ impl BackgroundWorker { error_support::error!("Failed to create ads-client worker thread `{ADS_CLIENT_WORKER_THREAD_NAME}` with: {err}") }).ok() else { - return BackgroundWorker { _worker_thread: None, worker_request_queue: None } + return BackgroundWorker { thread_handle: None, request_queue } }; BackgroundWorker { - _worker_thread: Some(worker_thread), - worker_request_queue: Some(request_queue), + thread_handle: Some(worker_thread), + request_queue, } } pub fn new_empty() -> BackgroundWorker { BackgroundWorker { - _worker_thread: None, - worker_request_queue: None, + thread_handle: None, + request_queue: RequestQueue::new(), } } @@ -56,9 +59,9 @@ impl BackgroundWorker { &self, ads_requests: Vec, ) -> Result<(), ComponentError> { - if let Some(worker_dispatch) = &self.worker_request_queue { + if self.thread_handle.is_some() { for req in ads_requests { - worker_dispatch.push_ad_request(req); + self.request_queue.push_ad_request(req); } Ok(()) } else { @@ -73,8 +76,8 @@ impl BackgroundWorker { &self, queued_request: QueuedRequest, ) -> Result<(), ComponentError> { - if let Some(worker_dispatch) = &self.worker_request_queue { - worker_dispatch.push_queued_request(queued_request); + if self.thread_handle.is_some() { + self.request_queue.push_queued_request(queued_request); Ok(()) } else { Err(BackgroundWorkerError::Closed.into()) From b60c9f25bc45fbb6df7297b32334dc8945719beb Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Fri, 16 Oct 2026 13:30:41 -0700 Subject: [PATCH 23/25] fix: goes back to dispatchrequest --- components/ads-client/src/request.rs | 54 +++++++++++++++------------- components/ads-client/src/worker.rs | 28 +++------------ 2 files changed, 33 insertions(+), 49 deletions(-) diff --git a/components/ads-client/src/request.rs b/components/ads-client/src/request.rs index 499bfd5af69..fbe25f7474e 100644 --- a/components/ads-client/src/request.rs +++ b/components/ads-client/src/request.rs @@ -7,8 +7,7 @@ use crate::{ worker::DispatchRequest, }; use std::{ - collections::{HashMap, VecDeque}, - sync::Arc, + collections::{HashMap, HashSet, VecDeque}, sync::Arc, }; pub const MAXIMUM_ADS_BATCH_COUNT: usize = 100; @@ -64,14 +63,27 @@ impl RequestQueue { inner.request_queue.pop_front().map(|c| c.into()) } - pub fn push_ad_request(&self, ad_request: AdPlacementRequest) { + // Add a `DispatchRequest` to the `RequestQueue` (primary entry point for queuing requests). + pub fn push_request(&self, request: DispatchRequest) { + // Split DispatchRequest into either a queue-able request, or a batchable ad request, and queue it. + match request { + DispatchRequest::RequestAds { ad_requests } => self.push_ad_requests(ad_requests), + DispatchRequest::RecordImpression { url } => self.push_queued_request(QueuedRequest::RecordImpression { url }), + DispatchRequest::RecordClick { url } => self.push_queued_request(QueuedRequest::RecordClick { url }), + DispatchRequest::ReportAd { url, reason } => self.push_queued_request(QueuedRequest::ReportAd { url, reason }), + } + } + + fn push_ad_requests(&self, ad_requests: HashSet) { let mut inner = self.inner.lock(); - inner - .queued_ads - .insert(ad_request.placement.clone().into(), ad_request); + for ad_request in ad_requests { + inner + .queued_ads + .insert(ad_request.placement.clone().into(), ad_request); + } } - pub fn push_queued_request(&self, queued_command: QueuedRequest) { + fn push_queued_request(&self, queued_command: QueuedRequest) { let mut inner = self.inner.lock(); inner.request_queue.push_back(queued_command); } @@ -79,7 +91,7 @@ impl RequestQueue { // Queue-able command (ReportAd, etc.) #[derive(Clone, Debug, PartialEq)] -pub enum QueuedRequest { +enum QueuedRequest { RecordClick { url: Url }, RecordImpression { url: Url }, ReportAd { reason: ReportReason, url: Url }, @@ -116,14 +128,6 @@ mod tests { } } - fn example_request_ads(identifier: usize) -> AdPlacementRequest { - AdPlacementRequest { - content: None, - count: 4, - placement: format!("test_placement_{identifier}"), - } - } - fn example_request_ads_dispatch(identifiers: &[usize]) -> DispatchRequest { let mut ad_requests = HashSet::new(); for id in identifiers { @@ -138,26 +142,26 @@ mod tests { #[test] fn identical_command_comes_out() { - let request = example_request_ads(0); + let request = example_request_ads_dispatch(&[0]); let command = example_request_ads_dispatch(&[0]); let queue = RequestQueue::new(); - queue.push_ad_request(request.clone()); + queue.push_request(request.clone()); let retrieved_dispatch = queue.next().expect("Request should exist in queue"); assert_eq!(command, retrieved_dispatch); } #[test] fn batch_similar_requests() { - let request_0 = example_request_ads(0); - let request_1 = example_request_ads(1); + let request_0 = example_request_ads_dispatch(&[0]); + let request_1 = example_request_ads_dispatch(&[1]); let dispatch = example_request_ads_dispatch(&[0, 1]); let queue = RequestQueue::new(); // Having two "placement_0" and one "placement_1" should result in returning a single DispatchRequest with one "placement_0" and one "placement_1" - queue.push_ad_request(request_0.clone()); - queue.push_ad_request(request_0.clone()); - queue.push_ad_request(request_1.clone()); + queue.push_request(request_0.clone()); + queue.push_request(request_0.clone()); + queue.push_request(request_1.clone()); let retrieved_dispatch = queue.next().expect("Request should exist in queue"); @@ -178,8 +182,8 @@ mod tests { // Queue enough of this command to go one-over the limit. for i in 0..(MAXIMUM_ADS_BATCH_COUNT + 1) { - let request = example_request_ads(i); - queue.push_ad_request(request.clone()); + let request = example_request_ads_dispatch(&[i]); + queue.push_request(request.clone()); } let mut retrieved_dispatch_many = diff --git a/components/ads-client/src/worker.rs b/components/ads-client/src/worker.rs index 37a82fbb74d..0d949844288 100644 --- a/components/ads-client/src/worker.rs +++ b/components/ads-client/src/worker.rs @@ -6,7 +6,7 @@ use crate::{ AdsClientInner, }, mars::{ad_request::AdPlacementRequest, ReportReason}, - request::{QueuedRequest, RequestQueue}, + request::RequestQueue, }; use std::{collections::HashSet, sync::Arc, thread::JoinHandle, time::Duration}; @@ -53,35 +53,15 @@ impl BackgroundWorker { } } - // Add to the background worker's queue an ads request. - // These are prioritized and will be batched before sending. - pub fn dispatch_ads_request( - &self, - ads_requests: Vec, - ) -> Result<(), ComponentError> { + // Add to the background worker's queue a request. It may be queued or batched, depending on the request. + pub fn dispatch(&self, request : DispatchRequest) -> Result<(), ComponentError> { if self.thread_handle.is_some() { - for req in ads_requests { - self.request_queue.push_ad_request(req); - } + self.request_queue.push_request(request); Ok(()) } else { Err(BackgroundWorkerError::Closed.into()) } - } - // Add to the background worker's queue a QueuedRequest (a non-ads request). - // (eg: RecordClick, RecordImpression) - // These are queued to send after any ads requests. - pub fn dispatch_queued_request( - &self, - queued_request: QueuedRequest, - ) -> Result<(), ComponentError> { - if self.thread_handle.is_some() { - self.request_queue.push_queued_request(queued_request); - Ok(()) - } else { - Err(BackgroundWorkerError::Closed.into()) - } } } From 82455103ba50d25b5a067d9c641516df3294878c Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Fri, 16 Oct 2026 13:47:01 -0700 Subject: [PATCH 24/25] fix: merg conflict, fixes --- components/ads-client/src/client.rs | 12 ++++++++++++ components/ads-client/src/request.rs | 15 +++++++++++---- components/ads-client/src/worker.rs | 3 +-- 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/components/ads-client/src/client.rs b/components/ads-client/src/client.rs index e3343f2eb05..5b78697b161 100644 --- a/components/ads-client/src/client.rs +++ b/components/ads-client/src/client.rs @@ -149,6 +149,18 @@ impl AdsClient { ) } + pub fn request_spoc_or_image_ads( + &self, + ad_placement_requests: Vec, + flags: AdRequestFlags, + options: Option, + ohttp: bool, + blocks: Vec, + ) -> Result>, RequestAdsError> { + self.inner + .request_spoc_or_image_ads(ad_placement_requests, flags, options, ohttp, blocks) + } + pub fn shutdown(&self) { self.inner.shutdown(); } diff --git a/components/ads-client/src/request.rs b/components/ads-client/src/request.rs index fbe25f7474e..8e520ebf930 100644 --- a/components/ads-client/src/request.rs +++ b/components/ads-client/src/request.rs @@ -7,7 +7,8 @@ use crate::{ worker::DispatchRequest, }; use std::{ - collections::{HashMap, HashSet, VecDeque}, sync::Arc, + collections::{HashMap, HashSet, VecDeque}, + sync::Arc, }; pub const MAXIMUM_ADS_BATCH_COUNT: usize = 100; @@ -68,9 +69,15 @@ impl RequestQueue { // Split DispatchRequest into either a queue-able request, or a batchable ad request, and queue it. match request { DispatchRequest::RequestAds { ad_requests } => self.push_ad_requests(ad_requests), - DispatchRequest::RecordImpression { url } => self.push_queued_request(QueuedRequest::RecordImpression { url }), - DispatchRequest::RecordClick { url } => self.push_queued_request(QueuedRequest::RecordClick { url }), - DispatchRequest::ReportAd { url, reason } => self.push_queued_request(QueuedRequest::ReportAd { url, reason }), + DispatchRequest::RecordImpression { url } => { + self.push_queued_request(QueuedRequest::RecordImpression { url }) + } + DispatchRequest::RecordClick { url } => { + self.push_queued_request(QueuedRequest::RecordClick { url }) + } + DispatchRequest::ReportAd { url, reason } => { + self.push_queued_request(QueuedRequest::ReportAd { url, reason }) + } } } diff --git a/components/ads-client/src/worker.rs b/components/ads-client/src/worker.rs index 0d949844288..f8dc2ec1230 100644 --- a/components/ads-client/src/worker.rs +++ b/components/ads-client/src/worker.rs @@ -54,14 +54,13 @@ impl BackgroundWorker { } // Add to the background worker's queue a request. It may be queued or batched, depending on the request. - pub fn dispatch(&self, request : DispatchRequest) -> Result<(), ComponentError> { + pub fn dispatch(&self, request: DispatchRequest) -> Result<(), ComponentError> { if self.thread_handle.is_some() { self.request_queue.push_request(request); Ok(()) } else { Err(BackgroundWorkerError::Closed.into()) } - } } From 18e7bdc02843aa5e56e252cfe7335b491a25ee40 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Fri, 16 Oct 2026 13:58:44 -0700 Subject: [PATCH 25/25] fix: fmt --- components/ads-client/src/client.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/components/ads-client/src/client.rs b/components/ads-client/src/client.rs index 5b78697b161..737ce47ecee 100644 --- a/components/ads-client/src/client.rs +++ b/components/ads-client/src/client.rs @@ -511,6 +511,7 @@ mod tests { }); AdsClient { inner: inner.clone(), + #[cfg(feature = "stateful")] _worker: BackgroundWorker::new(inner.clone(), ADS_CLIENT_WORKER_SLEEP_LENGTH_DEFAULT), } }