1514328 - Support using microtask-style Then() callbacks for single-threaded MozPromise

Status: NEW

(Core :: XPCOM, enhancement)

Description

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

This is important for the situations when the ordering of the resolve callback is important relative to other events, such as when doing async IPC callbacks. Performing an extra dispatch can unfortunately screw up ordering of e.g. async IPC messages with replies.

One possible answer if we only care about handling threads which already have a Microtask handler, is that we could add a |ThenMicrotask| method, which acts like |Then|, except that it doesn't take a nsISerialEventTarget, and insetad would create a MicrotaskRunnable, and call CycleCollectedJSContext::DispatchToMicrotask to flush it out when the microtask is run. (It would probably also RELEASE_ASSERT that the current thread when |ThenMicrotask| is called is the same as the current thread when the promise is resolved)

This however doesn't work on threads which don't currently have a Microtask queue. This includes threads like the background thread which never runs JS code.

Unfortunately, there's currently no queue like a Microtask queue on non-JS running processes. This would have to be added as a native feature to the XPCOM event loop in order to be usable in arbitrary contexts. This can _probably_ be done, but it may need to be kept separate from the JS implementation, which would be an unfortunate duplication of concerns.

Comment 1

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

ni? bholley for any thoughts he has on this.

Comment 2

[Bobby Holley (:bholley)]

If we don't care about ordering relative to JS, and basically just want to execute at the front of the event loop rather than the back, we probably want the semantics of the stable state queue, rather than those of microtasks. They're considerably simpler, I think, since it just means running after every task in the event loop [1].

We currently rely on this to implement "tail dispatch", which is something used by AbstractThread/StateWatching/StateMirroring to get the sort of "as soon as the current task is done" semantics that I think you want here.

I think we could probably implement an internal stable state queue directly as part of the event loop (in AfterProcessTask), use it for what you're proposing here, and perhaps move the existing StableState usage in the media code to that.

[1] https://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla23CycleCollectedJSContext23ProcessStableStateQueueEv&redirect=false

Comment 3

[Boris Zbarsky [:bzbarsky]]

Note that in the spec "stable state" now means "microtask", by the way...  So the behavior of our "stable state" stuff might change.

Comment 4

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

Why would we need microtask scheduling for UA things? microtasks are effectively synchronous from UA perspective.
And if there isn't JS running, microtask is end-of-task model.

Is there some reason we need some pseudo-asynchronousness there and not just execute synchronously?

Comment 5

[Bobby Holley (:bholley)]

So, the existing use-case of the tail dispatch stuff is described at [1]. In essence, we want to wait until we unwind out of related to code in order to invoke state change handlers. Code using this stuff code can run either on the main thread, or a TaskQueue-driven thread pool. In the latter case, the tail dispatch is handled explicitly. In the former, it's dispatched using StableState.

I think the most straightforward thing to do here is to add a general mechanism to XPCOM event loops to run something at the end of the current task. That's effectively what stable-state does now, except it's unnecessarily tied to the XPCOMJSRuntime. We can then port the tail-dispatch stuff from stable-state to this new thing (which should be equivalent timing-wise), which would extricate our internal C++ usage from the spec concepts. We can then use the new thing for the MozPromise use-case Nika is proposing in comment 0.

Does that seem sensible to everyone?

[1] https://searchfox.org/mozilla-central/rev/47edbd91c43db6229cf32d1fc4bae9b325b9e2d0/xpcom/threads/StateWatching.h#18

Comment 6

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

Sounds good to me.

Comment 7

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

Sounds like a good plan to me :-)

One question though, if we're in an end-of-task task, and dispatch a runnable to run at end-of-task, when would we be wanting that runnable to run? IIRC resolving microtasks during a microtask just adds to the end of the queue of microtasks, and doesn't start a new queue, so that's the behaviour I'd be inclined to replicate here. 

For example, the following prints 0 -> 1 -> 2 -> 1.1:

Promise.resolve().then(() => console.log("promise 0"));
Promise.resolve().then(() => {
  console.log("promise 1");
  Promise.resolve().then(() => console.log("promise 1.1"));
});
Promise.resolve().then(() => console.log("promise 2"));

Comment 8

[Jean-Yves Avenard [:jya]]

We've had request for "tail" dispatch like Then() in the past (or the task to be immediately run if the promise is already resolved).

What about a Tail Dispatch Then, which would queue the task on the TaskQueue's tail queue, or the main thread stable state ? (like state mirror does).

So something like:
Promise()->TailThen(AbstractThread_WithTail_Dispatch), __func, []() {});

That would be available to all AbstractThread that supports tail's dispatch (so not all nsISerialEventTarget), no need to be limited to JS ones.

Now, in regards to Nika's original problem, if all you use are async IPC that wraps a MozPromise, why would there be an issue with ordering so long as the target thread is the same (though I didn't see much the point of the new callback API that extended MozPromise)

Comment 9

[Bobby Holley (:bholley)]

(In reply to :Nika Layzell from comment #7)
> Sounds like a good plan to me :-)
> 
> One question though, if we're in an end-of-task task, and dispatch a
> runnable to run at end-of-task, when would we be wanting that runnable to
> run? IIRC resolving microtasks during a microtask just adds to the end of
> the queue of microtasks, and doesn't start a new queue, so that's the
> behaviour I'd be inclined to replicate here. 

Yes, I think that's what we want. It's the simplest, it's what TailDispatch already does, and i think it's what callers expect.

(In reply to Jean-Yves Avenard [:jya] from comment #8)
> We've had request for "tail" dispatch like Then() in the past (or the task
> to be immediately run if the promise is already resolved).
> 
> What about a Tail Dispatch Then, which would queue the task on the
> TaskQueue's tail queue, or the main thread stable state ? (like state mirror
> does).
> 
> So something like:
> Promise()->TailThen(AbstractThread_WithTail_Dispatch), __func, []() {});
> 
> That would be available to all AbstractThread that supports tail's dispatch
> (so not all nsISerialEventTarget), no need to be limited to JS ones.

Functionally, I think we all agree. Nika's objection is that doing this on AbstractThread doesn't work for XPCOM threads that are neither the main thread nor TaskQueues. So the proposal here is to generalize the tail-dispatch support we have for the main thread so that it works for arbitrary threads, and then use that.

Comment 10

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

If we still need this, I'd add end-of-task queue to nsThread. Tasks in it would get handled at the end of the normal task.
And if an end-of-task thingie adds more end-of-tasks, those would be handled asap (i.e. not waiting for another task to run). So basically modify current stable-state handling to become available on all the threads, and not be called stable-state to but end-of-task.

Whatever AbstractThread is doing could be hopefully implemented on top of that.

(But please, let's stop talking about microtasks and stable-state here, that is just confusing, since those are in the spec same scheduling and depend on js stack, which we don't want there.
We do want to get rid of random stable-state usage - that has already shown quite a few bugs when JS gets run when it shouldn't. And we should fix our stable-state to follow the spec.)