1374368 - Using promises in IPC has unexpected message ordering

Status: NEW

(Core :: IPC, enhancement, P3)

Description

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

This is a problem that came up in bug 1343728. Suppose the child does this:

SendAsyncMessageWithPromise().Then([] { foo; });

Now suppose the parent's RecvAsyncMessageWithPromise calls the resolve function and then sends message Bar afterwards. In the child, here's what happens:

1. On the main thread, we receive the AsyncMessageWithPromise reply and resolve the promise. However, this only causes us to dispatch another runnable to actually run the promise's code.
2. We receive the Bar message and call RecvBar.
3. We finally run the lambda containing foo.

One solution here is to resolve the promise from the I/O thread. I'm a little worried about that, though, because it means that we have to run deserialization on the I/O thread, and it's very important that the I/O thread not get janked (since it handles all protocols).

Another solution would be to add a special way to resolve a promise immediately (in the same turn of the event loop). I'm not sure what the consequences of that are, though. It seems less dangerous to me, though.

Kan-Ru, JW, what do you guys think? I'm somewhat in favor of the second solution (changing MozPromise so that you can optionally resolve immediately).

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Is this really a problem?  This is the same behavior you have if you build your own "op" style actor tied to a MozPromise.  This seems like a documentation issue to me.

Comment 2

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

If we don't fix it, then people just won't be able to use promises in cases where ordering is required. I suspect there will be quite a few messages where this matters, so it effectively makes the promise code a lot less useful. So I think we should fix it.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Bill McCloskey (:billm) from comment #2)
> If we don't fix it, then people just won't be able to use promises in cases
> where ordering is required. I suspect there will be quite a few messages
> where this matters, so it effectively makes the promise code a lot less
> useful. So I think we should fix it.

I'm not sure I understand.  If ordering is required, why can't the code just pass RecvBar through a dispatch or MozPromise as well?

Comment 4

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

(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #3)
> I'm not sure I understand.  If ordering is required, why can't the code just
> pass RecvBar through a dispatch or MozPromise as well?

It could be awkward to do that, or inefficient.

I guess I would just prefer for this stuff to work the "right" way. I hate having to say to people, "The reason you're crashing/failing/whatever is because of this weird, sucky part of IPDL that only makes sense to IPC experts. Let me explain...". Many of these issues (like all the issues with failures and actor deletion) are hard to fix without breaking existing code. So I'd like to fix this issue now before there are many users that come to depend on the current behavior.

Comment 5

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

I'm the person who needed this ordering constraint. In my case I am also able to work around this by polling the MozPromise to see if it is resolved on every spin of the event loop (as I am spinning a nested event loop at the time), but it's a bit crummy. I can also work around it by not using the promise infrastructure, and instead implementing it myself with a custom response message, which I handle directly when it is received.

I feel like having some sort of ResolveSynchronously method on the Promise might be a good solution here, as it would give us the ordering I expected when using the code, without potentially janking the I/O thread. It also doesn't seem strange to me that IPC code would resolve promises more synchronously. It kinda fits with the expectations from JS as well due to the use of microtasks in JS promises.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

It just strikes me as a red flag.  This whole thing was an attempt to simplify a common pattern of combining async IPC operations with MozPromise.  Thats fine and good.  When it starts requiring changes to MozPromise for some reason, though, then its no longer a convenience change equivalent to the old pattern.

Another way to put it, is this IPC+MozPromise API worth making it harder to reason about all MozPromise objects in the system?  Adding exceptions and configuration options to MozPromise makes it more complex to use them everywhere.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

The other option is not to use MozPromise if you need this ordering guarantee.

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

I don't think we should rely on MozPromise for this specific ordering.

1. You can supply an arbitrary thread to Then() so there is no guarantee foo() should run before RecvBar().
2. The child shouldn't assume when the promise will be resolved by the parent. So there is no guarantee foo() should run before RecvBar().

In your case, if the parent always sends a Bar message before resolving the promise, it means these 2 things are highly coupled and should be combined into one message.

Comment 9

[Kan-Ru Chen [:kanru] (UTC+9)]

TBH I'm leaning towards having MozPromise supports microtasks like behavior so the ordering is less surprising. But I agree with :bkelly and :jwwang that one should not assume the promise will be resolved immediately by the parent.

If the code looks like this:

  IPCResult
  RecvRequest(aResolve)
  {
    aResolve(rv);
    SendAnotherMessage();
  }

Then the SendAnotherMessage should be merged to the reply of Request.

In other cases the ordering requirement here can be resolved by chaining the operation to the first promise since the entire operation depends on it.

Comment 10

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

I like the microtask idea. We could do something like that pretty easily. We could create an event target whose Dispatch method calls RunInStableState. Then we could pass that event target to the promise's Then handler. We wouldn't need to change MozPromise at all.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Kick me, but we don't have proper microtasks for normal Promises yet either :(
But end of task model might be ok-ish.
Calling random code in RunInStableState won't work currently. If anything complicated happens there, you may end up spinning event loop and that asserts.

Comment 12

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

(In reply to Olli Pettay [:smaug] from comment #11)
> Kick me, but we don't have proper microtasks for normal Promises yet either
> :(
> But end of task model might be ok-ish.
> Calling random code in RunInStableState won't work currently. If anything
> complicated happens there, you may end up spinning event loop and that
> asserts.

I don't need to call random code in RunInStableState, so this should work for my case. In my case I just need to copy some values out of the promise callback, and then return.