Large change refactoring unidirectional streams. #70

shampson · 2018-10-04T23:58:02Z

This is a large change, as discussed in issue 69 & prototyped in this doc.

To view refer to this page:
https://rawgit.com/w3c/webrtc-quic/unidirectional-streams-and-state-update/index.html

I think this is a good starting point. Feel free to make commits to this. My hope is that we can get this change in quickly, then iterate on it with further details/issues.

Issues that can be closed after this PR goes through:

50 - restricting createStream to state = connected
54 - new/open/opening for a stream
55 - remote stream triggering with data sent
61 - Reset/STOP_SENDING
64 - readable/writable access
65 - Taking out RTCQuicStreamState

pthatcherg

Overall, I like it :).

pthatcherg · 2018-10-05T16:35:39Z

index.html

                    attribute EventHandler             onstatechange;
                    attribute EventHandler             onerror;
-                    attribute EventHandler             onquicstream;
+                    attribute EventHandler             onnewreceivestreamwithdata;
+                    attribute EventHandler             onnewbidirectionalstreamwithdata;


At first I thought "onnewXwithdata" was a weird, but then I realized you're trying to emphasize that this only fires once there is data, and that it's a stream that may get more data.

I can't think of a better naming scheme for it (it seems like the best name anyone has come up with so far). So I'm in favor of it.

Yeah, I was considering adding a comment about ideas for renaming this. We couldn't think of a better one, but I'm definitely open to other ideas.

Why are the events delayed until there is data? With data channels, the ondatachannel event isn't delayed until there are messages. Couldn't this disparity cause problems in implementing the RTCDataChannel API on top of this API?

Presumably if you were implementing RTCDataChannel on top, you'd have some amount of header in the front of the stream (with something like the data channel ID). That header would be the first data.

QUIC doesn't create a receive stream right away because it requires sending a STREAM frame from the send side. Presumably you could get around this by:

Sending a STREAM frame with 0 data, but currently gquic doesn't allow without including the FIN bit.

Bidirectional streams support creating a bidirectional stream on the receive side with the receipt of a MAX_STREAM_DATA frame (coming from the receive stream on the send side). I don't think this is supported by gquic, but it might take some digging to know for sure.

The long name also seems awkward to me. I'm wondering why that information needs to be provided as part of the event's name. Is it important that the event only fires when there is data? In addition, is it important to emphasize that it's new?

How about onreceivestream and onbidirectionalstream?

@lgrahl The event will only emit newly created streams, so “new” seems redundant. onreceivestream and onbirectionalstream seems more compact to me.

Also, the spirit of converging with WHATWG streams, should we support cancel (reason)?

Yes, for converging with the Streams API, adding cancel to the reader and abort to the writer would be sensible. IIRC, .abortReading and .abortWriting would just need to be renamed for that purpose. The Streams API also allows to return a Promise. This could for example be used to resolve once the closing procedure has been completed.

In my demo code (which is based on bidirectional streams), .abort and .cancel trigger an error on both readable/writable controllers which results in exceptions on pending async read/write operations. For unidirectional streams, only the associated stream controller would have to be error-ed.

I think we should leave them "abort" until we make a decision about WHATWG streams.

This is similar to what I think of in general for this PR: let's merge it to get the main things in place and then address little things like the names with followup PRs.

index.html

pthatcherg · 2018-10-05T16:45:15Z

index.html

+        interface mixin RTCQuicWritableStream {
+            readonly attribute boolean writable;
+            readonly attribute unsigned long writeBufferedAmount;
+            readonly attribute Promise&lt;RTCQuicStreamAbortInfo&gt; writingAborted;


A Promise attribute? I've never seen that before. What are the pros/cons vs. an event handler? Is it so that you can get the promise after it fired (unlike an event which you may miss)? Or is it to emphasize that it will only fire once?

I think it's nice for the reasons you mentioned:

You can get it after it has fired. If the promise has already been settled, it will still fulfill/reject appropriately.

Making it an attribute means that there should only be one promise that will only settled once.

We should only get a callback for a RST_STREAM/STOP_SENDING frame once

Here's a blog post where I learned about promises and explains a tiny amount about when they're more appropriate than events (spoiler alert it's pretty much what you said).
https://developers.google.com/web/fundamentals/primers/promises#events_arent_always_the_best_way

With the RTCDataChannel API, we have the onclose event that is only used once.

I don't disagree that an event could work here, but a promise seems more appropriate. The WHATWG streams do the same thing:
https://streams.spec.whatwg.org/#default-reader-closed
https://streams.spec.whatwg.org/#default-writer-closed

I agree that Promises are a useful alternative to fire once events.

How about I keep this as a Promise in this PR, and we can move this discussion for after it merges if anyone still feels strongly for using an event here?

index.html

shampson · 2018-10-05T22:35:59Z

Thanks for giving this a look Peter!

pthatcherg · 2018-10-06T00:09:21Z

LGTM

index.html

…WritableStreams]] slots, and added steps for removing stream in write() method.

index.html

lgrahl · 2018-10-10T20:16:44Z

(Having read @aboba's mailing list posting)

Among other things, Seth's proposed revision to the WebRTC-QUIC specification (see PR #70 ) provides a number of elements that would be required for support of WHATWG streams, such as providing support for readable and writable streams.

Peter Thatcher is now looking into what other changes might be necessary to align the two specifications.

Therefore those participants in the CG interested in WHATWG streams may want to look at the PR and provide comments to Seth and Peter.

At the moment, I'm not sure what kind of feedback you would like to get because, to be honest, due to the lack of reactions I felt largely ignored by this group and mostly stopped my efforts... apart from occasional pinging. The demo and the spec draft of #2 is still there. While the API has been changed since December 2017, it's not that far off and should provide enough insight into how QUIC can be married with streams. I'm there if you have questions and the WHATWG folks that helped me experiment with this can be pinged in freenode's #whatwg.

shampson · 2018-10-10T23:59:30Z

Hi @lgrahl. I apologize for not looking more deeply into WHATWG streams your demo & spec draft in your initial issue filing. It's being readdressed if WHATWG streams are the right solution here (especially as the spec moves closer to readable/writable streams) and I'm going to look into it more deeply when I get a chance. I think the best path forward here is to merge this PR first as it is something that has more consensus, and address WHATWG streams next.

index.html

pthatcherg · 2018-10-11T01:02:04Z

index.html

                    attribute EventHandler             onstatechange;
                    attribute EventHandler             onerror;
-                    attribute EventHandler             onquicstream;
+                    attribute EventHandler             onnewreceivestreamwithdata;
+                    attribute EventHandler             onnewbidirectionalstreamwithdata;


I think we should leave them "abort" until we make a decision about WHATWG streams.

This is similar to what I think of in general for this PR: let's merge it to get the main things in place and then address little things like the names with followup PRs.

index.html

…m events and added back waitForReadable() to the interface.

pthatcherg · 2018-10-18T15:53:47Z

Can we merge this? The fact that it hasn't been merged has caused some confusion in pre-TPAC discussions.

shampson · 2018-10-18T15:57:42Z

Sounds good to me. @aboba @robin-raymond any objections?

shampson · 2018-10-21T09:33:00Z

I closed issues 54, 61, 64, and 65.

I left 50, 55 in case these still want to be discussed further.

Large change refactoring to unidirectional streams.

0d9c91b

shampson requested review from aboba, pthatcherg and robin-raymond October 5, 2018 00:14

pthatcherg reviewed Oct 5, 2018

View reviewed changes

Addressed comments for large API unidirectional stream change.

6ed5edc

alvestrand reviewed Oct 9, 2018

View reviewed changes

index.html Show resolved Hide resolved

Created separate [[QuicTransportReadableStreams]] and [[QuicTransport…

6056769

…WritableStreams]] slots, and added steps for removing stream in write() method.

aboba added the List Discussion Needed label Oct 10, 2018

lgrahl reviewed Oct 10, 2018

View reviewed changes

index.html Show resolved Hide resolved

pthatcherg reviewed Oct 11, 2018

View reviewed changes

Added logic for failed RTCQuicTransportState, took out new with strea…

1acb9c1

…m events and added back waitForReadable() to the interface.

aboba merged commit 544f725 into master Oct 20, 2018

This was referenced Nov 6, 2018

Restricting createStream() to state = connected #50

Closed

createStream() triggering remote stream creation #55

Closed

shampson deleted the unidirectional-streams-and-state-update branch November 13, 2018 22:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Large change refactoring unidirectional streams. #70

Large change refactoring unidirectional streams. #70

shampson commented Oct 4, 2018

pthatcherg left a comment

pthatcherg Oct 5, 2018

shampson Oct 5, 2018

aboba Oct 6, 2018

pthatcherg Oct 6, 2018

shampson Oct 8, 2018

lgrahl Oct 10, 2018

aboba Oct 10, 2018

lgrahl Oct 10, 2018 •

edited

pthatcherg Oct 11, 2018

pthatcherg Oct 5, 2018

shampson Oct 5, 2018

aboba Oct 6, 2018 •

edited

shampson Oct 10, 2018

lgrahl Oct 10, 2018 •

edited

shampson Oct 11, 2018

aboba Oct 19, 2018

shampson commented Oct 5, 2018

pthatcherg commented Oct 6, 2018

lgrahl commented Oct 10, 2018

shampson commented Oct 10, 2018

pthatcherg Oct 11, 2018

pthatcherg commented Oct 18, 2018

shampson commented Oct 18, 2018

shampson commented Oct 21, 2018

Large change refactoring unidirectional streams. #70

Large change refactoring unidirectional streams. #70

Conversation

shampson commented Oct 4, 2018

pthatcherg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgrahl Oct 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aboba Oct 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgrahl Oct 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shampson commented Oct 5, 2018

pthatcherg commented Oct 6, 2018

lgrahl commented Oct 10, 2018

shampson commented Oct 10, 2018

Choose a reason for hiding this comment

pthatcherg commented Oct 18, 2018

shampson commented Oct 18, 2018

shampson commented Oct 21, 2018

lgrahl Oct 10, 2018 •

edited

aboba Oct 6, 2018 •

edited

lgrahl Oct 10, 2018 •

edited