Skip to content

Commit

Permalink
fix(internal/gensupport): check ctx in chunk retry (#1364)
Browse files Browse the repository at this point in the history
The select statement is non-deterministic, so currently, if the
pause is completed and ALSO the context has been canceled or
timeout elapsed, a request may still occur. This PR prevents that
circumstance from occurring.

Also removed something in a test that seems to be a workaround for
the race condition.

Inspired by #1359 & #1358.
  • Loading branch information
tritone authored Jan 6, 2022
1 parent 5063cf3 commit e10082d
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 3 deletions.
16 changes: 16 additions & 0 deletions internal/gensupport/resumable.go
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,22 @@ func (rx *ResumableUpload) Upload(ctx context.Context) (resp *http.Response, err
return prepareReturn(resp, err)
}

// Check for context cancellation or timeout once more. If more than one
// case in the select statement above was 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 or the timeout was reached.
select {
case <-ctx.Done():
if err == nil {
err = ctx.Err()
}
return prepareReturn(resp, err)
case <-quitAfter:
return prepareReturn(resp, err)
default:
}

resp, err = rx.transferChunk(ctx)

var status int
Expand Down
3 changes: 0 additions & 3 deletions internal/gensupport/resumable_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -213,9 +213,6 @@ func TestCancelUploadFast(t *testing.T) {

tr := &interruptibleTransport{
buf: make([]byte, 0, mediaSize),
// Shouldn't really need an event, but sometimes the test loses the
// race. So, this is just a filler event.
events: []event{{"bytes 0-9/*", http.StatusServiceUnavailable}},
}

pr := progressRecorder{}
Expand Down

0 comments on commit e10082d

Please sign in to comment.