Skip to content

Commit

Permalink
fix: Make SendRequestWithRetry check for canceled contexts twice
Browse files Browse the repository at this point in the history
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: googleapis#1358
  • Loading branch information
lhchavez committed Dec 21, 2021
1 parent 8fed2c8 commit 091b7c6
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 0 deletions.
13 changes: 13 additions & 0 deletions internal/gensupport/send.go
Expand Up @@ -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
Expand Down
22 changes: 22 additions & 0 deletions internal/gensupport/send_test.go
Expand Up @@ -6,6 +6,7 @@ package gensupport

import (
"context"
"errors"
"net/http"
"testing"
)
Expand All @@ -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)
}
}
}

0 comments on commit 091b7c6

Please sign in to comment.