Skip to content

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

Merged
merged 3 commits into from
Mar 9, 2018

Conversation

domenic
Copy link
Member

@domenic domenic commented Mar 8, 2018

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

@ricea
Copy link
Collaborator

ricea commented Mar 8, 2018

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".

@bzbarsky
Copy link

bzbarsky commented Mar 8, 2018

However, if I understand correctly, no task from the DOM manipulation task source can run while microtasks are executing.

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.

Copy link

@bzbarsky bzbarsky left a 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.

@domenic
Copy link
Member Author

domenic commented Mar 9, 2018

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.

@ricea
Copy link
Collaborator

ricea commented Mar 9, 2018

Looks okay as a stopgap.

@domenic
Copy link
Member Author

domenic commented Mar 9, 2018

Gotta remove the pointer to the TypeError since we fixed that while this PR was out.

@domenic domenic force-pushed the pipeTo-realm-queue-task branch from af5705c to 47573e8 Compare March 9, 2018 10:32
@domenic domenic merged commit 7b8dffe into master Mar 9, 2018
@domenic domenic deleted the pipeTo-realm-queue-task branch March 9, 2018 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants