net/http: redirection response has closed body

[JamesDunne]

1. What version of Go are you using (go version)?
   go version go1.4.2 darwin/amd64
2. What operating system and processor architecture are you using?
   Not really relevant to this issue...
   MacBook Pro (15-inch, Mid 2012)
   2.6 GHz Intel Core i7
   16 GB 1600 MHz DDR3
3. What did you do?
   Implemented a custom redirection policy handler via http.Client's CheckRedirect to stop redirection.
   After a Do() call on a request, the returned response's Body is not nil and reading from it returns an error http: read on closed response body. See example code below.
4. What did you expect to see?
   Expected either an unclosed, unread-from non-nil Body, or a nil Body.
5. What did you see instead?
   Saw a closed, already-read-from Body that was not nil.

This unexpected condition required me to hack around the case by explicitly setting resp.Body to nil when detecting my custom redirect policy abort error from the returned url.Error.

Also, I have no access to the intermediate redirection responses. I would want an interception function to be able to inspect the redirection response to see if I should redirect or not, but instead I get a request instance; what good is the request to determine if I should redirect? This design also prevents me from logging relevant redirection details to the end user, so as to properly trace the request-response path.

// Define our redirection policy:
const redirects_max_follow = 0 // for this test case
var redirectPolicyError = errors.New("redirect policy")
client := &http.Client{
    CheckRedirect: func(req *http.Request, via []*http.Request) error {
        if len(via) > redirects_max_follow {
            return redirectPolicyError
        }
        return nil
    },
}

// Make the request:
resp, err := client.Do(req)
if err != nil {
    if uerr, ok := err.(*url.Error); ok {
        if uerr.Err == redirectPolicyError {
            // Redirection responses get their bodies removed by `Do()` automatically.
            // Not sure if this is a bug or not.
            // Setting Body to nil prevents future "http: read on closed response body" error.
            resp.Body = nil
            goto ok
        }
    }
    Error("HTTP error: %s\n", err)
    return -2
ok:
}

[bradfitz]

Sorry, I'm just starting a two-week vacation now. I won't be able to look into this until the 19th or later. Maybe somebody else can in the meantime.

[JamesDunne]

No worries; it's easy to work around. I'm just curious if this was intended behavior or not. I can't quite decide if it's exactly a bug, so I figured I'd bring it up just in case it was.

[rsc]

Too late for Go 1.5.

[jshort]

Why is this body closed with a redirect? I'm writing a curl-like go executable and curl will, by default return a 301/302 response with applicable headers (Location, obviously) and the response body unless you tell it to follow. Currently I'm able to tell it to not follow the redirects by default, but the body is gone and I have to use the hack above to prevent my program from exiting.

[polomsky]

Is there any change in decision when this issue will be fixed? I had to make custom code around http.Client and http.Transport for block redirect and fetch response body of the redirect and i am not happy with the workaround.

[bradfitz]

Nobody looked at it during the Go 1.6 cycle, either, and I was only looking at the Go1.6 label (not Unplanned) myself. Let's tag it Go1.7 and then I will at least will see it when the 1.7 tree opens.

Others are more than welcome to send fixes too.

[gripedthumbtacks]

I am seeing similar issue. I also think the current required workaround is quite kludgy and really doesn't work for me. Ideally, there should be some intuitive mechanism to disable redirects entirely and allow parsing of the raw response. In my case, I want to simply reflect the server response AS IS back to the client that made the initial request (I have implemented a custom proxy while munging stuff in the middle). The workaround didn't seem to get me what I needed. I saw an HTTP 200 response with a Content-Length of 0 :( But it's also possible I am doing something very wrong as I am new to golang...

Edit: Just a note that I would expect golang net/http to function very much like curl or most browsers and currently it seems to be VERY different from that. I was incredibly surprised to see my custom headers disappear on redirect to the same server. That is not how your browser would respond. If you send a Cookie header to a server like http://example.com/home as a client, the client will still send that Cookie again following a 3xx redirect to http://example.com/home/defaultpage.html. I don't see why this wouldn't be the case with golang net/http.

[jbardin]

I was going to submit a patch, but need some clarification. The preconditions we have are:

• The Response after an error from CheckRedirect must be non-nil for go1 compatibility (issue #3795)
• We can't require closing the Response.Body, since there was an error, and the general practice is to discard the response altogether on errors.

Two possible solutions:

• Replace the Body with an empty, noop io.ReadCloser, so that a later Read or Close doesn't error, but we still discard the body.
• Read the response into a buffer (up to maxBodySlurpSize?), and replace the Body with that buffer.

[odeke-em]

In relation to #8633, CL https://go-review.googlesource.com/#/c/21364/ documented that during a redirection, on encountering a CheckRedirect error, the response is non-nil and its body is always closed. @bradfitz is that relevant to this issue?

[bradfitz]

@jbardin, sorry for the slow reply. Yes, we can't really change the old behavior. As @odeke-em pointed out, for #8633 I just documented the status quo in https://golang.org/cl/21364.

Some possibilities I see for this bug:

• make a new magic return value from CheckRedirect which causes the redirect to stop, and causes it to return (*Response, nil) with the Response.Body not closed or consumed.
• add a pointer to the Request type, pointing to the Response which caused it. This is somewhat consistent with the Response already having a pointer to its Request. It could be named Request.ForRedirect *Response, perhaps.
• add a new hook (i.e. CheckRedirect2, but with a better name) which provides the Response.

I think it's probably too late for major new API surface in Go 1.7, sadly, so I think the third option is out (and I don't like having two so-similar things anyway), but I think the Request.ForRedirect *Response field along with the magic return value from CheckRedirect might be sufficient.

@JamesDunne?

[JamesDunne]

That sounds good, combining your first two bullet points into a solution: magic return value from CheckRedirect and extra field added to Request to grab the RedirectReponse.

[gopherbot]

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