Replace HTTP_MALFORMED_FRAME error code

[LPardue]

This fixes #2551.

Several changes here that all go toward replacing the generic HTTP_MALFORMED_FRAME error. It adds 4 new error codes that cover specific error cases already highlighted. Chiefly, it restores the HTTP_FRAME_SIZE_ERROR capability we dropped from HTTP/2, just under a different name.

This PR may trigger the Yet-another-error-code early warning system. However, in practical terms this change reduces the error code space by ~250, based on the fact that en endpoint would do frame length validation of unknown frame types and send an error in such a case. It also avoids the future complexity required to signal errors on extension frames types above the limit aka the 0xff problem punt.

[LPardue]

For transparency sake, a straw poll identifies that three implementation have already implemented HTTP_MALFORMED_FRAME in its current version: lsquic, mvfst and nghttp3.

[LPardue]

I've rebased this PR to incorporate all of the error code rationalization changes that were merged to master. This PR does a few things:

• Resurrects H2's FRAME_SIZE_ERROR but broadens it to include frame layout errors - name is HTTP_FRAME_ERROR
• Introduces a new error code HTTP_INVALID_PRIORITY that captures cases where a frame with valid wire layout attempts to create a prioritiation relationship (see additional point below)
• Replaces occurrences of HTTP_MALFORMED_FRAME with: HTTP_ID_ERROR, HTTP_FRAME_ERROR, HTTP_INVALID_PRIORITY.
• Reshuffles all of the error codes. In doing so, I tried to align the code space with H2 and avoid there being any gaps.

I'm not 100% convinced we need a HTTP_INVALID_PRIORITY code. I'm open to simply using HTTP_FRAME_ERROR.

HTTP_SETTINGS_ERROR and HTTP_MISSING_SETTINGS are candidates to be consolidated (either together, or into another code). This is dependent on discussion in (and related to) #2783. I suggest we solve that in a different PR.

[kazuho]

@LPardue Thank you for all your effort on refactoring the error codes.

I think my preference goes to merging INVALID_PRIORITY with FRAME_ERROR. The reason is that it is the error code captures a syntactic error of a frame, and because I do not think that each frame should have it's own error code to indicate an syntactic error. As an example, "empty bits" could appear not just in the PRIORITY frame but in any frame. Should we have dedicated error code for frames that contain empty bits? I do not think so. Hence my preference to not have INVALID_PRIORITY.

Regarding rearranging the numbers, I wonder if it would be possible to split the error codes into two groups: connection-level error codes and stream-level ones. In contrary to HTTP/2, most of the HTTP/3 error code are either a connection-level error code or a stream-level error code, not both.

[LPardue]

I think my preference goes to merging INVALID_PRIORITY with FRAME_ERROR.
Thanks for the feedback. It's an easy change to make. I'll wait for some supporting or dissenting voices but otherwise will do this.

Regarding rearranging the numbers, I wonder if it would be possible to split the error codes into two groups: connection-level error codes and stream-level ones. In contrary to HTTP/2, most of the HTTP/3 error code are either a connection-level error code or a stream-level error code, not both.

This resonates with my earlier analysis. Some of this was captured in #2816. But I think what you are suggesting is a slightly different thing. Can you explain what splitting might look like and how it would help? I think it gets complicated because 1) we may have a proxy that is trying to translate errors between H2 and H3. 2) stream errors can be upgraded to connection errors

[kazuho]

@LPardue

I think my preference goes to merging INVALID_PRIORITY with FRAME_ERROR.

Thanks for the feedback. It's an easy change to make. I'll wait for some supporting or dissenting voices but otherwise will do this.

👍

Regarding rearranging the numbers, I wonder if it would be possible to split the error codes into two groups: connection-level error codes and stream-level ones. In contrary to HTTP/2, most of the HTTP/3 error code are either a connection-level error code or a stream-level error code, not both.

