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

Jump to bottom

net/http: redirection response has closed body #10069

Closed
JamesDunne opened this issue Mar 3, 2015 · 12 comments
Closed

net/http: redirection response has closed body #10069

JamesDunne opened this issue Mar 3, 2015 · 12 comments
Labels
FrozenDueToAge
Milestone
Go1.7

Comments

@JamesDunne
Copy link

  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
Copy link
Contributor

bradfitz commented Mar 4, 2015

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
Copy link
Author

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 rsc added this to the Go1.5Maybe milestone Apr 10, 2015
@rsc
Copy link
Contributor

rsc commented Jun 29, 2015

Too late for Go 1.5.

@rsc rsc modified the milestones: Unplanned, Go1.5Maybe Jun 29, 2015
@jshort
Copy link

jshort commented Sep 18, 2015

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
Copy link

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
Copy link
Contributor

bradfitz commented Feb 1, 2016

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.

@bradfitz bradfitz modified the milestones: Go1.7, Unplanned Feb 1, 2016
@gripedthumbtacks
Copy link

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
Copy link
Contributor

jbardin commented Mar 8, 2016

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

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
Copy link
Member

odeke-em commented Apr 8, 2016

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
Copy link
Contributor

@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
Copy link
Author

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
Copy link

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

@jasonrichardsmith jasonrichardsmith mentioned this issue Jun 27, 2016
Closed
@golang golang locked and limited conversation to collaborators May 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.7
Development

No branches or pull requests
9 participants
@bradfitz @jbardin @rsc @gripedthumbtacks @JamesDunne @jshort @polomsky @odeke-em @gopherbot