Skip to content

Commit

Permalink
fix(internal/gensupport): Make SendRequestWithRetry check for cancele…
Browse files Browse the repository at this point in the history
…d contexts twice (#1359)

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
  • Loading branch information
lhchavez authored Dec 21, 2021
1 parent 07d8e2c commit 520b227
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 0 deletions.
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -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
google.golang.org/appengine v1.6.7
google.golang.org/genproto v0.0.0-20211208223120-3a66f561d7aa
google.golang.org/grpc v1.40.1
Expand Down
11 changes: 11 additions & 0 deletions internal/gensupport/send.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,17 @@ func sendAndRetry(ctx context.Context, client *http.Client, req *http.Request, r
case <-time.After(pause):
}

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
// canceled before.
if err == nil {
err = ctx.Err()
}
return resp, err
}

resp, err = client.Do(req.WithContext(ctx))

var status int
Expand Down
23 changes: 23 additions & 0 deletions internal/gensupport/send_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import (
"context"
"net/http"
"testing"

"golang.org/x/xerrors"
)

func TestSendRequest(t *testing.T) {
Expand All @@ -29,3 +31,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, xerrors.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, nil)
if !xerrors.Is(err, context.Canceled) {
t.Fatalf("got %v, want %v", err, context.Canceled)
}
}
}

0 comments on commit 520b227

Please sign in to comment.