-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
fix(internal/gensupport): Make SendRequestWithRetry check for canceled contexts twice #1359
Conversation
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR and the detailed issue. Just a couple of small things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch on this. Thanks for the fix!
The presubmit is failing but if you run |
Unsure why `go get` didn't to it by itself :/
tidied up! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, look good to me!
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 googleapis#1359 & googleapis#1358.
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.
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
.Fixes: #1358