Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(internal/gensupport): Make SendRequestWithRetry check for canceled contexts twice #1359

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions go.mod
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
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
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)
}
}
}