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

Replace HTTP_MALFORMED_FRAME for something simpler #2551

Closed
LPardue opened this issue Mar 25, 2019 · 13 comments · Fixed by #2662
Closed

Replace HTTP_MALFORMED_FRAME for something simpler #2551

LPardue opened this issue Mar 25, 2019 · 13 comments · Fixed by #2662
Labels
-http design An issue that affects the design of the protocol; resolution requires consensus.

Comments

@LPardue
Copy link
Member

LPardue commented Mar 25, 2019

I remain unconvinced on the usefulness of the HTTP_MALFORMED_FRAME error code.

Draft 19 says:

the HTTP_MALFORMED_FRAME (0x01XX): An error in a specific frame type.
If the frame type is "0xfe" or less, the type is included as the
last byte of the error code. For example, an error in a
MAX_PUSH_ID frame would be indicated with the code (0x10D). The
last byte "0xff" is used to indicate any frame type greater than
"0xfe".

At the core of my perspective is that the document currently mixes the use of this error between truncated frames and more-semantic-level errors eg.

  • receiving a MAX_PUSH_ID with a value lower than a previously sent value.
  • PRIORITY (a super complicated frame)
  • SETTINGS (error with duplicate parameters).

I'm not convinced these semantic errors are due to the frame being malformed, it is more 'misused'. Levelling up, I wonder if this error code is the best way of approaching the problems or if we could do something simpler. HTTP/2 had a HTTP_FRAME_SIZE error which was replaced by HTTP_MALFORMED_FRAME.

Another wrinke is: What should an endpoint do if it receives an error that references an unknown frame type? An endpoint MUST ignore unknown types, and we have these in the form of grease frame types with values that are included in the error code space.

@MikeBishop MikeBishop added design An issue that affects the design of the protocol; resolution requires consensus. -http labels Mar 26, 2019
@LPardue
Copy link
Member Author

LPardue commented Apr 16, 2019

This relates to our principles: #2388

@dtikhonov
Copy link
Member

These principles are only @martinthomson's proposal for now.

@ianswett
Copy link
Contributor

ianswett commented May 7, 2019

Issue #2672 would make this frame more useful.

@LPardue
Copy link
Member Author

LPardue commented May 7, 2019

Maybe, but I still think the code is overkill. Each frame has to define what it means to be malformed, and most frames are simple enough that malforming them is only possible by getting the size wrong.

The proposal on #2662 would mean most error become a generic FRAME_SIZE_ERROR. This is equivalent to the HTTP/2 status quo. It would help to know if current deployments find that insufficient and/or why they feel that a similar error in HTTP/3 would be insufficient so as to warrant MALFORMED_FRAME.

@MikeBishop
Copy link
Contributor

I'd like to hear feedback on this particularly from people who've implemented the current error code. @dtikhonov, you nitpicked the connection to principles, but didn't express a position. @afrind, @tatsuhiro-t, I think you have the other two?

@dtikhonov
Copy link
Member

@MikeBishop, I don't have a strong opinion on this and thus decline to take a position.

@tatsuhiro-t
Copy link
Contributor

My h3 interop experience is quite short, but so far I haven't get any benefit from the current form of HTTP_MALFORMED_FRAME. If the gain is small, then I prefer the simpler form such as just a single HTTP_MALFORMED_FRAME generic error or #2662

@afrind
Copy link
Contributor

afrind commented May 15, 2019

I don't know that it's been specifically useful so far, but I don't think it's hard to implement and has the potential to be useful. If everyone wants to kill it I can live without it, plus we can say that at least once we chose the less complicated design.

@kazuho
Copy link
Member

kazuho commented May 16, 2019

Not that I have a strong opinion on this, but I'd like to point out that the current design is somewhat inconsistent.

Now, three frame-level errors exist: MALFORMED_FRAME, WRONG_STREAM, UNEXPECTED_FRAME. Among the three, only MALFORMED_FRAME is being accompanied by a frame type. I am not sure if there is a good reason for MALFORMED_FRAME to be exceptional.

@MikeBishop
Copy link
Contributor

(Now that you mention these, I'm not sure now why WRONG_STREAM and UNEXPECTED_FRAME are different. The frame is not expected to be on this stream, so...?)

@kazuho
Copy link
Member

kazuho commented May 16, 2019

(FWIW, UNEXPECTED_FRAME is generated when a frame is received on the correct stream at an unexpected moment. Though I am not sure if the distinction is important)

@LPardue
Copy link
Member Author

LPardue commented May 18, 2019

Quoting myself:

Another wrinke is: What should an endpoint do if it receives an error that references an unknown frame type? An endpoint MUST ignore unknown types, and we have these in the form of grease frame types with values that are included in the error code space.

Endpoint X receives a GREASE frame and for whatever reason decides to issue a stream error with a HTTP_MALFORMED_FRAME code that covers GREASE. Endpoint Y receives the error code, and takes this as a signal that X didn't follow the rule to ignore the frame. So it sends a connection error back?

This is an edge case yes, but I think it is another argument against declaring a very large codepoint space that is mostly useless in practice.

@MikeBishop
Copy link
Contributor

Discussed in London; we should do this, but @LPardue will follow up with the list in a discussion about the overall error code set and how they're grouped/used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-http design An issue that affects the design of the protocol; resolution requires consensus.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants