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

Improved type negotation #37

Open
ecton opened this issue Dec 16, 2021 · 5 comments
Open

Improved type negotation #37

ecton opened this issue Dec 16, 2021 · 5 comments

Comments

@ecton
Copy link
Member

ecton commented Dec 16, 2021

I tried adding version negotiation using the type exchange when accepting a stream, but there's no easy way to reject a stream due to a version mismatch.

The client's accept() call returns before the server has actually accepted the stream, and there's no way for the server to do anything with the Incoming type other than accept it -- but if we can't establish that we are on a compatible protocol version, how are we supposed to respond? The only way to respond is to use the type that we know is incompatible based on the exchange we just did.

Ideally the client's accept() call wouldn't return until the server has actually accepted the stream, and Fabruic would either use any built-in version mismatch errors possible, or it would have one extra bit of exchanged data before switching into the final mode of payload streaming.

I don't know if that impacts the underlying QUIC protocol -- ie, if I sent a payload on the sender before the server accepted the stream, is QUIC already sending those packets or are they being held by fabruic/quinn until the stream is accepted?

The last bit of feedback is that I personally find r# identifiers horrible -- I'd much prefer this to be renamed way from type rather than embracing r#type. Kind tends to be the usual suggestion.

@daxpedda
Copy link
Member

In QUIC streams are free, opening a stream does literally nothing without sending a message. r#type is horrible I agree. Let's discuss this in more detail over voice today.

@ecton
Copy link
Member Author

ecton commented Dec 17, 2021

Looking forward to chatting, just wanted to jot down a few thoughts I've had since last night. I didn't do a good job at expressing what I would like as a user of the library: A way on incoming() to reject the stream rather than needing to call accept(). The behavior on the client should be that there should be an error that gets returned somewhere on the client that can distinctly be used to determine that the stream was explicitly rejected, not that a disconnect happened.

I would go beyond and ask for an ability to provide a little bit of context information -- but I don't really want to complicate the type handling even further. I'd be happy with being able to specify an integer value "reason" during the stream rejection phase that gets passed through to the error message.

@daxpedda
Copy link
Member

Rejection could be done by simply dropping the handle. The integer value isn't something supported by QUIC, you can close the stream with an integer, but the sender can't see the error until it actually tries to send something. I will investigate further.

@ecton
Copy link
Member Author

ecton commented Dec 17, 2021

I can't tell the difference between a rejection and a disconnection. That's my problem -- I want to present a VersionMismatch error, but I can't differentiate.

@ecton ecton changed the title Improved type/version negotation Improved type negotation Dec 17, 2021
@ecton
Copy link
Member Author

ecton commented Dec 17, 2021

I only just now realized that I was conflating two things -- I saw the type() interface and thought that was how I was supposed to do versioning -- I now remember us talking about the protocols stuff, I just didn't realize it was all ALPN, not QUIC. I can use that to negotiate version.

My feedback on being able to "Reject" a type still stands, however -- there's no way to give a distinct error that you're rejecting the stream instead of having a connection issue.

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

2 participants