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: make Transport use GetBody, retry requests when no bytes were written #18241

Closed
bradfitz opened this issue Dec 8, 2016 · 5 comments

Comments

Projects
None yet
4 participants
@bradfitz
Copy link
Member

commented Dec 8, 2016

#15723 (https://golang.org/cl/27117) for Go 1.8 was too aggressive and buggy.

It was partially rolled back (see #18239).

This bug tracks fixing it properly for Go 1.9.

/cc @tombergan

@bradfitz bradfitz added the NeedsFix label Dec 8, 2016

@bradfitz bradfitz added this to the Go1.9 milestone Dec 8, 2016

@bradfitz bradfitz self-assigned this Dec 8, 2016

@gopherbot

This comment has been minimized.

Copy link

commented Dec 8, 2016

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

gopherbot pushed a commit that referenced this issue Dec 8, 2016

net/http: don't retry Transport requests if they have a body
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 <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Tom Bergan <tombergan@google.com>
@rogpeppe

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2016

I'm not convinced about GetBody as an abstraction - it's not natural when using some kinds of obviously-retriable bodies such as files. Why not just seek to the body start if it implements io.Seeker? (you'd probably want to make that behaviour opt-in rather than opt-out).

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented Dec 12, 2016

But since you need an opt-in anyway, this seems like a reasonable opt-in. And we can't change our previous Close behavior, so seeking back to the beginning of a closed file obviously isn't going to work. Plus the more experience I have with type sniffing magic, the more it bites me. I like the explicitness of GetBody. GetBody is not just for Transport retries on network failures, but also Client redirects, replaying the request body. Trying to reuse the same Body while coordinating across those two layers (not forgetting http2) and without adding new API surface seems like it'd be gross.

I'm not sure this is the best place to discuss GetBody, though.

@odeke-em

This comment has been minimized.

Copy link
Member

commented Apr 25, 2017

@bradfitz should we fold this issue into #17844? I ask since this one is more recent and no bytes written might be considered idempotency, so this one becomes a subset of the mentioned issue.

@gopherbot

This comment has been minimized.

Copy link

commented Apr 28, 2017

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.