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

Jump to bottom

sendAndRetry(): context error is not being returned when context is canceled #1685

Closed
matheuscscp opened this issue Sep 2, 2022 · 0 comments · Fixed by #1684
Closed

sendAndRetry(): context error is not being returned when context is canceled #1685

matheuscscp opened this issue Sep 2, 2022 · 0 comments · Fixed by #1684
Assignees
@quartzmo @tritone
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@matheuscscp
Copy link
Contributor

Environment details

  • Programming language: Go
  • OS: Linux
  • Language runtime version: 1.18.5
  • Package version: google.golang.org/api v0.66.0 and cloud.google.com/go/storage v1.19.0

Steps to reproduce

  1. Run an idempotent operation like this (creating a file in GCS):

bkt.Object(dstPath).If(storage.Conditions{DoesNotExist: true}).NewWriter(ctx).Write(b)

  1. Make the server return a 503, causing the underlying call of sendAndRetry() to fail and block until it's time for a second attempt.
  2. Cancel ctx before the second attempt.
  3. Observe that the error returned by .Write() will contain the 503 error message but not context.Canceled.

This is misleading because it makes it look like the error that caused the operation to fail and return was the 503 error from the server, but what actually caused the operation to fail and return was my code canceling the context. This is confusing because the docs tell you that the library will retry idempotent operations upon 503, so it's really weird seeing the operation return the 503 error.
@matheuscscp matheuscscp added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Sep 2, 2022
@matheuscscp matheuscscp mentioned this issue Sep 2, 2022
Merged
@codyoss codyoss assigned quartzmo and unassigned codyoss Sep 6, 2022
tritone pushed a commit that referenced this issue Sep 20, 2022
@matheuscscp
feat(internal/gensupport): wrap retry failures with context and prev …
f427ee3 
…error (#1684)

Previously, if a call to sendAndRetry ran with retries until the context deadline, the error returned would be the final error from the service rather than the context expiration error. This change uses error wrapping to wrap the error from the service, while also preserving the context error in the message and with errors.Is.

Fixes #1685
@release-please release-please bot mentioned this issue Sep 20, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@quartzmo quartzmo

@tritone tritone

Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Milestone
No milestone
4 participants
@quartzmo @matheuscscp @tritone @codyoss