This resonates with my earlier analysis. Some of this was captured in #2816. But I think what you are suggesting is a slightly different thing. Can you explain what splitting might look like and how it would help? I think it gets complicated because 1) we may have a proxy that is trying to translate errors between H2 and H3. 2) stream errors can be upgraded to connection errors

IIUC, the following H3 error codes are used at both connection and stream level.

• HTTP_NO_ERROR
• HTTP_GENERAL_PROTOCOL_ERROR
• HTTP_INTERNAL_ERROR

The following codes are used only at connection level, though HTTP_EXCESSIVE_LOAD could be carried at stream level too.

• HTTP_STREAM_CREATION_ERROR
• HTTP_CLOSED_CRITICAL_STREAM
• HTTP_UNEXPECTED_FRAME
• HTTP_FRAME_ERROR
• HTTP_EXCESSIVE_LOAD
• HTTP_WRONG_STREAM
• HTTP_ID_ERROR
• HTTP_INVALID_PRIORITY
• HTTP_SETTINGS_ERROR
• HTTP_MISSING_SETTINGS

The following codes are stream-level errors. While they can be promoted to connection errors, doing so has a risk of resetting other requests inflight. Therefore I'd assume that they would generally be used only at stream-level.

• HTTP_REQUEST_REJECTED
• HTTP_REQUEST_CANCELLED
• HTTP_INCOMPLETE_REQUEST
• HTTP_EARLY_RESPONSE
• HTTP_CONNECT_ERROR
• HTTP_VERSION_FALLBACK

Therefore I thought that it might make sense to explain these three groups differently in the draft.

While I can see the desire to assign codepoints that overlap with HTTP/2, I am not sure if that's a good idea. Not only the error codes are different between H2 and H3, some H2 error codes map to QUIC transport-level errors. Considering the fact that there cannot be a one-to-one mapping, I'm afraid having overlap might backfire against us; it just creates an incentive to pass-through the error codes between different versions of the protocol.

[LPardue]

Thanks for this analysis it does really clarify your position. I think there are two independent discussion points here:

1. should error codes be grouped in a logical order by their intended use?
2. should error codes align with or avoid HTTP/2 where possible?

For (1), I'm not against adopting your suggested order but keen to hear some other opinions here.

(2) I think is a bigger issue:

While I can see the desire to assign codepoints that overlap with HTTP/2, I am not sure if that's a good idea. Not only the error codes are different between H2 and H3, some H2 error codes map to QUIC transport-level errors. Considering the fact that there cannot be a one-to-one mapping, I'm afraid having overlap might backfire against us; it just creates an incentive to pass-through the error codes between different versions of the protocol.

Do you think the incentive grows with increasing number of aligned points? A sledgehammer of a disincentive is to shift these to avoid HTTP/2 completely; e.g. shift by 0x20, 0x50 or 0x100. Note that QPACK codes already start at 0x200

[ianswett]

I am not seeing much value in aligning with HTTP/2 error codes. Alignment would have value if the endpoints could treat a given HTTP/2 and HTTP/3 error code identically.

However, in our current GQUIC implementation, we have a handful of error codes with any special treatment and they're all transport level, so I don't think that's a good argument?

Aligning them has the chance of adding more confusion, so unless there is a technical reason to do so(ie: the above), I think it should not be a goal.

[LPardue]

I added two separate commits to shift the error code space to start from 0x100 (plays well with QPACK codes starting 0x200), and to apply @kazuho's proposed ordering. I didn't add any wordsmithing to explain this order as it didn't seem crucial.

[kazuho]

Thank you for the changes.

FWIW, my point was that trying to use the same codepoints for the some errors does not make sense (I think @ianswett's comment captures my feeling). I did not mean that we need to avoid using different block of codepoints than that of HTTP/2. Sorry if my comment was confusing.

That said, I would not oppose from starting at 0x100.

[LPardue]

@rmarx suggests that we rename HTTP_INCOMPLETE_REQUEST to HTTP_REQUEST_INCOMPLETE in order to be more consistent with other error codes. That SGTM so I'll make that change unless anyone disagrees.