From b85152e280306b87f77e3ecf7e805541ceaf9913 Mon Sep 17 00:00:00 2001
From: Chris Cotter <cjcotter@google.com>
Date: Tue, 5 May 2020 17:24:35 -0400
Subject: [PATCH] storage: add retry for transient network issues

Adds retry for connection reset and connection
refused errors for most operations (except for writes).

Splits invoke test into pre/post go1.10 versions in
order to not touch old code that is no longer covered
by CI.

Fixes #1638

Change-Id: Ie8f9857a49e1b4399b125dd9567db6d18b1d1dd8
Reviewed-on: https://code-review.googlesource.com/c/gocloud/+/55970
Reviewed-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Tyler Bui-Palsulich <tbp@google.com>
Reviewed-by: Frank Natividad <franknatividad@google.com>
---
 storage/go110.go                              | 18 +++++-
 storage/go110_test.go                         | 60 +++++++++++++++++++
 storage/{invoke_test.go => not_go110_test.go} |  2 +
 3 files changed, 79 insertions(+), 1 deletion(-)
 create mode 100644 storage/go110_test.go
 rename storage/{invoke_test.go => not_go110_test.go} (98%)

diff --git a/storage/go110.go b/storage/go110.go
index 206813f0cea4..c1273d59ade6 100644
--- a/storage/go110.go
+++ b/storage/go110.go
@@ -16,7 +16,12 @@
 
 package storage
 
-import "google.golang.org/api/googleapi"
+import (
+	"net/url"
+	"strings"
+
+	"google.golang.org/api/googleapi"
+)
 
 func shouldRetry(err error) bool {
 	switch e := err.(type) {
@@ -24,6 +29,17 @@ func shouldRetry(err error) bool {
 		// Retry on 429 and 5xx, according to
 		// https://cloud.google.com/storage/docs/exponential-backoff.
 		return e.Code == 429 || (e.Code >= 500 && e.Code < 600)
+	case *url.Error:
+		// Retry socket-level errors ECONNREFUSED and ENETUNREACH (from syscall).
+		// Unfortunately the error type is unexported, so we resort to string
+		// matching.
+		retriable := []string{"connection refused", "connection reset"}
+		for _, s := range retriable {
+			if strings.Contains(e.Error(), s) {
+				return true
+			}
+		}
+		return false
 	case interface{ Temporary() bool }:
 		return e.Temporary()
 	default:
diff --git a/storage/go110_test.go b/storage/go110_test.go
new file mode 100644
index 000000000000..f1cf9bb10624
--- /dev/null
+++ b/storage/go110_test.go
@@ -0,0 +1,60 @@
+// Copyright 2020 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// +build go1.10
+
+package storage
+
+import (
+	"context"
+	"errors"
+	"net/url"
+	"testing"
+
+	"google.golang.org/api/googleapi"
+)
+
+func TestInvoke(t *testing.T) {
+	t.Parallel()
+	ctx := context.Background()
+	// Time-based tests are flaky. We just make sure that invoke eventually
+	// returns with the right error.
+
+	for _, test := range []struct {
+		count      int   // Number of times to return retryable error.
+		initialErr error // Error to return initially.
+		finalErr   error // Error to return after count returns of retryCode.
+	}{
+		{0, &googleapi.Error{Code: 0}, nil},
+		{0, &googleapi.Error{Code: 0}, errors.New("foo")},
+		{1, &googleapi.Error{Code: 429}, nil},
+		{1, &googleapi.Error{Code: 429}, errors.New("bar")},
+		{2, &googleapi.Error{Code: 518}, nil},
+		{2, &googleapi.Error{Code: 599}, &googleapi.Error{Code: 428}},
+		{1, &url.Error{Op: "blah", URL: "blah", Err: errors.New("connection refused")}, nil},
+	} {
+		counter := 0
+		call := func() error {
+			counter++
+			if counter <= test.count {
+				return test.initialErr
+			}
+			return test.finalErr
+		}
+		got := runWithRetry(ctx, call)
+		if got != test.finalErr {
+			t.Errorf("%+v: got %v, want %v", test, got, test.finalErr)
+		}
+	}
+}
diff --git a/storage/invoke_test.go b/storage/not_go110_test.go
similarity index 98%
rename from storage/invoke_test.go
rename to storage/not_go110_test.go
index c24e10ffc8dc..8db49dc6d4d2 100644
--- a/storage/invoke_test.go
+++ b/storage/not_go110_test.go
@@ -12,6 +12,8 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+// +build !go1.10
+
 package storage
 
 import (