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

Compare H2 PROTOCOL_ERROR with HTTP_GENERAL_PROTOCOL_ERROR #2808

Merged
merged 3 commits into from Jun 27, 2019

Conversation

LPardue
Copy link
Member

@LPardue LPardue commented Jun 19, 2019

Fixes #2807

@@ -2174,8 +2174,7 @@ NO_ERROR (0x0):
: HTTP_NO_ERROR in {{http-error-codes}}.

PROTOCOL_ERROR (0x1):
: No single mapping. See new HTTP_MALFORMED_FRAME error codes defined in
{{http-error-codes}}.
: HTTP_GENERAL_PROTOCOL ERROR in {{http-error-codes}}.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it still the case that we have multiple error codes that map to HTTP_GENERAL_PROTOCOL error in HTTP/2?

Or are we going to merge all the connection-level error codes that do not map to an error code in HTTP/2 (e.g., WRONG_STREAM, UNEXPECTED_FRAME, CLOSED_CRITICAL_STREAM) to HTTP_GENERAL_PROTOCOL?

Copy link
Member Author

@LPardue LPardue Jun 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow sorry. This section states:

QUIC has the same concepts of “stream” and “connection” errors that HTTP/2 provides. However, there is no direct portability of HTTP/2 error codes.

The change is intended to address how the HTTP/2 error code PROTOCOL_ERROR, for HTTP/3 non-connection errors (usually frame errors), is equivalently expressed.

How would you suggest improving this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I should have stated that the removal of "No single mapping." is concerning, because IIUC we do not have one-to-one mapping between PROTOCOL_ERROR (of HTTP/2) and HTTP_GENEAL_PROTOCOL error (of HTTP/3).

What I had in mind is something like "Split into a few error codes, including HTTP_GENERAL_PROTOCOL, HTTP_WRONG_STREAM, HTTP_UNEXPECTED_FRAME, HTTP_CLOSED_CRITICAL_STREAM."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I agree and would like to adopt that text. The annoying thing is that it add more references to error codes I'm looking to consolidate but I can try to update the text to match the state of the draft as the PRs get merged.

@ianswett ianswett added the -http label Jun 19, 2019
Copy link
Contributor

@MikeBishop MikeBishop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than making everything have an even footing, I'd be more inclined to say that GPE is the equivalent error code, but point out that more specific error codes have also been added for cases that would have been PE in H2.

@@ -2174,7 +2174,9 @@ NO_ERROR (0x0):
: HTTP_NO_ERROR in {{http-error-codes}}.

PROTOCOL_ERROR (0x1):
: No single mapping. See new HTTP_MALFORMED_FRAME error codes defined in
: No single mapping. Split into a few error codes, including
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
: No single mapping. Split into a few error codes, including
: No single mapping. Split into multiple error codes, including

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say that "This is mapped to HTTP_GENERAL_PROTOCOL_ERROR except in cases where more specific error codes have been defined. This includes HTTP_MALFORMED_FRAME, ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, I applied your suggestion @martinthomson

@MikeBishop MikeBishop merged commit d6d6d0f into quicwg:master Jun 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

H3 appendix A.4 GENERAL_PROTOCOL_ERROR
5 participants