From dcad72b538df4e0be9d27fb251cac79dbb0300ed Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Sun, 2 Aug 2026 12:05:09 -0700 Subject: [PATCH 01/12] feat: Initial work for background worker --- components/ads-client/src/ads_cache.rs | 59 +++++++++++++++ components/ads-client/src/client.rs | 18 +++-- components/ads-client/src/ffi.rs | 10 +-- components/ads-client/src/lib.rs | 13 +++- components/ads-client/src/worker.rs | 99 ++++++++++++++++++++++++++ 5 files changed, 187 insertions(+), 12 deletions(-) create mode 100644 components/ads-client/src/ads_cache.rs create mode 100644 components/ads-client/src/worker.rs diff --git a/components/ads-client/src/ads_cache.rs b/components/ads-client/src/ads_cache.rs new file mode 100644 index 00000000000..03f986b4cfe --- /dev/null +++ b/components/ads-client/src/ads_cache.rs @@ -0,0 +1,59 @@ +use crate::mars::ad_response::{AdImage, AdSpoc, AdTile}; +use std::{collections::HashMap, time::Duration}; + +// TODO: This is an intentionally naive in-memory cache implementation of the ads cache. +// It functions as a skeleton to store ads fetched in the background, and has a naive expiration mechanism. +// The subsequent vertical slice will replace this in its entirety with the http_cache sqlite database instead, with TTLs, persistent storage, etc. +const DEFAULT_TTL: Duration = Duration::from_secs(300); + +pub struct AdsCache { + image_ads : HashMap)>, + spoc_ads : HashMap)>, + tile_ads : HashMap)> +} + +impl AdsCache { + pub fn new() -> Self { + AdsCache { image_ads: HashMap::new(), spoc_ads: HashMap::new(), tile_ads: HashMap::new() } + } + + pub fn cache_ads(&mut self, ads : HashMap>, timestamp: u64) { + AdsCacheable::cache_ads(ads, self, timestamp); + } +} + +pub trait AdsCacheable : Sized { + fn cache_ads(ads : HashMap>, ads_cache : &mut AdsCache, timestamp: u64); + fn fetch_cached_ad(self, ads_cache : &AdsCache); +} + +impl AdsCacheable for AdImage { + fn cache_ads(ads: HashMap>, ads_cache : &mut AdsCache, timestamp : u64) { + ads_cache.image_ads.extend(ads.into_iter().map(|(key, ad) |(key, (timestamp, ad)))); + ads_cache.image_ads.retain(|_, (x, _)| *x - timestamp > DEFAULT_TTL.as_secs()); + } + + fn fetch_cached_ad(self, ads_cache : &AdsCache) { + todo!() + } +} + +impl AdsCacheable for AdSpoc { + fn cache_ads(ads: HashMap>, ads_cache : &mut AdsCache, timestamp : u64) { + ads_cache.spoc_ads.extend(ads.into_iter().map(|(key, ad) |(key, (timestamp, ad)))); + ads_cache.spoc_ads.retain(|_, (x, _)| *x - timestamp > DEFAULT_TTL.as_secs()); + } + fn fetch_cached_ad(self, ads_cache : &AdsCache) { + todo!() + } +} + +impl AdsCacheable for AdTile { + fn cache_ads(ads: HashMap>, ads_cache : &mut AdsCache, timestamp : u64) { + ads_cache.tile_ads.extend(ads.into_iter().map(|(key, ad) |(key, (timestamp, ad)))); + ads_cache.tile_ads.retain(|_, (x, _)| *x - timestamp > DEFAULT_TTL.as_secs()); + } + fn fetch_cached_ad(self, ads_cache : &AdsCache) { + todo!() + } +} \ No newline at end of file diff --git a/components/ads-client/src/client.rs b/components/ads-client/src/client.rs index 5c818e2a4c5..7207f3ea6ef 100644 --- a/components/ads-client/src/client.rs +++ b/components/ads-client/src/client.rs @@ -5,7 +5,7 @@ use std::collections::HashMap; use std::time::Duration; - +use crate::ads_cache::{AdsCache, AdsCacheable}; use crate::http_cache::{ByteSize, CachePolicy, HttpCache}; use crate::mars::ad_request::{AdPlacementRequest, AdRequestFlags}; use crate::mars::ad_response::{AdImage, AdResponse, AdResponseValue, AdSpoc, AdTile}; @@ -42,6 +42,7 @@ where client: MARSClient, context_id_provider: Box, telemetry: T, + ads_cache : AdsCache, } impl AdsClient @@ -91,6 +92,7 @@ where client, context_id_provider, telemetry: telemetry.clone(), + ads_cache: AdsCache::new() } } @@ -98,6 +100,13 @@ where self.client.clear_cache() } + pub fn cache_ads(&mut self, ads : HashMap>) { + // TODO: is this timestamp correct? + // TODO: cast + let now = chrono::Utc::now().timestamp() as u64; + self.ads_cache.cache_ads(ads , now); + } + pub fn get_context_id(&self) -> context_id::ApiResult { self.context_id_provider.context_id() } @@ -264,12 +273,10 @@ pub enum ClientOperationEvent { #[cfg(test)] mod tests { use crate::{ - ffi::telemetry::MozAdsTelemetryWrapper, - mars::Environment, - test_utils::{ + ffi::telemetry::MozAdsTelemetryWrapper, mars::Environment, test_utils::{ get_example_happy_image_response, get_example_happy_spoc_response, get_example_happy_uatile_response, make_happy_placement_requests, - }, + } }; use super::*; @@ -286,6 +293,7 @@ mod tests { Box::new(DefaultContextIdCallback), )), telemetry: MozAdsTelemetryWrapper::noop(), + ads_cache: AdsCache::new(), } } diff --git a/components/ads-client/src/ffi.rs b/components/ads-client/src/ffi.rs index ba50352fdc1..7a88168be56 100644 --- a/components/ads-client/src/ffi.rs +++ b/components/ads-client/src/ffi.rs @@ -7,7 +7,6 @@ pub mod error; pub mod telemetry; use std::sync::Arc; - use crate::client::config::{AdsCacheConfig, AdsClientConfig}; use crate::client::{AdsClient, ContextIdProvider}; use crate::ffi::telemetry::MozAdsTelemetryWrapper; @@ -20,14 +19,13 @@ use crate::mars::ad_response::{ }; use crate::mars::Environment; use crate::mars::ReportReason; -use crate::AdsClientUrl; +use crate::{AdsClientUrl, worker}; use crate::MozAdsClient; use parking_lot::Mutex; use std::collections::HashMap; pub use error::{AdsClientApiResult, MozAdsClientApiError}; pub use telemetry::MozAdsTelemetry; - // TODO: Temporary workaround for HNT requirements — do not use for new integrations. // Context ID management should remain internal to the ads client and this interface should be removed. #[uniffi::export(with_foreign)] @@ -139,8 +137,12 @@ impl MozAdsClientBuilder { .unwrap_or_else(MozAdsTelemetryWrapper::noop), }; let client = AdsClient::new(client_config); + let inner = Arc::new(Mutex::new(client)); + let (worker_dispatch, worker_thread) = Option::unzip(worker::build_worker_thread(inner.clone())); MozAdsClient { - inner: Mutex::new(client), + inner, + _worker_thread: worker_thread, + worker_dispatch } } diff --git a/components/ads-client/src/lib.rs b/components/ads-client/src/lib.rs index 2088ee16cb6..b98b07b605f 100644 --- a/components/ads-client/src/lib.rs +++ b/components/ads-client/src/lib.rs @@ -3,7 +3,7 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -use std::collections::HashMap; +use std::{collections::HashMap, sync::{Arc, mpsc::SyncSender}, thread::JoinHandle}; use client::error::ComponentError; use error_support::handle_error; @@ -20,10 +20,12 @@ mod ffi; pub mod http_cache; mod mars; pub mod telemetry; +pub mod worker; +pub mod ads_cache; pub use ffi::*; -use crate::ffi::telemetry::MozAdsTelemetryWrapper; +use crate::{ffi::telemetry::MozAdsTelemetryWrapper, worker::DispatchCommand}; #[cfg(test)] mod test_utils; @@ -38,9 +40,14 @@ uniffi::custom_type!(AdsClientUrl, String, { #[derive(uniffi::Object)] pub struct MozAdsClient { - inner: Mutex>, + inner: MozAdsClientInner, + + _worker_thread: Option>, + worker_dispatch: Option>, } +pub type MozAdsClientInner = Arc>>; + #[uniffi::export] impl MozAdsClient { pub fn clear_cache(&self) -> AdsClientApiResult<()> { diff --git a/components/ads-client/src/worker.rs b/components/ads-client/src/worker.rs new file mode 100644 index 00000000000..b892d089da0 --- /dev/null +++ b/components/ads-client/src/worker.rs @@ -0,0 +1,99 @@ +use crate::{ + AdsClientApiResult, MozAdsClientApiError, MozAdsClientInner, MozAdsPlacementRequest, MozAdsRequestOptions, client::error::ComponentError, http_cache::CachePolicy, mars::{ + ad_request::{AdPlacementRequest, AdRequestFlags}, ad_response::AdImage, + } +}; +use error_support::{convert_log_report_error, handle_error}; +use std::{collections::HashMap, sync::mpsc::{self, Receiver, SyncSender}, thread::JoinHandle}; + +pub const ADS_CLIENT_WORKER_CHANNEL_BUFFER_SIZE: usize = 1000; +pub const ADS_CLIENT_WORKER_THREAD_NAME : &'static str = "ads-client.worker"; + +// 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) { + while let Ok(task) = rx.recv() { + + // Synchronously run tasks in the order they are passed in this separate channel. + let task: DispatchCommand = task; + task.run_command(&inner_client) + .expect("TODO: handle error here. maybe retries should be here?"); + } +} + +pub enum DispatchCommand { + // TODO: Extend this with other possible commands in V2. + RequestImageAds { + image_ad_requests: Vec, + options: Option, + callback: Option> + } +} + +impl DispatchCommand { + #[handle_error(ComponentError)] + pub fn run_command(self, ads_client_inner: &MozAdsClientInner) -> AdsClientApiResult<()> { + // TODO: Duplicated behavior with the sync functions. Behavior is pretty simple though- probably OK to duplicate. + match self { + DispatchCommand::RequestImageAds { + image_ad_requests, + options, + callback + } => { + let resp = (|| { + let mut inner = ads_client_inner.lock(); + let options = options.unwrap_or_default(); + let flags = AdRequestFlags::from(&options); + let ohttp = options.ohttp; + let cache_policy: CachePolicy = options.into(); + + // Image ads + if image_ad_requests.len() > 0 { + 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) + .map_err(ComponentError::RequestAds)?; + let image_response: HashMap> = image_response.into_iter().map(|(k, v)| (k, vec![v.into()])).collect(); + inner.cache_ads(image_response); + } + Ok::<(), ComponentError>(()) + })(); + match resp { + Ok(_) => (), + Err(e) => handle_background_worker_error(e, callback), + } + } + } + Ok(()) + } +} + +// Handles an error thrown by the background worker by converting it to public-facing error type (MozAdsClientApiError) and logging it. +// If a callback was provided, public error gets sent there too. +fn handle_background_worker_error(err : ComponentError, callback : Option>) { + let err : MozAdsClientApiError = convert_log_report_error(err); + if let Some(callback) = callback { + callback.on_error(err); + } +} + +// TODO: Uniffi does not currently support direct function callbacks, so we use an interface. +// TODO: We use #[uniffi::export(callback_interface)] rather than #[uniffi::export(with_foreign)] for reasons discussed here: +// - https://github.com/mozilla/application-services/pull/7443 +// In the future, this should be changed to uniffi::export(impl = "foreign") alongside the enum change, when uniffi = 0.32 +#[uniffi::export(callback_interface)] +pub trait ErrorOnlyRequestCallback: Send + Sync { + fn on_error(&self, err: MozAdsClientApiError); +} + From d36cebb0e72e4db163135e27bb6051de1251c523 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Sun, 2 Aug 2026 14:11:05 -0700 Subject: [PATCH 02/12] feat: Fleshes out the querying flow --- components/ads-client/src/ads_cache.rs | 21 ++++++++++------ components/ads-client/src/client.rs | 4 ++++ components/ads-client/src/client/error.rs | 9 ++++++- components/ads-client/src/lib.rs | 29 ++++++++++++++++++++++- 4 files changed, 54 insertions(+), 9 deletions(-) diff --git a/components/ads-client/src/ads_cache.rs b/components/ads-client/src/ads_cache.rs index 03f986b4cfe..ea54071c9ad 100644 --- a/components/ads-client/src/ads_cache.rs +++ b/components/ads-client/src/ads_cache.rs @@ -20,11 +20,18 @@ impl AdsCache { pub fn cache_ads(&mut self, ads : HashMap>, timestamp: u64) { AdsCacheable::cache_ads(ads, self, timestamp); } + + pub fn get_cached_ads<'a, T: AdsCacheable>(&'a self, placement : &str) -> Option<&'a Vec> { + // TODO: Note that some of them return T and some return Vec. Where should this handling be done? + // TODO: separate functions here? separate functions in surface? + // TODO: also there are existing trait for ads- use this? + AdsCacheable::fetch_cached_ads(&self, placement) + } } pub trait AdsCacheable : Sized { fn cache_ads(ads : HashMap>, ads_cache : &mut AdsCache, timestamp: u64); - fn fetch_cached_ad(self, ads_cache : &AdsCache); + fn fetch_cached_ads<'a>(ads_cache : &'a AdsCache, id : &str) -> Option<&'a Vec>; } impl AdsCacheable for AdImage { @@ -33,8 +40,8 @@ impl AdsCacheable for AdImage { ads_cache.image_ads.retain(|_, (x, _)| *x - timestamp > DEFAULT_TTL.as_secs()); } - fn fetch_cached_ad(self, ads_cache : &AdsCache) { - todo!() + fn fetch_cached_ads<'a>(ads_cache : &'a AdsCache, id : &str) -> Option<&'a Vec> { + ads_cache.image_ads.get(id).map(|(_, ads)| ads) } } @@ -43,8 +50,8 @@ impl AdsCacheable for AdSpoc { ads_cache.spoc_ads.extend(ads.into_iter().map(|(key, ad) |(key, (timestamp, ad)))); ads_cache.spoc_ads.retain(|_, (x, _)| *x - timestamp > DEFAULT_TTL.as_secs()); } - fn fetch_cached_ad(self, ads_cache : &AdsCache) { - todo!() + fn fetch_cached_ads<'a>(ads_cache : &'a AdsCache, id : &str) -> Option<&'a Vec> { + ads_cache.spoc_ads.get(id).map(|(_, ads)| ads) } } @@ -53,7 +60,7 @@ impl AdsCacheable for AdTile { ads_cache.tile_ads.extend(ads.into_iter().map(|(key, ad) |(key, (timestamp, ad)))); ads_cache.tile_ads.retain(|_, (x, _)| *x - timestamp > DEFAULT_TTL.as_secs()); } - fn fetch_cached_ad(self, ads_cache : &AdsCache) { - todo!() + fn fetch_cached_ads<'a>(ads_cache : &'a AdsCache, id : &str) -> Option<&'a Vec> { + ads_cache.tile_ads.get(id).map(|(_, ads)| ads) } } \ No newline at end of file diff --git a/components/ads-client/src/client.rs b/components/ads-client/src/client.rs index 7207f3ea6ef..9deca4b9e15 100644 --- a/components/ads-client/src/client.rs +++ b/components/ads-client/src/client.rs @@ -107,6 +107,10 @@ where self.ads_cache.cache_ads(ads , now); } + pub fn get_cached_ads(&self, placement_id : &str) -> Option<&Vec> { + self.ads_cache.get_cached_ads(&placement_id) + } + 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..8a5e78eedb4 100644 --- a/components/ads-client/src/client/error.rs +++ b/components/ads-client/src/client/error.rs @@ -3,7 +3,8 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -use crate::mars::error::{FetchAdsError, RecordClickError, RecordImpressionError, ReportAdError}; +use std::sync::mpsc::TrySendError; +use crate::{mars::error::{FetchAdsError, RecordClickError, RecordImpressionError, ReportAdError}, worker::DispatchCommand}; #[derive(Debug, thiserror::Error)] pub enum ComponentError { @@ -27,4 +28,10 @@ pub enum RequestAdsError { #[error("Error requesting ads from MARS: {0}")] FetchAds(#[from] FetchAdsError), + + #[error("Error requesting new ads from the background worker: {0}")] + BackgroundWorkerFullError(#[from] TrySendError), + + #[error("Error requesting new ads from the background worker: background worker is closed")] + BackgroundWorkerClosedError } diff --git a/components/ads-client/src/lib.rs b/components/ads-client/src/lib.rs index b98b07b605f..806e18e712e 100644 --- a/components/ads-client/src/lib.rs +++ b/components/ads-client/src/lib.rs @@ -25,7 +25,7 @@ pub mod ads_cache; pub use ffi::*; -use crate::{ffi::telemetry::MozAdsTelemetryWrapper, worker::DispatchCommand}; +use crate::{client::error::RequestAdsError, ffi::telemetry::MozAdsTelemetryWrapper, mars::ad_response::AdImage, worker::{DispatchCommand, ErrorOnlyRequestCallback}}; #[cfg(test)] mod test_utils; @@ -168,4 +168,31 @@ impl MozAdsClient { .map_err(ComponentError::RequestAds)?; Ok(response.into_iter().map(|(k, v)| (k, v.into())).collect()) } + + #[handle_error(ComponentError)] + #[uniffi::method(default(options = None))] + pub fn prefetch_image_ads( + &self, + image_ad_requests: Vec, + options: Option, + callback: Option> + ) -> AdsClientApiResult<()> { + if let Some(worker_dispatch) = &self.worker_dispatch { + worker_dispatch.try_send(DispatchCommand::RequestImageAds { image_ad_requests, options, callback }).map_err(RequestAdsError::from)?; + Ok(()) + } else { + Err(RequestAdsError::BackgroundWorkerClosedError.into()) + } + } + + #[handle_error(ComponentError)] + #[uniffi::method()] + pub fn query_image_ads( + &self, + placement_id: String, + ) -> AdsClientApiResult> { + let inner = self.inner.lock(); + let image_ads : Option<&Vec> = inner.get_cached_ads(&placement_id); + Ok(image_ads.and_then(|ad| ad.into_iter().next().cloned()).map(|ad| ad.into())) + } } From 860e022968005075c720a3feebf546b3afa6102e Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Sun, 2 Aug 2026 15:48:54 -0700 Subject: [PATCH 03/12] fix: Running test, ping, some refactoring --- .../integration-tests/tests/mars_async.rs | 66 ++++++++++++ components/ads-client/src/ads_cache.rs | 63 +++++++---- components/ads-client/src/client.rs | 20 ++-- components/ads-client/src/client/error.rs | 38 +++++-- components/ads-client/src/ffi.rs | 9 +- components/ads-client/src/lib.rs | 77 +++++++++++--- components/ads-client/src/worker.rs | 100 ++++++++++-------- 7 files changed, 276 insertions(+), 97 deletions(-) create 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 new file mode 100644 index 00000000000..c6579f85b04 --- /dev/null +++ b/components/ads-client/integration-tests/tests/mars_async.rs @@ -0,0 +1,66 @@ +/* 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 std::{sync::Arc, time::Duration}; + +use ads_client::{ + MozAdsClientBuilder, MozAdsEnvironment, MozAdsPlacementRequest, worker::ErrorOnlyRequestCallback, +}; + +fn init_backend() { + viaduct_hyper::viaduct_init_backend_hyper(); +} + +fn prod_client() -> ads_client::MozAdsClient { + Arc::new(MozAdsClientBuilder::new()) + .environment(MozAdsEnvironment::Prod) + .build() +} + +struct TestErrorCallback; +impl ErrorOnlyRequestCallback for TestErrorCallback { + fn on_error(&self,err: ads_client::MozAdsClientApiError) { + panic!("Error received in background worker callback: {err}") + } +} + +#[test] +#[ignore = "integration test: run manually with -- --ignored"] +fn test_contract_image_prod() { + init_backend(); + + let placement_id= "mock_billboard_1".to_string(); + let client = prod_client(); + let result = client.prefetch_image_ads(vec![MozAdsPlacementRequest { + iab_content: None, + placement_id: placement_id.clone(), + }], + None, Some(Box::new(TestErrorCallback))); + + assert!( + result.is_ok(), + "Image ad dispatch request failed: {:?}", + result.err() + ); + + // TODO: magic number + // TODO: good consistent ping + let ping = client.ping_background_worker(Some(Duration::from_secs(30)), None); + assert!( + ping.is_ok(), + "Ping failed: {:?}", + ping.err() + ); + + 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()); +} \ No newline at end of file diff --git a/components/ads-client/src/ads_cache.rs b/components/ads-client/src/ads_cache.rs index ea54071c9ad..cf0950fb379 100644 --- a/components/ads-client/src/ads_cache.rs +++ b/components/ads-client/src/ads_cache.rs @@ -1,27 +1,32 @@ use crate::mars::ad_response::{AdImage, AdSpoc, AdTile}; use std::{collections::HashMap, time::Duration}; -// TODO: This is an intentionally naive in-memory cache implementation of the ads cache. +// TODO: This is an intentionally naive in-memory cache implementation of the ads cache. // It functions as a skeleton to store ads fetched in the background, and has a naive expiration mechanism. // The subsequent vertical slice will replace this in its entirety with the http_cache sqlite database instead, with TTLs, persistent storage, etc. const DEFAULT_TTL: Duration = Duration::from_secs(300); +#[derive(Debug)] pub struct AdsCache { - image_ads : HashMap)>, - spoc_ads : HashMap)>, - tile_ads : HashMap)> + image_ads: HashMap)>, + spoc_ads: HashMap)>, + tile_ads: HashMap)>, } impl AdsCache { pub fn new() -> Self { - AdsCache { image_ads: HashMap::new(), spoc_ads: HashMap::new(), tile_ads: HashMap::new() } + AdsCache { + image_ads: HashMap::new(), + spoc_ads: HashMap::new(), + tile_ads: HashMap::new(), + } } - pub fn cache_ads(&mut self, ads : HashMap>, timestamp: u64) { + pub fn cache_ads(&mut self, ads: HashMap>, timestamp: u64) { AdsCacheable::cache_ads(ads, self, timestamp); } - pub fn get_cached_ads<'a, T: AdsCacheable>(&'a self, placement : &str) -> Option<&'a Vec> { + pub fn get_cached_ads<'a, T: AdsCacheable>(&'a self, placement: &str) -> Option<&'a Vec> { // TODO: Note that some of them return T and some return Vec. Where should this handling be done? // TODO: separate functions here? separate functions in surface? // TODO: also there are existing trait for ads- use this? @@ -29,38 +34,50 @@ impl AdsCache { } } -pub trait AdsCacheable : Sized { - fn cache_ads(ads : HashMap>, ads_cache : &mut AdsCache, timestamp: u64); - fn fetch_cached_ads<'a>(ads_cache : &'a AdsCache, id : &str) -> Option<&'a Vec>; +pub trait AdsCacheable: Sized { + fn cache_ads(ads: HashMap>, ads_cache: &mut AdsCache, timestamp: u64); + fn fetch_cached_ads<'a>(ads_cache: &'a AdsCache, id: &str) -> Option<&'a Vec>; } impl AdsCacheable for AdImage { - fn cache_ads(ads: HashMap>, ads_cache : &mut AdsCache, timestamp : u64) { - ads_cache.image_ads.extend(ads.into_iter().map(|(key, ad) |(key, (timestamp, ad)))); - ads_cache.image_ads.retain(|_, (x, _)| *x - timestamp > DEFAULT_TTL.as_secs()); + fn cache_ads(ads: HashMap>, ads_cache: &mut AdsCache, timestamp: u64) { + ads_cache + .image_ads + .extend(ads.into_iter().map(|(key, ad)| (key, (timestamp, ad)))); + ads_cache + .image_ads + .retain(|_, (x, _)| *x - timestamp < DEFAULT_TTL.as_secs()); } - fn fetch_cached_ads<'a>(ads_cache : &'a AdsCache, id : &str) -> Option<&'a Vec> { + fn fetch_cached_ads<'a>(ads_cache: &'a AdsCache, id: &str) -> Option<&'a Vec> { ads_cache.image_ads.get(id).map(|(_, ads)| ads) } } impl AdsCacheable for AdSpoc { - fn cache_ads(ads: HashMap>, ads_cache : &mut AdsCache, timestamp : u64) { - ads_cache.spoc_ads.extend(ads.into_iter().map(|(key, ad) |(key, (timestamp, ad)))); - ads_cache.spoc_ads.retain(|_, (x, _)| *x - timestamp > DEFAULT_TTL.as_secs()); + fn cache_ads(ads: HashMap>, ads_cache: &mut AdsCache, timestamp: u64) { + ads_cache + .spoc_ads + .extend(ads.into_iter().map(|(key, ad)| (key, (timestamp, ad)))); + ads_cache + .spoc_ads + .retain(|_, (x, _)| *x - timestamp < DEFAULT_TTL.as_secs()); } - fn fetch_cached_ads<'a>(ads_cache : &'a AdsCache, id : &str) -> Option<&'a Vec> { + fn fetch_cached_ads<'a>(ads_cache: &'a AdsCache, id: &str) -> Option<&'a Vec> { ads_cache.spoc_ads.get(id).map(|(_, ads)| ads) } } impl AdsCacheable for AdTile { - fn cache_ads(ads: HashMap>, ads_cache : &mut AdsCache, timestamp : u64) { - ads_cache.tile_ads.extend(ads.into_iter().map(|(key, ad) |(key, (timestamp, ad)))); - ads_cache.tile_ads.retain(|_, (x, _)| *x - timestamp > DEFAULT_TTL.as_secs()); + fn cache_ads(ads: HashMap>, ads_cache: &mut AdsCache, timestamp: u64) { + ads_cache + .tile_ads + .extend(ads.into_iter().map(|(key, ad)| (key, (timestamp, ad)))); + ads_cache + .tile_ads + .retain(|_, (x, _)| *x - timestamp < DEFAULT_TTL.as_secs()); } - fn fetch_cached_ads<'a>(ads_cache : &'a AdsCache, id : &str) -> Option<&'a Vec> { + fn fetch_cached_ads<'a>(ads_cache: &'a AdsCache, id: &str) -> Option<&'a Vec> { ads_cache.tile_ads.get(id).map(|(_, ads)| ads) } -} \ No newline at end of file +} diff --git a/components/ads-client/src/client.rs b/components/ads-client/src/client.rs index 9deca4b9e15..16531a44ed7 100644 --- a/components/ads-client/src/client.rs +++ b/components/ads-client/src/client.rs @@ -3,8 +3,6 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -use std::collections::HashMap; -use std::time::Duration; use crate::ads_cache::{AdsCache, AdsCacheable}; use crate::http_cache::{ByteSize, CachePolicy, HttpCache}; use crate::mars::ad_request::{AdPlacementRequest, AdRequestFlags}; @@ -15,6 +13,8 @@ use crate::telemetry::Telemetry; use config::AdsClientConfig; use context_id::{ContextIDComponent, DefaultContextIdCallback}; use error::RequestAdsError; +use std::collections::HashMap; +use std::time::Duration; use url::Url; use uuid::Uuid; @@ -42,7 +42,7 @@ where client: MARSClient, context_id_provider: Box, telemetry: T, - ads_cache : AdsCache, + ads_cache: AdsCache, } impl AdsClient @@ -92,7 +92,7 @@ where client, context_id_provider, telemetry: telemetry.clone(), - ads_cache: AdsCache::new() + ads_cache: AdsCache::new(), } } @@ -100,14 +100,14 @@ where self.client.clear_cache() } - pub fn cache_ads(&mut self, ads : HashMap>) { + pub fn cache_ads(&mut self, ads: HashMap>) { // TODO: is this timestamp correct? // TODO: cast let now = chrono::Utc::now().timestamp() as u64; - self.ads_cache.cache_ads(ads , now); + self.ads_cache.cache_ads(ads, now); } - pub fn get_cached_ads(&self, placement_id : &str) -> Option<&Vec> { + pub fn get_cached_ads(&self, placement_id: &str) -> Option<&Vec> { self.ads_cache.get_cached_ads(&placement_id) } @@ -277,10 +277,12 @@ pub enum ClientOperationEvent { #[cfg(test)] mod tests { use crate::{ - ffi::telemetry::MozAdsTelemetryWrapper, mars::Environment, test_utils::{ + ffi::telemetry::MozAdsTelemetryWrapper, + mars::Environment, + test_utils::{ get_example_happy_image_response, get_example_happy_spoc_response, get_example_happy_uatile_response, make_happy_placement_requests, - } + }, }; use super::*; diff --git a/components/ads-client/src/client/error.rs b/components/ads-client/src/client/error.rs index 8a5e78eedb4..d75b6c34a05 100644 --- a/components/ads-client/src/client/error.rs +++ b/components/ads-client/src/client/error.rs @@ -3,8 +3,11 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -use std::sync::mpsc::TrySendError; -use crate::{mars::error::{FetchAdsError, RecordClickError, RecordImpressionError, ReportAdError}, worker::DispatchCommand}; +use crate::{ + mars::error::{FetchAdsError, RecordClickError, RecordImpressionError, ReportAdError}, + worker::{Dispatch, DispatchCommand}, +}; +use std::sync::mpsc::{RecvTimeoutError, TrySendError}; #[derive(Debug, thiserror::Error)] pub enum ComponentError { @@ -19,6 +22,9 @@ pub enum ComponentError { #[error("Error requesting ads: {0}")] RequestAds(#[from] RequestAdsError), + + #[error("Error requesting ads from worker: {0}")] + BackgroundWorker(#[from] BackgroundWorkerError), } #[derive(Debug, thiserror::Error)] @@ -28,10 +34,30 @@ pub enum RequestAdsError { #[error("Error requesting ads from MARS: {0}")] FetchAds(#[from] FetchAdsError), +} + +#[derive(Debug, thiserror::Error)] +pub enum BackgroundWorkerError { + #[error("Error requesting new ads from the background worker: worker full")] + WorkerFull, - #[error("Error requesting new ads from the background worker: {0}")] - BackgroundWorkerFullError(#[from] TrySendError), + #[error("Error requesting new ads from the background worker: worker closed")] + WorkerClosed, + + #[error("Worker timed out waiting for response: {0}")] + WorkerTimedOut(#[from] RecvTimeoutError), + + #[error("Error sending pong back from background worker")] + PongFailure(Box>), +} - #[error("Error requesting new ads from the background worker: background worker is closed")] - BackgroundWorkerClosedError +impl From> for BackgroundWorkerError { + // TODO: Should we keep the 'dispatch' here somehow instead of just dropping it for retry reasons? + // Intended strategy for retry is probably using sqlite instead. + 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 7a88168be56..b5e299fa4f4 100644 --- a/components/ads-client/src/ffi.rs +++ b/components/ads-client/src/ffi.rs @@ -6,7 +6,6 @@ pub mod error; pub mod telemetry; -use std::sync::Arc; use crate::client::config::{AdsCacheConfig, AdsClientConfig}; use crate::client::{AdsClient, ContextIdProvider}; use crate::ffi::telemetry::MozAdsTelemetryWrapper; @@ -19,10 +18,11 @@ use crate::mars::ad_response::{ }; use crate::mars::Environment; use crate::mars::ReportReason; -use crate::{AdsClientUrl, worker}; use crate::MozAdsClient; +use crate::{worker, AdsClientUrl}; use parking_lot::Mutex; use std::collections::HashMap; +use std::sync::Arc; pub use error::{AdsClientApiResult, MozAdsClientApiError}; pub use telemetry::MozAdsTelemetry; @@ -138,11 +138,12 @@ impl MozAdsClientBuilder { }; let client = AdsClient::new(client_config); let inner = Arc::new(Mutex::new(client)); - let (worker_dispatch, worker_thread) = Option::unzip(worker::build_worker_thread(inner.clone())); + let (worker_dispatch, worker_thread) = + Option::unzip(worker::build_worker_thread(inner.clone())); MozAdsClient { inner, _worker_thread: worker_thread, - worker_dispatch + worker_dispatch, } } diff --git a/components/ads-client/src/lib.rs b/components/ads-client/src/lib.rs index 806e18e712e..e2287c6a430 100644 --- a/components/ads-client/src/lib.rs +++ b/components/ads-client/src/lib.rs @@ -3,7 +3,15 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -use std::{collections::HashMap, sync::{Arc, mpsc::SyncSender}, thread::JoinHandle}; +use std::{ + collections::HashMap, + sync::{ + mpsc::{self, SyncSender}, + Arc, + }, + thread::JoinHandle, + time::Duration, +}; use client::error::ComponentError; use error_support::handle_error; @@ -15,17 +23,22 @@ use client::AdsClient; use http_cache::CachePolicy; use mars::ad_request::{AdPlacementRequest, AdRequestFlags}; +pub mod ads_cache; mod client; mod ffi; pub mod http_cache; mod mars; pub mod telemetry; pub mod worker; -pub mod ads_cache; pub use ffi::*; -use crate::{client::error::RequestAdsError, ffi::telemetry::MozAdsTelemetryWrapper, mars::ad_response::AdImage, worker::{DispatchCommand, ErrorOnlyRequestCallback}}; +use crate::{ + client::error::BackgroundWorkerError, + ffi::telemetry::MozAdsTelemetryWrapper, + mars::ad_response::AdImage, + worker::{Dispatch, DispatchCommand, ErrorOnlyRequestCallback}, +}; #[cfg(test)] mod test_utils; @@ -43,7 +56,7 @@ pub struct MozAdsClient { inner: MozAdsClientInner, _worker_thread: Option>, - worker_dispatch: Option>, + worker_dispatch: Option>, } pub type MozAdsClientInner = Arc>>; @@ -175,24 +188,62 @@ impl MozAdsClient { &self, image_ad_requests: Vec, options: Option, - callback: Option> + callback: Option>, ) -> AdsClientApiResult<()> { if let Some(worker_dispatch) = &self.worker_dispatch { - worker_dispatch.try_send(DispatchCommand::RequestImageAds { image_ad_requests, options, callback }).map_err(RequestAdsError::from)?; + worker_dispatch + .try_send(Dispatch { + command: DispatchCommand::RequestImageAds { + image_ad_requests, + options, + }, + error_callback: callback, + }) + .map_err(BackgroundWorkerError::from)?; Ok(()) } else { - Err(RequestAdsError::BackgroundWorkerClosedError.into()) + Err(BackgroundWorkerError::WorkerClosed.into()) } } #[handle_error(ComponentError)] #[uniffi::method()] - pub fn query_image_ads( - &self, - placement_id: String, - ) -> AdsClientApiResult> { + pub fn query_image_ads(&self, placement_id: String) -> AdsClientApiResult> { let inner = self.inner.lock(); - let image_ads : Option<&Vec> = inner.get_cached_ads(&placement_id); - Ok(image_ads.and_then(|ad| ad.into_iter().next().cloned()).map(|ad| ad.into())) + let image_ads: Option<&Vec> = inner.get_cached_ads(&placement_id); + Ok(image_ads + .and_then(|ad| ad.into_iter().next().cloned()) + .map(|ad| ad.into())) + } + + // Pings the background worker and waits for a response back. + // 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, + callback: Option>, + ) -> AdsClientApiResult<()> { + // TODO: Qualify instead of use + if let Some(worker_dispatch) = &self.worker_dispatch { + let (tx, rx) = mpsc::sync_channel(0); + worker_dispatch + .try_send(Dispatch { + command: DispatchCommand::Ping(tx), + error_callback: callback, + }) + .map_err(BackgroundWorkerError::from)?; + if let Some(timeout) = timeout { + rx.recv_timeout(timeout) + .map_err(BackgroundWorkerError::from)?; + } else { + // TODO: is this necessarily true? its the channel hanging up, not the worker + rx.recv().map_err(|_| BackgroundWorkerError::WorkerClosed)?; + } + return Ok(()); + } else { + Err(BackgroundWorkerError::WorkerClosed.into()) + } } } diff --git a/components/ads-client/src/worker.rs b/components/ads-client/src/worker.rs index b892d089da0..dc4307a3255 100644 --- a/components/ads-client/src/worker.rs +++ b/components/ads-client/src/worker.rs @@ -1,17 +1,28 @@ use crate::{ - AdsClientApiResult, MozAdsClientApiError, MozAdsClientInner, MozAdsPlacementRequest, MozAdsRequestOptions, client::error::ComponentError, http_cache::CachePolicy, mars::{ - ad_request::{AdPlacementRequest, AdRequestFlags}, ad_response::AdImage, - } + client::error::{BackgroundWorkerError, ComponentError}, + http_cache::CachePolicy, + mars::{ + ad_request::{AdPlacementRequest, AdRequestFlags}, + ad_response::AdImage, + }, + AdsClientApiResult, MozAdsClientApiError, MozAdsClientInner, MozAdsPlacementRequest, + MozAdsRequestOptions, +}; +use error_support::handle_error; +use std::{ + collections::HashMap, + sync::mpsc::{self, Receiver, SyncSender}, + thread::JoinHandle, }; -use error_support::{convert_log_report_error, handle_error}; -use std::{collections::HashMap, sync::mpsc::{self, Receiver, SyncSender}, thread::JoinHandle}; pub const ADS_CLIENT_WORKER_CHANNEL_BUFFER_SIZE: usize = 1000; -pub const ADS_CLIENT_WORKER_THREAD_NAME : &'static str = "ads-client.worker"; +pub const ADS_CLIENT_WORKER_THREAD_NAME: &'static str = "ads-client.worker"; // 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<()>)> { +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()) @@ -21,23 +32,37 @@ pub fn build_worker_thread(inner_client: MozAdsClientInner) -> Option<(SyncSende Some((tx, worker_thread_handle)) } -fn worker(inner_client: MozAdsClientInner, rx: Receiver) { +fn worker(inner_client: MozAdsClientInner, rx: Receiver) { while let Ok(task) = rx.recv() { - // Synchronously run tasks in the order they are passed in this separate channel. - let task: DispatchCommand = task; - task.run_command(&inner_client) - .expect("TODO: handle error here. maybe retries should be here?"); + let Dispatch { + command, + error_callback, + } = task; + + if let Err(e) = command.run_command(&inner_client) { + // TODO: document this more clearly + // Error is logged through `handle_error` conversion macro. + // If an error callback is provided by the surface, we send the error to that, too. + if let Some(error_callback) = error_callback { + error_callback.on_error(e); + } + } } } +pub struct Dispatch { + pub command: DispatchCommand, + pub error_callback: Option>, +} + pub enum DispatchCommand { // TODO: Extend this with other possible commands in V2. RequestImageAds { image_ad_requests: Vec, options: Option, - callback: Option> - } + }, + Ping(SyncSender<()>), } impl DispatchCommand { @@ -48,43 +73,35 @@ impl DispatchCommand { DispatchCommand::RequestImageAds { image_ad_requests, options, - callback } => { - let resp = (|| { - let mut inner = ads_client_inner.lock(); - let options = options.unwrap_or_default(); - let flags = AdRequestFlags::from(&options); - let ohttp = options.ohttp; - let cache_policy: CachePolicy = options.into(); + let mut inner = ads_client_inner.lock(); + let options = options.unwrap_or_default(); + let flags = AdRequestFlags::from(&options); + let ohttp = options.ohttp; + let cache_policy: CachePolicy = options.into(); - // Image ads - if image_ad_requests.len() > 0 { + // Image ads + if image_ad_requests.len() > 0 { 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) .map_err(ComponentError::RequestAds)?; - let image_response: HashMap> = image_response.into_iter().map(|(k, v)| (k, vec![v.into()])).collect(); - inner.cache_ads(image_response); - } - Ok::<(), ComponentError>(()) - })(); - match resp { - Ok(_) => (), - Err(e) => handle_background_worker_error(e, callback), + let image_response: HashMap> = image_response + .into_iter() + .map(|(k, v)| (k, vec![v.into()])) + .collect(); + inner.cache_ads(image_response); } + Ok(()) + } + DispatchCommand::Ping(sender) => { + sender + .try_send(()) + .map_err(|err| BackgroundWorkerError::PongFailure(Box::new(err)))?; + Ok(()) } } - Ok(()) - } -} - -// Handles an error thrown by the background worker by converting it to public-facing error type (MozAdsClientApiError) and logging it. -// If a callback was provided, public error gets sent there too. -fn handle_background_worker_error(err : ComponentError, callback : Option>) { - let err : MozAdsClientApiError = convert_log_report_error(err); - if let Some(callback) = callback { - callback.on_error(err); } } @@ -96,4 +113,3 @@ fn handle_background_worker_error(err : ComponentError, callback : Option Date: Thu, 6 Aug 2026 00:17:38 -0700 Subject: [PATCH 04/12] fix: Adds other two commands, clippy --- .../integration-tests/tests/mars_async.rs | 206 +++++++++++++++++- components/ads-client/src/ads_cache.rs | 45 ++-- components/ads-client/src/client.rs | 12 +- components/ads-client/src/client/error.rs | 2 +- components/ads-client/src/lib.rs | 82 ++++++- components/ads-client/src/worker.rs | 71 +++++- 6 files changed, 366 insertions(+), 52 deletions(-) diff --git a/components/ads-client/integration-tests/tests/mars_async.rs b/components/ads-client/integration-tests/tests/mars_async.rs index c6579f85b04..3f3d2d7e0fe 100644 --- a/components/ads-client/integration-tests/tests/mars_async.rs +++ b/components/ads-client/integration-tests/tests/mars_async.rs @@ -3,11 +3,15 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -use std::{sync::Arc, time::Duration}; - +use std::sync::Arc; use ads_client::{ - MozAdsClientBuilder, MozAdsEnvironment, MozAdsPlacementRequest, worker::ErrorOnlyRequestCallback, + MozAdsClientBuilder, MozAdsEnvironment, MozAdsPlacementRequest, MozAdsRequestOptions, worker::ErrorRequestCallback, }; +use ads_client::MozAdsIABContentTaxonomy; +use ads_client::MozAdsIABContent; +use ads_client::MozAdsPlacementRequestWithCount; + +pub const TEST_TIMEOUT_DURATION : std::time::Duration = std::time::Duration::from_secs(10); fn init_backend() { viaduct_hyper::viaduct_init_backend_hyper(); @@ -19,8 +23,10 @@ fn prod_client() -> ads_client::MozAdsClient { .build() } +// TODO: You actually probably should get rid of this. +// TODO: or have a 'drain' option for the worker that skips unning each thing struct TestErrorCallback; -impl ErrorOnlyRequestCallback for TestErrorCallback { +impl ErrorRequestCallback for TestErrorCallback { fn on_error(&self,err: ads_client::MozAdsClientApiError) { panic!("Error received in background worker callback: {err}") } @@ -28,9 +34,10 @@ impl ErrorOnlyRequestCallback for TestErrorCallback { #[test] #[ignore = "integration test: run manually with -- --ignored"] -fn test_contract_image_prod() { +fn test_contract_image_prod_async() { init_backend(); + // Prefetch let placement_id= "mock_billboard_1".to_string(); let client = prod_client(); let result = client.prefetch_image_ads(vec![MozAdsPlacementRequest { @@ -45,15 +52,15 @@ fn test_contract_image_prod() { result.err() ); - // TODO: magic number - // TODO: good consistent ping - let ping = client.ping_background_worker(Some(Duration::from_secs(30)), None); + // Ping + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), None); assert!( ping.is_ok(), "Ping failed: {:?}", ping.err() ); + // Query let result = client.query_image_ads(placement_id); assert!( result.is_ok(), @@ -63,4 +70,185 @@ fn test_contract_image_prod() { let placements = result.unwrap(); assert!(placements.is_some()); -} \ No newline at end of file +} + +#[test] +#[ignore = "integration test: run manually with -- --ignored"] +fn test_contract_image_with_categories_prod_async() { + init_backend(); + + // Prefetch + let placement_id= "mock_billboard_1".to_string(); + let client = prod_client(); + let result = client.prefetch_image_ads(vec![MozAdsPlacementRequest { + iab_content: Some(MozAdsIABContent { + category_ids: vec!["338".to_string()], + taxonomy: MozAdsIABContentTaxonomy::IAB3_0, + }), + placement_id: placement_id.clone() + }], + Some(MozAdsRequestOptions { + flags: std::collections::HashMap::from([("contextual_placement".to_string(), true)]), + ..Default::default() + }), Some(Box::new(TestErrorCallback))); + + assert!( + result.is_ok(), + "Image ad dispatch request failed: {:?}", + result.err() + ); + + // Ping + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), None); + 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= "mock_spoc_1".to_string(); + let client = prod_client(); + let result = client.prefetch_spoc_ads(vec![MozAdsPlacementRequestWithCount { + count: 3, + iab_content: None, + placement_id: placement_id.clone(), + }], + None, Some(Box::new(TestErrorCallback))); + + assert!( + result.is_ok(), + "Spoc ad dispatch request failed: {:?}", + result.err() + ); + + // Ping + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), None); + 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= "mock_tile_1".to_string(); + let client = prod_client(); + let result = client.prefetch_tile_ads( vec![MozAdsPlacementRequest { + iab_content: None, + placement_id: placement_id.clone() + }], + None, Some(Box::new(TestErrorCallback))); + + assert!( + result.is_ok(), + "Tile ad dispatch request failed: {:?}", + result.err() + ); + + // Ping + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), None); + 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= "mock_tile_1".to_string(); + let client = prod_client(); + let result = client.prefetch_tile_ads( vec![MozAdsPlacementRequest { + iab_content: None, + placement_id: placement_id.clone(), + }], + Some(MozAdsRequestOptions { + ohttp: true, + ..Default::default() + }), Some(Box::new(TestErrorCallback))); + + assert!( + result.is_ok(), + "Tile ad dispatch request failed: {:?}", + result.err() + ); + + // Ping + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), None); + 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"); +} diff --git a/components/ads-client/src/ads_cache.rs b/components/ads-client/src/ads_cache.rs index cf0950fb379..3d2e168f5a6 100644 --- a/components/ads-client/src/ads_cache.rs +++ b/components/ads-client/src/ads_cache.rs @@ -8,9 +8,15 @@ const DEFAULT_TTL: Duration = Duration::from_secs(300); #[derive(Debug)] pub struct AdsCache { - image_ads: HashMap)>, + image_ads: HashMap, spoc_ads: HashMap)>, - tile_ads: HashMap)>, + tile_ads: HashMap, +} + +impl Default for AdsCache { + fn default() -> Self { + Self::new() + } } impl AdsCache { @@ -22,25 +28,34 @@ impl AdsCache { } } - pub fn cache_ads(&mut self, ads: HashMap>, timestamp: u64) { - AdsCacheable::cache_ads(ads, self, timestamp); + pub fn cache_ads( + &mut self, + ads: HashMap, + timestamp: u64, + ) { + T::cache_ads(ads, self, timestamp); } - pub fn get_cached_ads<'a, T: AdsCacheable>(&'a self, placement: &str) -> Option<&'a Vec> { - // TODO: Note that some of them return T and some return Vec. Where should this handling be done? - // TODO: separate functions here? separate functions in surface? + pub fn get_cached_ads<'a, T: AdsCacheable>( + &'a self, + placement: &str, + ) -> Option<&'a T::StorageType> { // TODO: also there are existing trait for ads- use this? - AdsCacheable::fetch_cached_ads(&self, placement) + T::fetch_cached_ads(self, placement) } } pub trait AdsCacheable: Sized { - fn cache_ads(ads: HashMap>, ads_cache: &mut AdsCache, timestamp: u64); - fn fetch_cached_ads<'a>(ads_cache: &'a AdsCache, id: &str) -> Option<&'a Vec>; + // The cached ad(s) to store (eg: this may be a single ad, or an array of ads) + type StorageType; + + fn cache_ads(ads: HashMap, ads_cache: &mut AdsCache, timestamp: u64); + fn fetch_cached_ads<'a>(ads_cache: &'a AdsCache, id: &str) -> Option<&'a Self::StorageType>; } impl AdsCacheable for AdImage { - fn cache_ads(ads: HashMap>, ads_cache: &mut AdsCache, timestamp: u64) { + type StorageType = AdImage; + fn cache_ads(ads: HashMap, ads_cache: &mut AdsCache, timestamp: u64) { ads_cache .image_ads .extend(ads.into_iter().map(|(key, ad)| (key, (timestamp, ad)))); @@ -49,12 +64,13 @@ impl AdsCacheable for AdImage { .retain(|_, (x, _)| *x - timestamp < DEFAULT_TTL.as_secs()); } - fn fetch_cached_ads<'a>(ads_cache: &'a AdsCache, id: &str) -> Option<&'a Vec> { + fn fetch_cached_ads<'a>(ads_cache: &'a AdsCache, id: &str) -> Option<&'a AdImage> { ads_cache.image_ads.get(id).map(|(_, ads)| ads) } } impl AdsCacheable for AdSpoc { + type StorageType = Vec; fn cache_ads(ads: HashMap>, ads_cache: &mut AdsCache, timestamp: u64) { ads_cache .spoc_ads @@ -69,7 +85,8 @@ impl AdsCacheable for AdSpoc { } impl AdsCacheable for AdTile { - fn cache_ads(ads: HashMap>, ads_cache: &mut AdsCache, timestamp: u64) { + type StorageType = AdTile; + fn cache_ads(ads: HashMap, ads_cache: &mut AdsCache, timestamp: u64) { ads_cache .tile_ads .extend(ads.into_iter().map(|(key, ad)| (key, (timestamp, ad)))); @@ -77,7 +94,7 @@ impl AdsCacheable for AdTile { .tile_ads .retain(|_, (x, _)| *x - timestamp < DEFAULT_TTL.as_secs()); } - fn fetch_cached_ads<'a>(ads_cache: &'a AdsCache, id: &str) -> Option<&'a Vec> { + fn fetch_cached_ads<'a>(ads_cache: &'a AdsCache, id: &str) -> Option<&'a AdTile> { ads_cache.tile_ads.get(id).map(|(_, ads)| ads) } } diff --git a/components/ads-client/src/client.rs b/components/ads-client/src/client.rs index 16531a44ed7..659cc518b42 100644 --- a/components/ads-client/src/client.rs +++ b/components/ads-client/src/client.rs @@ -100,15 +100,13 @@ where self.client.clear_cache() } - pub fn cache_ads(&mut self, ads: HashMap>) { - // TODO: is this timestamp correct? - // TODO: cast - let now = chrono::Utc::now().timestamp() as u64; - self.ads_cache.cache_ads(ads, now); + pub fn cache_ads(&mut self, ads: HashMap) { + let now = chrono::Utc::now().timestamp().unsigned_abs(); + self.ads_cache.cache_ads::(ads, now); } - pub fn get_cached_ads(&self, placement_id: &str) -> Option<&Vec> { - self.ads_cache.get_cached_ads(&placement_id) + pub fn get_cached_ads(&self, placement_id: &str) -> Option<&A::StorageType> { + self.ads_cache.get_cached_ads::(placement_id) } pub fn get_context_id(&self) -> context_id::ApiResult { diff --git a/components/ads-client/src/client/error.rs b/components/ads-client/src/client/error.rs index d75b6c34a05..0bc0531ffdd 100644 --- a/components/ads-client/src/client/error.rs +++ b/components/ads-client/src/client/error.rs @@ -5,7 +5,7 @@ use crate::{ mars::error::{FetchAdsError, RecordClickError, RecordImpressionError, ReportAdError}, - worker::{Dispatch, DispatchCommand}, + worker::Dispatch, }; use std::sync::mpsc::{RecvTimeoutError, TrySendError}; diff --git a/components/ads-client/src/lib.rs b/components/ads-client/src/lib.rs index e2287c6a430..18f9fee8e94 100644 --- a/components/ads-client/src/lib.rs +++ b/components/ads-client/src/lib.rs @@ -36,8 +36,8 @@ pub use ffi::*; use crate::{ client::error::BackgroundWorkerError, ffi::telemetry::MozAdsTelemetryWrapper, - mars::ad_response::AdImage, - worker::{Dispatch, DispatchCommand, ErrorOnlyRequestCallback}, + mars::ad_response::{AdImage, AdSpoc, AdTile}, + worker::{Dispatch, DispatchCommand, ErrorRequestCallback}, }; #[cfg(test)] @@ -188,7 +188,7 @@ impl MozAdsClient { &self, image_ad_requests: Vec, options: Option, - callback: Option>, + callback: Option>, ) -> AdsClientApiResult<()> { if let Some(worker_dispatch) = &self.worker_dispatch { worker_dispatch @@ -206,14 +206,79 @@ impl MozAdsClient { } } + #[handle_error(ComponentError)] + #[uniffi::method(default(options = None))] + pub fn prefetch_spoc_ads( + &self, + spoc_ad_requests: Vec, + options: Option, + callback: Option>, + ) -> AdsClientApiResult<()> { + if let Some(worker_dispatch) = &self.worker_dispatch { + worker_dispatch + .try_send(Dispatch { + command: DispatchCommand::RequestSpocAds { + spoc_ad_requests, + options, + }, + error_callback: callback, + }) + .map_err(BackgroundWorkerError::from)?; + Ok(()) + } else { + Err(BackgroundWorkerError::WorkerClosed.into()) + } + } + + #[handle_error(ComponentError)] + #[uniffi::method(default(options = None))] + pub fn prefetch_tile_ads( + &self, + tile_ad_requests: Vec, + options: Option, + callback: Option>, + ) -> AdsClientApiResult<()> { + if let Some(worker_dispatch) = &self.worker_dispatch { + worker_dispatch + .try_send(Dispatch { + command: DispatchCommand::RequestTileAds { + tile_ad_requests, + options, + }, + error_callback: callback, + }) + .map_err(BackgroundWorkerError::from)?; + Ok(()) + } else { + Err(BackgroundWorkerError::WorkerClosed.into()) + } + } + #[handle_error(ComponentError)] #[uniffi::method()] pub fn query_image_ads(&self, placement_id: String) -> AdsClientApiResult> { let inner = self.inner.lock(); - let image_ads: Option<&Vec> = inner.get_cached_ads(&placement_id); - Ok(image_ads - .and_then(|ad| ad.into_iter().next().cloned()) - .map(|ad| ad.into())) + let image_ads: Option<&AdImage> = inner.get_cached_ads::(&placement_id); + Ok(image_ads.map(|ad| ad.clone().into())) + } + + #[handle_error(ComponentError)] + #[uniffi::method()] + pub fn query_spoc_ads( + &self, + placement_id: String, + ) -> AdsClientApiResult>> { + let inner = self.inner.lock(); + let spoc_ads: Option<&Vec> = inner.get_cached_ads::(&placement_id); + Ok(spoc_ads.map(|res| res.iter().map(|ad| ad.clone().into()).collect())) + } + + #[handle_error(ComponentError)] + #[uniffi::method()] + pub fn query_tile_ads(&self, placement_id: String) -> AdsClientApiResult> { + let inner = self.inner.lock(); + let image_ads: Option<&AdTile> = inner.get_cached_ads::(&placement_id); + Ok(image_ads.map(|ad| ad.clone().into())) } // Pings the background worker and waits for a response back. @@ -223,9 +288,8 @@ impl MozAdsClient { pub fn ping_background_worker( &self, timeout: Option, - callback: Option>, + callback: Option>, ) -> AdsClientApiResult<()> { - // TODO: Qualify instead of use if let Some(worker_dispatch) = &self.worker_dispatch { let (tx, rx) = mpsc::sync_channel(0); worker_dispatch diff --git a/components/ads-client/src/worker.rs b/components/ads-client/src/worker.rs index dc4307a3255..5998defecef 100644 --- a/components/ads-client/src/worker.rs +++ b/components/ads-client/src/worker.rs @@ -3,20 +3,19 @@ use crate::{ http_cache::CachePolicy, mars::{ ad_request::{AdPlacementRequest, AdRequestFlags}, - ad_response::AdImage, + ad_response::{AdImage, AdSpoc, AdTile}, }, AdsClientApiResult, MozAdsClientApiError, MozAdsClientInner, MozAdsPlacementRequest, - MozAdsRequestOptions, + MozAdsPlacementRequestWithCount, MozAdsRequestOptions, }; use error_support::handle_error; use std::{ - collections::HashMap, sync::mpsc::{self, Receiver, SyncSender}, thread::JoinHandle, }; pub const ADS_CLIENT_WORKER_CHANNEL_BUFFER_SIZE: usize = 1000; -pub const ADS_CLIENT_WORKER_THREAD_NAME: &'static str = "ads-client.worker"; +pub const ADS_CLIENT_WORKER_THREAD_NAME: &str = "ads-client.worker"; // 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. @@ -53,7 +52,7 @@ fn worker(inner_client: MozAdsClientInner, rx: Receiver) { pub struct Dispatch { pub command: DispatchCommand, - pub error_callback: Option>, + pub error_callback: Option>, } pub enum DispatchCommand { @@ -62,6 +61,14 @@ pub enum DispatchCommand { image_ad_requests: Vec, options: Option, }, + RequestSpocAds { + spoc_ad_requests: Vec, + options: Option, + }, + RequestTileAds { + tile_ad_requests: Vec, + options: Option, + }, Ping(SyncSender<()>), } @@ -69,6 +76,7 @@ impl DispatchCommand { #[handle_error(ComponentError)] pub fn run_command(self, ads_client_inner: &MozAdsClientInner) -> AdsClientApiResult<()> { // TODO: Duplicated behavior with the sync functions. Behavior is pretty simple though- probably OK to duplicate. + // TODO: Should we skip these if cache exists? match self { DispatchCommand::RequestImageAds { image_ad_requests, @@ -81,20 +89,59 @@ impl DispatchCommand { let cache_policy: CachePolicy = options.into(); // Image ads - if image_ad_requests.len() > 0 { + 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) .map_err(ComponentError::RequestAds)?; - let image_response: HashMap> = image_response - .into_iter() - .map(|(k, v)| (k, vec![v.into()])) - .collect(); - inner.cache_ads(image_response); + inner.cache_ads::(image_response); + } + Ok(()) + } + DispatchCommand::RequestSpocAds { + spoc_ad_requests, + options, + } => { + let mut inner = ads_client_inner.lock(); + let options = options.unwrap_or_default(); + let flags = AdRequestFlags::from(&options); + let ohttp = options.ohttp; + let cache_policy: CachePolicy = options.into(); + + // Spoc ads + 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) + .map_err(ComponentError::RequestAds)?; + inner.cache_ads::(spoc_response); } Ok(()) } + DispatchCommand::RequestTileAds { + tile_ad_requests, + options, + } => { + let mut inner = ads_client_inner.lock(); + let options = options.unwrap_or_default(); + let flags = AdRequestFlags::from(&options); + let ohttp = options.ohttp; + let cache_policy: CachePolicy = options.into(); + + // Image ads + 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) + .map_err(ComponentError::RequestAds)?; + inner.cache_ads::(tile_response); + } + Ok(()) + } + DispatchCommand::Ping(sender) => { sender .try_send(()) @@ -110,6 +157,6 @@ impl DispatchCommand { // - https://github.com/mozilla/application-services/pull/7443 // In the future, this should be changed to uniffi::export(impl = "foreign") alongside the enum change, when uniffi = 0.32 #[uniffi::export(callback_interface)] -pub trait ErrorOnlyRequestCallback: Send + Sync { +pub trait ErrorRequestCallback: Send + Sync { fn on_error(&self, err: MozAdsClientApiError); } From ed31114deae815881a5475948c13d7744d1a24dc Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Thu, 6 Aug 2026 15:10:44 -0700 Subject: [PATCH 05/12] feat: Combines into one prefetch and adds multi-test --- .../integration-tests/tests/mars_async.rs | 89 +++++++++++++-- components/ads-client/src/ads_cache.rs | 1 - components/ads-client/src/client/error.rs | 3 +- components/ads-client/src/ffi.rs | 2 +- components/ads-client/src/lib.rs | 103 ++++++++---------- components/ads-client/src/worker.rs | 10 +- 6 files changed, 130 insertions(+), 78 deletions(-) diff --git a/components/ads-client/integration-tests/tests/mars_async.rs b/components/ads-client/integration-tests/tests/mars_async.rs index 3f3d2d7e0fe..3ad0b32433d 100644 --- a/components/ads-client/integration-tests/tests/mars_async.rs +++ b/components/ads-client/integration-tests/tests/mars_async.rs @@ -23,8 +23,6 @@ fn prod_client() -> ads_client::MozAdsClient { .build() } -// TODO: You actually probably should get rid of this. -// TODO: or have a 'drain' option for the worker that skips unning each thing struct TestErrorCallback; impl ErrorRequestCallback for TestErrorCallback { fn on_error(&self,err: ads_client::MozAdsClientApiError) { @@ -40,10 +38,10 @@ fn test_contract_image_prod_async() { // Prefetch let placement_id= "mock_billboard_1".to_string(); let client = prod_client(); - let result = client.prefetch_image_ads(vec![MozAdsPlacementRequest { + let result = client.prefetch_ads(vec![MozAdsPlacementRequest { iab_content: None, placement_id: placement_id.clone(), - }], + }], vec![], vec![], None, Some(Box::new(TestErrorCallback))); assert!( @@ -80,13 +78,13 @@ fn test_contract_image_with_categories_prod_async() { // Prefetch let placement_id= "mock_billboard_1".to_string(); let client = prod_client(); - let result = client.prefetch_image_ads(vec![MozAdsPlacementRequest { + 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() - }], + }], vec![], vec![], Some(MozAdsRequestOptions { flags: std::collections::HashMap::from([("contextual_placement".to_string(), true)]), ..Default::default() @@ -129,11 +127,11 @@ fn test_contract_spoc_prod_async() { // Prefetch let placement_id= "mock_spoc_1".to_string(); let client = prod_client(); - let result = client.prefetch_spoc_ads(vec![MozAdsPlacementRequestWithCount { + let result = client.prefetch_ads(vec![], vec![MozAdsPlacementRequestWithCount { count: 3, iab_content: None, placement_id: placement_id.clone(), - }], + }], vec![], None, Some(Box::new(TestErrorCallback))); assert!( @@ -170,7 +168,7 @@ fn test_contract_tile_prod_async() { // Prefetch let placement_id= "mock_tile_1".to_string(); let client = prod_client(); - let result = client.prefetch_tile_ads( vec![MozAdsPlacementRequest { + let result = client.prefetch_ads( vec![], vec![], vec![MozAdsPlacementRequest { iab_content: None, placement_id: placement_id.clone() }], @@ -218,7 +216,7 @@ fn test_contract_tile_ohttp_prod_async() { // Prefetch let placement_id= "mock_tile_1".to_string(); let client = prod_client(); - let result = client.prefetch_tile_ads( vec![MozAdsPlacementRequest { + let result = client.prefetch_ads( vec![],vec![], vec![MozAdsPlacementRequest { iab_content: None, placement_id: placement_id.clone(), }], @@ -252,3 +250,74 @@ fn test_contract_tile_ohttp_prod_async() { 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= "mock_billboard_1".to_string(); + let placement_spoc_id= "mock_spoc_1".to_string(); + let placement_tile_id= "mock_tile_1".to_string(); + let client = prod_client(); + let result = client.prefetch_ads( + vec![MozAdsPlacementRequest { + iab_content: None, + placement_id: placement_image_id.clone(), + }], vec![MozAdsPlacementRequestWithCount { + count: 4, + iab_content: None, + placement_id: placement_spoc_id.clone(), + }], vec![MozAdsPlacementRequest { + iab_content: None, + placement_id: placement_tile_id.clone(), + }], + None, Some(Box::new(TestErrorCallback))); + + assert!( + result.is_ok(), + "Image ad dispatch request failed: {:?}", + result.err() + ); + + // Ping + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), None); + 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_cache.rs b/components/ads-client/src/ads_cache.rs index 3d2e168f5a6..9b5120b8c2a 100644 --- a/components/ads-client/src/ads_cache.rs +++ b/components/ads-client/src/ads_cache.rs @@ -40,7 +40,6 @@ impl AdsCache { &'a self, placement: &str, ) -> Option<&'a T::StorageType> { - // TODO: also there are existing trait for ads- use this? T::fetch_cached_ads(self, placement) } } diff --git a/components/ads-client/src/client/error.rs b/components/ads-client/src/client/error.rs index 0bc0531ffdd..a90ae510a4c 100644 --- a/components/ads-client/src/client/error.rs +++ b/components/ads-client/src/client/error.rs @@ -52,8 +52,7 @@ pub enum BackgroundWorkerError { } impl From> for BackgroundWorkerError { - // TODO: Should we keep the 'dispatch' here somehow instead of just dropping it for retry reasons? - // Intended strategy for retry is probably using sqlite instead. + // TODO: For future vertical slice (for retries), we may want to keep the failed dispatch for retrying fn from(value: TrySendError) -> Self { match value { TrySendError::Disconnected(_) => BackgroundWorkerError::WorkerClosed, diff --git a/components/ads-client/src/ffi.rs b/components/ads-client/src/ffi.rs index b5e299fa4f4..8312c4b3dd1 100644 --- a/components/ads-client/src/ffi.rs +++ b/components/ads-client/src/ffi.rs @@ -53,7 +53,7 @@ impl From for Box { } } -#[derive(Default, uniffi::Record)] +#[derive(Default, uniffi::Record, Clone)] pub struct MozAdsRequestOptions { pub cache_policy: Option, #[uniffi(default)] diff --git a/components/ads-client/src/lib.rs b/components/ads-client/src/lib.rs index 18f9fee8e94..7eb8f03c761 100644 --- a/components/ads-client/src/lib.rs +++ b/components/ads-client/src/lib.rs @@ -183,71 +183,55 @@ impl MozAdsClient { } #[handle_error(ComponentError)] - #[uniffi::method(default(options = None))] - pub fn prefetch_image_ads( + #[uniffi::method(default(image_ad_requests = [], spoc_ad_requests = [], tile_ad_requests = [], options = None, callback = None))] + pub fn prefetch_ads( &self, image_ad_requests: Vec, - options: Option, - callback: Option>, - ) -> AdsClientApiResult<()> { - if let Some(worker_dispatch) = &self.worker_dispatch { - worker_dispatch - .try_send(Dispatch { - command: DispatchCommand::RequestImageAds { - image_ad_requests, - options, - }, - error_callback: callback, - }) - .map_err(BackgroundWorkerError::from)?; - Ok(()) - } else { - Err(BackgroundWorkerError::WorkerClosed.into()) - } - } - - #[handle_error(ComponentError)] - #[uniffi::method(default(options = None))] - pub fn prefetch_spoc_ads( - &self, spoc_ad_requests: Vec, - options: Option, - callback: Option>, - ) -> AdsClientApiResult<()> { - if let Some(worker_dispatch) = &self.worker_dispatch { - worker_dispatch - .try_send(Dispatch { - command: DispatchCommand::RequestSpocAds { - spoc_ad_requests, - options, - }, - error_callback: callback, - }) - .map_err(BackgroundWorkerError::from)?; - Ok(()) - } else { - Err(BackgroundWorkerError::WorkerClosed.into()) - } - } - - #[handle_error(ComponentError)] - #[uniffi::method(default(options = None))] - pub fn prefetch_tile_ads( - &self, tile_ad_requests: Vec, options: Option, callback: Option>, ) -> AdsClientApiResult<()> { + let callback = callback.map(Arc::new); if let Some(worker_dispatch) = &self.worker_dispatch { - worker_dispatch - .try_send(Dispatch { - command: DispatchCommand::RequestTileAds { - tile_ad_requests, - options, - }, - error_callback: callback, - }) - .map_err(BackgroundWorkerError::from)?; + // Dispatch image requests + if !image_ad_requests.is_empty() { + worker_dispatch + .try_send(Dispatch { + command: DispatchCommand::RequestImageAds { + image_ad_requests, + options: options.clone(), + }, + error_callback: callback.clone(), + }) + .map_err(BackgroundWorkerError::from)?; + } + + // Dispatch spoc requests + if !spoc_ad_requests.is_empty() { + worker_dispatch + .try_send(Dispatch { + command: DispatchCommand::RequestSpocAds { + spoc_ad_requests, + options: options.clone(), + }, + error_callback: callback.clone(), + }) + .map_err(BackgroundWorkerError::from)?; + } + + // Dispatch tiles requests + if !tile_ad_requests.is_empty() { + worker_dispatch + .try_send(Dispatch { + command: DispatchCommand::RequestTileAds { + tile_ad_requests, + options: options.clone(), + }, + error_callback: callback.clone(), + }) + .map_err(BackgroundWorkerError::from)?; + } Ok(()) } else { Err(BackgroundWorkerError::WorkerClosed.into()) @@ -281,7 +265,7 @@ impl MozAdsClient { Ok(image_ads.map(|ad| ad.clone().into())) } - // Pings the background worker and waits for a response back. + // 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)] @@ -295,14 +279,13 @@ impl MozAdsClient { worker_dispatch .try_send(Dispatch { command: DispatchCommand::Ping(tx), - error_callback: callback, + error_callback: callback.map(Arc::new), }) .map_err(BackgroundWorkerError::from)?; if let Some(timeout) = timeout { rx.recv_timeout(timeout) .map_err(BackgroundWorkerError::from)?; } else { - // TODO: is this necessarily true? its the channel hanging up, not the worker rx.recv().map_err(|_| BackgroundWorkerError::WorkerClosed)?; } return Ok(()); diff --git a/components/ads-client/src/worker.rs b/components/ads-client/src/worker.rs index 5998defecef..78a3947b6a9 100644 --- a/components/ads-client/src/worker.rs +++ b/components/ads-client/src/worker.rs @@ -10,7 +10,10 @@ use crate::{ }; use error_support::handle_error; use std::{ - sync::mpsc::{self, Receiver, SyncSender}, + sync::{ + mpsc::{self, Receiver, SyncSender}, + Arc, + }, thread::JoinHandle, }; @@ -32,15 +35,14 @@ pub fn build_worker_thread( } fn worker(inner_client: MozAdsClientInner, rx: Receiver) { + // Synchronously run tasks in the order they are passed in this separate channel. while let Ok(task) = rx.recv() { - // Synchronously run tasks in the order they are passed in this separate channel. let Dispatch { command, error_callback, } = task; if let Err(e) = command.run_command(&inner_client) { - // TODO: document this more clearly // Error is logged through `handle_error` conversion macro. // If an error callback is provided by the surface, we send the error to that, too. if let Some(error_callback) = error_callback { @@ -52,7 +54,7 @@ fn worker(inner_client: MozAdsClientInner, rx: Receiver) { pub struct Dispatch { pub command: DispatchCommand, - pub error_callback: Option>, + pub error_callback: Option>>, } pub enum DispatchCommand { From 2539df522d934bf3c3f36de557f05f5196e6a343 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Thu, 6 Aug 2026 17:51:37 -0700 Subject: [PATCH 06/12] feat: Removed callbacks --- .../integration-tests/tests/mars_async.rs | 33 ++++++++----------- components/ads-client/src/lib.rs | 16 ++------- components/ads-client/src/worker.rs | 31 +++-------------- 3 files changed, 21 insertions(+), 59 deletions(-) diff --git a/components/ads-client/integration-tests/tests/mars_async.rs b/components/ads-client/integration-tests/tests/mars_async.rs index 3ad0b32433d..fe31eb89e45 100644 --- a/components/ads-client/integration-tests/tests/mars_async.rs +++ b/components/ads-client/integration-tests/tests/mars_async.rs @@ -5,7 +5,7 @@ use std::sync::Arc; use ads_client::{ - MozAdsClientBuilder, MozAdsEnvironment, MozAdsPlacementRequest, MozAdsRequestOptions, worker::ErrorRequestCallback, + MozAdsClientBuilder, MozAdsEnvironment, MozAdsPlacementRequest, MozAdsRequestOptions, }; use ads_client::MozAdsIABContentTaxonomy; use ads_client::MozAdsIABContent; @@ -23,13 +23,6 @@ fn prod_client() -> ads_client::MozAdsClient { .build() } -struct TestErrorCallback; -impl ErrorRequestCallback for TestErrorCallback { - fn on_error(&self,err: ads_client::MozAdsClientApiError) { - panic!("Error received in background worker callback: {err}") - } -} - #[test] #[ignore = "integration test: run manually with -- --ignored"] fn test_contract_image_prod_async() { @@ -42,7 +35,7 @@ fn test_contract_image_prod_async() { iab_content: None, placement_id: placement_id.clone(), }], vec![], vec![], - None, Some(Box::new(TestErrorCallback))); + None); assert!( result.is_ok(), @@ -51,7 +44,7 @@ fn test_contract_image_prod_async() { ); // Ping - let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), None); + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION)); assert!( ping.is_ok(), "Ping failed: {:?}", @@ -88,7 +81,7 @@ fn test_contract_image_with_categories_prod_async() { Some(MozAdsRequestOptions { flags: std::collections::HashMap::from([("contextual_placement".to_string(), true)]), ..Default::default() - }), Some(Box::new(TestErrorCallback))); + })); assert!( result.is_ok(), @@ -97,7 +90,7 @@ fn test_contract_image_with_categories_prod_async() { ); // Ping - let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), None); + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION)); assert!( ping.is_ok(), "Ping failed: {:?}", @@ -132,7 +125,7 @@ fn test_contract_spoc_prod_async() { iab_content: None, placement_id: placement_id.clone(), }], vec![], - None, Some(Box::new(TestErrorCallback))); + None); assert!( result.is_ok(), @@ -141,7 +134,7 @@ fn test_contract_spoc_prod_async() { ); // Ping - let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), None); + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION)); assert!( ping.is_ok(), "Ping failed: {:?}", @@ -172,7 +165,7 @@ fn test_contract_tile_prod_async() { iab_content: None, placement_id: placement_id.clone() }], - None, Some(Box::new(TestErrorCallback))); + None); assert!( result.is_ok(), @@ -181,7 +174,7 @@ fn test_contract_tile_prod_async() { ); // Ping - let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), None); + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION)); assert!( ping.is_ok(), "Ping failed: {:?}", @@ -223,7 +216,7 @@ fn test_contract_tile_ohttp_prod_async() { Some(MozAdsRequestOptions { ohttp: true, ..Default::default() - }), Some(Box::new(TestErrorCallback))); + })); assert!( result.is_ok(), @@ -232,7 +225,7 @@ fn test_contract_tile_ohttp_prod_async() { ); // Ping - let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), None); + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION)); assert!( ping.is_ok(), "Ping failed: {:?}", @@ -274,7 +267,7 @@ fn test_contract_multi_ad_type_prod_async() { iab_content: None, placement_id: placement_tile_id.clone(), }], - None, Some(Box::new(TestErrorCallback))); + None); assert!( result.is_ok(), @@ -283,7 +276,7 @@ fn test_contract_multi_ad_type_prod_async() { ); // Ping - let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), None); + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION)); assert!( ping.is_ok(), "Ping failed: {:?}", diff --git a/components/ads-client/src/lib.rs b/components/ads-client/src/lib.rs index 15e1fa40f81..1f493aadb73 100644 --- a/components/ads-client/src/lib.rs +++ b/components/ads-client/src/lib.rs @@ -37,7 +37,7 @@ use crate::{ client::error::BackgroundWorkerError, ffi::telemetry::MozAdsTelemetryWrapper, mars::ad_response::{AdImage, AdSpoc, AdTile}, - worker::{Dispatch, DispatchCommand, ErrorRequestCallback}, + worker::{Dispatch, DispatchCommand}, }; #[cfg(test)] @@ -195,16 +195,14 @@ impl MozAdsClient { } #[handle_error(ComponentError)] - #[uniffi::method(default(image_ad_requests = [], spoc_ad_requests = [], tile_ad_requests = [], options = None, callback = None))] + #[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, - callback: Option>, ) -> AdsClientApiResult<()> { - let callback = callback.map(Arc::new); if let Some(worker_dispatch) = &self.worker_dispatch { // Dispatch image requests if !image_ad_requests.is_empty() { @@ -214,7 +212,6 @@ impl MozAdsClient { image_ad_requests, options: options.clone(), }, - error_callback: callback.clone(), }) .map_err(BackgroundWorkerError::from)?; } @@ -227,7 +224,6 @@ impl MozAdsClient { spoc_ad_requests, options: options.clone(), }, - error_callback: callback.clone(), }) .map_err(BackgroundWorkerError::from)?; } @@ -240,7 +236,6 @@ impl MozAdsClient { tile_ad_requests, options: options.clone(), }, - error_callback: callback.clone(), }) .map_err(BackgroundWorkerError::from)?; } @@ -281,17 +276,12 @@ impl MozAdsClient { // 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, - callback: Option>, - ) -> AdsClientApiResult<()> { + pub fn ping_background_worker(&self, timeout: Option) -> AdsClientApiResult<()> { if let Some(worker_dispatch) = &self.worker_dispatch { let (tx, rx) = mpsc::sync_channel(0); worker_dispatch .try_send(Dispatch { command: DispatchCommand::Ping(tx), - error_callback: callback.map(Arc::new), }) .map_err(BackgroundWorkerError::from)?; if let Some(timeout) = timeout { diff --git a/components/ads-client/src/worker.rs b/components/ads-client/src/worker.rs index 78a3947b6a9..fd11f375a1f 100644 --- a/components/ads-client/src/worker.rs +++ b/components/ads-client/src/worker.rs @@ -5,15 +5,12 @@ use crate::{ ad_request::{AdPlacementRequest, AdRequestFlags}, ad_response::{AdImage, AdSpoc, AdTile}, }, - AdsClientApiResult, MozAdsClientApiError, MozAdsClientInner, MozAdsPlacementRequest, + AdsClientApiResult, MozAdsClientInner, MozAdsPlacementRequest, MozAdsPlacementRequestWithCount, MozAdsRequestOptions, }; use error_support::handle_error; use std::{ - sync::{ - mpsc::{self, Receiver, SyncSender}, - Arc, - }, + sync::mpsc::{self, Receiver, SyncSender}, thread::JoinHandle, }; @@ -37,24 +34,15 @@ pub fn build_worker_thread( fn worker(inner_client: MozAdsClientInner, rx: Receiver) { // Synchronously run tasks in the order they are passed in this separate channel. while let Ok(task) = rx.recv() { - let Dispatch { - command, - error_callback, - } = task; + let Dispatch { command } = task; - if let Err(e) = command.run_command(&inner_client) { - // Error is logged through `handle_error` conversion macro. - // If an error callback is provided by the surface, we send the error to that, too. - if let Some(error_callback) = error_callback { - error_callback.on_error(e); - } - } + // Error is logged through `handle_error` conversion macro. + let _ = command.run_command(&inner_client); } } pub struct Dispatch { pub command: DispatchCommand, - pub error_callback: Option>>, } pub enum DispatchCommand { @@ -153,12 +141,3 @@ impl DispatchCommand { } } } - -// TODO: Uniffi does not currently support direct function callbacks, so we use an interface. -// TODO: We use #[uniffi::export(callback_interface)] rather than #[uniffi::export(with_foreign)] for reasons discussed here: -// - https://github.com/mozilla/application-services/pull/7443 -// In the future, this should be changed to uniffi::export(impl = "foreign") alongside the enum change, when uniffi = 0.32 -#[uniffi::export(callback_interface)] -pub trait ErrorRequestCallback: Send + Sync { - fn on_error(&self, err: MozAdsClientApiError); -} From fb9e54f1f673432ab58382d424ec08a21bbe41a1 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Fri, 7 Aug 2026 15:43:52 -0700 Subject: [PATCH 07/12] Revert "feat: Removed callbacks" This reverts commit 2539df522d934bf3c3f36de557f05f5196e6a343. --- .../integration-tests/tests/mars_async.rs | 33 +++++++++++-------- components/ads-client/src/lib.rs | 16 +++++++-- components/ads-client/src/worker.rs | 31 ++++++++++++++--- 3 files changed, 59 insertions(+), 21 deletions(-) diff --git a/components/ads-client/integration-tests/tests/mars_async.rs b/components/ads-client/integration-tests/tests/mars_async.rs index fe31eb89e45..3ad0b32433d 100644 --- a/components/ads-client/integration-tests/tests/mars_async.rs +++ b/components/ads-client/integration-tests/tests/mars_async.rs @@ -5,7 +5,7 @@ use std::sync::Arc; use ads_client::{ - MozAdsClientBuilder, MozAdsEnvironment, MozAdsPlacementRequest, MozAdsRequestOptions, + MozAdsClientBuilder, MozAdsEnvironment, MozAdsPlacementRequest, MozAdsRequestOptions, worker::ErrorRequestCallback, }; use ads_client::MozAdsIABContentTaxonomy; use ads_client::MozAdsIABContent; @@ -23,6 +23,13 @@ fn prod_client() -> ads_client::MozAdsClient { .build() } +struct TestErrorCallback; +impl ErrorRequestCallback for TestErrorCallback { + fn on_error(&self,err: ads_client::MozAdsClientApiError) { + panic!("Error received in background worker callback: {err}") + } +} + #[test] #[ignore = "integration test: run manually with -- --ignored"] fn test_contract_image_prod_async() { @@ -35,7 +42,7 @@ fn test_contract_image_prod_async() { iab_content: None, placement_id: placement_id.clone(), }], vec![], vec![], - None); + None, Some(Box::new(TestErrorCallback))); assert!( result.is_ok(), @@ -44,7 +51,7 @@ fn test_contract_image_prod_async() { ); // Ping - let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION)); + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), None); assert!( ping.is_ok(), "Ping failed: {:?}", @@ -81,7 +88,7 @@ fn test_contract_image_with_categories_prod_async() { Some(MozAdsRequestOptions { flags: std::collections::HashMap::from([("contextual_placement".to_string(), true)]), ..Default::default() - })); + }), Some(Box::new(TestErrorCallback))); assert!( result.is_ok(), @@ -90,7 +97,7 @@ fn test_contract_image_with_categories_prod_async() { ); // Ping - let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION)); + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), None); assert!( ping.is_ok(), "Ping failed: {:?}", @@ -125,7 +132,7 @@ fn test_contract_spoc_prod_async() { iab_content: None, placement_id: placement_id.clone(), }], vec![], - None); + None, Some(Box::new(TestErrorCallback))); assert!( result.is_ok(), @@ -134,7 +141,7 @@ fn test_contract_spoc_prod_async() { ); // Ping - let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION)); + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), None); assert!( ping.is_ok(), "Ping failed: {:?}", @@ -165,7 +172,7 @@ fn test_contract_tile_prod_async() { iab_content: None, placement_id: placement_id.clone() }], - None); + None, Some(Box::new(TestErrorCallback))); assert!( result.is_ok(), @@ -174,7 +181,7 @@ fn test_contract_tile_prod_async() { ); // Ping - let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION)); + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), None); assert!( ping.is_ok(), "Ping failed: {:?}", @@ -216,7 +223,7 @@ fn test_contract_tile_ohttp_prod_async() { Some(MozAdsRequestOptions { ohttp: true, ..Default::default() - })); + }), Some(Box::new(TestErrorCallback))); assert!( result.is_ok(), @@ -225,7 +232,7 @@ fn test_contract_tile_ohttp_prod_async() { ); // Ping - let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION)); + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), None); assert!( ping.is_ok(), "Ping failed: {:?}", @@ -267,7 +274,7 @@ fn test_contract_multi_ad_type_prod_async() { iab_content: None, placement_id: placement_tile_id.clone(), }], - None); + None, Some(Box::new(TestErrorCallback))); assert!( result.is_ok(), @@ -276,7 +283,7 @@ fn test_contract_multi_ad_type_prod_async() { ); // Ping - let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION)); + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), None); assert!( ping.is_ok(), "Ping failed: {:?}", diff --git a/components/ads-client/src/lib.rs b/components/ads-client/src/lib.rs index 1f493aadb73..15e1fa40f81 100644 --- a/components/ads-client/src/lib.rs +++ b/components/ads-client/src/lib.rs @@ -37,7 +37,7 @@ use crate::{ client::error::BackgroundWorkerError, ffi::telemetry::MozAdsTelemetryWrapper, mars::ad_response::{AdImage, AdSpoc, AdTile}, - worker::{Dispatch, DispatchCommand}, + worker::{Dispatch, DispatchCommand, ErrorRequestCallback}, }; #[cfg(test)] @@ -195,14 +195,16 @@ impl MozAdsClient { } #[handle_error(ComponentError)] - #[uniffi::method(default(image_ad_requests = [], spoc_ad_requests = [], tile_ad_requests = [], options = None))] + #[uniffi::method(default(image_ad_requests = [], spoc_ad_requests = [], tile_ad_requests = [], options = None, callback = None))] pub fn prefetch_ads( &self, image_ad_requests: Vec, spoc_ad_requests: Vec, tile_ad_requests: Vec, options: Option, + callback: Option>, ) -> AdsClientApiResult<()> { + let callback = callback.map(Arc::new); if let Some(worker_dispatch) = &self.worker_dispatch { // Dispatch image requests if !image_ad_requests.is_empty() { @@ -212,6 +214,7 @@ impl MozAdsClient { image_ad_requests, options: options.clone(), }, + error_callback: callback.clone(), }) .map_err(BackgroundWorkerError::from)?; } @@ -224,6 +227,7 @@ impl MozAdsClient { spoc_ad_requests, options: options.clone(), }, + error_callback: callback.clone(), }) .map_err(BackgroundWorkerError::from)?; } @@ -236,6 +240,7 @@ impl MozAdsClient { tile_ad_requests, options: options.clone(), }, + error_callback: callback.clone(), }) .map_err(BackgroundWorkerError::from)?; } @@ -276,12 +281,17 @@ impl MozAdsClient { // 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<()> { + pub fn ping_background_worker( + &self, + timeout: Option, + callback: Option>, + ) -> AdsClientApiResult<()> { if let Some(worker_dispatch) = &self.worker_dispatch { let (tx, rx) = mpsc::sync_channel(0); worker_dispatch .try_send(Dispatch { command: DispatchCommand::Ping(tx), + error_callback: callback.map(Arc::new), }) .map_err(BackgroundWorkerError::from)?; if let Some(timeout) = timeout { diff --git a/components/ads-client/src/worker.rs b/components/ads-client/src/worker.rs index fd11f375a1f..78a3947b6a9 100644 --- a/components/ads-client/src/worker.rs +++ b/components/ads-client/src/worker.rs @@ -5,12 +5,15 @@ use crate::{ ad_request::{AdPlacementRequest, AdRequestFlags}, ad_response::{AdImage, AdSpoc, AdTile}, }, - AdsClientApiResult, MozAdsClientInner, MozAdsPlacementRequest, + AdsClientApiResult, MozAdsClientApiError, MozAdsClientInner, MozAdsPlacementRequest, MozAdsPlacementRequestWithCount, MozAdsRequestOptions, }; use error_support::handle_error; use std::{ - sync::mpsc::{self, Receiver, SyncSender}, + sync::{ + mpsc::{self, Receiver, SyncSender}, + Arc, + }, thread::JoinHandle, }; @@ -34,15 +37,24 @@ pub fn build_worker_thread( fn worker(inner_client: MozAdsClientInner, rx: Receiver) { // Synchronously run tasks in the order they are passed in this separate channel. while let Ok(task) = rx.recv() { - let Dispatch { command } = task; + let Dispatch { + command, + error_callback, + } = task; - // Error is logged through `handle_error` conversion macro. - let _ = command.run_command(&inner_client); + if let Err(e) = command.run_command(&inner_client) { + // Error is logged through `handle_error` conversion macro. + // If an error callback is provided by the surface, we send the error to that, too. + if let Some(error_callback) = error_callback { + error_callback.on_error(e); + } + } } } pub struct Dispatch { pub command: DispatchCommand, + pub error_callback: Option>>, } pub enum DispatchCommand { @@ -141,3 +153,12 @@ impl DispatchCommand { } } } + +// TODO: Uniffi does not currently support direct function callbacks, so we use an interface. +// TODO: We use #[uniffi::export(callback_interface)] rather than #[uniffi::export(with_foreign)] for reasons discussed here: +// - https://github.com/mozilla/application-services/pull/7443 +// In the future, this should be changed to uniffi::export(impl = "foreign") alongside the enum change, when uniffi = 0.32 +#[uniffi::export(callback_interface)] +pub trait ErrorRequestCallback: Send + Sync { + fn on_error(&self, err: MozAdsClientApiError); +} From 1b784f71d5e7cf827882eb834f443b307e7e0f3a Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Fri, 7 Aug 2026 16:21:59 -0700 Subject: [PATCH 08/12] fix: Adds record impression, click, report ad --- .../integration-tests/tests/mars_async.rs | 127 +++++++++++++++++- components/ads-client/src/lib.rs | 103 +++++++++++++- components/ads-client/src/worker.rs | 79 +++++++---- 3 files changed, 280 insertions(+), 29 deletions(-) diff --git a/components/ads-client/integration-tests/tests/mars_async.rs b/components/ads-client/integration-tests/tests/mars_async.rs index 3ad0b32433d..eeb34aada3d 100644 --- a/components/ads-client/integration-tests/tests/mars_async.rs +++ b/components/ads-client/integration-tests/tests/mars_async.rs @@ -5,7 +5,7 @@ use std::sync::Arc; use ads_client::{ - MozAdsClientBuilder, MozAdsEnvironment, MozAdsPlacementRequest, MozAdsRequestOptions, worker::ErrorRequestCallback, + MozAdsClient, MozAdsClientBuilder, MozAdsEnvironment, MozAdsPlacementRequest, MozAdsReportReason, MozAdsRequestOptions, worker::ErrorRequestCallback, MozAdsTile, }; use ads_client::MozAdsIABContentTaxonomy; use ads_client::MozAdsIABContent; @@ -26,10 +26,46 @@ fn prod_client() -> ads_client::MozAdsClient { struct TestErrorCallback; impl ErrorRequestCallback for TestErrorCallback { fn on_error(&self,err: ads_client::MozAdsClientApiError) { - panic!("Error received in background worker callback: {err}") + let ads_client::MozAdsClientApiError::Other { reason } = err; + panic!("Error received in background worker callback: {:?}", reason) } } +// Reusable helper to prefetches a tile ad, wait for completion, and query it. +// Should mimic the `test_contract_tile_prod_async` test. +fn generate_tile_ad_async_helper(client : &MozAdsClient) -> MozAdsTile { + // Prefetch + let placement_id= "mock_tile_1".to_string(); + let result = client.prefetch_ads( vec![], vec![], vec![MozAdsPlacementRequest { + iab_content: None, + placement_id: placement_id.clone() + }], + None, Some(Box::new(TestErrorCallback))); + + assert!( + result.is_ok(), + "Tile ad dispatch request failed: {:?}", + result.err() + ); + + // Ping + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), Some(Box::new(TestErrorCallback))); + 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() + ); + result.unwrap().expect("`query_tile_ads` in `generate_tile_ad_sync` should return Some") +} + #[test] #[ignore = "integration test: run manually with -- --ignored"] fn test_contract_image_prod_async() { @@ -200,6 +236,93 @@ fn test_contract_tile_prod_async() { assert!(placements.is_some()); } +#[test] +#[ignore = "integration test: run manually with -- --ignored"] +fn test_record_impression_async() { + init_backend(); + + let client = prod_client(); + let ad = generate_tile_ad_async_helper(&client); + + // Dispatch record_impression asynchronously + let result = client.dispatch_record_impression(ad.callbacks.impression.to_string(), None, Some(Box::new(TestErrorCallback))); + assert!( + result.is_ok(), + "record_impression failed: {:?}", + result.err() + ); + + // Ping (waits for queue to clear) + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), None); + assert!( + ping.is_ok(), + "Ping failed: {:?}", + ping.err() + ); + +} + +#[test] +#[ignore = "integration test: run manually with -- --ignored"] +fn test_record_click_async() { + init_backend(); + let client = prod_client(); + let ad = generate_tile_ad_async_helper(&client); + + // Dispatch record_click asynchronously + let result = client.dispatch_record_click(ad.callbacks.click.to_string(), None, Some(Box::new(TestErrorCallback))); + assert!(result.is_ok(), "record_click failed: {:?}", result.err()); + + // Ping (waits for queue to clear) + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), None); + assert!( + ping.is_ok(), + "Ping failed: {:?}", + ping.err() + ); + +} + +#[test] +#[ignore = "integration test: run manually with -- --ignored"] +fn test_report_ad_async() { + init_backend(); + + let client = prod_client(); + let ad = generate_tile_ad_async_helper(&client); + + let report_url = ad + .callbacks + .report + .as_ref() + .expect("mock_tile_1 should have a report URL"); + + let pairs: Vec<(_, _)> = report_url.query_pairs().collect(); + let placement_id_count = pairs.iter().filter(|(k, _)| k == "placement_id").count(); + let position_count = pairs.iter().filter(|(k, _)| k == "position").count(); + assert_eq!(placement_id_count, 1, "expected exactly one placement_id"); + assert_eq!(position_count, 1, "expected exactly one position"); + + // Dispatch report_ad asynchronously + let result = client.dispatch_report_ad( + report_url.to_string(), + MozAdsReportReason::NotInterested, + None, + Some(Box::new(TestErrorCallback)) + ); + assert!(result.is_ok(), "report_ad failed: {:?}", result.err()); + + // Ping (waits for queue to clear) + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), None); + assert!( + ping.is_ok(), + "Ping failed: {:?}", + ping.err() + ); + +} + + #[test] #[ignore = "integration test: run manually with -- --ignored"] fn test_contract_tile_ohttp_prod_async() { diff --git a/components/ads-client/src/lib.rs b/components/ads-client/src/lib.rs index 15e1fa40f81..6627f88049d 100644 --- a/components/ads-client/src/lib.rs +++ b/components/ads-client/src/lib.rs @@ -205,6 +205,11 @@ impl MozAdsClient { callback: Option>, ) -> AdsClientApiResult<()> { let callback = callback.map(Arc::new); + let options = options.unwrap_or_default(); + let flags = AdRequestFlags::from(&options); + let ohttp = options.ohttp; + let cache_policy: CachePolicy = options.into(); + if let Some(worker_dispatch) = &self.worker_dispatch { // Dispatch image requests if !image_ad_requests.is_empty() { @@ -212,7 +217,9 @@ impl MozAdsClient { .try_send(Dispatch { command: DispatchCommand::RequestImageAds { image_ad_requests, - options: options.clone(), + ohttp, + cache_policy, + flags: flags.clone(), }, error_callback: callback.clone(), }) @@ -225,7 +232,9 @@ impl MozAdsClient { .try_send(Dispatch { command: DispatchCommand::RequestSpocAds { spoc_ad_requests, - options: options.clone(), + ohttp, + cache_policy, + flags: flags.clone(), }, error_callback: callback.clone(), }) @@ -238,7 +247,9 @@ impl MozAdsClient { .try_send(Dispatch { command: DispatchCommand::RequestTileAds { tile_ad_requests, - options: options.clone(), + ohttp, + cache_policy, + flags: flags.clone(), }, error_callback: callback.clone(), }) @@ -277,6 +288,92 @@ impl MozAdsClient { Ok(image_ads.map(|ad| ad.clone().into())) } + #[handle_error(ComponentError)] + #[uniffi::method(default(options = None))] + pub fn dispatch_record_click( + &self, + click_url: String, + options: Option, + callback: Option>, + ) -> AdsClientApiResult<()> { + let callback = callback.map(Arc::new); + if let Some(worker_dispatch) = &self.worker_dispatch { + let url = AdsClientUrl::parse(&click_url).map_err(|e| { + ComponentError::RecordClick(CallbackRequestError::InvalidUrl(e).into()) + })?; + let ohttp = options.map(|o| o.ohttp).unwrap_or(false); + worker_dispatch + .try_send(Dispatch { + command: DispatchCommand::RecordClick { url, ohttp }, + error_callback: callback, + }) + .map_err(BackgroundWorkerError::from)?; + Ok(()) + } else { + Err(BackgroundWorkerError::WorkerClosed.into()) + } + } + + #[handle_error(ComponentError)] + #[uniffi::method(default(options = None))] + pub fn dispatch_record_impression( + &self, + impression_url: String, + options: Option, + callback: Option>, + ) -> AdsClientApiResult<()> { + let callback = callback.map(Arc::new); + if let Some(worker_dispatch) = &self.worker_dispatch { + let url = AdsClientUrl::parse(&impression_url).map_err(|e| { + ComponentError::RecordImpression(CallbackRequestError::InvalidUrl(e).into()) + })?; + let ohttp = options.map(|o| o.ohttp).unwrap_or(false); + + worker_dispatch + .try_send(Dispatch { + command: DispatchCommand::RecordImpression { url, ohttp }, + error_callback: callback, + }) + .map_err(BackgroundWorkerError::from)?; + + Ok(()) + } else { + Err(BackgroundWorkerError::WorkerClosed.into()) + } + } + + #[handle_error(ComponentError)] + #[uniffi::method(default(options = None))] + pub fn dispatch_report_ad( + &self, + report_url: String, + reason: MozAdsReportReason, + options: Option, + callback: Option>, + ) -> AdsClientApiResult<()> { + let callback = callback.map(Arc::new); + if let Some(worker_dispatch) = &self.worker_dispatch { + let url = AdsClientUrl::parse(&report_url).map_err(|e| { + ComponentError::ReportAd(CallbackRequestError::InvalidUrl(e).into()) + })?; + let ohttp = options.map(|o| o.ohttp).unwrap_or(false); + worker_dispatch + .try_send(Dispatch { + command: DispatchCommand::ReportAd { + url, + reason: reason.into(), + ohttp, + }, + error_callback: callback, + }) + .map_err(BackgroundWorkerError::from)?; + + Ok(()) + } else { + Err(BackgroundWorkerError::WorkerClosed.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. diff --git a/components/ads-client/src/worker.rs b/components/ads-client/src/worker.rs index 78a3947b6a9..3c876b0a294 100644 --- a/components/ads-client/src/worker.rs +++ b/components/ads-client/src/worker.rs @@ -2,20 +2,23 @@ use crate::{ client::error::{BackgroundWorkerError, ComponentError}, http_cache::CachePolicy, mars::{ - ad_request::{AdPlacementRequest, AdRequestFlags}, + ad_request::AdPlacementRequest, ad_response::{AdImage, AdSpoc, AdTile}, + ReportReason, }, AdsClientApiResult, MozAdsClientApiError, MozAdsClientInner, MozAdsPlacementRequest, - MozAdsPlacementRequestWithCount, MozAdsRequestOptions, + MozAdsPlacementRequestWithCount, }; use error_support::handle_error; use std::{ + collections::HashMap, sync::{ mpsc::{self, Receiver, SyncSender}, Arc, }, thread::JoinHandle, }; +use url::Url; pub const ADS_CLIENT_WORKER_CHANNEL_BUFFER_SIZE: usize = 1000; pub const ADS_CLIENT_WORKER_THREAD_NAME: &str = "ads-client.worker"; @@ -58,18 +61,36 @@ pub struct Dispatch { } pub enum DispatchCommand { - // TODO: Extend this with other possible commands in V2. RequestImageAds { image_ad_requests: Vec, - options: Option, + cache_policy: CachePolicy, + ohttp: bool, + flags: HashMap, }, RequestSpocAds { spoc_ad_requests: Vec, - options: Option, + cache_policy: CachePolicy, + ohttp: bool, + flags: HashMap, }, RequestTileAds { tile_ad_requests: Vec, - options: Option, + cache_policy: CachePolicy, + ohttp: bool, + flags: HashMap, + }, + RecordClick { + url: Url, + ohttp: bool, + }, + RecordImpression { + url: Url, + ohttp: bool, + }, + ReportAd { + url: Url, + reason: ReportReason, + ohttp: bool, }, Ping(SyncSender<()>), } @@ -77,18 +98,14 @@ pub enum DispatchCommand { impl DispatchCommand { #[handle_error(ComponentError)] pub fn run_command(self, ads_client_inner: &MozAdsClientInner) -> AdsClientApiResult<()> { - // TODO: Duplicated behavior with the sync functions. Behavior is pretty simple though- probably OK to duplicate. - // TODO: Should we skip these if cache exists? match self { DispatchCommand::RequestImageAds { image_ad_requests, - options, + cache_policy, + flags, + ohttp, } => { let mut inner = ads_client_inner.lock(); - let options = options.unwrap_or_default(); - let flags = AdRequestFlags::from(&options); - let ohttp = options.ohttp; - let cache_policy: CachePolicy = options.into(); // Image ads if !image_ad_requests.is_empty() { @@ -103,13 +120,11 @@ impl DispatchCommand { } DispatchCommand::RequestSpocAds { spoc_ad_requests, - options, + cache_policy, + flags, + ohttp, } => { let mut inner = ads_client_inner.lock(); - let options = options.unwrap_or_default(); - let flags = AdRequestFlags::from(&options); - let ohttp = options.ohttp; - let cache_policy: CachePolicy = options.into(); // Spoc ads if !spoc_ad_requests.is_empty() { @@ -124,15 +139,13 @@ impl DispatchCommand { } DispatchCommand::RequestTileAds { tile_ad_requests, - options, + cache_policy, + flags, + ohttp, } => { let mut inner = ads_client_inner.lock(); - let options = options.unwrap_or_default(); - let flags = AdRequestFlags::from(&options); - let ohttp = options.ohttp; - let cache_policy: CachePolicy = options.into(); - // Image ads + // Tile ads if !tile_ad_requests.is_empty() { let tile_ad_requests: Vec = tile_ad_requests.iter().map(|r| r.into()).collect(); @@ -143,6 +156,24 @@ impl DispatchCommand { } Ok(()) } + DispatchCommand::RecordClick { url, ohttp } => { + let inner = ads_client_inner.lock(); + inner + .record_click(url, ohttp) + .map_err(ComponentError::RecordClick) + } + DispatchCommand::RecordImpression { url, ohttp } => { + let inner = ads_client_inner.lock(); + inner + .record_impression(url, ohttp) + .map_err(ComponentError::RecordImpression) + } + DispatchCommand::ReportAd { url, ohttp, reason } => { + let inner = ads_client_inner.lock(); + inner + .report_ad(url, reason, ohttp) + .map_err(ComponentError::ReportAd) + } DispatchCommand::Ping(sender) => { sender From bdf8e432aa10b8c2f6f65e000a3ba5ecc4530400 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Fri, 7 Aug 2026 18:24:12 -0700 Subject: [PATCH 09/12] fix: Removes it again, clippy --- .../integration-tests/tests/mars_async.rs | 254 ++++++++---------- components/ads-client/src/lib.rs | 25 +- components/ads-client/src/worker.rs | 31 +-- 3 files changed, 126 insertions(+), 184 deletions(-) diff --git a/components/ads-client/integration-tests/tests/mars_async.rs b/components/ads-client/integration-tests/tests/mars_async.rs index eeb34aada3d..4e58954dad5 100644 --- a/components/ads-client/integration-tests/tests/mars_async.rs +++ b/components/ads-client/integration-tests/tests/mars_async.rs @@ -3,15 +3,16 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -use std::sync::Arc; -use ads_client::{ - MozAdsClient, MozAdsClientBuilder, MozAdsEnvironment, MozAdsPlacementRequest, MozAdsReportReason, MozAdsRequestOptions, worker::ErrorRequestCallback, MozAdsTile, -}; -use ads_client::MozAdsIABContentTaxonomy; use ads_client::MozAdsIABContent; +use ads_client::MozAdsIABContentTaxonomy; use ads_client::MozAdsPlacementRequestWithCount; +use ads_client::{ + MozAdsClient, MozAdsClientBuilder, MozAdsEnvironment, MozAdsPlacementRequest, + MozAdsReportReason, MozAdsRequestOptions, MozAdsTile, +}; +use std::sync::Arc; -pub const TEST_TIMEOUT_DURATION : std::time::Duration = std::time::Duration::from_secs(10); +pub const TEST_TIMEOUT_DURATION: std::time::Duration = std::time::Duration::from_secs(10); fn init_backend() { viaduct_hyper::viaduct_init_backend_hyper(); @@ -23,25 +24,21 @@ fn prod_client() -> ads_client::MozAdsClient { .build() } -struct TestErrorCallback; -impl ErrorRequestCallback for TestErrorCallback { - fn on_error(&self,err: ads_client::MozAdsClientApiError) { - let ads_client::MozAdsClientApiError::Other { reason } = err; - panic!("Error received in background worker callback: {:?}", reason) - } -} - // Reusable helper to prefetches a tile ad, wait for completion, and query it. // Should mimic the `test_contract_tile_prod_async` test. -fn generate_tile_ad_async_helper(client : &MozAdsClient) -> MozAdsTile { +fn generate_tile_ad_async_helper(client: &MozAdsClient) -> MozAdsTile { // Prefetch - let placement_id= "mock_tile_1".to_string(); - let result = client.prefetch_ads( vec![], vec![], vec![MozAdsPlacementRequest { + let placement_id = "mock_tile_1".to_string(); + let result = client.prefetch_ads( + vec![], + vec![], + vec![MozAdsPlacementRequest { iab_content: None, - placement_id: placement_id.clone() + placement_id: placement_id.clone(), }], - None, Some(Box::new(TestErrorCallback))); - + None, + ); + assert!( result.is_ok(), "Tile ad dispatch request failed: {:?}", @@ -49,21 +46,19 @@ fn generate_tile_ad_async_helper(client : &MozAdsClient) -> MozAdsTile { ); // Ping - let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), Some(Box::new(TestErrorCallback))); - assert!( - ping.is_ok(), - "Ping failed: {:?}", - ping.err() - ); + 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!( + assert!( result.is_ok(), "Querying for ads failed: {:?}", result.err() ); - result.unwrap().expect("`query_tile_ads` in `generate_tile_ad_sync` should return Some") + result + .unwrap() + .expect("`query_tile_ads` in `generate_tile_ad_sync` should return Some") } #[test] @@ -72,14 +67,18 @@ fn test_contract_image_prod_async() { init_backend(); // Prefetch - let placement_id= "mock_billboard_1".to_string(); + let placement_id = "mock_billboard_1".to_string(); let client = prod_client(); - let result = client.prefetch_ads(vec![MozAdsPlacementRequest { + let result = client.prefetch_ads( + vec![MozAdsPlacementRequest { iab_content: None, placement_id: placement_id.clone(), - }], vec![], vec![], - None, Some(Box::new(TestErrorCallback))); - + }], + vec![], + vec![], + None, + ); + assert!( result.is_ok(), "Image ad dispatch request failed: {:?}", @@ -87,16 +86,12 @@ fn test_contract_image_prod_async() { ); // Ping - let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), None); - assert!( - ping.is_ok(), - "Ping failed: {:?}", - ping.err() - ); + 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!( + assert!( result.is_ok(), "Querying for ads failed: {:?}", result.err() @@ -112,20 +107,24 @@ fn test_contract_image_with_categories_prod_async() { init_backend(); // Prefetch - let placement_id= "mock_billboard_1".to_string(); + let placement_id = "mock_billboard_1".to_string(); let client = prod_client(); - let result = client.prefetch_ads(vec![MozAdsPlacementRequest { + 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() - }], vec![], vec![], + placement_id: placement_id.clone(), + }], + vec![], + vec![], Some(MozAdsRequestOptions { flags: std::collections::HashMap::from([("contextual_placement".to_string(), true)]), ..Default::default() - }), Some(Box::new(TestErrorCallback))); - + }), + ); + assert!( result.is_ok(), "Image ad dispatch request failed: {:?}", @@ -133,21 +132,17 @@ fn test_contract_image_with_categories_prod_async() { ); // Ping - let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), None); - assert!( - ping.is_ok(), - "Ping failed: {:?}", - ping.err() - ); + 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!( + assert!( result.is_ok(), "Querying for ads failed: {:?}", result.err() ); - + let placements = result.unwrap(); assert!(placements.is_some()); let ad = placements.unwrap(); @@ -161,15 +156,19 @@ fn test_contract_spoc_prod_async() { init_backend(); // Prefetch - let placement_id= "mock_spoc_1".to_string(); + let placement_id = "mock_spoc_1".to_string(); let client = prod_client(); - let result = client.prefetch_ads(vec![], vec![MozAdsPlacementRequestWithCount { + let result = client.prefetch_ads( + vec![], + vec![MozAdsPlacementRequestWithCount { count: 3, iab_content: None, placement_id: placement_id.clone(), - }], vec![], - None, Some(Box::new(TestErrorCallback))); - + }], + vec![], + None, + ); + assert!( result.is_ok(), "Spoc ad dispatch request failed: {:?}", @@ -177,16 +176,12 @@ fn test_contract_spoc_prod_async() { ); // Ping - let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), None); - assert!( - ping.is_ok(), - "Ping failed: {:?}", - ping.err() - ); + 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!( + assert!( result.is_ok(), "Querying for ads failed: {:?}", result.err() @@ -202,14 +197,18 @@ fn test_contract_tile_prod_async() { init_backend(); // Prefetch - let placement_id= "mock_tile_1".to_string(); + let placement_id = "mock_tile_1".to_string(); let client = prod_client(); - let result = client.prefetch_ads( vec![], vec![], vec![MozAdsPlacementRequest { + let result = client.prefetch_ads( + vec![], + vec![], + vec![MozAdsPlacementRequest { iab_content: None, - placement_id: placement_id.clone() + placement_id: placement_id.clone(), }], - None, Some(Box::new(TestErrorCallback))); - + None, + ); + assert!( result.is_ok(), "Tile ad dispatch request failed: {:?}", @@ -217,16 +216,12 @@ fn test_contract_tile_prod_async() { ); // Ping - let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), None); - assert!( - ping.is_ok(), - "Ping failed: {:?}", - ping.err() - ); + 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!( + assert!( result.is_ok(), "Querying for ads failed: {:?}", result.err() @@ -245,7 +240,7 @@ fn test_record_impression_async() { let ad = generate_tile_ad_async_helper(&client); // Dispatch record_impression asynchronously - let result = client.dispatch_record_impression(ad.callbacks.impression.to_string(), None, Some(Box::new(TestErrorCallback))); + let result = client.dispatch_record_impression(ad.callbacks.impression.to_string(), None); assert!( result.is_ok(), "record_impression failed: {:?}", @@ -253,13 +248,9 @@ fn test_record_impression_async() { ); // Ping (waits for queue to clear) - let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), None); - assert!( - ping.is_ok(), - "Ping failed: {:?}", - ping.err() - ); - + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION)); + assert!(ping.is_ok(), "Ping failed: {:?}", ping.err()); + // TODO: This doesn't actually guarantee the background worker call was successful, doing so requires a callback. } #[test] @@ -270,17 +261,13 @@ fn test_record_click_async() { let ad = generate_tile_ad_async_helper(&client); // Dispatch record_click asynchronously - let result = client.dispatch_record_click(ad.callbacks.click.to_string(), None, Some(Box::new(TestErrorCallback))); + let result = client.dispatch_record_click(ad.callbacks.click.to_string(), None); assert!(result.is_ok(), "record_click failed: {:?}", result.err()); // Ping (waits for queue to clear) - let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), None); - assert!( - ping.is_ok(), - "Ping failed: {:?}", - ping.err() - ); - + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION)); + assert!(ping.is_ok(), "Ping failed: {:?}", ping.err()); + // TODO: This doesn't actually guarantee the background worker call was successful, doing so requires a callback. } #[test] @@ -308,21 +295,16 @@ fn test_report_ad_async() { report_url.to_string(), MozAdsReportReason::NotInterested, None, - Some(Box::new(TestErrorCallback)) ); assert!(result.is_ok(), "report_ad failed: {:?}", result.err()); // Ping (waits for queue to clear) - let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), None); - assert!( - ping.is_ok(), - "Ping failed: {:?}", - ping.err() - ); + let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION)); + assert!(ping.is_ok(), "Ping failed: {:?}", ping.err()); + // TODO: This doesn't actually guarantee the background call was successful, doing so requires a callback. } - #[test] #[ignore = "integration test: run manually with -- --ignored"] fn test_contract_tile_ohttp_prod_async() { @@ -337,17 +319,21 @@ fn test_contract_tile_ohttp_prod_async() { .expect("OHTTP channel configuration should succeed"); // Prefetch - let placement_id= "mock_tile_1".to_string(); + let placement_id = "mock_tile_1".to_string(); let client = prod_client(); - let result = client.prefetch_ads( vec![],vec![], vec![MozAdsPlacementRequest { + let result = client.prefetch_ads( + vec![], + vec![], + vec![MozAdsPlacementRequest { iab_content: None, placement_id: placement_id.clone(), }], - Some(MozAdsRequestOptions { - ohttp: true, - ..Default::default() - }), Some(Box::new(TestErrorCallback))); - + Some(MozAdsRequestOptions { + ohttp: true, + ..Default::default() + }), + ); + assert!( result.is_ok(), "Tile ad dispatch request failed: {:?}", @@ -355,50 +341,51 @@ fn test_contract_tile_ohttp_prod_async() { ); // Ping - let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), None); - assert!( - ping.is_ok(), - "Ping failed: {:?}", - ping.err() - ); + 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!( + 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"); + 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= "mock_billboard_1".to_string(); - let placement_spoc_id= "mock_spoc_1".to_string(); - let placement_tile_id= "mock_tile_1".to_string(); + let placement_image_id = "mock_billboard_1".to_string(); + let placement_spoc_id = "mock_spoc_1".to_string(); + let placement_tile_id = "mock_tile_1".to_string(); let client = prod_client(); let result = client.prefetch_ads( vec![MozAdsPlacementRequest { iab_content: None, placement_id: placement_image_id.clone(), - }], vec![MozAdsPlacementRequestWithCount { + }], + vec![MozAdsPlacementRequestWithCount { count: 4, iab_content: None, placement_id: placement_spoc_id.clone(), - }], vec![MozAdsPlacementRequest { + }], + vec![MozAdsPlacementRequest { iab_content: None, placement_id: placement_tile_id.clone(), }], - None, Some(Box::new(TestErrorCallback))); - + None, + ); + assert!( result.is_ok(), "Image ad dispatch request failed: {:?}", @@ -406,16 +393,12 @@ fn test_contract_multi_ad_type_prod_async() { ); // Ping - let ping = client.ping_background_worker(Some(TEST_TIMEOUT_DURATION), None); - assert!( - ping.is_ok(), - "Ping failed: {:?}", - ping.err() - ); + 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!( + assert!( result.is_ok(), "Querying for image ads failed: {:?}", result.err() @@ -424,7 +407,7 @@ fn test_contract_multi_ad_type_prod_async() { assert!(placements.is_some()); let result = client.query_spoc_ads(placement_spoc_id); - assert!( + assert!( result.is_ok(), "Querying for spoc ads failed: {:?}", result.err() @@ -434,7 +417,7 @@ fn test_contract_multi_ad_type_prod_async() { assert!(placements.unwrap().len() == 4); let result = client.query_tile_ads(placement_tile_id); - assert!( + assert!( result.is_ok(), "Querying for ads failed: {:?}", result.err() @@ -442,5 +425,4 @@ fn test_contract_multi_ad_type_prod_async() { 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 6627f88049d..bd35f812e9b 100644 --- a/components/ads-client/src/lib.rs +++ b/components/ads-client/src/lib.rs @@ -37,7 +37,7 @@ use crate::{ client::error::BackgroundWorkerError, ffi::telemetry::MozAdsTelemetryWrapper, mars::ad_response::{AdImage, AdSpoc, AdTile}, - worker::{Dispatch, DispatchCommand, ErrorRequestCallback}, + worker::{Dispatch, DispatchCommand}, }; #[cfg(test)] @@ -195,16 +195,14 @@ impl MozAdsClient { } #[handle_error(ComponentError)] - #[uniffi::method(default(image_ad_requests = [], spoc_ad_requests = [], tile_ad_requests = [], options = None, callback = None))] + #[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, - callback: Option>, ) -> AdsClientApiResult<()> { - let callback = callback.map(Arc::new); let options = options.unwrap_or_default(); let flags = AdRequestFlags::from(&options); let ohttp = options.ohttp; @@ -221,7 +219,6 @@ impl MozAdsClient { cache_policy, flags: flags.clone(), }, - error_callback: callback.clone(), }) .map_err(BackgroundWorkerError::from)?; } @@ -236,7 +233,6 @@ impl MozAdsClient { cache_policy, flags: flags.clone(), }, - error_callback: callback.clone(), }) .map_err(BackgroundWorkerError::from)?; } @@ -251,7 +247,6 @@ impl MozAdsClient { cache_policy, flags: flags.clone(), }, - error_callback: callback.clone(), }) .map_err(BackgroundWorkerError::from)?; } @@ -294,9 +289,7 @@ impl MozAdsClient { &self, click_url: String, options: Option, - callback: Option>, ) -> AdsClientApiResult<()> { - let callback = callback.map(Arc::new); if let Some(worker_dispatch) = &self.worker_dispatch { let url = AdsClientUrl::parse(&click_url).map_err(|e| { ComponentError::RecordClick(CallbackRequestError::InvalidUrl(e).into()) @@ -305,7 +298,6 @@ impl MozAdsClient { worker_dispatch .try_send(Dispatch { command: DispatchCommand::RecordClick { url, ohttp }, - error_callback: callback, }) .map_err(BackgroundWorkerError::from)?; Ok(()) @@ -320,9 +312,7 @@ impl MozAdsClient { &self, impression_url: String, options: Option, - callback: Option>, ) -> AdsClientApiResult<()> { - let callback = callback.map(Arc::new); if let Some(worker_dispatch) = &self.worker_dispatch { let url = AdsClientUrl::parse(&impression_url).map_err(|e| { ComponentError::RecordImpression(CallbackRequestError::InvalidUrl(e).into()) @@ -332,7 +322,6 @@ impl MozAdsClient { worker_dispatch .try_send(Dispatch { command: DispatchCommand::RecordImpression { url, ohttp }, - error_callback: callback, }) .map_err(BackgroundWorkerError::from)?; @@ -349,9 +338,7 @@ impl MozAdsClient { report_url: String, reason: MozAdsReportReason, options: Option, - callback: Option>, ) -> AdsClientApiResult<()> { - let callback = callback.map(Arc::new); if let Some(worker_dispatch) = &self.worker_dispatch { let url = AdsClientUrl::parse(&report_url).map_err(|e| { ComponentError::ReportAd(CallbackRequestError::InvalidUrl(e).into()) @@ -364,7 +351,6 @@ impl MozAdsClient { reason: reason.into(), ohttp, }, - error_callback: callback, }) .map_err(BackgroundWorkerError::from)?; @@ -378,17 +364,12 @@ impl MozAdsClient { // 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, - callback: Option>, - ) -> AdsClientApiResult<()> { + pub fn ping_background_worker(&self, timeout: Option) -> AdsClientApiResult<()> { if let Some(worker_dispatch) = &self.worker_dispatch { let (tx, rx) = mpsc::sync_channel(0); worker_dispatch .try_send(Dispatch { command: DispatchCommand::Ping(tx), - error_callback: callback.map(Arc::new), }) .map_err(BackgroundWorkerError::from)?; if let Some(timeout) = timeout { diff --git a/components/ads-client/src/worker.rs b/components/ads-client/src/worker.rs index 3c876b0a294..e71373e83bc 100644 --- a/components/ads-client/src/worker.rs +++ b/components/ads-client/src/worker.rs @@ -6,16 +6,13 @@ use crate::{ ad_response::{AdImage, AdSpoc, AdTile}, ReportReason, }, - AdsClientApiResult, MozAdsClientApiError, MozAdsClientInner, MozAdsPlacementRequest, + AdsClientApiResult, MozAdsClientInner, MozAdsPlacementRequest, MozAdsPlacementRequestWithCount, }; use error_support::handle_error; use std::{ collections::HashMap, - sync::{ - mpsc::{self, Receiver, SyncSender}, - Arc, - }, + sync::mpsc::{self, Receiver, SyncSender}, thread::JoinHandle, }; use url::Url; @@ -40,24 +37,15 @@ pub fn build_worker_thread( fn worker(inner_client: MozAdsClientInner, rx: Receiver) { // Synchronously run tasks in the order they are passed in this separate channel. while let Ok(task) = rx.recv() { - let Dispatch { - command, - error_callback, - } = task; + let Dispatch { command } = task; - if let Err(e) = command.run_command(&inner_client) { - // Error is logged through `handle_error` conversion macro. - // If an error callback is provided by the surface, we send the error to that, too. - if let Some(error_callback) = error_callback { - error_callback.on_error(e); - } - } + // Error is logged through `handle_error` conversion macro. + let _ = command.run_command(&inner_client); } } pub struct Dispatch { pub command: DispatchCommand, - pub error_callback: Option>>, } pub enum DispatchCommand { @@ -184,12 +172,3 @@ impl DispatchCommand { } } } - -// TODO: Uniffi does not currently support direct function callbacks, so we use an interface. -// TODO: We use #[uniffi::export(callback_interface)] rather than #[uniffi::export(with_foreign)] for reasons discussed here: -// - https://github.com/mozilla/application-services/pull/7443 -// In the future, this should be changed to uniffi::export(impl = "foreign") alongside the enum change, when uniffi = 0.32 -#[uniffi::export(callback_interface)] -pub trait ErrorRequestCallback: Send + Sync { - fn on_error(&self, err: MozAdsClientApiError); -} From ddb1f4fafefd9fef3fa21cfec048a3fa525bbe4f Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Fri, 7 Aug 2026 20:08:26 -0700 Subject: [PATCH 10/12] feat: Refactoring, telemetry --- components/ads-client/src/client.rs | 30 +++ components/ads-client/src/client/error.rs | 6 +- components/ads-client/src/ffi.rs | 20 +- components/ads-client/src/ffi/telemetry.rs | 23 ++- components/ads-client/src/lib.rs | 175 ++++++---------- components/ads-client/src/worker.rs | 217 +++++++------------- components/ads-client/src/worker/command.rs | 201 ++++++++++++++++++ 7 files changed, 393 insertions(+), 279 deletions(-) create mode 100644 components/ads-client/src/worker/command.rs diff --git a/components/ads-client/src/client.rs b/components/ads-client/src/client.rs index 1546a44dc28..901c32fd954 100644 --- a/components/ads-client/src/client.rs +++ b/components/ads-client/src/client.rs @@ -275,6 +275,7 @@ where } } +// Event fires in both sync and background strategies. #[derive(Clone, Debug, PartialEq, Eq)] pub enum ClientOperationEvent { New, @@ -284,6 +285,35 @@ pub enum ClientOperationEvent { RequestAds, } +// Event fires when dispatch is fired, not when the event resolves. +pub enum CommandDispatchedOperationEvent { + RecordClick, + RecordImpression, + ReportAd, + RequestAds, +} + +// Event fires when the corresponding background event resolves. +pub enum CommandProcessedOperationEvent { + RecordClick, + RecordImpression, + ReportAd, + RequestAds, +} + +// Event fires when the corresponding background event fails to resolve. +pub enum CommandFailedOperationEvent { + RecordClick, + RecordImpression, + ReportAd, + RequestAds, +} + +pub enum WorkerMetaEvent { + Start, + Stop, +} + #[cfg(test)] mod tests { use std::{assert_eq, assert_ne, sync::Arc}; diff --git a/components/ads-client/src/client/error.rs b/components/ads-client/src/client/error.rs index a90ae510a4c..480a1c0bc73 100644 --- a/components/ads-client/src/client/error.rs +++ b/components/ads-client/src/client/error.rs @@ -5,7 +5,7 @@ use crate::{ mars::error::{FetchAdsError, RecordClickError, RecordImpressionError, ReportAdError}, - worker::Dispatch, + worker::command, }; use std::sync::mpsc::{RecvTimeoutError, TrySendError}; @@ -51,9 +51,9 @@ pub enum BackgroundWorkerError { PongFailure(Box>), } -impl From> for BackgroundWorkerError { +impl From> for BackgroundWorkerError { // TODO: For future vertical slice (for retries), we may want to keep the failed dispatch for retrying - fn from(value: TrySendError) -> Self { + 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 8312c4b3dd1..e4c7e8de79b 100644 --- a/components/ads-client/src/ffi.rs +++ b/components/ads-client/src/ffi.rs @@ -122,6 +122,11 @@ impl MozAdsClientBuilder { pub fn build(&self) -> MozAdsClient { let inner = self.0.lock(); + let telemetry = inner + .telemetry + .clone() + .map(MozAdsTelemetryWrapper::new) + .unwrap_or_else(MozAdsTelemetryWrapper::noop); let client_config = AdsClientConfig { cache_config: inner.cache_config.clone().map(Into::into), context_id_provider: inner @@ -130,21 +135,12 @@ impl MozAdsClientBuilder { .map(MozAdsContextIdProviderWrapper::new) .map(Into::into), environment: inner.environment.unwrap_or_default().into(), - telemetry: inner - .telemetry - .clone() - .map(MozAdsTelemetryWrapper::new) - .unwrap_or_else(MozAdsTelemetryWrapper::noop), + telemetry: telemetry.clone(), }; let client = AdsClient::new(client_config); let inner = Arc::new(Mutex::new(client)); - let (worker_dispatch, worker_thread) = - Option::unzip(worker::build_worker_thread(inner.clone())); - MozAdsClient { - inner, - _worker_thread: worker_thread, - worker_dispatch, - } + let worker = worker::AdsClientWorkerWrapper::new(inner.clone(), telemetry); + MozAdsClient { inner, worker } } pub fn cache_config(self: Arc, cache_config: MozAdsCacheConfig) -> Arc { diff --git a/components/ads-client/src/ffi/telemetry.rs b/components/ads-client/src/ffi/telemetry.rs index 02a6fee2e46..c222f5f3a23 100644 --- a/components/ads-client/src/ffi/telemetry.rs +++ b/components/ads-client/src/ffi/telemetry.rs @@ -9,7 +9,7 @@ use std::sync::Arc; use parking_lot::RwLock; use crate::client::error::RequestAdsError; -use crate::client::ClientOperationEvent; +use crate::client::{ClientOperationEvent, CommandDispatchedOperationEvent, WorkerMetaEvent}; use crate::http_cache::{CacheOutcome, HttpCacheBuilderError}; use crate::mars::error::{RecordClickError, RecordImpressionError, ReportAdError}; use crate::telemetry::Telemetry; @@ -101,6 +101,27 @@ impl Telemetry for MozAdsTelemetryWrapper { }); return; } + + if let Some(client_op) = event.downcast_ref::() { + inner.record_client_operation_total(match client_op { + CommandDispatchedOperationEvent::RecordClick => "dispatch_record_click".to_string(), + CommandDispatchedOperationEvent::RecordImpression => { + "dispatch_record_impression".to_string() + } + CommandDispatchedOperationEvent::ReportAd => "dispatch_report_ad".to_string(), + CommandDispatchedOperationEvent::RequestAds => "dispatch_report_ad".to_string(), + }); + return; + } + + if let Some(client_op) = event.downcast_ref::() { + inner.record_client_operation_total(match client_op { + WorkerMetaEvent::Start => "worker_started".to_string(), + WorkerMetaEvent::Stop => "worker_ended".to_string(), + }); + return; + } + if let Some(cache_builder_error) = event.downcast_ref::() { inner.record_build_cache_error( match cache_builder_error { diff --git a/components/ads-client/src/lib.rs b/components/ads-client/src/lib.rs index bd35f812e9b..e35688a93ea 100644 --- a/components/ads-client/src/lib.rs +++ b/components/ads-client/src/lib.rs @@ -5,11 +5,7 @@ use std::{ collections::HashMap, - sync::{ - mpsc::{self, SyncSender}, - Arc, - }, - thread::JoinHandle, + sync::{mpsc, Arc}, time::Duration, }; @@ -37,7 +33,7 @@ use crate::{ client::error::BackgroundWorkerError, ffi::telemetry::MozAdsTelemetryWrapper, mars::ad_response::{AdImage, AdSpoc, AdTile}, - worker::{Dispatch, DispatchCommand}, + worker::{command::DispatchCommand, AdsClientWorkerWrapper}, }; #[cfg(test)] @@ -54,9 +50,7 @@ uniffi::custom_type!(AdsClientUrl, String, { #[derive(uniffi::Object)] pub struct MozAdsClient { inner: MozAdsClientInner, - - _worker_thread: Option>, - worker_dispatch: Option>, + worker: AdsClientWorkerWrapper, } pub type MozAdsClientInner = Arc>>; @@ -208,52 +202,36 @@ impl MozAdsClient { let ohttp = options.ohttp; let cache_policy: CachePolicy = options.into(); - if let Some(worker_dispatch) = &self.worker_dispatch { - // Dispatch image requests - if !image_ad_requests.is_empty() { - worker_dispatch - .try_send(Dispatch { - command: DispatchCommand::RequestImageAds { - image_ad_requests, - ohttp, - cache_policy, - flags: flags.clone(), - }, - }) - .map_err(BackgroundWorkerError::from)?; - } - - // Dispatch spoc requests - if !spoc_ad_requests.is_empty() { - worker_dispatch - .try_send(Dispatch { - command: DispatchCommand::RequestSpocAds { - spoc_ad_requests, - ohttp, - cache_policy, - flags: flags.clone(), - }, - }) - .map_err(BackgroundWorkerError::from)?; - } - - // Dispatch tiles requests - if !tile_ad_requests.is_empty() { - worker_dispatch - .try_send(Dispatch { - command: DispatchCommand::RequestTileAds { - tile_ad_requests, - ohttp, - cache_policy, - flags: flags.clone(), - }, - }) - .map_err(BackgroundWorkerError::from)?; - } - Ok(()) - } else { - Err(BackgroundWorkerError::WorkerClosed.into()) + // Dispatch image requests + if !image_ad_requests.is_empty() { + self.worker.dispatch(DispatchCommand::RequestImageAds { + image_ad_requests, + ohttp, + cache_policy, + flags: flags.clone(), + })?; + } + // Dispatch spoc requests + if !spoc_ad_requests.is_empty() { + self.worker.dispatch(DispatchCommand::RequestSpocAds { + spoc_ad_requests, + ohttp, + cache_policy, + flags: flags.clone(), + })?; } + + // Dispatch tiles requests + if !tile_ad_requests.is_empty() { + self.worker.dispatch(DispatchCommand::RequestTileAds { + tile_ad_requests, + ohttp, + cache_policy, + flags: flags.clone(), + })?; + } + + Ok(()) } #[handle_error(ComponentError)] @@ -290,20 +268,12 @@ impl MozAdsClient { click_url: String, options: Option, ) -> AdsClientApiResult<()> { - if let Some(worker_dispatch) = &self.worker_dispatch { - let url = AdsClientUrl::parse(&click_url).map_err(|e| { - ComponentError::RecordClick(CallbackRequestError::InvalidUrl(e).into()) - })?; - let ohttp = options.map(|o| o.ohttp).unwrap_or(false); - worker_dispatch - .try_send(Dispatch { - command: DispatchCommand::RecordClick { url, ohttp }, - }) - .map_err(BackgroundWorkerError::from)?; - Ok(()) - } else { - Err(BackgroundWorkerError::WorkerClosed.into()) - } + let url = AdsClientUrl::parse(&click_url) + .map_err(|e| ComponentError::RecordClick(CallbackRequestError::InvalidUrl(e).into()))?; + let ohttp = options.map(|o| o.ohttp).unwrap_or(false); + + self.worker + .dispatch(DispatchCommand::RecordClick { url, ohttp }) } #[handle_error(ComponentError)] @@ -313,22 +283,13 @@ impl MozAdsClient { impression_url: String, options: Option, ) -> AdsClientApiResult<()> { - if let Some(worker_dispatch) = &self.worker_dispatch { - let url = AdsClientUrl::parse(&impression_url).map_err(|e| { - ComponentError::RecordImpression(CallbackRequestError::InvalidUrl(e).into()) - })?; - let ohttp = options.map(|o| o.ohttp).unwrap_or(false); - - worker_dispatch - .try_send(Dispatch { - command: DispatchCommand::RecordImpression { url, ohttp }, - }) - .map_err(BackgroundWorkerError::from)?; + let url = AdsClientUrl::parse(&impression_url).map_err(|e| { + ComponentError::RecordImpression(CallbackRequestError::InvalidUrl(e).into()) + })?; + let ohttp = options.map(|o| o.ohttp).unwrap_or(false); - Ok(()) - } else { - Err(BackgroundWorkerError::WorkerClosed.into()) - } + self.worker + .dispatch(DispatchCommand::RecordImpression { url, ohttp }) } #[handle_error(ComponentError)] @@ -339,25 +300,14 @@ impl MozAdsClient { reason: MozAdsReportReason, options: Option, ) -> AdsClientApiResult<()> { - if let Some(worker_dispatch) = &self.worker_dispatch { - let url = AdsClientUrl::parse(&report_url).map_err(|e| { - ComponentError::ReportAd(CallbackRequestError::InvalidUrl(e).into()) - })?; - let ohttp = options.map(|o| o.ohttp).unwrap_or(false); - worker_dispatch - .try_send(Dispatch { - command: DispatchCommand::ReportAd { - url, - reason: reason.into(), - ohttp, - }, - }) - .map_err(BackgroundWorkerError::from)?; - - Ok(()) - } else { - Err(BackgroundWorkerError::WorkerClosed.into()) - } + let url = AdsClientUrl::parse(&report_url) + .map_err(|e| ComponentError::ReportAd(CallbackRequestError::InvalidUrl(e).into()))?; + let ohttp = options.map(|o| o.ohttp).unwrap_or(false); + self.worker.dispatch(DispatchCommand::ReportAd { + url, + reason: reason.into(), + ohttp, + }) } // Pings the background worker and waits for a response back, for use in tests. @@ -365,22 +315,15 @@ impl MozAdsClient { // 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<()> { - if let Some(worker_dispatch) = &self.worker_dispatch { - let (tx, rx) = mpsc::sync_channel(0); - worker_dispatch - .try_send(Dispatch { - command: DispatchCommand::Ping(tx), - }) + 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)?; - if let Some(timeout) = timeout { - rx.recv_timeout(timeout) - .map_err(BackgroundWorkerError::from)?; - } else { - rx.recv().map_err(|_| BackgroundWorkerError::WorkerClosed)?; - } - return Ok(()); } else { - Err(BackgroundWorkerError::WorkerClosed.into()) + rx.recv().map_err(|_| BackgroundWorkerError::WorkerClosed)?; } + Ok(()) } } diff --git a/components/ads-client/src/worker.rs b/components/ads-client/src/worker.rs index e71373e83bc..1b6a7600e4e 100644 --- a/components/ads-client/src/worker.rs +++ b/components/ads-client/src/worker.rs @@ -1,174 +1,97 @@ use crate::{ - client::error::{BackgroundWorkerError, ComponentError}, - http_cache::CachePolicy, - mars::{ - ad_request::AdPlacementRequest, - ad_response::{AdImage, AdSpoc, AdTile}, - ReportReason, + client::{ + error::{BackgroundWorkerError, ComponentError}, + WorkerMetaEvent, }, - AdsClientApiResult, MozAdsClientInner, MozAdsPlacementRequest, - MozAdsPlacementRequestWithCount, + telemetry::Telemetry, + worker::command::DispatchCommand, + MozAdsClientInner, }; -use error_support::handle_error; use std::{ - collections::HashMap, sync::mpsc::{self, Receiver, SyncSender}, thread::JoinHandle, }; -use url::Url; + +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 +where + T: Clone + Telemetry, +{ + _worker_thread: Option>, + worker_dispatch: Option>, + + telemetry: T, +} + +impl AdsClientWorkerWrapper { + pub fn new(inner: MozAdsClientInner, telemetry: T) -> AdsClientWorkerWrapper { + let (worker_dispatch, worker_thread) = + Option::unzip(build_worker_thread(inner.clone(), telemetry.clone())); + AdsClientWorkerWrapper { + _worker_thread: worker_thread, + worker_dispatch, + telemetry, + } + } + + pub fn dispatch(&self, command: DispatchCommand) -> Result<(), ComponentError> { + let telemetry_event = command.dispatch_telemetry_event(); + if let Some(worker_dispatch) = &self.worker_dispatch { + worker_dispatch + .try_send(command) + .map_err(BackgroundWorkerError::from) + .inspect_err(|e| { + self.telemetry.record(e); + }) + .inspect(|_| { + if let Some(event) = telemetry_event { + self.telemetry.record(&event); + } + })?; + + 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( +pub fn build_worker_thread( inner_client: MozAdsClientInner, -) -> Option<(SyncSender, JoinHandle<()>)> { + telemetry: T, +) -> 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| { + .spawn(move || crate::worker::worker(inner_client, rx, telemetry)).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(task) = rx.recv() { - let Dispatch { command } = task; - - // Error is logged through `handle_error` conversion macro. - let _ = command.run_command(&inner_client); - } -} - -pub struct Dispatch { - pub command: DispatchCommand, -} - -pub enum DispatchCommand { - RequestImageAds { - image_ad_requests: Vec, - cache_policy: CachePolicy, - ohttp: bool, - flags: HashMap, - }, - RequestSpocAds { - spoc_ad_requests: Vec, - cache_policy: CachePolicy, - ohttp: bool, - flags: HashMap, - }, - RequestTileAds { - tile_ad_requests: Vec, - cache_policy: CachePolicy, - ohttp: bool, - flags: HashMap, - }, - RecordClick { - url: Url, - ohttp: bool, - }, - RecordImpression { - url: Url, - ohttp: bool, - }, - ReportAd { - url: Url, - reason: ReportReason, - ohttp: bool, - }, - Ping(SyncSender<()>), -} - -impl DispatchCommand { - #[handle_error(ComponentError)] - pub fn run_command(self, ads_client_inner: &MozAdsClientInner) -> AdsClientApiResult<()> { - match self { - DispatchCommand::RequestImageAds { - image_ad_requests, - cache_policy, - flags, - ohttp, - } => { - let mut inner = ads_client_inner.lock(); - - // Image ads - 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) - .map_err(ComponentError::RequestAds)?; - inner.cache_ads::(image_response); - } - Ok(()) - } - DispatchCommand::RequestSpocAds { - spoc_ad_requests, - cache_policy, - flags, - ohttp, - } => { - let mut inner = ads_client_inner.lock(); - - // Spoc ads - 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) - .map_err(ComponentError::RequestAds)?; - inner.cache_ads::(spoc_response); - } - Ok(()) - } - DispatchCommand::RequestTileAds { - tile_ad_requests, - cache_policy, - flags, - ohttp, - } => { - let mut inner = ads_client_inner.lock(); +fn worker( + inner_client: MozAdsClientInner, + rx: Receiver, + telemetry: T, +) { + telemetry.record(&WorkerMetaEvent::Start); - // Tile ads - 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) - .map_err(ComponentError::RequestAds)?; - inner.cache_ads::(tile_response); - } - Ok(()) - } - DispatchCommand::RecordClick { url, ohttp } => { - let inner = ads_client_inner.lock(); - inner - .record_click(url, ohttp) - .map_err(ComponentError::RecordClick) - } - DispatchCommand::RecordImpression { url, ohttp } => { - let inner = ads_client_inner.lock(); - inner - .record_impression(url, ohttp) - .map_err(ComponentError::RecordImpression) - } - DispatchCommand::ReportAd { url, ohttp, reason } => { - let inner = ads_client_inner.lock(); - inner - .report_ad(url, reason, ohttp) - .map_err(ComponentError::ReportAd) - } + // Synchronously run tasks in the order they are passed in this separate channel. + while let Ok(command) = rx.recv() { + let failure_telemetry_event = command.failed_telemetry_event(); - DispatchCommand::Ping(sender) => { - sender - .try_send(()) - .map_err(|err| BackgroundWorkerError::PongFailure(Box::new(err)))?; - Ok(()) - } + // Error is naturally logged through `handle_error` conversion macro. + if let Err(_) = command.run_command(&inner_client, &telemetry) { + // This telemetry logs which command fails, but does not separately record the error itself. + // Because the command hits the underlying client's method, it reuses the `.record(e)` call (eg: for RequestAdsError) + telemetry.record(&failure_telemetry_event); } } + telemetry.record(&WorkerMetaEvent::Stop); } diff --git a/components/ads-client/src/worker/command.rs b/components/ads-client/src/worker/command.rs new file mode 100644 index 00000000000..5e9fb46f441 --- /dev/null +++ b/components/ads-client/src/worker/command.rs @@ -0,0 +1,201 @@ +use std::{collections::HashMap, sync::mpsc::SyncSender}; + +use error_support::handle_error; +use url::Url; + +use crate::{ + client::{ + error::{BackgroundWorkerError, ComponentError}, + CommandDispatchedOperationEvent, CommandFailedOperationEvent, + CommandProcessedOperationEvent, + }, + http_cache::CachePolicy, + mars::{ + ad_request::AdPlacementRequest, + ad_response::{AdImage, AdSpoc, AdTile}, + ReportReason, + }, + telemetry::Telemetry, + AdsClientApiResult, MozAdsClientInner, MozAdsPlacementRequest, MozAdsPlacementRequestWithCount, +}; + +pub enum DispatchCommand { + RequestImageAds { + image_ad_requests: Vec, + cache_policy: CachePolicy, + ohttp: bool, + flags: HashMap, + }, + RequestSpocAds { + spoc_ad_requests: Vec, + cache_policy: CachePolicy, + ohttp: bool, + flags: HashMap, + }, + RequestTileAds { + tile_ad_requests: Vec, + cache_policy: CachePolicy, + ohttp: bool, + flags: HashMap, + }, + 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, + telemetry: &T, + ) -> AdsClientApiResult<()> { + match self { + DispatchCommand::RequestImageAds { + image_ad_requests, + cache_policy, + flags, + ohttp, + } => { + let mut inner = ads_client_inner.lock(); + + // Image ads + 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) + .map_err(ComponentError::RequestAds)?; + inner.cache_ads::(image_response); + } + + telemetry.record(&CommandProcessedOperationEvent::RequestAds); + Ok(()) + } + DispatchCommand::RequestSpocAds { + spoc_ad_requests, + cache_policy, + flags, + ohttp, + } => { + let mut inner = ads_client_inner.lock(); + + // Spoc ads + 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) + .map_err(ComponentError::RequestAds)?; + inner.cache_ads::(spoc_response); + } + + telemetry.record(&CommandProcessedOperationEvent::RequestAds); + Ok(()) + } + DispatchCommand::RequestTileAds { + tile_ad_requests, + cache_policy, + flags, + ohttp, + } => { + let mut inner = ads_client_inner.lock(); + + // Tile ads + 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) + .map_err(ComponentError::RequestAds)?; + inner.cache_ads::(tile_response); + } + + telemetry.record(&CommandProcessedOperationEvent::RequestAds); + Ok(()) + } + DispatchCommand::RecordClick { url, ohttp } => { + let inner = ads_client_inner.lock(); + inner + .record_click(url, ohttp) + .map_err(ComponentError::RecordClick)?; + telemetry.record(&CommandProcessedOperationEvent::RecordClick); + Ok(()) + } + DispatchCommand::RecordImpression { url, ohttp } => { + let inner = ads_client_inner.lock(); + inner + .record_impression(url, ohttp) + .map_err(ComponentError::RecordImpression)?; + telemetry.record(&CommandProcessedOperationEvent::RecordImpression); + Ok(()) + } + DispatchCommand::ReportAd { url, ohttp, reason } => { + let inner = ads_client_inner.lock(); + inner + .report_ad(url, reason, ohttp) + .map_err(ComponentError::ReportAd)?; + telemetry.record(&CommandProcessedOperationEvent::ReportAd); + Ok(()) + } + + DispatchCommand::Ping(sender) => { + sender + .try_send(()) + .map_err(|err| BackgroundWorkerError::PongFailure(Box::new(err)))?; + Ok(()) + } + } + } + + pub fn dispatch_telemetry_event(&self) -> Option { + match self { + DispatchCommand::RequestImageAds { .. } + | DispatchCommand::RequestSpocAds { .. } + | DispatchCommand::RequestTileAds { .. } => { + Some(CommandDispatchedOperationEvent::RequestAds) + } + DispatchCommand::RecordClick { .. } => { + Some(CommandDispatchedOperationEvent::RecordClick) + } + DispatchCommand::RecordImpression { .. } => { + Some(CommandDispatchedOperationEvent::RecordImpression) + } + DispatchCommand::ReportAd { .. } => Some(CommandDispatchedOperationEvent::ReportAd), + DispatchCommand::Ping(_) => None, + } + } + + pub fn failed_telemetry_event(&self) -> Option { + match self { + DispatchCommand::RequestImageAds { .. } + | DispatchCommand::RequestSpocAds { .. } + | DispatchCommand::RequestTileAds { .. } => { + Some(CommandFailedOperationEvent::RequestAds) + } + DispatchCommand::RecordClick { .. } => Some(CommandFailedOperationEvent::RecordClick), + DispatchCommand::RecordImpression { .. } => { + Some(CommandFailedOperationEvent::RecordImpression) + } + DispatchCommand::ReportAd { .. } => Some(CommandFailedOperationEvent::ReportAd), + DispatchCommand::Ping(_) => None, + } + } +} From 754dd50d109af77fcb92d075abda472173898712 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Fri, 7 Aug 2026 20:45:23 -0700 Subject: [PATCH 11/12] fix: adds some missing telemetry --- components/ads-client/src/ffi/telemetry.rs | 34 ++++++++++++++++++---- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/components/ads-client/src/ffi/telemetry.rs b/components/ads-client/src/ffi/telemetry.rs index c222f5f3a23..ca00b79b7ed 100644 --- a/components/ads-client/src/ffi/telemetry.rs +++ b/components/ads-client/src/ffi/telemetry.rs @@ -9,7 +9,7 @@ use std::sync::Arc; use parking_lot::RwLock; use crate::client::error::RequestAdsError; -use crate::client::{ClientOperationEvent, CommandDispatchedOperationEvent, WorkerMetaEvent}; +use crate::client::{ClientOperationEvent, CommandDispatchedOperationEvent, CommandFailedOperationEvent, CommandProcessedOperationEvent, WorkerMetaEvent}; use crate::http_cache::{CacheOutcome, HttpCacheBuilderError}; use crate::mars::error::{RecordClickError, RecordImpressionError, ReportAdError}; use crate::telemetry::Telemetry; @@ -104,12 +104,36 @@ impl Telemetry for MozAdsTelemetryWrapper { if let Some(client_op) = event.downcast_ref::() { inner.record_client_operation_total(match client_op { - CommandDispatchedOperationEvent::RecordClick => "dispatch_record_click".to_string(), + CommandDispatchedOperationEvent::RecordClick => "cmd_dispatch_record_click".to_string(), CommandDispatchedOperationEvent::RecordImpression => { - "dispatch_record_impression".to_string() + "cmd_dispatch_record_impression".to_string() } - CommandDispatchedOperationEvent::ReportAd => "dispatch_report_ad".to_string(), - CommandDispatchedOperationEvent::RequestAds => "dispatch_report_ad".to_string(), + CommandDispatchedOperationEvent::ReportAd => "cmd_dispatch_report_ad".to_string(), + CommandDispatchedOperationEvent::RequestAds => "cmd_dispatch_report_ad".to_string(), + }); + return; + } + + if let Some(client_op) = event.downcast_ref::() { + inner.record_client_operation_total(match client_op { + CommandProcessedOperationEvent::RecordClick => "cmd_processed_record_click".to_string(), + CommandProcessedOperationEvent::RecordImpression => { + "cmd_processed_record_impression".to_string() + } + CommandProcessedOperationEvent::ReportAd => "cmd_processed_report_ad".to_string(), + CommandProcessedOperationEvent::RequestAds => "cmd_processed_report_ad".to_string(), + }); + return; + } + + if let Some(client_op) = event.downcast_ref::() { + inner.record_client_operation_total(match client_op { + CommandFailedOperationEvent::RecordClick => "cmd_failed_record_click".to_string(), + CommandFailedOperationEvent::RecordImpression => { + "cmd_failed_record_impression".to_string() + } + CommandFailedOperationEvent::ReportAd => "cmd_failed_report_ad".to_string(), + CommandFailedOperationEvent::RequestAds => "cmd_failed_report_ad".to_string(), }); return; } From af15b8396faceb54ed15a553c593a2c7b81d4971 Mon Sep 17 00:00:00 2001 From: Wyatt Verchere Date: Fri, 7 Aug 2026 20:48:30 -0700 Subject: [PATCH 12/12] fix: some fmt clippy --- components/ads-client/src/ffi/telemetry.rs | 13 ++++++++++--- components/ads-client/src/worker.rs | 2 +- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/components/ads-client/src/ffi/telemetry.rs b/components/ads-client/src/ffi/telemetry.rs index ca00b79b7ed..cbefd154622 100644 --- a/components/ads-client/src/ffi/telemetry.rs +++ b/components/ads-client/src/ffi/telemetry.rs @@ -9,7 +9,10 @@ use std::sync::Arc; use parking_lot::RwLock; use crate::client::error::RequestAdsError; -use crate::client::{ClientOperationEvent, CommandDispatchedOperationEvent, CommandFailedOperationEvent, CommandProcessedOperationEvent, WorkerMetaEvent}; +use crate::client::{ + ClientOperationEvent, CommandDispatchedOperationEvent, CommandFailedOperationEvent, + CommandProcessedOperationEvent, WorkerMetaEvent, +}; use crate::http_cache::{CacheOutcome, HttpCacheBuilderError}; use crate::mars::error::{RecordClickError, RecordImpressionError, ReportAdError}; use crate::telemetry::Telemetry; @@ -104,7 +107,9 @@ impl Telemetry for MozAdsTelemetryWrapper { if let Some(client_op) = event.downcast_ref::() { inner.record_client_operation_total(match client_op { - CommandDispatchedOperationEvent::RecordClick => "cmd_dispatch_record_click".to_string(), + CommandDispatchedOperationEvent::RecordClick => { + "cmd_dispatch_record_click".to_string() + } CommandDispatchedOperationEvent::RecordImpression => { "cmd_dispatch_record_impression".to_string() } @@ -116,7 +121,9 @@ impl Telemetry for MozAdsTelemetryWrapper { if let Some(client_op) = event.downcast_ref::() { inner.record_client_operation_total(match client_op { - CommandProcessedOperationEvent::RecordClick => "cmd_processed_record_click".to_string(), + CommandProcessedOperationEvent::RecordClick => { + "cmd_processed_record_click".to_string() + } CommandProcessedOperationEvent::RecordImpression => { "cmd_processed_record_impression".to_string() } diff --git a/components/ads-client/src/worker.rs b/components/ads-client/src/worker.rs index 1b6a7600e4e..46a3b2c8f8a 100644 --- a/components/ads-client/src/worker.rs +++ b/components/ads-client/src/worker.rs @@ -87,7 +87,7 @@ fn worker( let failure_telemetry_event = command.failed_telemetry_event(); // Error is naturally logged through `handle_error` conversion macro. - if let Err(_) = command.run_command(&inner_client, &telemetry) { + if command.run_command(&inner_client, &telemetry).is_err() { // This telemetry logs which command fails, but does not separately record the error itself. // Because the command hits the underlying client's method, it reuses the `.record(e)` call (eg: for RequestAdsError) telemetry.record(&failure_telemetry_event);