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

API change - solving state & unidirectional streams #69

Closed
shampson opened this issue Sep 28, 2018 · 6 comments
Closed

API change - solving state & unidirectional streams #69

shampson opened this issue Sep 28, 2018 · 6 comments
Labels
PR exists A pull request has been submitted

Comments

@shampson
Copy link
Collaborator

Okay bear with me, this is long...

The spec has lot of issues dependent on how we want to treat the state of a QUIC stream:

  • 50 - restricting createStream to state = connected
  • 54 - new/open/opening for a stream
  • 61 - Reset/STOP_SENDING
  • 64 - readable/writable access
  • 65 - Taking out RTCQuicStreamState

We also want a clear API for unidirectional streams, and currently we're going to have to go through things with a fine toothed comb to make sure there aren't any bugs in the spec (certain methods/events are only applicable to specific directions).

Steve and I white boarded what a revision of the API could look like that solves the unidrectional and stream state issues:

API:

Send stream:

RTCQuicSendStream {
  readonly attribute boolean writable;
  readonly attribute unsigned long writeBufferedAmount;
  Promise<void> waitForWriteBufferedAmountBelow(unsigned long threshold);
  void write(Uint8Array data);
  // A hard shutdown. Sends a RST_STREAM frame. [[Writable]] goes to false,
  // write buffer is cleared. Could also be called 'reset()'  
  void close(); 
  // A "soft" shutdown. Sends the FIN bit, to allow the receive side to read all 
  // data up to the FIN bit.
  void finish();
  // Promise that is resolved when STOP_SENDING frame is received. Another option
  // here is to have an 'onclosed/onstopsending' event.
  Promise<void> getClosed();
  // These could expose stream_id, stream_priority, etc.
  RTCQuicStreamParameters getParameters();
}

Receive stream:

RTCQuicReceiveStream {
  readonly attribute boolean readable;
  readonly attribute unsigned long readableAmount;
  Promise<void> waitForReadable(unsigned long amount);
  // By returning a negative this handles the receive side case for finish() being called
  // on the send side (after all data is read).
  long readInto(Uint8Array data);
  // A hard shutdown. Sends a STOP_SENDING frame, clears the read buffer, 
  // and [[Readable]] goes to false. Could also be called 'stopSending()'
  void close();
  // Promise that is resolved when RST_STREAM frame is received. Another option
  // here is to have an 'onclosed/onreset' event.
  Promise<void> getClosed();
  // These could expose stream_id, stream_priority, etc.
  RTCQuicStreamParameters getParameters();
}

Bidirectional stream:

RTCQuicBiDirectionalStream {
  RTCQuicReceiveStream getReceiveStream();
  RTCQuicSendStream getSendStream();
  //  Calls close() on both streams.
  void close();
}

createStream/onstream would need to be updated appropriately, but that doesn't need a code example.

I spent more time looking at the quic-transport spec, and I don't see any problems supporting this.

But what about state?

At the minimum the opening/closing states should not be necessary. All we are concerned with from an API perspective is when we can read/write. If we limit the stream creation to after the transport is connected there's no need for an initial "new" state. I explained in 54 why a stream can begin in a 'Ready' or Recv' state.

Send stream state mapping

  • 'Ready,' 'Send' ==> writable/open
  • 'Data Sent', 'Data Recvd', 'Reset Sent', 'Reset Recvd' ==> !writable/closed
Receive stream state mapping
  • 'Recv', 'Size Known,' 'Data Recvd' ==> readable/open
  • 'Data Read,' 'Reset Recvd,' 'Reset Read' ==>!readable/closed

Other issues

  • The receive stream state diagram waits for a RST_STREAM frame to be received in response to the STOP_SENDING frame before transitioning away from 'Recv' state. Although it is perfectly fine to ignore incoming data after sending a STOP_SENDING.
  • Closing a bidirectional stream. Do we want to allow the send stream to send a RST_STREAM frame w/o the receive stream sending a STOP_SENDING frame and vice versa? This is what the spec says on the matter:

RST_STREAM terminates one direction of a stream abruptly.... it will often be the case that upon receipt of a RST_STREAM an endpoint will choose to stop sending data in its own direction. If the sender of a RST_STREAM wishes to explicitly state that no future data will be processed, that endpoint MAY send a STOP_SENDING frame at the same time.>

@shampson
Copy link
Collaborator Author

shampson commented Sep 28, 2018

tl;dr

  • opening/closing aren't necessary
  • If we enforce 'connected' state before createStream, we can also get rid of new
    • If we do this we could get rid of RTCQuicStreamState
  • Creating two separate send/receive stream objects could clean things up and make the API more straightforward
  • added a function for STOP_SENDING (receive side) & updated RST_STREAM (send side) to be a reset in one direction

I'm happy to make a PR on what there's consensus on changing. I didn't want to spend the time prototyping a PR with all of this, because it incorporates a lot of changes into one. Looking forward to hearing thoughts.

@aboba
Copy link
Collaborator

aboba commented Sep 28, 2018

@shampson Early on, when uni-directional streams were first proposed, we roughed out something similar to what you describe. The trickiest part was relating the receive and send streams to each other. Some questions:

  • Do you envisage createStream() always returning an RTCQuicBidirectionalStream? There might be advantages of doing it this way, and always eventing an RTCQuicBidirectionalStream in onquicstream, since this makes it easier to track the send and receive streams within a bi-directional stream.

To make this work, you could do something like this:

partial interface RTCQuicBiDirectionalStream {
readonly attribute RTCQuicReceiveStream receiveStream;
readonly attribute RTCQuicSendStream sendStream;
}

If done this way, creating a 'send' stream would return a bidi strream with receiveStream null. The corresponding onquicstream event for the remote peer would return a bidi stream with sendStream null.

@aboba aboba added the question label Sep 28, 2018
@aboba
Copy link
Collaborator

aboba commented Sep 28, 2018

@shampson @pthatcherg Since this is a pretty big change, my suggestion would be to create a Google doc with the proposal so we can collaborate on it prior to Wednesday's call.

@aboba
Copy link
Collaborator

aboba commented Oct 1, 2018

@shampson
Copy link
Collaborator Author

shampson commented Oct 1, 2018

Thanks for making the doc @aboba!

@aboba aboba added PR exists A pull request has been submitted and removed question labels Oct 19, 2018
@pthatcherg
Copy link
Collaborator

Can't we close this now?

@shampson shampson closed this as completed Nov 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR exists A pull request has been submitted
Projects
None yet
Development

No branches or pull requests

3 participants