Right now only sync messages can return data. The idea here would be to allow async messages to return data via MozPromises. That way the event loop isn't tied up waiting for a response.

Coincidentally, I was also wondering about cross-process promises recently. It would be interesting to see how much of the existing actor hierarchy could be expressed more simply (and with less error-prone manual state-wrangling) using promises and ADTs. If nothing else, one-shot actors that exist only to be immediately __delete__d would be prime candidates — that's more or less equivalent to an async message returning a promise, but with a bunch of extra boilerplate actor classes. Also: actor state machine violations can become type errors, and you can guarantee that a protocol meant to be half-duplex actually is and doesn't somehow have races between to-parent and to-child messages.

Brilliant idea!

I'd like to work on this as it will ease our effort to remove all sync messages.

This is not finished but the basics are there and working so I'd like to get some early feedback. The most obvious missing pieces are shutdown handling. MozPromise asserts that promise should not be pending when it's destroyed so I have to find some way to destroy the unresolved promises when shutting down channels. The ipdl patch looks terrible so I'll attach a diff of generated code so it will be easier to understand the change.

This looks great! I have a few comments: - At this point I don't see much use for the rejection value. Rejection would only be used if the message sending failed. That would allow us to make the Send function look like this: RefPtr<MozPromise<Tuple<...>, bool, false>> SendActivate(...); And then you could do: SendActivate(...).Then(...); which means you don't have to write out the big promise type, which is nice. The boolean value passed to the rejection would always be false I guess. Or maybe some kind of enum if we wanted that. - I guess we'll also want to call the rejection handlers if the process crashes or the channel closes before the promise is resolved? I think that makes sense. - If there's only one "return value" provided, let's avoid using Tuple. - It would be great if you would declare a custom type for each promise. Something like: using ActivatePromise = MozPromise<Foo, Bar, false>::Private; Then the RecvActivate function would be declared as: bool RecvActivate(...args, RefPtr<ActivatePromise>&& aPromise); I don't want people to have to write out the big promise types if we can avoid it.

Oh, and I suspect that the AbstractThread initialization code is going to run into problems. Maybe JW knows what to do there.

