[thesuzerain]

This adds the background worker part of the first layer of the fire-and-forget ads client.

This is still missing:

• The FFI interface

• The actual resolution of DispatchRequest

• The integration tests (which are written and worked in the original version of this PR, but used the FFI functions- I'm happy though to revamp all these tests if we want them in this PR)

• Telemetry

• Request bulking (eg: getting multiple of the requests in one send)

Pull Request checklist

• Breaking changes: This PR follows our breaking change policy
  • This PR follows the breaking change policy:
    • This PR has no breaking API changes, or
    • There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
• Quality: This PR builds and tests run cleanly
  • Note:
    • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
    • If this pull request includes a breaking change, consider cutting a new release after merging.
• Tests: This PR includes thorough tests or an explanation of why it does not
• Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
  • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
• Dependencies: This PR follows our dependency management guidelines
  • Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

[Almaju]

I would say, let's no put anything in "What's changed" yet because right now it's still dev only. This can be misleading if someone wanted to try it. If we wanted to be extra clean we could even flag all the stateful code under some cargo flag. I think we could argue not to do it because we have a strong test suite and we mostly manage the vendoring end to end right now but that's on the table I think.

[Almaju]

Do you think it would be a lot of work on the MARS side to be able to just accept one request variant with Vec<MozAdsPlacementRequestWithCount> (count default to 1 if not provided for example)? I feel the 3 variants create a lot of code. I am not against doing this in future cleanup PR though if you prefer.

[thesuzerain]

See my other comment- I definitely want to do this but I think it makes more sense to do in a separate PR as it will require MARS changes.

[Almaju]

I get the point but my worry is that this isn't a refactor we'd be deferring, it's code we'd rather not write at all.

A command returns nothing precisely so the queue can be heterogeneous: anything goes in, one drain is one call. Typing it by ad type puts that constraint back, and the three variants plus the split/merge around them only exist to be deleted once MARS takes a batch.

So if MARS is the blocker, I would prefer if we add some batch request(...) function on mars.rs that takes all placements in one call, then build this on top of it once? I can look into it if you need!

[Almaju]

I think we should not use the word "cache" because it's not really a cache, it's more like a store / view / aggregate / projection / query model. (like CQRS pattern)
Also I am not sure why this would belong to client.rs which should be essentially the public API.

[thesuzerain]

Good point, this is a vestige from the old version.

[Almaju]

Looking at this, I feel it would probably be better not to use StorableAd but instead do something like:

pub fn query_spoc() -> Option<Spoc>
pub fn query_tile() -> Option<Spoc>
etc.

Then on the store side, have a generic parameter T: Deserialize.
I don't think we want to send errors because what would the client do with them? Instead, if there is an error, we should send it through telemetry, reschedule a command request and return None.

[thesuzerain]

So, my initial plan for it was to do that on the next layer. eg: query_tile that calls get_stored_ad with an extraction mechanism. But I'm happy to move that to this layer instead. I'm not sure though if it makes sense to immediately reschedule a command request on that though given more than just the placement_id is needed, right?

I'll replace this with what my idea was.

[Almaju]

Fair enough! It feels like the downcast logic could be part of the store directly as it sounds like store logic to me (and later the query as part of the client).

[Almaju]

Why not just BackgroundWorker?
AdsClient -> we are already in the ads-client crate so not sure we need that prefix
Wrapper -> when I see this, I expect to see some AdsClientWorker and something like:

pub struct AdsClientWorkerWrapper(Arc<Mutex<AdsClientWorker>>);

at least in a uniffi environment.

[thesuzerain]

My logic was that it wasn't the thread itself, but I agree with your point.

[Almaju]

Why the _ prefix?

[thesuzerain]

Mostly because the JoinHandle is never actually read, it just needs to be held somewhere so it isn't dropped. _ prevents clippy + linting processes from identifying it as dead code because it's never referenced

[Almaju]

I need to dig that, I am not sure how buffer size work but I would maybe explain in the comment why we think "10000" is a good idea. Also, if it's in bytes, we could maybe use the Byte struct directly. I never feel safe with unit conversion when using raw integers.

[thesuzerain]

I'm happy to change it to use bytes in theory, but:

• I'm not totally sure if there is a good justification for any particular number- my thought was that it shouldn't be unlimited (if we have an infinite queue backup for some reason, we shouldn't allow it to build up and OOM), but it should be suitably very large to be extremely unlikely to hit it .
• This ends up being plugged into the sync::channel as a number itself, not converted to bytes before its use. If we store this as bytes, we would need to convert it from bytes using sizeof and DispatchCommand

[Almaju]

I dug into this and I found that a positive bound preallocates the whole buffer (array.rs#L104) so possibly megabytes before anything is dispatched.

But now that I am reading it again, I'm not sure a channel is the right data structure here at all. It can't drain-all at a tick, can't merge duplicate placements. Something we want is to be able to see there are 5 pending requests and batch them all in one go.

[Almaju]

Given the signature, this very much looks like a constructor method.

[thesuzerain]

Sure- mostly its just a helper, but I can incorporate this into the builder.

[Almaju]

Hmmm, this is coupling. I don't think the background thread should know about MozAdsClient stuff. It should be solely responsible for managing the background thread. The client should be responsible of the orchestration.

[thesuzerain]

This kind of goes back to the problem of the client being locked with almost every function. Unless we rewrite a lot of things, the MozAdsClient layer gets a lock on the inner AdsClient with every single ads fetch etc.

We need to be able to send commands to the worker thread fire-and-forget, which means we can't have that be inside the Mutex<AdsClient<...>>, or whenever it is locked for any reason (eg: running a synchronous command or if one is running in the background).

We can maybe make it so that there is a second MARSClient separately held by the worker thread and clone some logic but I'm not sure how that would work with synchronous behaviour.

FWIW, MozAdsClientInner is also not inherently knowing about the MozAdsClient, but it does allow for it to do similar things as the MozAdsClient.
pub type MozAdsClientInner = Arc<Mutex<AdsClient<MozAdsTelemetryWrapper>>>;

[Almaju]

I really don't like that we start adding business logic to the FFI object. It should be minimal and only be responsible of FFI concerns, not owning anything -> that's the role of the core client.rs. I understand that this is probably because of the shutdown requirement and it seemed okay as a quick fix for HNT but I would prefer if we can discuss a better long term solution for this one. 🤔

[thesuzerain]

See my above comment about the fire-and-forget- we need to refactor these locks IMO if so. I'm not sure how easy that would be. 'ts not for the shutdown requirement but its for a similar reason as the shutdown requirement

[Almaju]

I just realized we might not need the mut as ads_store is behing a Mutex already so we never actually use self as a mutable ref.

[Almaju]

And not sure why we would need the client to hold that function actually?
I feel like we would have some command queue, the worker pulls them and write to the store directly but I don't see why the client would write directly?

[thesuzerain]

Open for discussion on this. We obviously don't want this to be very long to ensure requests are sent out ASAP. I'm comfortable reducing this or fully removing it (and having the below loop repeatedly incessantly get locks).

[thesuzerain]

Update: it will be 100ms and set as ad efault.

[Almaju]

BackgroundWorker shouldn't depend on the FFI layer (MozAdsClientInner, AdsClientApiResult, #[handle_error]). It can take Arc<Mutex<AdsClient<T>>> and return core errors instead, and let FFI wrap it.

Core should stand on its own, with FFI built on top:

1. No cyclic dependencies
2. Room for other bindings (e.g. wasm)
3. FFI only translates, no business logic

[Almaju]

Not sure about that.

1. Option -> why would this be None? I feel like we should always have a queue available (that can be empty)
2. Arc -> I guess we need it because we will push to the queue outside of the worker buuuut perhaps RequestQueue could be Send + Sync by itself? And would it be a good scenario to have an actual lifetime parameter meaning the queue would be owned by the client and shared to the background worker?
3. Mutex -> I guess that makes sense to me but also could use &mut (see above)

I am not saying it's wrong but more that I get why we need Arc<Mutex<...>> in the upper FFI layer because of uniffi requirements however not a big fan of wrapping everything in those smart pointers if there is an option to do it more like rust expect us to do?

Again, I might be wrong here, just want to make sure you considered it.

[thesuzerain]

1. My logic here is that if the background worker is not enabled, we should have a corresponding mark for the queue for when we try to push it. In the long run, I know we are expecting background worker to always be on, but that's not necessarily the case for a while. If the background worker is disabled, I want to be able to return an Error or something similar if we try to push to the queue, rather then just have the queue silently grow without ever being read from. If we are OK with building the queue silently with no worker consuming it, I'm totally happy to get rid rid of this.

2/3. I'm not opposed to this but I don't really think this will work unless I am misunderstanding your idea. We can't just pass a &mut RequestQueue to the background worker- even if we got past any difficulties doing so, we would not be able to take another mutable reference to the RequestQueue at the same time in the client, which we would need to write to the queue.

We could move the Mutexes inside the RequestQueue, but that to me isn't really any better or worse then having them be outside.

We could keep the RequestQueue entirely owned by the client, no mutex, then have a Mutex to the client held in the background thread, and the background thread could 'request' the next queue items from the client (this is kinda what is reflected below, as we do have Mutex<AdsClient<...>> for now). But in this case, we can never get rid of the Mutex<AdsClient<...>> lock, even with the refactor we are discussing elsewhere.

[thesuzerain]

An update on (1) based on later conversations in this PR, I'm fine with worker_request_queue: Option<Arc<Mutex<RequestQueue>>>, being changed to worker_request_queue: RequestQueue, if its internally mutable. I still think we shouldn't push to a queue if nothing is expected to query from it, but we can have a function in the background worker such as is_running to confirm that and keep the RequestQueue part simple.

[Almaju]

Hmmm perhaps we should wait for the telemetry refactor first then so that we don't need that complicated generic type

[Almaju]

I feel like the background worker is doing too much here, I feel like it should accept dispatch(request) but something else (the queue itself?) should do the actual routing logic. In my opinion, background worker should be as much as possible only responsible for thread logic, not business logic.

[thesuzerain]

I'm totally in agreement about that. Originally I had this logic all in the queue, but it got refactored when we were discussing having two distinct kinds of requests (ones that required queuing and ones that could be batched). I can create an enum supertype that includes both and separates them on the RequestQueue end.

[thesuzerain]

I've 'remerged' it to use DispatchRequest. Now stuff like QueuedRequest will be internal to request

[Almaju]

not sure why this has Mutex has it's never using mut

[thesuzerain]

Yes, but the AdsClient is already wrapped by Mutex in MozAdsClient.

In addition this actually would be necessary within handle_request if we fully implemented handle_request now, because doing any request on the current AdsClient does require a lock on it. (the locks problem)

If we fully remove the locks (see my other PRs), then yes, we can remove Mutex from the MozAdsClient hold of AdsClient, and correspondingly remove it here.

(tldr: This is necessary if we merge this but we can get rid of it if we get rid of locks)

[Almaju]

I think this belongs in AdsClientConfig, not AdsStoreConfig. The store only reads and writes ads, and the batch interval is about when requests are sent.

Perhaps we don't need AdsStoreConfig at all if this is just for db_path and this could be directly in AdsClientConfig?

[thesuzerain]

This was something I was considering, and ended up 50/50ish on it. My logic for putting it here was that the batch interval is for the worker which only applies to the ads store (and is thus optional and conceptually tied together here under the same feature flags). But I don't have an objection to moving it.

[Almaju]

Could be a Duration directly.
Also not sure if this needs to be an Option. The default to 100ms could happen in the AdsClient part directly, also it would be easier to track.
So basically MozAdsClientConfig (ffi) -> bunch of Option -> turns into AdsClientConfig (core) -> no more Option, everything is defaulted (this where db_path can become :memory: and batch_interval can become 'Duration::from_millis(100)`

[thesuzerain]

Could be a Duration directly.
Good point, I am seeing that uniffi allows duration to be passed, so I'll use duration the whole way through.

I'm not totally sure I agree with the defaulting of the option in that From<AdsClientConfig> though. This is because:

1. the worker thread is generated in MozAdsClientBuilder::build() and not in the AdsClient.
2. ADS_CLIENT_WORKER_SLEEP_LENGTH_DEFAULT (or equivalent) should be stored in the worker module and should be used there IMO I would at least expect the BackgroundWorker::new() to take in the Option. This is more of a nit though and not one I'm tied to.

Regarding (1), we don't even particularly need the field in AdsClientConfig, as it's used immediately in the same function. We could remove it from AdsClientConfig and unwrap_or in MozAdsClientBuilder::build()

Another strategy would be now that the locks are gone, we can keep the BackgroundWorker inside the AdsClient rather then the MozAdsClient (then it would make more sense to unwrap_or as it turns into AdsClientConfig, and we can keep ADS_CLIENT_WORKER_SLEEP_LENGTH_DEFAULT in the client module alongside some other constants. But given that the worker probably also wants a reference to AdsClient (see other response below about fn worker), the design becomes a bit circular and the initialization of both things becomes a bit uglier (but this might be the best design choice here if we want to keep MozAdsClient trim).

[Almaju]

Do we really need the whole AdsClient as a dependency? It feels like this should be rather MarsClient, maybe even &'a MarsClient?

[thesuzerain]

We will also need the ads_store to dump ads into, the context_id_component to make the call to request ads, and those fields are the two other fields of AdsClient (and until revamp of telemetry is in, we need that telemtry field too). We could pass all of those individually but we would then need to also recreate the AdsClient functions on the worker side- it seems a bit redundant to me when we could just pass the AdsClient. WDYT?

[Almaju]

I realize perhaps it would be easier if the lock is managed by the RequestQueue internally

[thesuzerain]

I'm fine with that. These are quite similar to me but if you prefer the lock to be encapsulated I have no objection. I may make an internal struct for it so that only one lock needs to be gotten (avoids any potential thinking about deadlocks like the field doc comments in LazyDb).

[thesuzerain]

I'll remove the _ on this once its used.