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

net/http: also make Transport retry POST request if no bytes were written #15723

Closed
bradfitz opened this issue May 17, 2016 · 4 comments
Closed

Comments

@bradfitz
Copy link
Contributor

We should retry non-idempotent HTTP requests (like POSTs) too if no bytes were ever written.

A fix for #15446 adds tracking of the number of bytes written. This makes this trivial to fix, but it's too late for Go 1.7.

Easy to switch early in Go 1.8. Could also use a few more tests.

@bradfitz bradfitz added this to the Go1.8Early milestone May 17, 2016
@bradfitz bradfitz self-assigned this May 17, 2016
@bradfitz
Copy link
Contributor Author

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/23160 mentions this issue.

gopherbot pushed a commit that referenced this issue May 18, 2016
In https://golang.org/3210, Transport errors occurring before
receiving response headers were wrapped in another error type to
indicate to the retry logic elsewhere that the request might be
re-tryable. But a check for err == io.EOF was missed, which then became
false once io.EOF was wrapped in the beforeRespHeaderError type.

The beforeRespHeaderError was too fragile. Remove it. I tried to fix
it in an earlier version of this CL and just broke different things
instead.

Also remove the "markBroken" method. It's redundant and confusing.

Also, rename the checkTransportResend method to shouldRetryRequest and
make it return a bool instead of an error. This also helps readability.

Now the code recognizes the two main reasons we'd want to retry a
request: because we never wrote the request in the first place (so:
count the number of bytes we've written), or because the server hung
up on us before we received response headers for an idempotent request.

As an added bonus, this could make POST requests safely re-tryable
since we know we haven't written anything yet. But it's too late in Go
1.7 to enable that, so we'll do that later (filed #15723).

This also adds a new internal (package http) test, since testing this
blackbox at higher levels in transport_test wasn't possible.

Fixes #15446

Change-Id: I2c1dc03b1f1ebdf3f04eba81792bd5c4fb6b6b66
Reviewed-on: https://go-review.googlesource.com/23160
Reviewed-by: Andrew Gerrand <[email protected]>
Run-TryBot: Brad Fitzpatrick <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
@gopherbot
Copy link
Contributor

CL https://golang.org/cl/27117 mentions this issue.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/34134 mentions this issue.

gopherbot pushed a commit that referenced this issue Dec 8, 2016
This rolls back https://golang.org/cl/27117 partly, softening it so it
only retries POST/PUT/DELETE etc requests where there's no Body (nil
or NoBody). This is a little useless, since most idempotent requests
have a body (except maybe DELETE), but it's late in the Go 1.8 release
cycle and I want to do the proper fix.

The proper fix will look like what we did for http2 and only retrying
the request if Request.GetBody is defined, and then creating a new request
for the next attempt. See https://golang.org/cl/33971 for the http2 fix.

Updates #15723
Fixes #18239
Updates #18241

Change-Id: I6ebaa1fd9b19b5ccb23c8d9e7b3b236e71cf57f3
Reviewed-on: https://go-review.googlesource.com/34134
Run-TryBot: Brad Fitzpatrick <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Tom Bergan <[email protected]>
@golang golang locked and limited conversation to collaborators Dec 8, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants