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

Using WHATWG streams for stream creation #40

Closed
pthatcherg opened this issue Jul 11, 2019 · 17 comments
Closed

Using WHATWG streams for stream creation #40

pthatcherg opened this issue Jul 11, 2019 · 17 comments
Assignees

Comments

@pthatcherg
Copy link
Contributor

The current explainer now uses WHATWG streams for datagrams and for the reading/writing of data for a given uindirectional streams and bidirectional streams. It also uses a stream of streams for receiving streams (so backpressure could be applied if one did not receive a large number of incoming streams fast enough).

But there is one place we don't use streams: creating streams (either unidirectional or bidirectional).

Some options:

A. Don't use streams of streams for creating streams and just apply backpressure to the streams. So while you could have tons of streams created, writes on them all get backpressure. Presumably an app would know not to have too many pending streams (that are receiving back pressure). This would leave the API a big asymmetric between send and receive (making it harder to pipe a stream of messages into a stream-per-message approach), but it works well for both unidirectional and bidirectional streams.

B. Use a stream of streams for creation of unidirectional send streams where you pass in a ReadableStream that gets is used to write to the send stream. This has the oddity that you're using a read stream to write, but one can trivially make a ReadableStream out of a byte array, so this makes it easy to do a "stream-per-message" approach. I don't see how it would work for bidirectional streams, but a "stream-per-message" approach will likely only want unidirectional streams anyway.

C. Don't use streams of streams for creating streams, but apply backpressure on creation using the fact that creating streams is async. Just don't resolve the promise until stream creation is unblocked. This seems like a natural thing to do. Like A, it's not ideal for stream-per-message approaches, but I believe one could implement a B on top of C with some JS.

Right now, I'm inclined toward doing both C and maybe B. C seems to work well for most use cases but B seems to offer more symmetry and also is better for stream-per-message approaches. And even though I think you could do it with JS on top, there might be performance and convenience reasons to make it part of the API.

@Atrius
Copy link

Atrius commented Jul 11, 2019

With respect to B: I would expect an incoming, bidirectional stream would be represented as a pair of (readable stream, writable stream), where the readable part is incoming data and the writable part is for outgoing data. So I guess that we could provide a readable stream of such pairs for incoming bidi streams?

In that case, I would think the natural way to create outgoing bidi streams would be to provide a writable stream of such (readable stream, writable stream) pairs, where the readable stream is a source for bytes to send and the writable stream is a sink for bytes received.

That construct feels truly bizarre to me, as it seems like the user is creating the opposite type of stream for each direction: the readable end is outgoing and the writable end is incoming. But I suppose there are two ways to make sense of it:

  • The pair passed to the transport are what the remote endpoint will get as an incoming bidi stream. So when you hand a (readable, writable) pair to the transport as an outgoing bidi stream, an identical (readable, writable) pair pops out of the remote transport's incoming bidi streams.
  • The types can easily be trivially swapped using identity transform streams. In fact, this seems like one of the main use-cases for identity transforms (turning a readable stream into a writable stream so that you can pass bytes to something that takes a readable stream, or vice versa).

So maybe this is a reasonable construct after all.

One downside is that users would have to handle backpressure on the writable stream of outgoing streams, in order to account for flow-control on the number of open streams. Handling backpressure by simply not returning new outgoing streams until the server is willing to accept them seems like it might be a lot simpler for ordinary usage.

@pthatcherg
Copy link
Contributor Author

For incoming bidi streams: yes, having a stream of bidi streams is what we can do. That's easy.

You idea of passing in a pair of streams is interesting. But it leaves me wondering who would ever want to use that. Streams of streams only seems useful for large number of streams (such as with one-stream-per-message) and who will be doing that with bidirectional streams, especially when the API has to be so "truly bizarre".

I guess we could allow passing in TransformStreams and then you can view a bidi stream as a transform that you write to and then read from and the remote side is the one doing the transforming. And then the direction isn't backwards.

But I'm still not sure who would be doing that with large numbers of such streams vs. creating a small number and calling createBidiStream.

@Atrius
Copy link

Atrius commented Jul 12, 2019

One use case for a large number of bidi streams could be a request/response protocol. If you don't care what order the requests are processed and/or don't want them to block each other, it might make sense to put them on separate bidi streams.

The application may have a stream of requests, each of which maps to a new, outgoing bidi stream. For each request, the app might want to await the full response (eg. await the fin on the bidi stream) and then trigger some callback.

In fact, this is very similar to how HTTP/3 itself works (for example, see the first few paragraphs of https://tools.ietf.org/html/draft-ietf-quic-http-22#section-4). It may be the case that applications that want this type of flow just use HTTP, but I could imagine someone wanting an RPC protocol that's decoupled from HTTP. WebTransport might be a nice abstraction for that.

@jan-ivar
Copy link
Member

jan-ivar commented Jan 25, 2021

I think we should do B here by adding transport.outgoingUnidirectionalStreams, and then come back to bidi streams.

Here's a polyfill that works in Canary (Update: I fixed a bug in an earlier version)

const p = streams.readable.pipeTo(transport.outgoingUnidirectionalStreams);

Easy enough to polyfill perhaps, but has a nice symmetry with the existing incomingUnidirectionalStreams I think:

for await (const stream of transport.incomingUnidirectionalStreams) {
  await stream.readable.pipeThrough(new TextDecoderStream("utf-8")).pipeTo(new WritableConsole("From server: "))
}

@jan-ivar
Copy link
Member

jan-ivar commented Jan 26, 2021

In fact, I'd argue for a "duplex stream" rename here regardless, similar to what #181 proposes for datagrams:

-   const p = streams.readable.pipeTo(transport.outgoingUnidirectionalStreams);
+   const p = streams.readable.pipeTo(transport.unidirectionalStreams.writable);

-   for await (const stream of transport.incomingUnidirectionalStreams) {
+   for await (const stream of transport.unidirectionalStreams.readable) {

This would allow an app to pipe through a server processing step, staying at the stream-per-message abstraction level.

Here's a polyfill for that as well, that also works in Canary:

await streams.readable
  .pipeThrough(transport.unidirectionalStreams)
  .pipeTo(new WritableMessageStreamsConsole("From server: "));

I think this would help answer what a "WebSocket-like API without head-of-line blocking" (#178 (comment)) would look like.

@jan-ivar
Copy link
Member

jan-ivar commented Jan 26, 2021

Lastly, I think incomingBidirectionalStreams deserve the same treatment, as suggested in #178 (comment).

Here's a polyfill for that that works in Canary:

await streams.readable
  .pipeThrough(transport.bidirectionalStreams)
  .pipeTo(new WritableMessageStreamsConsole("Server-initiated bidi: "));

A message (each chunk) in this case is a bidi stream represented by a {readable, writable} pair, where server responses appear on the writable. Thus the final pipeTo is only for server-initiated bidis (which I believe we have no way to test atm?)

@jan-ivar
Copy link
Member

jan-ivar commented Jan 26, 2021

That construct feels truly bizarre to me, as it seems like the user is creating the opposite type of stream for each direction: the readable end is outgoing and the writable end is incoming.

@Atrius It required some thought, but wasn't too bad in practice. Temporaries like encoder and decoder help with bearings:

// Create a "message", which is a bidi stream represented by a {readable, writable}
const id = ++streamNum;
const encoder = new TextEncoderStream();
const decoder = new TextDecoderStream();
const message = {readable: encoder.readable, writable: decoder.writable};
await streamsWriter.write(message);

const writer = encoder.writable.getWriter();
await writer.write(text.value);
await writer.close();

await decoder.readable.pipeTo(new WritableConsole(`Stream #${id} server response: `));

This feels par for the course with streams, where finding ourselves on the other side the abstraction isn't uncommon. E.g.

const streamify = data => new ReadableStream({start: controller => controller.enqueue(data)}); // a write!

@jan-ivar
Copy link
Member

jan-ivar commented Feb 2, 2021

For a larger data test, here's a unidirectional file upload fiddle, showing that a stream-of-streams polyfill that awaits every step ends up serializing uploads:

Contrast with the same file upload fiddle with a polyfill that doesn't:

Which is least surprising, which is most useful? Which to pick?

These polyfills are perhaps simple enough for folks to cook up their own, so this was more to get people started doing basic things with stream-of-streams, and to have outgoing counterparts to our existing incoming stream-of-streams (for both bi and unidirectional transmission).

@jan-ivar
Copy link
Member

jan-ivar commented Feb 3, 2021

Meeting feedback:

@jan-ivar jan-ivar self-assigned this Feb 3, 2021
This was referenced Feb 16, 2021
@jan-ivar
Copy link
Member

Btw, something appears broken in Canary. As you can see above, server received bytes is an order of magnitude below sent.

To see what's going on I've increased the chunk size to 5MB with instrumentation https://jsfiddle.net/jib1/3g7jvL08/27/

Select several Firefox 24.0b9.dmg

Sending Firefox 24.0b9.dmg (46032939 bytes)...
0.000: chunk #1 size 5242880 pos 0
0.014: chunk #2 size 5242880 pos 5242880
2.082: chunk #3 size 5242880 pos 10485760
2.090: chunk #4 size 5242880 pos 15728640
2.096: chunk #5 size 5242880 pos 20971520
2.104: chunk #6 size 5242880 pos 26214400
3.899: chunk #7 size 5242880 pos 31457280
3.907: chunk #8 size 5242880 pos 36700160
3.912: chunk #9 size 4089899 pos 41943040
3.916: done
From server: 10485760
From server: 👋

The progress bar jumps to chunk #2, pauses 2 seconds, then jumps to chunk #6, pauses 2 seconds, then jumps to done.

This plus the total from the server suggests only two chunks were sent, the others dropped. 😕

cc @yutakahirano, @vasilvv, @ricea

@yutakahirano
Copy link
Contributor

Just to check, the client sends 46032939 bytes but the server receives 10485760 bytes, right?

@jan-ivar
Copy link
Member

jan-ivar commented Mar 1, 2021

@yutakahirano If by "client" you mean the JS, then yes. Given the timing (left column), I suspect Chrome only sent 10485760 (2 x 5242880) of the 46032939 bytes it was supposed to send.

I suppose it's possible there's a bug in the server instead, but then I think the timing would have been different. It looks like Chrome never even tried to send the other chunks.

@yutakahirano
Copy link
Contributor

@yutakahirano
Copy link
Contributor

Thank you, I filed https://crbug.com/1225189 for the Chrome issue.

Closing this issue as we're using WHATWG streams.

@jan-ivar
Copy link
Member

jan-ivar commented Jul 6, 2021

Closing this issue as we're using WHATWG streams.

Re-opening since this issue is not about using streams but about using streams for stream creation.

We may end up closing it for lack of interest, but we should make sure the right reasons are recorded.

@jan-ivar jan-ivar reopened this Jul 6, 2021
@yutakahirano yutakahirano added this to the Minimum viable ship milestone Jul 13, 2021
@yutakahirano
Copy link
Contributor

Thanks, I was wrong.

Sounds like this needs to be resolved before the first release, so setting the milestone.

If we choose B, we'll never be able to attach methods or attributes to SendStream. I'm removing the interface at #306, but this is different, because #306 will allow us to bring back the interface in the future. This is not.

@yutakahirano yutakahirano added the Discuss at next meeting Flags an issue to be discussed at the next WG working label Jul 16, 2021
@jan-ivar
Copy link
Member

Meeting:

  • No interest

@yutakahirano yutakahirano removed the Discuss at next meeting Flags an issue to be discussed at the next WG working label Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants