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

max_data and max_stream_data are inconsistent with transport params #895

Closed
siyengar opened this issue Oct 22, 2017 · 4 comments
Closed
Labels
-transport design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list.

Comments

@siyengar
Copy link

max_data and max_stream_data seem to be inconsistent between the stream flow control and transport parameters

In transport params they are encoded as 32 bit values and in the MAX_STREAM_DATA it is a 64 bit value. Similarly now MAX_DATA is a 64 bit value in octets, however initial_max_data is a 32 bit value expressed in KB. It's not a big deal to implement, but just a bit weird.

@martinthomson
Copy link
Member

I think that the intent was to specify both in the same units, and one got missed by both patch author and editors. I'll fix that.

@martinthomson martinthomson reopened this Oct 23, 2017
@martinthomson martinthomson added -transport design An issue that affects the design of the protocol; resolution requires consensus. labels Oct 23, 2017
@martinthomson
Copy link
Member

Oh, I missed the 32/64 thing. I agree that it's odd. Then there's the possibility of using a varint. I await feedback from implementers on this one.

@siyengar
Copy link
Author

siyengar commented Oct 23, 2017

I implemented 32/64, and it wasn't too bad, since these parsers are necessarily different between the handshake and the frame, and 32 bits should technically be enough even for connection flow control since this is just an initial offset, however it just seems that having these 2 be different is error prone.

@mnot mnot added this to Streams in QUIC Dec 13, 2017
@MikeBishop
Copy link
Contributor

We don't have varint in transport params, and this seems fine. The initial doesn't need to be as big as the max for the connection.

@mnot mnot removed this from Streams in QUIC Mar 15, 2018
@mnot mnot added the has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list. label Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list.
Projects
None yet
Development

No branches or pull requests

4 participants