Closed Bug 1418048 Opened 2 years ago Closed 2 years ago

Add a callback-based Send API to async returning IPDL methods

Categories

(Core :: IPC, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: Nika, Assigned: Nika)

References

Details

Attachments

(3 files)

Currently if you write an async IPDL method which has a return value, we expose a SendXXX method which returns a MozPromise. This MozPromise can then be ->Then-ed to run code when it is resolved or rejected.

Unfortunately, using this API loses ordering guarantees which IPDL provides. MozPromise::Then takes an event target, which the resolve runnable is dispatched to. This means that the resolve callback's code doesn't have any ordering guarantees relative to the processing of other IPC messages coming over the same protocol.

This adds a new overload to SendXXX with two additional arguments, a lambda callback which is called if the call succeeds, and a lambda callback which is called if the call fails. These will be called in order with other IPC messages sent over the same protocol.

I want this to fix some IPC ordering issues which are being encountered in bug 1416728.
Are you able to express the MozPromise variant in terms of this new callback-based method?

Overall seems like a good idea to me.  MozPromise is good, but sometimes you don't need everything it provides.
Yes, the MozPromise variant has been (for the most part) implemented on top of this new variant in the patches I have locally.
Currently if you write an async IPDL method which has a return value, we expose
a SendXXX method which returns a MozPromise. This MozPromise can then be
->Then-ed to run code when it is resolved or rejected.

Unfortunately, using this API loses ordering guarantees which IPDL provides.
MozPromise::Then takes an event target, which the resolve runnable is dispatched
to. This means that the resolve callback's code doesn't have any ordering
guarantees relative to the processing of other IPC messages coming over the same
protocol.

This adds a new overload to SendXXX with two additional arguments, a lambda
callback which is called if the call succeeds, and a lambda callback which is
called if the call fails. These will be called in order with other IPC messages
sent over the same protocol.

MozReview-Commit-ID: FZHJJaSDoZy
Attachment #8929522 - Flags: review?(wmccloskey)
Attachment #8929524 - Attachment is patch: false
Attachment #8929525 - Attachment is patch: false
Something which I don't do here but might be nice, would be to generate a default argument for the aReject callback when the method is defined on the child side of an IPC actor, so that you can choose not to handle the failure case from within the content process.

I figure that should be done in a follow-up, although it shouldn't be too tricky to implement.
Comment on attachment 8929522 [details] [diff] [review]
Add a callback-based Send API to async returning IPDL methods

Review of attachment 8929522 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/glue/MessageChannel.h
@@ +75,5 @@
>  };
>  
> +template<typename T>
> +using ResolveCallback = std::function<void (T&&)>;
> +using RejectCallback = std::function<void (ResponseRejectReason)>;

Can you put a newline between these? Otherwise it kind of looks like the second definition is also templated.

@@ +125,5 @@
> +                       RejectCallback aReject)
> +            : mResolve(Move(aResolve))
> +        {
> +            this->mReject = Move(aReject);
> +            this->mActorId = aActorId;

Why not provide a constructor for UntypedCallbackHolder?
Attachment #8929522 - Flags: review?(wmccloskey) → review+
Pushed by nika@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e37007fcbd34
Add a callback-based Send API to async returning IPDL methods, r=billm
Pushed by nika@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/29e4123595f5
Part 2: Pass callbacks by rvalue reference when possible, a=bustage
Pushed by nika@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c84df1050600
Part 3: Accept callbacks passed to async-returning SendXXX methods as rvalue references on a CLOSED TREE, a=bustage
You need to log in before you can comment on or make changes to this bug.