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

Do not retry requests without GetBody #3299

Merged
merged 1 commit into from Mar 25, 2020

Conversation

emilymye
Copy link
Contributor

@emilymye emilymye commented Mar 25, 2020

Now, if a request doesn't have GetBody (to copy the request), the request is treated as nonretryable and just sent once as is.

Fixes hashicorp/terraform-provider-google#5970

Release Note Template for Downstream PRs (will be copied)

provider: Fixed an error with resources failing to upload large files (e.g. with `google_storage_bucket_object`) during retried requests

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 2 files changed, 61 insertions(+), 16 deletions(-))
Terraform Beta: Diff ( 2 files changed, 61 insertions(+), 16 deletions(-))
TF Conversion: Diff ( 1 file changed, 16 insertions(+), 16 deletions(-))

// consume here. Since this won't affect the request itself,
// we do this before the actual Retry loop so we can consume the request Body as needed
// e.g. if the request couldn't be retried, we use the original request
if _, err := httputil.DumpRequestOut(req, true); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will cause an error for non-retried requests I believe?

DumpRequestOut consumes the request body of req which we later try to send directly in some cases: resp, respErr = t.internal.RoundTrip(req)

Wouldn't sending that request fail if the body has already been consumed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no - DumpRequestOut consumes the body bytes, reading it into memory, and then replaces the request body with an Reader with the same bytes, so the actual request isn't really changed.

I am conflicted though - originally I wanted to avoid doing the dump to copy the request, since a) it creates copy in memory and b) the copy created by httputil isn't supposed to be used to actually send again, and while it might work for now, it's not clear whether that'll be true in the future. That does mean, instead of the GetBody error, we could do the copy-bytes approach if GetBody isn't given?

I'm leaning towards keeping this as is (i.e. with the changes in this PR), since it seemed like the storage/googleapi libraries purposely doesn't set GetBody if the request isn't retryable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we at least dump the request after we copy/send it so that the original body is sent rather than the copy?

Yeah, I would say that since we are seeing this error come up with non-retryable requests (uploading large files) that we should assume that if GetBody isn't set we shouldn't retry.

@emilymye emilymye requested a review from slevenick March 25, 2020 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants