Closed Bug 1368031 Opened 2 years ago Closed 2 years ago

Pass a Resolve function instead of a MozPromise to async return methods

Categories

(Core :: IPC, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: kanru, Assigned: kanru)

References

Details

Attachments

(3 files, 1 obsolete file)

The rationale behind this are:

1. We already don't want the user to call Reject
2. It is easier to control the flow if the Resolve method immediately send the results back. The IPC message is already async so there is no point to the added delay of MozPromise.
New code gen looks like

// PContentParent.h
class PContentParent :
    public mozilla::ipc::IToplevelProtocol,
    public SupportsWeakPtr<PContentParent>
{
public:
    MOZ_DECLARE_WEAKREFERENCE_TYPENAME(PContentParent)

...

public:
    typedef std::function<void(PipelineId)> AllocPipelineIdResolver;

...

    virtual mozilla::ipc::IPCResult
    RecvAllocPipelineId(AllocPipelineIdResolver&& aResolve) = 0;

// PContentParent.cpp
    case PContent::Msg_AllocPipelineId__ID:
        {
            if (mozilla::ipc::LoggingEnabledFor("PContentParent")) {
                mozilla::ipc::LogMessageForProtocol("PContentParent", OtherPid(), "Received ", ((&(msg__)))->type(), mozilla::ipc::MessageDirection::eReceiving);
            }
            PROFILER_LABEL("PContent", "Msg_AllocPipelineId", js::ProfileEntry::Category::OTHER);

            PContent::Transition(PContent::Msg_AllocPipelineId__ID, (&(mState)));
            int32_t id__ = MSG_ROUTING_CONTROL;

            int32_t seqno__ = (msg__).seqno();
            WeakPtr<PContentParent> self__ = this;
            AllocPipelineIdResolver resolver = [this, self__, id__, seqno__](PipelineId aParam) {
                if ((!(self__))) {
                    NS_WARNING("Not resolving promise because actor is dead.");
                    return;
                }
                if ((mState) == (PContent::__Dead)) {
                    NS_WARNING("Not resolving promise because actor is destroyed.");
                    return;
                }
                bool resolve__ = true;
                PipelineId pipelineId;
                pipelineId = aParam;
                IPC::Message* reply__ = PContent::Reply_AllocPipelineId(id__);
                Write(resolve__, reply__);
                // Sentinel = 'resolve__'
                (reply__)->WriteSentinel(3997392463);
                Write(pipelineId, reply__);
                // Sentinel = 'pipelineId'
                (reply__)->WriteSentinel(4067092043);
                (reply__)->set_reply();
                (reply__)->set_seqno(seqno__);

                if (mozilla::ipc::LoggingEnabledFor("PContentParent")) {
                    mozilla::ipc::LogMessageForProtocol("PContentParent", OtherPid(), "Sending reply ", (reply__)->type(), mozilla::ipc::MessageDirection::eSending);
                }
                bool sendok__ = (GetIPCChannel())->Send(reply__);
                if ((!(sendok__))) {
                    NS_WARNING("Error sending reply");
                }
            };
            if ((!(RecvAllocPipelineId(mozilla::Move(resolver))))) {
                mozilla::ipc::ProtocolErrorBreakpoint("Handler returned error code!");
                // Error handled in mozilla::ipc::IPCResult
                return MsgProcessingError;
            }

            return MsgProcessed;
        }
Attachment #8871698 - Flags: review?(wmccloskey)
Attachment #8871699 - Flags: review?(sotaro.ikeda.g)
Assignee: nobody → kchen
Comment on attachment 8871697 [details] [diff] [review]
0001-Bug-1368031-Do-not-send-reply-if-actor-is-dead-r-bil.patch

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

::: ipc/ipdl/ipdl/lower.py
@@ +4329,5 @@
>                          StmtDecl(Decl(Type.INT32, seqno.name),
>                                   init=ExprCall(ExprSelect(self.msgvar, '.', 'seqno'))),
> +                        StmtDecl(Decl(Type('WeakPtr', T=ExprVar(self.clsname)),
> +                                      selfvar.name),
> +                                 init=ExprVar.THIS),

What's this for?
Attachment #8871697 - Flags: review?(wmccloskey) → review+
Attachment #8871698 - Flags: review?(wmccloskey) → review+
(In reply to Bill McCloskey (:billm) from comment #4)
> Comment on attachment 8871697 [details] [diff] [review]
> 0001-Bug-1368031-Do-not-send-reply-if-actor-is-dead-r-bil.patch
> 
> Review of attachment 8871697 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: ipc/ipdl/ipdl/lower.py
> @@ +4329,5 @@
> >                          StmtDecl(Decl(Type.INT32, seqno.name),
> >                                   init=ExprCall(ExprSelect(self.msgvar, '.', 'seqno'))),
> > +                        StmtDecl(Decl(Type('WeakPtr', T=ExprVar(self.clsname)),
> > +                                      selfvar.name),
> > +                                 init=ExprVar.THIS),
> 
> What's this for?

The WeakPtr will be captured by the resolve lambda function and be used to check if it's still alive.
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #5)
> (In reply to Bill McCloskey (:billm) from comment #4)
> > Comment on attachment 8871697 [details] [diff] [review]
> > 0001-Bug-1368031-Do-not-send-reply-if-actor-is-dead-r-bil.patch
> > 
> > Review of attachment 8871697 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: ipc/ipdl/ipdl/lower.py
> > @@ +4329,5 @@
> > >                          StmtDecl(Decl(Type.INT32, seqno.name),
> > >                                   init=ExprCall(ExprSelect(self.msgvar, '.', 'seqno'))),
> > > +                        StmtDecl(Decl(Type('WeakPtr', T=ExprVar(self.clsname)),
> > > +                                      selfvar.name),
> > > +                                 init=ExprVar.THIS),
> > 
> > What's this for?
> 
> The WeakPtr will be captured by the resolve lambda function and be used to
> check if it's still alive.

Generated code:

            WeakPtr<PContentParent> self__ = this;
            AllocPipelineIdResolver resolver = [this, self__, id__, seqno__](PipelineId aParam) {
                if ((!(self__))) {
                    NS_WARNING("Not resolving promise because actor is dead.");
                    return;
                }
Attachment #8871699 - Flags: review?(sotaro.ikeda.g) → review+
Comment on attachment 8871699 [details] [diff] [review]
0003-Bug-1368031-Update-AllocPipelineId-method-in-Content.patch

Bug 1366915 removed this part.
Attachment #8871699 - Attachment is obsolete: true
Blocks: 1313200
The IPDL codegen changed the resolver promise to a resolver function. This patch fixes the PProfiler::GatherProfile.
Attachment #8872999 - Flags: review?(mstange)
Attachment #8872999 - Flags: review?(mstange) → review+
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #0)
> The IPC message is already async so there is no point to
> the added delay of MozPromise.

Well, there is a point, but it seems no user of this needs it at the moment: If the receiver runs on the main thread, and it needs to wait for the network or the file system to compute its answer, then it would want to resolve the promise asynchronously.

If you're enforcing a synchronous reply, I think returning the value would be more obvious than calling a callback function.
Maybe with a mozilla::Result<YourReturnType, IPCResult>.
(In reply to Markus Stange [:mstange] from comment #9)
> (In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #0)
> > The IPC message is already async so there is no point to
> > the added delay of MozPromise.
> 
> Well, there is a point, but it seems no user of this needs it at the moment:
> If the receiver runs on the main thread, and it needs to wait for the
> network or the file system to compute its answer, then it would want to
> resolve the promise asynchronously.
> 
> If you're enforcing a synchronous reply, I think returning the value would
> be more obvious than calling a callback function.
> Maybe with a mozilla::Result<YourReturnType, IPCResult>.

You can wrap the callback function in a promise so it's more flexible.

  MozPromiseHolder<ReturnType> holder;
  RefPtr<ReturnTypePromise> promise = holder.Ensure(__func__);
  promise->Then(AbstractThread::MainThread(), __func__, [aResolve](ReturnType aRv) {
    aResolve(aRv);
  },
  [](){});
Oh, I see, so the callback function can be hold on to and called at a later time. That's great then, thanks.
Pushed by kchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4507e6e48d48
Do not send reply if actor is dead r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a39e2222a47
Pass resolver function to Recv methods instead of a MozPromise r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6f613b7ec08
Fix PProfiler::GatherProfile r=mstange
You need to log in before you can comment on or make changes to this bug.