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

"await writer.write(bytes);" in example seems wrong #874

Closed
domenic opened this issue Jan 31, 2018 · 2 comments
Closed

"await writer.write(bytes);" in example seems wrong #874

domenic opened this issue Jan 31, 2018 · 2 comments
Assignees

Comments

@domenic
Copy link
Member

domenic commented Jan 31, 2018

https://streams.spec.whatwg.org/#example-manual-write-with-backpressure

We're already awaiting writer.ready. Awaiting writer.write() seems counterproductive, as it means that even if the writable stream has a large high water mark, we won't be writing multiple chunks to fill the queue, instead waiting until every individual chunk has been processed sequentially.

domenic added a commit that referenced this issue Jan 31, 2018
It should not be awaiting on writer.write() in addition to writer.ready. Closes #874.
@ricea
Copy link
Collaborator

ricea commented Feb 8, 2018

Sorry, I really should have responded to this sooner. Because the write() always uses all of desiredSize, desiredSize will go to 0 and stay at 0 until the write finishes. So writer.ready cannot fulfill before writer.write().

Additionally, if we don't wait on writer.write(), there will be an uncaught rejection, which is not ideal. My preferred way to write the loop is:

  await writer.ready; // ensure desiredSize > 0
  while (true) {
    const bytes = new Uint8Array(writer.desiredSize);
    window.crypto.getRandomValues(bytes);

    await writer.write(bytes);
  }

Except now it's not a good example of writer.ready. Okay, how about

  while (true) {
    await writer.ready;
    const bytes = new Uint8Array(1024);
    window.crypto.getRandomValues(bytes);

    writer.write(bytes).catch(() => {});
  }

? Oh, but we're also trying to demonstrate desiredSize. Never mind, let's just use write().catch() instead of await write().

By the way, why is it window.crypto.getRandomValues()? If it was just crypto.getRandomValues() it would work in a worker without modification.

@domenic
Copy link
Member Author

domenic commented Feb 8, 2018

Oh, right, in this particular example it is the same, good point. Hmm. I'd still like to not encourage waiting on writer.write() as except in cases where you're explicitly using desiredSize, it's not a good idea.

This may benefit from two examples, one to demonstrate desiredSize and one to demonstrate how if you don't use desiredSize, ready and write() will get out of sync.

And yeah, we should get rid of the window.

@domenic domenic self-assigned this Feb 8, 2018
domenic added a commit that referenced this issue Feb 22, 2018
It should not be awaiting on writer.write() in addition to writer.ready. Closes #874.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants