8000 fix(internal/gensupport): update shouldRetry for GCS uploads (#2634) · googleapis/google-api-go-client@ea513cb · GitHub
[go: up one dir, main page]
Skip to content

Commit ea513cb

Browse files
tritonetritone
authored
fix(internal/gensupport): update shouldRetry for GCS uploads (#2634)
* fix(storage): update shouldRetry for GCS uploads A few fixes to shouldRetry, which is used only for storage uploads. * Remove attempt to unwrap syscall errors and use string matching instead, as is used in the veneer client layer for storage. * Use errors.Is and appropriate sentinel for net.ErrClosed. * Add broken pipe error, which is causing flakes in veneer layer tests (see googleapis/google-cloud-go#10374 * Add unit test for shouldRetry. Closes googleapis/google-cloud-go#9178 * don't use xerrors
1 parent 66c2e4a commit ea513cb

File tree

3 files changed

+115
-32
lines changed

3 files changed

+115
-32
lines changed

‎internal/gensupport/retry.go

Lines changed: 18 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"errors"
99
"io"
1010
"net"
11+
"net/url"
1112
"strings"
1213
"time"
1314

@@ -29,8 +30,6 @@ var (
2930
backoff = func() Backoff {
3031
return &gax.Backoff{Initial: 100 * time.Millisecond}
3132
}
32-
// syscallRetryable is a platform-specific hook, specified in retryable_linux.go
33-
syscallRetryable func(error) bool = func(err error) bool { return false }
3433
)
3534

3635
const (
@@ -56,30 +55,33 @@ func shouldRetry(status int, err error) bool {
5655
if status == statusTooManyRequests || status == statusRequestTimeout {
5756
return true
5857
}
59-
if err == io.ErrUnexpectedEOF {
58+
if errors.Is(err, io.ErrUnexpectedEOF) {
6059
return true
6160
}
62-
// Transient network errors should be retried.
63-
if syscallRetryable(err) {
61+
if errors.Is(err, net.ErrClosed) {
6462
return true
6563
}
66-
if err, ok := err.(interface{ Temporary() bool }); ok {
67-
if err.Temporary() {
68-
return true
64+
switch e := err.(type) {
65+
case *net.OpError, *url.Error:
66+
// Retry socket-level errors ECONNREFUSED and ECONNRESET (from syscall).
67+
// Unfortunately the error type is unexported, so we resort to string
68+
// matching.
69+
retriable := []string{"connection refused", "connection reset", "broken pipe"}
70+
for _, s := range retriable {
71+
if strings.Contains(e.Error(), s) {
72+
return true
73+
}
6974
}
70-
}
71-
var opErr *net.OpError
72-
if errors.As(err, &opErr) {
73-
if strings.Contains(opErr.Error(), "use of closed network connection") {
74-
// TODO: check against net.ErrClosed (go 1.16+) instead of string
75+
case interface{ Temporary() bool }:
76+
if e.Temporary() {
7577
return true
7678
}
7779
}
7880

79-
// If Go 1.13 error unwrapping is available, use this to examine wrapped
81+
// If error unwrapping is available, use this to examine wrapped
8082
// errors.
81-
if err, ok := err.(interface{ Unwrap() error }); ok {
82-
return shouldRetry(status, err.Unwrap())
83+
if e, ok := err.(interface{ Unwrap() error }); ok {
84+
return shouldRetry(status, e.Unwrap())
8385
}
8486
return false
8587
}

‎internal/gensupport/retry_test.go

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
// Copyright 2024 Google LLC.
2+
// Use of this source code is governed by a BSD-style
3+
// license that can be found in the LICENSE file.
4+
5+
package gensupport
6+
7+
import (
8+
"errors"
9+
"fmt"
10+
"io"
11+
"net"
12+
"net/url"
13+
"testing"
14+
)
15+
16+
func TestShouldRetry(t *testing.T) {
17+
t.Parallel()
18+
19+
for _, test := range []struct {
20+
desc string
21+
code int
22+
inputErr error
23+
shouldRetry bool
24+
}{
25+
{
26+
desc: "nil error",
27+
inputErr: nil,
28+
shouldRetry: false,
29+
},
30+
{
31+
desc: "429 error",
32+
inputErr: fmt.Errorf("too many requests"),
33+
code: 429,
34+
shouldRetry: true,
35+
},
36+
{
37+
desc: "408 error",
38+
inputErr: fmt.Errorf("request timed out"),
39+
code: 408,
40+
shouldRetry: true,
41+
},
42+
{
43+
desc: "unknown error",
44+
inputErr: errors.New("foo"),
45+
shouldRetry: false,
46+
},
47+
{
48+
desc: "503 error",
49+
inputErr: fmt.Errorf("service unavailable"),
50+
code: 503,
51+
shouldRetry: true,
52+
},
53+
{
54+
desc: "403 error",
55+
inputErr: fmt.Errorf("forbidden"),
56+
code: 403,
57+
shouldRetry: false,
58+
},
59+
{
60+
desc: "connection refused",
61+
inputErr: &url.Error{Op: "blah", URL: "blah", Err: errors.New("connection refused")},
62+
shouldRetry: true,
63+
},
64+
{
65+
desc: "connection reset",
66+
inputErr: &net.OpError{Op: "blah", Net: "tcp", Err: errors.New("connection reset by peer")},
67+
shouldRetry: true,
68+
},
69+
{
70+
desc: "io.ErrUnexpectedEOF",
71+
inputErr: io.ErrUnexpectedEOF,
72+
shouldRetry: true,
73+
},
74+
{
75+
desc: "wrapped retryable error",
76+
inputErr: fmt.Errorf("Test unwrapping of a temporary error: %w", io.ErrUnexpectedEOF),
77+
shouldRetry: true,
78+
},
79+
{
80+
desc: "wrapped non-retryable error",
81+
inputErr: fmt.Errorf("Test unwrapping of a non-retriable error: %w", io.EOF),
82+
shouldRetry: false,
83+
},
84+
{
85+
desc: "wrapped net.ErrClosed",
86+
inputErr: &net.OpError{Err: net.ErrClosed},
87+
shouldRetry: true,
88+
},
89+
} {
90+
t.Run(test.desc, func(s *testing.T) {
91+
got := shouldRetry(test.code, test.inputErr)
92+
if got != test.shouldRetry {
93+
s.Errorf("got %v, want %v", got, test.shouldRetry)
94+
}
95+
})
96+
}
97+
}

‎internal/gensupport/retryable_linux.go

Lines changed: 0 additions & 16 deletions
This file was deleted.

0 commit comments

Comments
 (0)
0