Clarify realm and task-queuing situation in pipeTo() - #902
Conversation
|
I'm concerned that this change is user-visible, and that it will be problematic to implement in Chrome. Take this example: async function ObserveTaskQueue() {
let pipeToComplete = false;
const rs = new ReadableStream({
async start(controller) {
controller.close();
for (let i = 0; i < 1000; ++i) {
if (pipeToComplete) {
console.log('done');
return;
}
await Promise.resolve();
}
console.log('fail');
}
});
const ws = new WritableStream();
await rs.pipeTo(ws);
pipeToComplete = true;
}With Chrome's current implementation, this function prints "done". I believe that Chrome's implementation is compliant with the standard in this respect. I think an implementation is also permitted to print "fail". However, if I understand correctly, no task from the DOM manipulation task source can run while microtasks are executing. If this change to the standard is made, all implementations must print "fail". |
No task at all can run while microtasks are executing. I don't see how the code pasted above can ever print "done" if there is a task being queued from the close() call that needs to run to resolve the promise returned by pipeTo. |
There was a problem hiding this comment.
This seems fine in general, but why DOM manipulation task source? Seems like some networking-related task source would be more appropriate...
In general, we need a sane story around task sources.
|
After thinking about this more, I realized the current "in parallel" block is a bit bonkers. You could do all that work in parallel unobservably for UA-provided streams, but for developer-created streams, reading and writing will call back into the JS underlying source/sink, and we haven't noted how or whether to queue a task in those cases. So, that's pretty wrong right now. As such, I'll scope this PR down to just object creation, and add a red "XXX" box around "in parallel" linking to a new issue for straightening that out as a second step. |
|
Looks okay as a stopgap. |
|
Gotta remove the pointer to the TypeError since we fixed that while this PR was out. |
af5705c to
47573e8
Compare
Closes #845.
@bzbarsky, does this address your concerns?
(I'm always a bit skeptical of the DOM manipulation task source, but it appears to be the default...)
Preview | Diff