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

Consider making SETTINGS part of the control stream header #2783

Closed
LPardue opened this issue Jun 11, 2019 · 6 comments
Closed

Consider making SETTINGS part of the control stream header #2783

LPardue opened this issue Jun 11, 2019 · 6 comments
Labels

Comments

@LPardue
Copy link
Member

LPardue commented Jun 11, 2019

Right now we have a SETTINGS frame that can only be sent once (in each direction), and if it is sent it has to be sent at a very precise point (the start of the stream) or the connection is bust.

For these reasons, it strikes me that we could simplify things, in line with today's HTTP/3 design, by unframing settings and placing them as the control stream header. For illustration this would look something like below, where Identifier and Value can be sent multiple times.

0                   1                   2                   3
 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|                           0x00 (i)                          ...
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|                          Setting Length (i)             ...
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|                        Identifier (i)                       ...
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|                           Value (i)                         ...
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

For

  • Simplifies some parts of error handling.
    • The HTTP_MISSING_SETTINGS snowflake becomes redundant
    • Not possible to send multiple SETTINGS on the control stream to trigger HTTP_UNEXPECTED_FRAME
    • Not possible to send a SETTINGS frame on the wrong stream to trigger HTTP_WRONG_STREAM
  • A peer that is happy with defaults can send 00 (2 bytes) rather than 040 (3 bytes). This is a tinsy tiny benefit
  • Forces endpoints to commit to SETTINGS very early (this is of dubious benefit if SETTINGS only loosen values)

Against

  • We already have implementations that handle SETTINGS just fine
  • Keeping the frame allows us change today's design to allow multiple SETTINGS in the future.

The arguments for and against seem to net out evenly. Changing control stream seems like a cleaner design to me but shipping is a feature ™️ .

@LPardue
Copy link
Member Author

LPardue commented Jun 11, 2019

aprops @dtikhonov: One way to remove [HTTP_MISSING_SETTINGS] is to allow not sending the SETTINGS frame at all. By that I meant that if first thing on the control stream isn't SETTINGS, then SETTINGS is not coming, use defaults

@ianswett ianswett added the -http label Jun 11, 2019
@kazuho
Copy link
Member

kazuho commented Jun 12, 2019

While the proposal is interesting, I think my preference goes to retaining the current design.

I agree that HTTP_MISSING_SETTINGS is unnecessary, it is my view that it should be merged to HTTP_UNEXPECTED_FRAME.

But once it gets done, it would be my view that the complexity is indifferent; there is no difference in the encoding-side (just sending different series of octets). On the decoding-side, it either having a state that receives series of special octets or a state that receives a particular frame.

That said, the reason I prefer using a frame for carrying the settings is because it makes the design consistent. Having a layer that is used for "frame"-ing all the information being exchanged is a good thing.

IMO the use of HTTP_UNEXPECTED_FRAME or HTTP_WRONG_STREAM in relation to SETTINGS frame is fine, because use of these errors are the design pattern we use for other frames too. For example, HTTP_UNEXPECTED_FRAME is used to indicate error when the peer sends a DATA frame before a HEADERS frame. Or transmission of a HEADERS frame on the control stream triggers the HTTP_WRONG_STREAM error.

I prefer using that same design pattern for everything, rather than creating different rules for the way things are being sent in each state.

@LPardue
Copy link
Member Author

LPardue commented Jun 12, 2019

I remembered that we touched on some of this in #2526

Thanks for the feedback @kazuho

I prefer using that same design pattern for everything, rather than creating different rules for the way things are being sent in each state.

Push streams break this pattern but c'est la vie.

I think I'm happy to kill this issue/proposal.

@MikeBishop
Copy link
Contributor

MikeBishop commented Jun 12, 2019

If we took this proposal, we'd be cementing a new pattern: Unidirectional streams might have a header following the type. Push already does (and in London we rejected a proposal to make Push ID behave the way SETTINGS does instead, #2526). I like that pattern, and alongside #2781 this would remove the concept of frame-that-can-only-be-first. That's decidedly appealing.

However, we're close to late-stage and this isn't demonstrably better, just arguably cleaner.

@kazuho
Copy link
Member

kazuho commented Jun 13, 2019

I think that Push Streams is a good argument for pushing this change, although I might argue the Push Streams header is not a length-value structure.

I think I'd prefer reusing the type-length-value structure (i.e. frames) for sending SETTINGS rather than defining it's own length-value structure just for one purpose.

(not that I care much, but just my two cents)

@LPardue
Copy link
Member Author

LPardue commented Oct 18, 2019

I believe the group did not want to make this change so I am closing the issue. @MikeBishop please reopen if I am mistaken.

@LPardue LPardue closed this as completed Oct 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants