w3ctag / promises-guide

A guide for spec authors on how to use Promises in prose and WebIDL.
https://www.w3.org/2001/tag/doc/promises-guide
192 stars 24 forks source link

"Resolve" and "reject" should specify when the code runs #47

Closed jyasskin closed 8 years ago

jyasskin commented 8 years ago

We're calling Resolve p with x and Reject p with r from in parallel blocks, but they invoke ECMAScript primitives that I believe can only be invoked from the main thread.

In particular, these should say whether they interleave with another microtask chain.

I think the right thing to do is to queue a task on p's relevant settings object's responsible event loop, but that introduces a dependency on HTML that isn't otherwise present.

jan-ivar commented 8 years ago

Isn't this covered in https://www.w3.org/2001/tag/doc/promises-guide#queue-tasks ?

jyasskin commented 8 years ago

https://www.w3.org/2001/tag/doc/promises-guide#queue-tasks is guidance for authors, and asserts that you don't need to queue tasks because "resolve p with x" does it for you, but I can't find anything in the definition of "resolve p with x" that actually does the queuing.

domenic commented 8 years ago

The intent was basically that resolve and reject are "threadsafe" and can be called even from off the main thread. Since they use internal promise operations, and their only results are observable at microtask timing (after transitioning back to script), this seems OK. @bzbarsky, thoughts?

bzbarsky commented 8 years ago

Doing it that way seems like it fundamentally introduces races, in several ways.

First, say we have two promises A and B that will get resolved by a background thread from some spec algorithm. If we've done:

A.then(f).then(g);
B.then(h);

in that order on the main thread and the non-main thread resolves A and B in a single synchronous algorithm running on that thread, then it seems like depending on thread scheduling we can see either f,g,h or f,h,g as execution orders on the main thread.

Of course in practice JS engines tend to be single-threaded, so an actual implementation would probably have to queue a task to the main thread to resolve a promise. But now you have the choice of queuing a single task that resolves both A and B (which guarantees f,h,g execution order, I believe) or queuing separate tasks to resolve A and B (which could lead to f,h,g or f,g,h depending on what other tasks happen to get queued in between the two the background thread queues). EDIT: Separate tasks lead to guaranteed f,g,h, as @jyasskin points out.

Note that in this particular situation having "resolve p with x" automatically post the runnable puts us right back in the racy condition... EDIT: This claim is false, agains as @jyasskin points out.

OK, but what if the background thread is only resolving a single promise? Now say we have promises A and B. A will get resolved on the main thread, B on a non-main one. Script on the main thread does:

A.then(f).then(g).then(h);
B.then(k);

now the execution orders k,f,g,h, f,k,g,h, f,g,k,h, and f,g,h,k are all possible, and which one happens will depend on thread scheduling. If both A and B were resolved on the main thread, and if we know that B is not resolved by any of f, g, h, then I believe the only possible orders would be k,f,g,h, f,k,g,h, and f,g,h,k. The order f,g,k,h would not be possible. I assume that's what @jyasskin meant about interleaving with another microtask chain....

Again, in practice UAs would be queuing tasks to do this, so in practice you would never get f,g,k,h anyway, though the spec would allow for it. EDIT: Again, as @jyasskin points out if B is resolved off a queued task then the only possible orders here are f,g,h,k and k,f,g,h.

jyasskin commented 8 years ago

Thanks for the examples. In

A.then(f).then(g);
B.then(h);

with a non-main thread resolving A and B in a single algorithm in that order, the implementation has 3 choices:

  1. Post a single task to the main thread that resolves both. Then we get the f,h,g order.
  2. Post two tasks to one of the event loop's task queues. Then we get the f,g,h order, because step 6 of the event loop's processing model runs a checkpoint that completely drains the microtask queue holding f and g before the processing model gets back to step 1 to pull B's resolving task off the main event loop.
  3. Post two microtasks to the event loop's microtask queue. Then we could get either the f,h,g or the f,g,h order depending on whether another microtask was queued between the operations queueing resolve(A) and resolve(B).

I don't think choice (1) is very realistic in general, since it requires the UA to collect up resolutions from multiple algorithm steps before sending any of them to the main thread. I believe (3) requires a new thread-safe operation on the microtask queue that V8 doesn't currently have. (3) is also bad for more aesthetic reasons around code maintenance and predictability. So it'd be nice to guarantee (2).

domenic commented 8 years ago

I haven't had time to read through the examples in detail, but their very existence does seem to imply that my naive ideas were wrong.

I think this may require spec authors to queue a task to resolve the promise? Or I guess we could update "resolve a promise" to do something like "if currently running in parallel, queue a task; otherwise don't"

bzbarsky commented 8 years ago

Post two tasks to one of the event loop's task queues. Then we get the f,g,h order

Ah, good call on the microtask queue draining between tasks. Here's a simple testcase to exercise this situation, all within the main thread:

var r1, r2;
new Promise((r) => r1 = r).then(() => console.log('f')).then(() => console.log('g'));
new Promise((r) => r2 = r).then(() => console.log('h'));
setTimeout(r1); setTimeout(r2);

I agree that in this case we always get f,g,h.

Post two microtasks to the event loop's microtask queue. Then we could get either the f,h,g or the f,g,h order

Yep, for exactly the reason you give.

Given that, I think you're right and just guaranteeing a separate task for each "off-thread" resolution makes a lot of sense in terms of reasoning about this and implementation complexity.

jyasskin commented 8 years ago

"if currently running in parallel, queue a task; otherwise don't" captures the meaning that I think spec authors will expect, but it does introduce a dependency on HTML in a document you want to merge to WebIDL. If that's ok, I'll send a PR.

domenic commented 8 years ago

Oh, that should be fine. Web IDL is already pretty intertwingled with HTML I believe.