http3: introduce an HTTP/3 error type

[marten-seemann]

Fixes #3737.

@kgersen Would this resolve your issue? Would you mind reviewing / trying it out?

[codecov]

Codecov Report

Merging #4039 (a47f6b4) into master (ab1c1be) will decrease coverage by 0.00%.
The diff coverage is 91.67%.

@@            Coverage Diff             @@
##           master    #4039      +/-   ##
==========================================
- Coverage   83.59%   83.59%   -0.00%     
==========================================
  Files         148      149       +1     
  Lines       15227    15267      +40     
==========================================
+ Hits        12728    12761      +33     
- Misses       1999     2005       +6     
- Partials      500      501       +1     

Files Changed	Coverage Δ
http3/body.go	73.91% <33.33%> (-1.64%)	⬇️
http3/response_writer.go	84.13% <50.00%> (+0.13%)	⬆️
http3/client.go	85.89% <100.00%> (ø)
http3/error.go	100.00% <100.00%> (ø)
http3/error_codes.go	100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

[jfgiorgi]

(kgersen here).
I've updated our poc: https://github.com/kgersen/h3ctx

when using it with this PR we get:

QUIC_GO_DISABLE_GSO=true go run main.go -t 1s
...
receiving data with HTTP/1.1
s->c: client got error of type context.deadlineExceededError
...
receiving data with HTTP/2.0
s->c: client got error of type context.deadlineExceededError
...
receiving data with HTTP/3.0
s->c: client got error of type http3.Error
client error for https://localhost:2222/10000000000: H3_REQUEST_CANCELLED (local)
...

so it works and it's better : we do get now a proper http3.Error instead of previoulsy a quic.StreamError
but
It's not a context.deadlineExceededError like http1.1 and http/2.

[marten-seemann]

Which error are you looking at? Is this the error the client gets when performing a requests whose context has been canceled? I agree that the client should get a context error in that case, and the server should receive a H3_REQUEST_CANCELLED.

[jfgiorgi]

yes it's the error received when performing a request whose context has been canceled (timeout after 1 second in the posted example).
We don't see anything server side (but I'll have to double check this tomorrow). Might be the same issue as #3892 ?

[marten-seemann]

@jfgiorgi Your issue is a bit different, in that it's dealing with the local error code, not with the error sent by the remote.

I suggest merging this PR first, and then dealing with the request cancelation and #3892. Would you want to contribute a fix?

[jfgiorgi]

This PR is ok for me because it's cleaner since it follows the HTTP/3 error RFC.

But when a context is cancelled or expire, we get a HTTP/3 error instead of the context error.
I think the issue is here:

quic-go/http3/client.go

Lines 296 to 300 in 824fd8a

	case <-req.Context().Done():
	str.CancelWrite(quic.StreamErrorCode(ErrCodeRequestCanceled))
	str.CancelRead(quic.StreamErrorCode(ErrCodeRequestCanceled))
	case <-reqDone:
	}

• it returns a HTTP/3 error (H3_REQUEST_CANCELLED) instead of a context error
• so we lose the information of the context error (which is available in req.Context().Err())

With net/http we can distinguish between a context cancel or timeout for instance because the context error is returned by the transport.

[marten-seemann]

@jfgiorgi I agree. It's not immediately obvious though how to get the error, since this code is running in a separate Go routine, so you'll probably need to check the context before returning from RoundTripOpt.

[jfgiorgi]

@jfgiorgi I agree. It's not immediately obvious though how to get the error, since this code is running in a separate Go routine, so you'll probably need to check the context before returning from RoundTripOpt.

I looked at this part of the code and compared to how it's done in net/http http2 roundtrip.
It's the other way: the whole roundtrip is done in a separate Go routine. It seems cleaner to deal with contexts that way.
see https://github.com/golang/net/blob/master/http2/transport.go#L1233 (spawn of the Go routine)
and also : https://github.com/golang/net/blob/master/http2/transport.go#L1464-L1480 (main event loop for the go routine)

But doing that in quic-go might involve (or not) a lot of refactoring,

[marten-seemann]

It's the other way: the whole roundtrip is done in a separate Go routine. It seems cleaner to deal with contexts that way. see https://github.com/golang/net/blob/master/http2/transport.go#L1233 (spawn of the Go routine) and also : https://github.com/golang/net/blob/master/http2/transport.go#L1464-L1480 (main event loop for the go routine)

But doing that in quic-go might involve (or not) a lot of refactoring,

@jfgiorgi Want to give it a shot? I'd be happy to review a PR :)

[jfgiorgi]

@jfgiorgi Want to give it a shot? I'd be happy to review a PR :)

tbh we're very uncertain about investing more time in quic-go. The Go team own QUIC implementation is around corner and their http/3 is probably next.
Submitting small PRs is fine, bigger ones not so sure.
And if quic.http3 is to be refactored it should be done to be interoperable with the api changes happening here: golang/go#60746
I need more understanding about all these parts before committing to anything. So it's a no for now.

[marten-seemann]

Good point about API compatibility. Regarding the Go team’s QUIC effort, I wouldn’t place to much confidence in the reliability and performance of a QUIC stack that hasn’t even completed a single QUIC handshake so far…