-
Notifications
You must be signed in to change notification settings - Fork 167
Clarify realm and task-queuing situation in pipeTo() #902
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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