Skip to content
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

Clarify realm and task-queuing situation in pipeTo() #902

Merged
merged 3 commits into from Mar 9, 2018

Conversation

3 participants
@domenic
Copy link
Member

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

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link

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.

@bzbarsky
Copy link

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

@domenic

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Collaborator

commented Mar 9, 2018

Looks okay as a stopgap.

@domenic

This comment has been minimized.

Copy link
Member Author

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 Mar 9, 2018

@domenic domenic merged commit 7b8dffe into master Mar 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@domenic domenic deleted the pipeTo-realm-queue-task branch Mar 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.