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

Specify WritableStream.[[Transfer]] #629

Closed
wants to merge 2 commits into from
Closed

Conversation

isonmad
Copy link
Contributor

@isonmad isonmad commented Dec 13, 2016

The only trouble is that, currently, writer.write() is specified to indirectly invoke strategy.size() synchronously, which is bad if strategy.size runs in a different JS context entirely.

Cf. #623.

@domenic
Copy link
Member

domenic commented Dec 16, 2016

The only trouble is that, currently, writer.write() is specified to indirectly invoke strategy.size() synchronously, which is bad if strategy.size runs in a different JS context entirely.

That is an example of why I think speccing these things as piping between distinct streams will work better, instead of tying together the objects behind the scenes :-/.

@isonmad
Copy link
Contributor Author

isonmad commented Dec 21, 2016

I definitely found it a point in its favor, but wrapping it in a new underlying sink, etc provides its own complications that seem somewhat unappealing when it comes to backpressure signalling.

  • For one, it creates an additional queue in the new realm.
  • Every time you transfer the already-transferred stream again, you create another stream+queue, chaining to another intermediate stream+queue in every previous realm it's ever been in before chunks reach the original.
  • [[strategySize]]() only gets to run in the original JS context, so it can't accurately measure any of these other queues which would have to fallback to plain old CountQueuingStrategy instead.
  • There's an observable difference between piping through two queues each with a HWM of 1, and one queue with a HWM of 1, so it can't exactly be ignored with an invisible optimization.
  • Always setting HWM = 0 wouldn't be an option as that would mean that writer.ready is just never fulfilled, even when there is actually no backpressure being exerted by the original underlyingSink.

And there's the question of when, exactly, individual write() promises fulfill for the wrapper writablestream.

  • Fulfill as soon as the chunk is enqueued into the original realm's WritableStreamController.[[queue]]? This results in no backpressure signalling whatsoever and constantly sending more and more chunks.
  • Fulfill when the original underlyingSink.write() / writer.write() fulfills? The opposite problem: Then you basically ignore the highWaterMark of the original writablestream, because the wrapper writablestream will never send a new chunk until the previous one it sent is processed.
  • Fulfill when the original writer.ready fulfills? What happens when the original WritableStream has HWM = 0, and so it never fulfills at all?

Is it essential that strategySize be invoked synchronously at write time, or can that be changed?

@ricea ricea added the do not merge yet Pull request must not be merged per rationale in comment label Sep 6, 2018
Base automatically changed from master to main January 15, 2021 07:25
@ricea
Copy link
Collaborator

ricea commented Mar 3, 2021

Closing as we have implemented transferable streams in a different way.

@ricea ricea closed this Mar 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge yet Pull request must not be merged per rationale in comment
Development

Successfully merging this pull request may close these issues.

3 participants