(In reply to Bill McCloskey (:billm) from comment #7) > This looks great! I have a few comments: > > - At this point I don't see much use for the rejection value. Rejection > would only be used if the message sending failed. That would allow us to > make the Send function look like this: > RefPtr<MozPromise<Tuple<...>, bool, false>> SendActivate(...); > And then you could do: > SendActivate(...).Then(...); > which means you don't have to write out the big promise type, which is nice. > The boolean value passed to the rejection would always be false I guess. Or > maybe some kind of enum if we wanted that. I thought it would be useful for caller to have a standard way to return fail reasons. Rejection could be used when in sync message you return IPC_FAIL. I use uint32_t here so the caller can customize the code with some kind of enum. The caller has to provide both resolve and reject callback anyway because they take different types. We can use custom types for promises to make it shorter. > - I guess we'll also want to call the rejection handlers if the process > crashes or the channel closes before the promise is resolved? I think that > makes sense. Yeah, maybe using nsresult for the rejection value could cover both needs. > - If there's only one "return value" provided, let's avoid using Tuple. This surely will make the lower.py patch more complex but should not be too hard to do. > - It would be great if you would declare a custom type for each promise. > Something like: > using ActivatePromise = MozPromise<Foo, Bar, false>::Private; > Then the RecvActivate function would be declared as: > bool RecvActivate(...args, RefPtr<ActivatePromise>&& aPromise); > I don't want people to have to write out the big promise types if we can > avoid it. We will need one for the resolver and one for the consumer promise, so maybe something like ActivatePromise and ActivatePromiseResolver JW, you're welcome to provide more idiomatic name :)

(In reply to Kan-Ru Chen [:kanru] (UTC-4, EDT) from comment #9) > I thought it would be useful for caller to have a standard way to return > fail reasons. Rejection could be used when in sync message you return > IPC_FAIL. I use uint32_t here so the caller can customize the code with some > kind of enum. The caller has to provide both resolve and reject callback > anyway because they take different types. We can use custom types for > promises to make it shorter. IPC_FAIL serves a different purpose: it crashes the child process. If you just want to return an error to the child from a sync message, you have to use one of the "returns" values to do it. I think we should do the same thing for promises: errors that are not fatal should be passed via the "returns" type (so through normal resolution, not rejection). > > - It would be great if you would declare a custom type for each promise. > > Something like: > > using ActivatePromise = MozPromise<Foo, Bar, false>::Private; > > Then the RecvActivate function would be declared as: > > bool RecvActivate(...args, RefPtr<ActivatePromise>&& aPromise); > > I don't want people to have to write out the big promise types if we can > > avoid it. > > We will need one for the resolver and one for the consumer promise, so maybe > something like If we do as I propose, then people are unlikely to have to write the consumer type. They can just do: SendFoo(...)->Then(...) That would mean we wouldn't need two typedefs.

(In reply to Bill McCloskey (:billm) from comment #10) > If we do as I propose, then people are unlikely to have to write the > consumer type. They can just do: > SendFoo(...)->Then(...) > That would mean we wouldn't need two typedefs. I see. So the meaning of rejection becomes simpler: only when sending failed asynchronously. Like you proposed to call the rejection handlers if the process crashes or the channel closes before the promise is resolved. Cool, I think this is better than what I originally have!

Comment on attachment 8849999 [details] [diff] [review] wip.diff Review of attachment 8849999 [details] [diff] [review]: ----------------------------------------------------------------- The overall cpp changes look fine to me. I will wait for more details to reveal to give more comments about the usage of MozPromise. ::: ipc/glue/MessageChannel.cpp @@ +836,5 @@ > + RefPtr<MozPromiseRefcountable> ret = iter->second; > + mPromises.erase(iter); > + return ret.forget(); > + } > + return nullptr; Is it an error if you can't find the seq number in the map? ::: ipc/glue/MessageChannel.h @@ +156,5 @@ > bool Send(Message* aMsg); > > + // Asynchronously send a message to the other side of the channel > + // and wait for asynchronous reply > + bool Send(Message* aMsg, RefPtr<MozPromiseRefcountable>&& aPromise); I would prefer to pass an already_AddRefed<>. @@ +497,5 @@ > void RunMessage(MessageTask& aTask); > > typedef LinkedList<RefPtr<MessageTask>> MessageQueue; > typedef std::map<size_t, Message> MessageMap; > + typedef std::map<size_t, RefPtr<MozPromiseRefcountable>> PromiseMap; You can't really do much with MozPromiseRefcountable. Maybe you want to store MozPromise. Also I would prefer to store already_AddRefed<> to avoid accidental ref-counting overhead.

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #12) > Comment on attachment 8849999 [details] [diff] [review] > wip.diff > > Review of attachment 8849999 [details] [diff] [review]: > ----------------------------------------------------------------- > > The overall cpp changes look fine to me. I will wait for more details to > reveal to give more comments about the usage of MozPromise. > > ::: ipc/glue/MessageChannel.cpp > @@ +836,5 @@ > > + RefPtr<MozPromiseRefcountable> ret = iter->second; > > + mPromises.erase(iter); > > + return ret.forget(); > > + } > > + return nullptr; > > Is it an error if you can't find the seq number in the map? It is but it could happen if the message was malformed or crafted. In that case I think we will want to raise a IPC channel error. > ::: ipc/glue/MessageChannel.h > @@ +156,5 @@ > > bool Send(Message* aMsg); > > > > + // Asynchronously send a message to the other side of the channel > > + // and wait for asynchronous reply > > + bool Send(Message* aMsg, RefPtr<MozPromiseRefcountable>&& aPromise); > > I would prefer to pass an already_AddRefed<>. Yeah, I was lazy because it was hard to generate .downcast in the code generator but I have wrote the codegen part anyway this can be fixed. > @@ +497,5 @@ > > void RunMessage(MessageTask& aTask); > > > > typedef LinkedList<RefPtr<MessageTask>> MessageQueue; > > typedef std::map<size_t, Message> MessageMap; > > + typedef std::map<size_t, RefPtr<MozPromiseRefcountable>> PromiseMap; > > You can't really do much with MozPromiseRefcountable. Maybe you want to > store MozPromise. Also I would prefer to store already_AddRefed<> to avoid > accidental ref-counting overhead. I wish I could store MozPromise. The problem is each MozPromise is a different type so you cannot store them in std::map. I will need to be able to call Reject in case the channel shuts down so I really which I can call MozPromise methods. I was thinking storing a pair like type: (MozPromiseRefcountable r, RejectFunctionPointer f) and call r->f() when needed. Do you have any suggestion?

(In reply to Kan-Ru Chen [:kanru] (UTC-4, EDT) from comment #13) > I wish I could store MozPromise. The problem is each MozPromise is a > different type so you cannot store them in std::map. > > I will need to be able to call Reject in case the channel shuts down so I > really which I can call MozPromise methods. I was thinking storing a pair > like type: (MozPromiseRefcountable r, RejectFunctionPointer f) and call > r->f() when needed. > > Do you have any suggestion? The declaration of MozPromise is template<typename ResolveValueT, typename RejectValueT, bool IsExclusive> class MozPromise. I expect different async functions to have different types for ResolveValueT. So it is impossible to store heterogeneous types in a container. I think you can create a abstract super class to wrap MozPromises of different types so they can be store in a container. You also need RTTI to downcast a generic type to a specific type of MozPromose on which you can call Resolve() or Reject(). I believe our ipdl has its own custom RTTI which involves unmarshelling a series of bytes transferred across process boundaries to a specific type designated by the ipdl.

Comment on attachment 8856763 [details] Bug 1313200 - Allow IPC messages to async return MozPromises. https://reviewboard.mozilla.org/r/128690/#review131770 ::: ipc/chromium/src/base/message_loop.cc:174 (Diff revision 1) > pump_ = new base::MessagePumpLibevent(); > } else { > pump_ = new base::MessagePumpDefault(); > } > #endif // OS_POSIX > + mozilla::Unused << Why discarding the result? ::: ipc/glue/MessageChannel.h:92 (Diff revision 1) > typedef mozilla::Monitor Monitor; > > + struct PromiseHolder > + { > + RefPtr<MozPromiseRefcountable> mPromise; > + std::function<void(const char*)> mRejectFunction; Let mRejectFunction capture a RefPtr<Promise> instead of a raw pointer at #179 below so you don't need to store mPromise. ::: ipc/glue/MessageChannel.h:95 (Diff revision 1) > + { > + RefPtr<MozPromiseRefcountable> mPromise; > + std::function<void(const char*)> mRejectFunction; > + }; > + static Atomic<size_t> gUnresolvedPromises; > + friend class PromiseReporter; You don't need a friend class since PromiseHolder is an inner class which is able to access the private members of the outer class. ::: ipc/glue/MessageChannel.h:722 (Diff revision 1) > // in-calls racing with replies to outstanding in-calls. See > // https://bugzilla.mozilla.org/show_bug.cgi?id=521929. > MessageMap mOutOfTurnReplies; > > + // Map of async Promises that are still waiting replies. > + PromiseMap mPromises; Might be worth naming it mPendingPromises to indicate these promises are waiting to be resolved. ::: ipc/glue/MessageChannel.cpp:553 (Diff revision 1) > #ifdef OS_WIN > mEvent = CreateEventW(nullptr, TRUE, FALSE, nullptr); > MOZ_RELEASE_ASSERT(mEvent, "CreateEvent failed! Nothing is going to work!"); > #endif > + > + static Atomic<bool> registered; IIRC, Function-static initialization is not thread-safe in MSVC. See bug 1314787. ::: ipc/glue/MessageLoopAbstractThreadWrapper.h:44 (Diff revision 1) > + NS_NewRunnableFunction([wrapper]() { sCurrentThreadTLS.set(wrapper); }); > + aMessageLoop->PostTask(r.forget()); > + return wrapper.forget(); > + } > + > + explicit MessageLoopAbstractThreadWrapper(MessageLoop* aMessageLoop) This should be made private as we always use Create() above. ::: ipc/glue/MessageLoopAbstractThreadWrapper.h:53 (Diff revision 1) > + } > + virtual void Dispatch(already_AddRefed<nsIRunnable> aRunnable, > + DispatchFailureHandling aFailureHandling = AssertDispatchSuccess, > + DispatchReason aReason = NormalDispatch) override > + { > + if (aReason != NormalDispatch) { Can be as simple as MOZ_RELEASE_ASSERT(aReason == NormalDispatch, ...);

Comment on attachment 8856763 [details] Bug 1313200 - Allow IPC messages to async return MozPromises. https://reviewboard.mozilla.org/r/128690/#review131770 > Why discarding the result? The function creates a wrapper and set it to a thread local like mozilla::AbstractThread::CreateXPCOMThreadWrapper does so the result can be discarded. > Let mRejectFunction capture a RefPtr<Promise> instead of a raw pointer at #179 below so you don't need to store mPromise. It is needed to store mPromise in order to get it back in MessageChannel::PopPromise(). I think I can't retrieve a variable captured in a lambda, right? > Might be worth naming it mPendingPromises to indicate these promises are waiting to be resolved. Good idea. > IIRC, Function-static initialization is not thread-safe in MSVC. See bug 1314787. TBH, this is copied from ShmemReporter in ipc. I'll check if there is other way to initialize this.

Comment on attachment 8856763 [details] Bug 1313200 - Allow IPC messages to async return MozPromises. https://reviewboard.mozilla.org/r/128690/#review131770 > TBH, this is copied from ShmemReporter in ipc. I'll check if there is other way to initialize this. Just FYI, we have threadsafe function-static initialization nowadays. The last blocker to doing so was Windows XP support. Go forth and program as the C++11 committee intended you to do! :)

(In reply to Nathan Froyd [:froydnj] from comment #25) > Comment on attachment 8856763 [details] > Bug 1313200 - Allow IPC messages to async return MozPromises. > > https://reviewboard.mozilla.org/r/128690/#review131770 > > > TBH, this is copied from ShmemReporter in ipc. I'll check if there is other way to initialize this. > > Just FYI, we have threadsafe function-static initialization nowadays. The > last blocker to doing so was Windows XP support. Go forth and program as > the C++11 committee intended you to do! :) Oh! It's good to know that :)

Comment on attachment 8856763 [details] Bug 1313200 - Allow IPC messages to async return MozPromises. https://reviewboard.mozilla.org/r/128690/#review132264 It still seems weird to me to allow the Recv handler to reject the promise with |false|, since that's also the value that the Send method will get if the send failed. Could we assert that the user only rejects with the value |true|? That way they could still reject, but they wouldn't be able to get the different codes confused. It might also be better to use an enum for this type. Something like kSendError versus kHandlerRejected. ::: ipc/glue/MessageChannel.cpp:856 (Diff revision 1) > mLink->SendMessage(msg.forget()); > return true; > } > > +already_AddRefed<MozPromiseRefcountable> > +MessageChannel::PopPromise(const Message* aMsg) Passing const Message& is a little more idiomatic. ::: ipc/ipdl/ipdl/cxx/ast.py:299 (Diff revision 1) > def __init__(self, name, const=0, > ptr=0, ptrconst=0, ptrptr=0, ptrconstptr=0, > ref=0, > hasimplicitcopyctor=True, > - T=None): > + T=None, > + inner=None): Can you comment somewhere what these parameters mean? Maybe something like this: Represents the type |name<T>::inner| with the ptr and const modifiers as specified. ::: ipc/ipdl/ipdl/lower.py:4208 (Diff revision 1) > + seqno = ExprVar('seqno__') > + resolve = ExprVar('resolve__') > + reason = ExprVar('unused__') > + promise = Type(md.promiseName()) > + failifsendok = StmtIf(ExprNot(sendok)) > + failifsendok.addifstmts(errfn('Error sending reply', None)) We shouldn't use FatalError here. There are cases where the child has crashed or something and the child won't find out until it tries to send a message. In this case, we would crash the parent unnecessarily. I think we should just ignore the error here.

I really like the patch in general though. Thanks.

(In reply to Bill McCloskey (:billm) from comment #27) > Comment on attachment 8856763 [details] > Bug 1313200 - Allow IPC messages to async return MozPromises. > > https://reviewboard.mozilla.org/r/128690/#review132264 > > It still seems weird to me to allow the Recv handler to reject the promise > with |false|, since that's also the value that the Send method will get if > the send failed. Could we assert that the user only rejects with the value > |true|? That way they could still reject, but they wouldn't be able to get > the different codes confused. It might also be better to use an enum for > this type. Something like kSendError versus kHandlerRejected. So I'll use a enum class RejectCode and assert that user only rejects with kHandlerRejected. If the user reject with other values in release build I'll just send kHandlerRejected anyway.

Comment on attachment 8856763 [details] Bug 1313200 - Allow IPC messages to async return MozPromises. https://reviewboard.mozilla.org/r/128690/#review131770 > You don't need a friend class since PromiseHolder is an inner class which is able to access the private members of the outer class. This is PromiseReporter for memory reporting not PromiseHolder though.

Pushed by kchen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/325bf1bf5a1f Allow IPC messages to async return MozPromises. r=billm,jwwang

JW and Bill, thanks for the review! I know the code is not the easiest to review. I'll write a email to dev-platform to explain how to use this.

Backed out for Android and reftest bustage: https://hg.mozilla.org/integration/autoland/rev/4d6f71c78e1fdb726a424311c7592ed26c66979a Push with failures (there is another mass bustage from a different bug in the original push): https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b708d01388e3862be454e28985755687fe2bb0d4&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=retry There are at least mass XPCshell failures and leaks.

Comment on attachment 8859583 [details] Bug 1313200 - Init AbstractThread properly and early. https://reviewboard.mozilla.org/r/131584/#review134378 r=me with changes below and an appropriate answer to the following: It's kind of sad that XPCOM initialization can't take care of this, but I guess we're doing IPC before XPCOM even starts up, is that a correct understanding of the situation here? ::: toolkit/xre/nsEmbedFunctions.cpp:424 (Diff revision 1) > GeckoProfilerInitRAII profiler(&aLocal); > > PROFILER_LABEL("Startup", "XRE_InitChildProcess", > js::ProfileEntry::Category::OTHER); > > + AbstractThread::InitStatics(); Let's add a comment: // Ensure AbstractThread is minimally setup, so async IPC messages work properly. ::: xpcom/threads/AbstractThread.cpp:254 (Diff revision 1) > MOZ_ASSERT(sMainThread); > return sMainThread; > } > > void > AbstractThread::InitStatics() Let's call this function InitTLS now.

(In reply to Nathan Froyd [:froydnj] from comment #39) > Comment on attachment 8859583 [details] > Bug 1313200 - Init AbstractThread properly and early. > > https://reviewboard.mozilla.org/r/131584/#review134378 > > r=me with changes below and an appropriate answer to the following: It's > kind of sad that XPCOM initialization can't take care of this, but I guess > we're doing IPC before XPCOM even starts up, is that a correct understanding > of the situation here? For plugin and gmp processes we are doing IPC even without init XPCOM. For content process init XPCOM requires IPC. It's sad we have so many way to initialize a process to fully functional. > ::: toolkit/xre/nsEmbedFunctions.cpp:424 > (Diff revision 1) > > GeckoProfilerInitRAII profiler(&aLocal); > > > > PROFILER_LABEL("Startup", "XRE_InitChildProcess", > > js::ProfileEntry::Category::OTHER); > > > > + AbstractThread::InitStatics(); > > Let's add a comment: > > // Ensure AbstractThread is minimally setup, so async IPC messages work > properly. > > ::: xpcom/threads/AbstractThread.cpp:254 > (Diff revision 1) > > MOZ_ASSERT(sMainThread); > > return sMainThread; > > } > > > > void > > AbstractThread::InitStatics() > > Let's call this function InitTLS now. Thanks!

Pushed by kchen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/343ba6948356 Init AbstractThread properly and early. r=froydnj https://hg.mozilla.org/integration/autoland/rev/5af938bc434b Allow IPC messages to async return MozPromises. r=billm,jwwang

Thank you!

I filed bug 1357853 to see if we can have XPCOM everywhere. It would sure be nice.

Does this work with PBackground actors?

(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #47) > Does this work with PBackground actors? Yes, it should.

Have you actually tested that? Because last I checked AbstractThread was not initialized on the PBackground parent worker thread, for example.

(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #49) > Have you actually tested that? Because last I checked AbstractThread was > not initialized on the PBackground parent worker thread, for example. With this patch the MessageChannel will setup the necessary AbstractThread for worker threads to use MozPromise. So I would expect it to work for all IPC actors.

So in a PBackground Recv*() call in the parent side of the actor I can do AbstractThread::GetCurrent() when doing the MozPromise Then() call? I guess I would have expected a change to BackgroundImpl.cpp if that was done. Sorry for my confusion.

(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #51) > So in a PBackground Recv*() call in the parent side of the actor I can do > AbstractThread::GetCurrent() when doing the MozPromise Then() call? I guess > I would have expected a change to BackgroundImpl.cpp if that was done. > Sorry for my confusion. Yes, you can use AbstractThread::GetCurrent(). BackgroundImpl.cpp doesn't need change because the underlying IPC channel does it.

https://hg.mozilla.org/mozilla-central/rev/343ba6948356 https://hg.mozilla.org/mozilla-central/rev/5af938bc434b

:kanru, when I tried to use this capability, I faced a build failure on some platforms like the following. Can you comment about it? https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f1c273e373e0942dff54f5678697e140ccf989b https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a160970c64524b7aa4213e71b6c8aa8abb0efdc

Oh, the static analysis is too smart. Could you try this patch?

Thanks, the problem was addressed with attachment 8860370 [details] [diff] [review] https://treeherder.mozilla.org/#/jobs?repo=try&revision=629d5f93780a0ab290f726711902b3f73b2c8576

(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #52) > (In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment > #51) > > So in a PBackground Recv*() call in the parent side of the actor I can do > > AbstractThread::GetCurrent() when doing the MozPromise Then() call? I guess > > I would have expected a change to BackgroundImpl.cpp if that was done. > > Sorry for my confusion. > > Yes, you can use AbstractThread::GetCurrent(). BackgroundImpl.cpp doesn't > need change because the underlying IPC channel does it. Bug 1362732 is found while using AbstractThread::GetCurrent() on PBackground thread.