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

Ambiguous wording about error codes in HTTP/3 #3276

Closed
LPardue opened this issue Nov 27, 2019 · 12 comments · Fixed by #3331
Closed

Ambiguous wording about error codes in HTTP/3 #3276

LPardue opened this issue Nov 27, 2019 · 12 comments · Fixed by #3331
Labels
-http editorial An issue that does not affect the design of the protocol; does not require consensus.

Comments

@LPardue
Copy link
Member

LPardue commented Nov 27, 2019

Creating an issue on behalf of @SamB that reflects #3239. On that PR they quote a section of the document:

Because new error codes can be defined without negotiation (see
Section 9), receipt of an unknown error code or use of an error code
in an unexpected context MUST NOT be treated as an error. However,
closing a stream can constitute an error regardless of the error code
(see Section 4.1).

and say:

It's not very clear what it means to not treat an error code as an error.
Maybe [replacing "an error" with "a further error"] is what's meant? (If not, please explain it better.)

@LPardue
Copy link
Member Author

LPardue commented Nov 27, 2019

I don't think the suggested change is aligned with the intent but there might be something we can do to improve the readability.

"error codes" are used for normal operations when it comes to closing streams. So one interpretation of the current text is to say that if you get an unknown error code in a RESET_STREAM, don't treat it as any kind of error. The suggested text "further error" confuses this interpretation.

@kazuho
Copy link
Member

kazuho commented Nov 27, 2019

Maybe something like "MUST NOT be interpreted as an indication of a problem"?

@mnot mnot added -http editorial An issue that does not affect the design of the protocol; does not require consensus. labels Nov 29, 2019
@dtikhonov
Copy link
Member

The paragraph in question (see HTTP draft, section 8) talks about different things:

   Because new error codes can be defined without negotiation (see
   Section 9), receipt of an unknown error code or use of an error code
   in an unexpected context MUST NOT be treated as an error.  However,
   closing a stream can constitute an error regardless of the error code
   (see Section 4.1).

Let's break it up to make it easier to talk about:

  1. Treating an unexpected error code as an error is not allowed, in particular:
    a. Receiving unknown error is not an error in itself (a "further" error); and
    b. Receiving an unexpected error code (known or unknown) is not an error.
  2. A stream close may be an error independent of error code.

Item 1(a) follows from Section 9. This does not need to be stated here.

Item 1(b) seems to suggest that error codes are not to be used to make decisions, for they could be anything, notwithstanding all the "MUST treat as error type" admonitions elsewhere in the code. (This is what the text implies, which may not be what was intended.)

Item 2 says that some error occur due to changes in stream state and are not communicated via an error code from peer.

I would drop Items 1(a) and 1(b); Item 2 can be rephrased.

P.S. Given the loose treatment given to error codes by their recipient, it is logical to allow their sender to keep on treating specified errors as errors ("MUST treat as error..."), but allow setting the error codes to anything. Stating that "MUST treat as error of type X and SHOULD set error code to X" sounds awkward; perhaps this guidance could be added to Section 8 as a new item.

@MikeBishop
Copy link
Contributor

MikeBishop commented Dec 16, 2019

1a/1b are two sides of the same coin. Because error codes are extensible, a future document might define a new error code, or might prescribe the use of an existing error code in a new circumstance. Therefore, the takeaway should be:

  • An error code the recipient knows in an situation where they expect it has understood semantics that the recipient might be able to act on.
  • An unknown error code, or an error code in an unexpected situation, has semantics which the recipient does not understand and cannot act on.

The statement here is that the second case is not a violation of the protocol by the peer; you know the stream (or connection) is closing even if you don't understand why. The admonition is to respond to the closure itself, not the unknown semantics of why it was closed.

While I agree that both versions of the second case follow from Section 9, I think it's useful to mention them in the context of error handling as well. We've seen recently with HTTP/2 what happens when the possibility of extension values isn't mentioned/considered in the description of how the base-protocol pieces are to be handled.

@martinthomson
Copy link
Member

@afrind points out that promotion from stream to connection error is something you shouldn't do absent an understanding of the error semantics.

@MikeBishop
Copy link
Contributor

The only case where this matters is for stream errors (RESET_STREAM / STOP_SENDING). The scenario we want to avoid is closing the connection in response to an unexpected error code in a stream closure. So the right guidance here might be to say that you MUST NOT close the connection based on the unexpected error code.

@MikeBishop
Copy link
Contributor

Correction: We also care about closing one side of bidirectional streams. You shouldn't close the other direction because the first direction closed. One possibility is to treat unknown error codes as equivalent to NO_ERROR.

@afrind
Copy link
Contributor

afrind commented Jan 9, 2020

You shouldn't close the other direction because the first direction closed

If you receive a STOP_SENDING, aren't you supposed to RESET_STREAM if you haven't sent a FIN yet? I guess the other direction is weirder (abrupt termination of response, should you terminate the request too. etc?) but probably tearing everything down is the right move most of the time? At least it's not totally unexpected or catastrophic. Preventing connection closure still seems like the main objective.

I think the general guidance of 'treat unknown error codes as you would treat NO_ERROR' is good, though.

@MikeBishop
Copy link
Contributor

You shouldn't close the other direction because the first direction closed

If you receive a STOP_SENDING, aren't you supposed to RESET_STREAM if you haven't sent a FIN yet?

Right, but if you receive a RESET_STREAM, that doesn't tell you anything about what to do with the other direction.

@MikeBishop MikeBishop removed the editorial An issue that does not affect the design of the protocol; does not require consensus. label Jan 13, 2020
@MikeBishop
Copy link
Contributor

@larseggert / @mnot, this is borderline, but the PR does touch the specifics of the "MUST." Shall we run the process on this?

@mnot
Copy link
Member

mnot commented Jan 14, 2020

I don't think it changes the intent; let's keep it editorial unless someone explicitly objects to that.

@MikeBishop
Copy link
Contributor

SOLD, as editorial, to the chairs.

@MikeBishop MikeBishop added the editorial An issue that does not affect the design of the protocol; does not require consensus. label Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-http editorial An issue that does not affect the design of the protocol; does not require consensus.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants