net/http/httputil: ReverseProxy incompatible with http.Transport retries

[glasser]

http://golang.org/cl/3210 by @bgentry (go 1.6) introduced code in http.Transport to retry certain replayable requests in certain error cases. http://golang.org/cl/23160 by @bradfitz recently refactored this code, but not in a way relevant to the issue I am reporting.

This logic introduced a Request's isReplayable method, which is only true if r.Body == nil (and the method is GET, HEAD, etc).

httputil.ReverseProxy creates the outgoing request by making a copy of the incoming request, which is probably a server request and which thus is very likely to have non-nil Body. It then overwrites the outgoing request's Body with a new body in a the common case where request cancellation is possible.

Thus, if you use ReverseProxy to send requests over an http.Transport, you do not get the benefit of http://golang.org/cl/3210 because the outgoing request's Body is not nil.

Would there be a downside to changing isReplayable to check for r.ContentLength == 0 instead of r.Body == nil? Happy to send a CL if this is reasonable.

(We are working around this in our project by setting r.Body to nil if ContentLength is 0 before passing the request to ReverseProxy's ServeHTTP, but that only happens to work because we we have an intermediate RoundTripper between the ReverseProxy and the http.Transport which does not have a CancelRequest method.)

[glasser]

Ah, hmm, my suggested fix wouldn't actually work because ContentLength 0 means something different in client Requests. So maybe this could be a specific fix to ReverseProxy: if incoming ContentLength is 0, set Body to nil and don't do the CancelRequest stuff at all? (I don't understand why the request canceler goroutine happens at the beginning of reading the Body specifically.)

[glasser]

Note that the http/2 implementation of RoundTrip does have explicit code to work around this; maybe the http/1 implementation should too? https://github.com/golang/net/blob/3f122ce3dbbe488b7e6a8bdb26f41edec852a40b/http2/transport.go#L624-L639

[bradfitz]

Related: #13722

[bradfitz]

@glasser,

I don't understand why the request canceler goroutine happens at the beginning of reading the Body specifically.

I don't remember either. That was added in ececbe8 from @pwaller for #8406. There's probably some history in https://golang.org/cl/2320 which I'll have to read.

Or maybe I'll just convert it to use context.Context and see whether some test breaks.

[gopherbot]

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