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

Writable Streams #163

Closed
2 of 5 tasks
domenic opened this issue Mar 31, 2017 · 10 comments
Closed
2 of 5 tasks

Writable Streams #163

domenic opened this issue Mar 31, 2017 · 10 comments
Assignees

Comments

@domenic
Copy link
Member

domenic commented Mar 31, 2017

Hello TAG!

I'm requesting a TAG review of:

Further details (optional):

  • Relevant time constraints or deadlines: We are hoping to launch in Blink in M59, for which the branch point is in a couple weeks
  • I have read and filled out the Self-Review Questionnare on Security and Privacy: not applicable to this specification, as it specifies base primitives; future specifications which build on writable streams will likely make use of this, however.
  • I have reviewed the TAG's API Design Principles

You should also know that...

As this specification is for a base primitive applicable across all platforms, it does not use Web IDL, but instead is written in the style of the JavaScript specification.

We'd prefer the TAG provide feedback as (please select one):

  • open issues in our Github repo for each point of feedback
  • open a single issue in our Github repo for the entire review
  • leave review feedback as a comment in this issue and @-notify [github usernames]
@dbaron
Copy link
Member

dbaron commented Apr 28, 2017

So a question that I don't feel knowledgeable enough to file an issue on yet:

There are a few places where the spec describes how backpressure on a stream works, but I'm having trouble figuring out where those concepts are exposed in the API. But it might be that I'm not used to reading this style of specification. Where is the API that explains how backpressure works?

(If there's an answer, it might be clearer if the introductory spec sections, e.g., the "Model" section (2) and the "Using Writable Streams" section (4.1) had more links into the more technical parts of the specification?)

@dbaron
Copy link
Member

dbaron commented Apr 28, 2017

I guess the desiredSize getters on ReadableStreamDefaultController and WritableStreamDefaultController answer that. Though I'm a little worried about this text:

At this point in time the division of work between the stream and its controller may seems somewhat arbitrary, but centralizing much of the logic in the controller is a useful structure for the future.

Perhaps this should at least explain the logic of the separation a little bit further so that future spec editors can extend the platform in reasonable ways without messing up the design?

But it's interesting that backpressure itself appears to be signaled just as a boolean. @travisleithead was wondering how well the backpressure approach here extends to things like media streams, where backpressure might be handled by doing things like reducing resolution rather than by writing the same data more slowly. Is more than just a boolean backpressure signal needed to handle that?

@triblondon
Copy link

triblondon commented Apr 28, 2017

Thanks for requesting a review! Streams are a very exciting addition to the platform. Thanks for doing this work.

I'm assuming that reader.close() releases the lock and closes the writer, and that subsequent calls to getWriter will create a new writer for the same stream, but I was a bit confused in the spec by the closeRequest slot which gave me the impression that calling close on a writer closes the stream itself.

Like David I'm not familiar enough with this to file an issue and it may not need anything from us.

@travisleithead
Copy link
Contributor

Looked it over, aside from learning a lot, I don't have any additional comments. Looks good.

@ricea
Copy link

ricea commented Apr 28, 2017

@dbaron Backpressure is largely implicit, based on the principle that stuff should "just work". An underlying sink signals backpressure by not resolving the promise it returned from write() until the data has been written. I think this is the natural way for a write() implementation to behave anyway. An underlying source experiences backpressure as pull() not being called. Again, I feel it's natural that a source would not produce data until it is asked.

One point that perhaps could be stated more clearly is that for optimum throughput the highWaterMark that is set when creating a ReadableStream or WritableStream should reflect the burstiness and latency of the source or sink.

My hope is that pipeTo() and pipeThrough() will be the main interfaces used by the consumers of streams. These respond correctly to backpressure without any specific action needing to be taken.

I think the bitrate of a media stream needs to be set based on the throughput. Streams don't attempt to measure the throughput, leaving this to applications that need it.

Regarding

At this point in time the division of work between the stream and its controller may seems somewhat arbitrary, but centralizing much of the logic in the controller is a useful structure for the future.

I believe this is because we intend to add a 'byte' type of WritableStream in the future. I'm not sure why it doesn't just say that. Also, myself, @domenic and @tyoshino have considered the division of labour very carefully, so I think this statement could be worded better.

@ricea
Copy link

ricea commented Apr 28, 2017

I'm assuming that reader.close() releases the lock and closes the writer, and that subsequent calls to getWriter will create a new writer for the same stream, but I was a bit confused in the spec by the closeRequest slot which gave me the impression that calling close on a writer closes the stream itself.

It's actually releaseLock() that releases the lock and detaches the writer. close() requests the stream to close. It's possible to call releaseLock() while the close is in progress, and the writer will be detached but the close will continue.

I didn't realise that the semantics were confusing. Thank you for the feedback.

@dbaron
Copy link
Member

dbaron commented Apr 28, 2017

@ricea wrote in #163 (comment) :

Backpressure is largely implicit, based on the principle that stuff should "just work".

Ah, so I guess the internal [[backpressure]] slot on WritableStream is just stream/controller communication. Still, the TAG has generally supported exposing the underlying concepts that explain how things work so that people have the ability to fix them when needed -- at least as long as that doesn't add lots of complexity. It's not obvious to me at first glance how that idea fits in here.

I'm still curious if you've thought through whether backpressure works well for media streams.

@ricea
Copy link

ricea commented Apr 28, 2017

I'm still curious if you've thought through whether backpressure works well for media streams.

I think so, by analogy with TCP/IP. The network stack does careful accounting of window sizes, but all that is exposed to user space is "can write now" or "can't write now".

I am expecting to start initial work on integration between media and streams in the next few months. Backpressure is one area I am very confident in.

I know @domenic considered many more use cases in the design of backpressure for streams, so he will probably have more to say here.

@domenic
Copy link
Member Author

domenic commented Apr 28, 2017

About backpressure vs. [[backpressure]], I wouldn't say [[backpressure]] is an important underlying concept. The user-facing API is much more than a boolean, it's the desiredSize number which tells you "how close" you are getting to backpressure kicking in. The [[backpressure]] internal slot is more of a bookkeeping device to allow caching the computation of "desiredSize >= 0".

@torgo torgo modified the milestones: tag-f2f-london-2017-07-25, tag-f2f-tokyo-2017-04-27 Jul 25, 2017
@triblondon
Copy link

It seems like the feedback we gave earlier is taken on board here, the design is stable and we don't have any further feedback to add at this point.

Thank you for flying TAG, come again soon.

@triblondon triblondon removed their assignment Apr 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants