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

explicity operate on underlying source and sink instead of public methods #321

Closed
wanderview opened this issue Apr 8, 2015 · 16 comments
Closed

Comments

@wanderview
Copy link
Member

Certain optimizations like piping off-main-thread are much more difficult in a world where the public methods of a ReadableStream and WritableStream can be monkey patched with js. To enable UAs to implement these optimizations the spec should be written such that it operates directly on the underlying sources and sinks.

For example, ReadableStream.pipeTo(ws) should not call ws.write(), but should write data to ws's underlying sink.

This makes the API a bit more "DOMy" and a bit less "JSy", but makes it much easier to create efficient implementations in multi-threaded browsers.

@wanderview
Copy link
Member Author

In IRC @domenic also suggested that we could add a WritableStream cast function if duck-typed WritableStreams are found to be needed.

@yutakahirano
Copy link
Member

We planned to introduce a generic pipeTo function that interacts with public reader / writer APIs and specific ones implemented natively. What's wrong with the idea?

@yutakahirano
Copy link
Member

We planned to introduce a generic pipeTo function that interacts with public reader / writer APIs and specific ones implemented natively. What's wrong with the idea?

I mean, the existing (planned) pipeTo can handle arbitrary streams correctly and can handle specific ones (that possibly includes ReadableStream and WritableStream) efficiently. Your proposal seems to drop the former property and I don't see what we get instead.

@domenic
Copy link
Member

domenic commented Apr 8, 2015

So, the current reference implementation is generic on both this (using this.getReader()) and on its argument (using dest.write(), or in the future probably dest.getWriter()). And it's also generic on the objects returned from those.

I think the genericness on this is not too important. It's main benefit is that it would allow duck-type readable streams that don't inherit from ReadableStream to do MyReadableStream.prototype.pipeTo = ReadableStream.prototype.pipeTo and get a pipe algorithm automatically. (It would also allow the edge case of subclasses which use "return override" but that is pretty crazy... can explain more if interested.) So let's put that aside for now and say that we abandon it, just like when formalizing .tee() I abandoned it.

The genericness on dest I was quite attached to because I thought it would be a good way to allow polymorphic behavior in a wide ecosystem of writable streams, which might not be per se writable streams. The main problem with this approach, as discussed in #276, is that it becomes harder to ensure we go down a "fast path" (i.e., off-main-thread piping) for UA-created writable streams, since e.g. someone could overwrite ws.getWriter or WritableStream.prototype.getWriter or WritableStreamWriter.prototype.write. You can have workarounds, e.g. by installing guards on the prototype properties and checking the value of ws.getWriter. But that is kind of crazy, especially the guards.

One argument against genericness on dest is that the polymorphism in writable stream implementations should not be accomplished by creating new writable stream classes, but instead passing new underlying sinks to WritableStream. Notably, instances created this way are guaranteed to obey all invariants, since we program those invariants into the WritableStream method and algorithm definitions. I was still resistant, however, as it felt un-JavaScriptey... JS programs would just be generic.

I then had an epiphany, which @wanderview alludes to above. All along I have been trying to make the analogy that promise is to promise executor function as readable/writable stream is to underlying source/sink. What I was missing was to further extend this analogy to the way in which promises handle genericness. Which is, they always "cast" incoming duck-promises (called thenables) into real promises, using PromiseResolve (exposed as Promise.resolve). For example, Promise.all takes an iterable, and casts each element before using it. onFulfilled allows you to return any object, but casts it before using it. And any web platform API that accepts promises, or any future ECMAScript API, will also cast the incoming object to a real promise before using it.

If we want genericness over writable streams, we should do the same thing. As a bonus we would get a guarantee that the writable stream obeys the correct invariants (currently there is the possibility of bizarre failures that we would in theory want to detect and add errors for, like ws.state === "arghblargh"). This is just like promise-accepting APIs which Promise.resolve incoming arguments do, to avoid e.g. jQuery promises' broken invariants.

And with this in mind, there's no reason to define WritableStream.cast or similar until someone actually asks for it. We could even let it be implemented as an npm package or whatever first and only if it gets lots of usage roll it in.

So, I have kind of changed my opinion on all this a lot.

@wanderview
Copy link
Member Author

What do you mean by "specific ones implemented natively"? I think we want to be able to do this piping off-main-thread:

rs.pipeTo(ws);

But if pipeTo() operates on public methods, then this cannot occur with:

ws.write = function() { ... };
rs.pipeTo(ws);

Code working with streams should not have to do an "if native, call different functions" switch.

Trying to detect a function override complicates optimizations and also comes at a perf cost due to constant checking on every call.

@domenic
Copy link
Member

domenic commented Apr 8, 2015

Oh. I forgot an important caveat. It's not yet clear whether the (Readable|Writable)ByteStream versions will be a subclass or a separate duck-compatible class. Subclass means they could pass the brand check; duck-compatible means we'd either have to make them aware of each other (e.g. pipeTo doing a brand check for either WritableByteStream or WritableStream and allowing both), or we'd have to do structural matching, or we'd have to cast WritableByteStreams to WritableStreams :-/. So, that's a bit tricky.

@wanderview
Copy link
Member Author

Unless we expect a proliferation of stream types, this does not seem a terrible problem. I think you would want ReadableByteStreams to understand WritableStream+WritableByteStream, but ReadableStream might only understand WritableStream underlying sink.

It seems this can be done by just looking for the right sink duck types, no?

@domenic
Copy link
Member

domenic commented Apr 8, 2015

I think it would suck if you couldn't pipe a ReadableStream to a WritableByteStream.

@wanderview
Copy link
Member Author

I think it would suck if you couldn't pipe a ReadableStream to a WritableByteStream.

You absolutely could, but it would use whatever path WritableStream.write() method uses on that object. (The underlying sinks are un-specc'd, but it seems there must be a way to do a simple write.)

@wanderview
Copy link
Member Author

I guess I would recommend creating "Write to sink" algorithms that are associated with the stream. Then ReadableStream.pipeTo() could say "using destination streams Write to sink algorithm...".

A ReadableByteStream.pipeTo(), however, might say "using destinations Optimized Byte Write To Sink algorithm, if available, or falling back to Write To Sink algorithm, ..."

@domenic
Copy link
Member

domenic commented Apr 8, 2015

OK, but then we need some way for ReadableStream.prototype.pipeTo to recognize that if dest is a WritableByteStream, it should not immediately reject (like it would for a HTMLElement or whatever). The solutions I can think of are:

  • Duck type on public API (problematic as discussed above at length)
  • Make WritableByteStream a subclass of WritableStream. This might be tricky because it means their internal models need to be similar enough for e.g. WritableStream.prototype.write.call(wbs, chunk) to work. (I think.)
  • Hard code knowledge of WritableByteStream into ReadableStream.
  • Duck type on private API?? I guess this is workable in ES spec terms (check for an internal slot that both share). Although it's not terribly different than just hard coding the knowledge.

@wanderview
Copy link
Member Author

Right... it needs to check the type which I guess you called a brand check.

I'm used to getting this for free through the use of webidl.

interface ReadableStream
{
  Promise<undefined> pipeTo(WritableStream or WritableByteStream);
}

@domenic
Copy link
Member

domenic commented Apr 8, 2015

I'm used to getting this for free through the use of webidl.

Sure, but that's pretty crappy and non-extensible layering, making ReadableStream explicitly aware of WritableByteStream. That's my "Hard code knowledge of WritableByteStream into ReadableStream" option.


BTW we're not going to exactly interface with the underlying sink because that means you could do writes out of order. More like, we're going to use abstract operations that take WritableStreams or ReadableStreams as parameters, and use those. Effectively it goes directly to the underlying sink, but also with all the guarantees you would get through a (non-monkey-patched) use of the public API.

See e.g. EnqueueInReadableStream for an example. I anticipate having a WriteToWritableStream with similar properties.

@domenic
Copy link
Member

domenic commented Apr 8, 2015

I guess I would recommend creating "Write to sink" algorithms that are associated with the stream.

That sounds like underlyingSink.write ;). Which is exactly what WriteToWritableStream would call, after ensuring that previous writes had finished, etc.

@wanderview
Copy link
Member Author

Ok. I guess I don't really care how its spec'd as long as it lets the implementation operate on the underlying sink without going through a potentially-monkey-patched public interface.

@domenic
Copy link
Member

domenic commented Nov 2, 2016

Oh hey, this is finished now that pipeTo is specced to not operate publicly.

@domenic domenic closed this as completed Nov 2, 2016
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

3 participants