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

Conversation

lhchavez
Copy link
Contributor

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

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
@lhchavez lhchavez requested review from yoshi-approver and a team as code owners December 21, 2021 03:43
Copy link
Member

@codyoss codyoss left a 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.

internal/gensupport/send_test.go Outdated Show resolved Hide resolved
internal/gensupport/send.go Outdated Show resolved Hide resolved
@codyoss codyoss changed the title fix: Make SendRequestWithRetry check for canceled contexts twice fix(internal/gapicgen): Make SendRequestWithRetry check for canceled contexts twice Dec 21, 2021
@codyoss codyoss requested a review from tritone December 21, 2021 17:01
@codyoss codyoss changed the title fix(internal/gapicgen): Make SendRequestWithRetry check for canceled contexts twice fix(internal/gensupport): Make SendRequestWithRetry check for canceled contexts twice Dec 21, 2021
Copy link
Contributor

@tritone tritone left a 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!

@tritone tritone added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 21, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 21, 2021
@tritone
Copy link
Contributor

tritone commented Dec 21, 2021

The presubmit is failing but if you run go mod tidy that should fix it. @lhchavez

Unsure why `go get` didn't to it by itself :/
@lhchavez
Copy link
Contributor Author

The presubmit is failing but if you run go mod tidy that should fix it. @lhchavez

tidied up!

@tritone tritone added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 21, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 21, 2021
Copy link
Member

@codyoss codyoss left a 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!

@codyoss codyoss merged commit 520b227 into googleapis:main Dec 21, 2021
tritone added a commit to tritone/google-api-go-client that referenced this pull request Dec 22, 2021
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.
@lhchavez lhchavez deleted the fix-spurious-send-request-with-write-calls branch December 27, 2021 14:47
tritone added a commit that referenced this pull request Jan 6, 2022
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

internal/gensupport: SendRequestWithRetry sometimes spuriously makes requests when its context was canceled
4 participants