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

http3: design a proper error API #3737

Closed
kgersen opened this issue Mar 27, 2023 · 12 comments · Fixed by #4039
Closed

http3: design a proper error API #3737

kgersen opened this issue Mar 27, 2023 · 12 comments · Fixed by #4039

Comments

@kgersen
Copy link

kgersen commented Mar 27, 2023

We're using a context.WithTimeout for a HTTP/3 connection.
In case of a timeout, we'd like to catch the quic.StreamError with errorRequestCanceled (errorCode = 0x10c).
The ErrorCode field is exported but its possible values are not so we have to do a test with 0x10c (or duplicate http3/error_codes.go and cast)

We could eventually test against the string representation H3_REQUEST_CANCELLED which is more user friendly than 0x10c but errorCode is not exported.

May be I've missing something and there is a better way to test for an errorRequestCanceled than using 0x10c ?

Our current error handling code :

...
// err has the error
var qerr *quic.StreamError
if errors.As(err, &qerr) && qerr.ErrorCode == 0x10c {
  //this is an errorRequestCanceled error, handle it
}
...
@marten-seemann marten-seemann changed the title quic.StreamError.ErrorCode values not exported http3: error code values not exported Mar 27, 2023
@marten-seemann
Copy link
Member

How would you do this in HTTP/2?

@kgersen
Copy link
Author

kgersen commented Mar 27, 2023

How would you do this in HTTP/2?

It depends:

general case: we currently use the exported values from https://github.com/golang/net/blob/master/http2/errors.go (for instance we use this to check for http2.ErrCodeInternal if the server WriteTimeout expires).

our specific context.WithTimeout case: the HTTP/2 client returns a context.DeadlineExceeded error instead of a http2.StreamError or a http2.ConnectionError. It's the same behavior as net/http (HTTP/1.1 mode) so we don't even need to distinguish between HTTP/1.1 and HTTP/2. we just do:

....
// err has the error
if errors.Is(err, context.DeadlineExceeded) {
  // this request was timed out, handle it
}
...

if HTTP/3 could return context errors that would be also nice to have but that is probably a separate issue from this one. Not all errors are context errors so it would be nice to have public "user friendly" values for http3 & quic errors.

@marten-seemann
Copy link
Member

It's nice to have the http2 package as a precedent. I think it makes sense to expose a http3.StreamError that looks like a http2.StreamError. @kgersen, would you want to contribute a patch? Happy to review.

Returning a (wrapped?) context error in case of context expiration definitely makes sense.

@kgersen
Copy link
Author

kgersen commented Mar 28, 2023

I've submitted PR #3744 (with my work account): a simple rename + make public.
It's a trivial & partial fix but it allows us to use user friendly names instead of numbers.

Having a http3.StreamError instead of quic.StreamError is more complex to do (involve warping & casting) and would require more time to carefully design (also may be check and wait on what the Go team is planning to do for QUIC and HTTP/3 and conform to their design/error codes ?).

@marten-seemann
Copy link
Member

Our current error handling code :

...
// err has the error
var qerr *quic.StreamError
if errors.As(err, &qerr) && qerr.ErrorCode == 0x10c {
  //this is an errorRequestCanceled error, handle it
}
...

Where does this error come from? Is this from http.Response.Body.Read?

@kgersen
Copy link
Author

kgersen commented Mar 28, 2023

Where does this error come from? Is this from http.Response.Body.Read?

yes among others.
we do GET and POST requests with big bodies and a time limit.
During a GET we get this error from an io.Copy of the body so it comes from a http.Response.Body.Read
During a POST we get this error when doing a http.Client.Do(request).

@marten-seemann
Copy link
Member

How do you get a context error then? You don't pass any context to http.Response.Body.Read in the first place.

@kgersen
Copy link
Author

kgersen commented Mar 28, 2023

How do you get a context error then? You don't pass any context to http.Response.Body.Read in the first place.

i use a http.Request with a context (use http.NewRequestWithContext for instance).

I built a small POC here: https://github.com/kgersen/h3ctx ( see the Download func ).
To use go run main.go -t 1s for a timeout of 1s
This will download using HTTP/2 then HTTP/3 and display the timeout error.

@marten-seemann
Copy link
Member

Related discussion: caddyserver/caddy#5766 (comment)

@marten-seemann marten-seemann added this to the v0.39 milestone Aug 21, 2023
@marten-seemann marten-seemann changed the title http3: error code values not exported http3: design a proper error API Aug 21, 2023
@bt90
Copy link
Contributor

bt90 commented Aug 21, 2023

@marten-seemann
Copy link
Member

traefik/traefik#8927 (comment)

@bt90 Can you elaborate? I'm not sure I understand what this means for the design of the error API here.

@bt90
Copy link
Contributor

bt90 commented Aug 21, 2023

I just saw that comment and thought it might be helpful. Feel free to ignore it if it isn't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants