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

Status: RESOLVED FIXED

(Core :: IPC, enhancement)

Description

[Nika Layzell [:nika] (ni? for response)]

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.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Yes, the MozPromise variant has been (for the most part) implemented on top of this new variant in the patches I have locally.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

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

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

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Attachment: PBrowserChild.cpp.diff

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: PBrowserChild.h.diff

Comment 6

[Nika Layzell [:nika] (ni? for response)]

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 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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?

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e37007fcbd34
Add a callback-based Send API to async returning IPDL methods, r=billm

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/29e4123595f5
Part 2: Pass callbacks by rvalue reference when possible, a=bustage

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/e37007fcbd34
https://hg.mozilla.org/mozilla-central/rev/29e4123595f5
https://hg.mozilla.org/mozilla-central/rev/c84df1050600