From 091b7c6c02b83cb51ea5102e3ca838fbac4661b0 Mon Sep 17 00:00:00 2001 From: lhchavez Date: Tue, 21 Dec 2021 03:39:21 +0000 Subject: [PATCH 1/3] fix: Make SendRequestWithRetry check for canceled contexts twice This change makes it deterministic to call SendRequestWithRetry with a canceled context. This is going to be useful because some Cloud APIs rely on the context being canceled to prevent some operations from proceeding, like [`storage.(*ObjectHandle).NewWriter`](https://pkg.go.dev/cloud.google.com/go/storage#ObjectHandle.NewWriter). Fixes: #1358 --- internal/gensupport/send.go | 13 +++++++++++++ internal/gensupport/send_test.go | 22 ++++++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/internal/gensupport/send.go b/internal/gensupport/send.go index dab64aef367..2eff614f5d7 100644 --- a/internal/gensupport/send.go +++ b/internal/gensupport/send.go @@ -99,6 +99,19 @@ func sendAndRetry(ctx context.Context, client *http.Client, req *http.Request, r case <-time.After(pause): } + select { + case <-ctx.Done(): + // Check for context cancellation once more. If more than one case in a + // select is satisfied at the same time, Go will choose one arbitrarily. + // That can cause an operation to go through even if the context was + // canceled before. + if err == nil { + err = ctx.Err() + } + return resp, err + default: + } + resp, err = client.Do(req.WithContext(ctx)) var status int diff --git a/internal/gensupport/send_test.go b/internal/gensupport/send_test.go index d6af483e66e..52c8ad09c36 100644 --- a/internal/gensupport/send_test.go +++ b/internal/gensupport/send_test.go @@ -6,6 +6,7 @@ package gensupport import ( "context" + "errors" "net/http" "testing" ) @@ -29,3 +30,24 @@ func TestSendRequestWithRetry(t *testing.T) { t.Error("got nil, want error") } } + +type brokenRoundTripper struct{} + +func (t *brokenRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { + return nil, errors.New("this should not happen") +} + +func TestCanceledContextDoesNotPerformRequest(t *testing.T) { + client := http.Client{ + Transport: &brokenRoundTripper{}, + } + for i := 0; i < 1000; i++ { + req, _ := http.NewRequest("GET", "url", nil) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + _, err := SendRequestWithRetry(ctx, &client, req) + if !errors.Is(err, context.Canceled) { + t.Fatalf("got %v, want %v", err, context.Canceled) + } + } +} From b873c7460b3f548e992052ce6fdd364919b14397 Mon Sep 17 00:00:00 2001 From: lhchavez Date: Tue, 21 Dec 2021 17:10:38 +0000 Subject: [PATCH 2/3] ctx.Err() is idempotent, let's use it instead --- go.mod | 1 + internal/gensupport/send.go | 4 +--- internal/gensupport/send_test.go | 9 +++++---- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/go.mod b/go.mod index 402d3bf2662..93115f2d30e 100644 --- a/go.mod +++ b/go.mod @@ -12,6 +12,7 @@ require ( golang.org/x/oauth2 v0.0.0-20211104180415-d3ed0bb246c8 golang.org/x/sync v0.0.0-20210220032951-036812b2e83c golang.org/x/sys v0.0.0-20211210111614-af8b64212486 + golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 // indirect google.golang.org/appengine v1.6.7 google.golang.org/genproto v0.0.0-20211208223120-3a66f561d7aa google.golang.org/grpc v1.40.1 diff --git a/internal/gensupport/send.go b/internal/gensupport/send.go index 2eff614f5d7..dd50cc20a58 100644 --- a/internal/gensupport/send.go +++ b/internal/gensupport/send.go @@ -99,8 +99,7 @@ func sendAndRetry(ctx context.Context, client *http.Client, req *http.Request, r case <-time.After(pause): } - select { - case <-ctx.Done(): + if ctx.Err() != nil { // Check for context cancellation once more. If more than one case in a // select is satisfied at the same time, Go will choose one arbitrarily. // That can cause an operation to go through even if the context was @@ -109,7 +108,6 @@ func sendAndRetry(ctx context.Context, client *http.Client, req *http.Request, r err = ctx.Err() } return resp, err - default: } resp, err = client.Do(req.WithContext(ctx)) diff --git a/internal/gensupport/send_test.go b/internal/gensupport/send_test.go index 52c8ad09c36..b4ce4cfde2f 100644 --- a/internal/gensupport/send_test.go +++ b/internal/gensupport/send_test.go @@ -6,9 +6,10 @@ package gensupport import ( "context" - "errors" "net/http" "testing" + + "golang.org/x/xerrors" ) func TestSendRequest(t *testing.T) { @@ -34,7 +35,7 @@ func TestSendRequestWithRetry(t *testing.T) { type brokenRoundTripper struct{} func (t *brokenRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { - return nil, errors.New("this should not happen") + return nil, xerrors.New("this should not happen") } func TestCanceledContextDoesNotPerformRequest(t *testing.T) { @@ -45,8 +46,8 @@ func TestCanceledContextDoesNotPerformRequest(t *testing.T) { req, _ := http.NewRequest("GET", "url", nil) ctx, cancel := context.WithCancel(context.Background()) cancel() - _, err := SendRequestWithRetry(ctx, &client, req) - if !errors.Is(err, context.Canceled) { + _, err := SendRequestWithRetry(ctx, &client, req, nil) + if !xerrors.Is(err, context.Canceled) { t.Fatalf("got %v, want %v", err, context.Canceled) } } From b61f9b58cb8e8d31fd10810dc7d2e86bd6de458e Mon Sep 17 00:00:00 2001 From: lhchavez Date: Tue, 21 Dec 2021 17:51:15 +0000 Subject: [PATCH 3/3] Tidy up those mods Unsure why `go get` didn't to it by itself :/ --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index 93115f2d30e..eda5236aa67 100644 --- a/go.mod +++ b/go.mod @@ -12,7 +12,7 @@ require ( golang.org/x/oauth2 v0.0.0-20211104180415-d3ed0bb246c8 golang.org/x/sync v0.0.0-20210220032951-036812b2e83c golang.org/x/sys v0.0.0-20211210111614-af8b64212486 - golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 // indirect + golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 google.golang.org/appengine v1.6.7 google.golang.org/genproto v0.0.0-20211208223120-3a66f561d7aa google.golang.org/grpc v1.40.1