Open Bug 1626703 Opened 5 years ago Updated 2 years ago

Factor-out Ferry

Categories

(Firefox :: Sync, task, P3)

task

Tracking

()

People

(Reporter: eoger, Unassigned)

Details

The fxa crate will also need to do some work in a background thread, and it seems that Ferry might help us with that, so we should try to make Ferry as generic as possible.

Thanks for filing this, Ed! Here's a bit more about how this works in golden_gate (https://phabricator.services.mozilla.com/D65268)...

Ferry is an enum, and holds arguments for whatever operations you want to run on the background thread. It has constructors for each one of those operations, and those constructors take care of converting between XPCOM and Rust types—nsACString and ThinVec<nsCString> into String and Vec<String>, for example. There's also a FerryResult, which holds any return value that can be converted into an nsIVariant. Finally, there's a FerryTask that actually takes care of calling the code on the background thread. FerryTask leans on the moz_task library, which defines both a Task trait (which FerryTask implements), and a TaskRunnable that actually dispatches the Task to another thread. A FerryTask has two more important pieces: it makes a ThreadPtrHolder for holding a thread-safe reference to the callback, and then calls it in done with the FerryResult transformed into an nsIVariant.

So, why do we need all these? And what can we make generic?

  • ThreadPtrHandle and ThreadPtrHolder are needed so that our task can safely hold a reference to our callback. Our task is created on the main thread, runs on the background thread (where moz_task calls run), and then ping-pongs back to the main thread (where done is called). That means its reference count (and the reference count of its RefPtr members) might be touched on either thread. But, JavaScript callbacks aren't safe to use from another thread—just accessing the callback's reference count (not even calling it!) is enough to fatally assert.
  • The callback type can be generic—any XPCOM interface (anything that implements the XpCom trait). But, the callback might take different arguments—something other than an nsIVariant. We might be able to abstract this into a trait that we have our nsIWhatever type implement. A complication there is that, if we have a generic trait Callback in our ferry crate, and a mozIFxACallback interface, we can't actually do impl ferry::Callback for xpcom::interfaces::mozIFxACallback in our fxa crate, because neither the trait nor the type live in fxa. One way around this is to have ferry "know" all the callback types, and implement Callback for all of them automatically; another might be to make it generic over a Fn. But something to think about.
  • The Ferry and FerryResult variants are trickier—they depend on the actual methods we want to call. In golden_gate, Ferry is for an Arc<E: BridgedEngine>, and all the variants are named after methods on BridgedEngine and mozIBridgedSyncEngine. But, for a generic implementation—what are the variants, and what's the type of E? I don't know if we'll need macro magic here, or something like it, to generate those method bindings.
  • Ditto for FerryResult—without something like a macro, how do we know what E's methods are, and how do we know what they return?
  • Converting between Rust and XPCOM types can build on the generic helpers also.

It might be that we make Ferry a trait also, and parameterize FerryTask over it—something like a trait Ferry { type Error; fn call<T: IntoVariant>(&self) -> Result<T, E> }, or even an Fn. That would keep the callback machinery around, and leave the consuming crate like fxa or golden_gate to define its Ferry type. But then, at some point, it gets to be more complex defining and implementing all these traits, than using what we have as a conceptual blueprint, and accepting a bit of copy-pasting. I don't think we're at that point, but something to keep in mind.

Just a few hurried thoughts I had earlier. Thanks for kicking off the discussion, Ed! 🎉

Priority: -- → P3
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.