rework error return values #3159
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3159 +/- ##
=======================================
Coverage 85.43% 85.43%
=======================================
Files 131 132 +1
Lines 9747 9824 +77
=======================================
+ Hits 8327 8393 +66
- Misses 1048 1058 +10
- Partials 372 373 +1
Continue to review full report at Codecov.
|
marten-seemann
force-pushed
the
error-returns
branch
5 times, most recently
from
129306f
to
5578482
Compare
| ) | ||
|
|
||
| type ( | ||
| TransportError = qerr.TransportError |
There was a problem hiding this comment.
Now this is a bit weird. We have a
TransportError: the type of a transport errorTransportErrorCode: the type of the error code for transport errors (auint64)TransportErrorErrorCode: the error code the the TRANSPORT_ERROR (see0xain https://www.ietf.org/archive/id/draft-ietf-quic-transport-34.html#name-transport-error-codes)
There was a problem hiding this comment.
TransportErrorCode
I guess just ErrorCode is not an option?
TransportErrorErrorCode: the error code the the TRANSPORT_ERROR (see0xain https://www.ietf.org/archive/id/draft-ietf-quic-transport-34.html#name-transport-error-codes)
not sure I follow; that page shows PROTOCOL_VIOLATION for 0xa. Could you not use ProtocolViolation?
There was a problem hiding this comment.
Sorry, I meant 0xc.
There was a problem hiding this comment.
Oh I see. Perhaps ErrorCodeApplication or TransportErrorCodeApplication? since the name seems to be APPLICATION_ERROR.
|
|
||
| // A StatelessResetError occurs when we receive a stateless reset. | ||
| type StatelessResetError struct { | ||
| Token protocol.StatelessResetToken |
There was a problem hiding this comment.
It's nice to have all errors in the qerr package, but this means that we have to export the Token here. There's nothing wrong with that, but it's a piece of information that has no value to the user, so it would be nicer if it wasn't exported.
There was a problem hiding this comment.
since this is an internal package, does that matter?
There was a problem hiding this comment.
Yes, because we're exporting it in the quic package: type StatelessResetError = qerr.StatelessResetError
session.go
Outdated
| if !errors.Is(e, qerr.ErrIdleTimeout) && !errors.Is(e, qerr.ErrHandshakeTimeout) && | ||
| !errors.Is(e, &StatelessResetError{}) && !errors.Is(e, &VersionNegotiationError{}) && | ||
| !errors.Is(e, &errCloseForRecreating{}) && | ||
| !errors.Is(e, &qerr.ApplicationError{}) && !errors.Is(e, &qerr.TransportError{}) { |
There was a problem hiding this comment.
Hmm, can we not use a type switch here? I get that As is meant to peek at the error's layers, but I think Is isn't meant to do that - so just doing a type switch on the top-level error might be fine.
There was a problem hiding this comment.
A type switch with 7 empty cases and one default case that has some logic? Maybe that's at least a little bit nicer?
There was a problem hiding this comment.
Indeed, I'd find that nicer. You can even group the cases, like:
switch e.(type) {
case T1, T2, T3...:
default:
// your logic
}
There was a problem hiding this comment.
I get that As is meant to peek at the error's layers, but I think Is isn't meant to do that
I think it is. From the docs:
Is reports whether any error in err's chain matches target.
Now we shouldn't encounter any wrapped errors here (at least not with the current code), but I guess it's still safer to use errors.Is.
From the PR description you wrote, it sounds fine. |
marten-seemann
force-pushed
the
error-returns
branch
from
5578482
to
3af0597
Compare
Fixes #2441.
This is a massive refactor changing the way we return QUIC errors. Calls to the session (and to streams) can now different errors, which can be checked with
errors.Is/errors.As:quic.TransportError: for errors triggered by the QUIC transport (in many cases a misbehaving peer)quic.ApplicationError: for errors triggered by the application running on top of QUICquic.IdleTimeoutError: when the peer goes away unexpectedly (this is anet.Errortimeout error)quic.HandshakeTimeoutError: when the cryptographic handshake takes too long (this is anet.Errortimeout error)quic.StatelessResetError: when we receive a stateless reset (this is anet.Errortemporary error)quic.VersionNegotiationError: returned by the client, when there's no version overlap between the peersIn addition to that, streams that are canceled return a:
quic.StreamError: both for local and remote cancellations@mvdan, what do you think about this? Do the API changes here make sense to you?