client: fail fast on non-retryable errors #10875
Conversation
The snap client would always retry requests for a long time, regardless of error type (e.g., snap list retries for 2min, if it fails to read auth.json). This commit prevents retries caused by internal and auth errors.
client/client.go
Outdated
| @@ -200,6 +206,17 @@ func (e ConnectionError) Unwrap() error { | |||
| return e.Err | |||
| } | |||
|
|
|||
| type InternalError struct{ Err error } | |||
There was a problem hiding this comment.
I like this usage of the new errors.Is etc a lot, but I'm not sure on the name, InternalError is very vague, and it's also not clear which component the error is "internal to", maybe instead this could be ClientInternalError to make it clear this type of error is only for internal errors on the client side and not i.e. Internal Server Error 500 from the snapd
There was a problem hiding this comment.
I see your point, I renamed it so it's clearer
client/client.go
Outdated
| func notRetryable(err error) bool { | ||
| return errors.Is(err, AuthorizationError{}) || | ||
| errors.Is(err, InternalError{}) | ||
| } | ||
|
|
Codecov Report
@@ Coverage Diff @@
## master #10875 +/- ##
==========================================
+ Coverage 78.29% 78.31% +0.02%
==========================================
Files 900 901 +1
Lines 102217 102286 +69
==========================================
+ Hits 80032 80107 +75
+ Misses 17193 17186 -7
- Partials 4992 4993 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
client/client.go
Outdated
| @@ -396,6 +413,11 @@ func (client *Client) do(method, path string, query url.Values, headers map[stri | |||
| return rsp.StatusCode, nil | |||
| } | |||
|
|
|||
| func notRetryable(err error) bool { | |||
There was a problem hiding this comment.
For the sake of a little bikeshedding ;), I wonder if it would make sense to reverse the semantics here and follow the naming of the existing ShouldRetryError(err) bool from httputil, this would make it more easy to find all these helpers later.
There was a problem hiding this comment.
I opted not to do that because there were two options:
a) have the function determine which errors are retryable instead of which are not retryable. If we do that and a new error is added later or I forget to include one now, then it's not retried (which is worse than unnecessarily retrying).
b) write the function in the negative (like: !(errors.Is(err, AuthorizationError{}) || errors.Is(err, InternalClientError{}))) and then again reverse the result again when calling. It's safer but it seemed more convoluted than just changing the function name.
But I can change the naming to "shouldNotRetryError", to make it consistent with existing code
| @@ -200,6 +206,17 @@ func (e ConnectionError) Unwrap() error { | |||
| return e.Err | |||
| } | |||
|
|
|||
| type InternalClientError struct{ Err error } | |||
|
|
|||
| func (e InternalClientError) Error() string { | |||
There was a problem hiding this comment.
I was thinking that maybe we should use pointer receivers, but I see that there's a couple of other errors here that already use values in Error(), so it's probably not worth it.
The snap client retries all requests for a long time, regardless of the kind of error (e.g., snap list retries for 2min, if it fails to read auth.json). This is unnecessary and a bit unexpected since not all errors are transient so it obfuscates the error a bit. This commit prevents internal and auth errors from retrying since they're probably not transient.
You can test this by running snap list or version with an incorrect auth.json file, like:
touch ~/.snap/auth.json; snap list(assuming you don't already have one).