mitchellh / goamz

Golang Amazon Library
Other
673 stars 217 forks source link

Retrying transport will not correctly retry requests containing a body. #63

Open rlmcpherson opened 10 years ago

rlmcpherson commented 10 years ago

The retrying transport calls the RoundTrip() method on the transport here: https://github.com/mitchellh/goamz/blob/master/aws/client.go#L79

Per the net/http docs, the RoundTripper interface will always read and close the body, even on errors: http://golang.org/pkg/net/http/#RoundTripper. In addition, since a request body is an io.ReadCloser it is impossible to seek within even an unclosed body for retries.

The only workaround that I can think of that will allow a request to be correctly be retried at transport level is to copy the request body and into a new buffer to be passed to the RoundTripper on each attempt. This is obviously ugly and potentially expensive for large bodies, but something like this could work:

func (t *ResilientTransport) tries(req *http.Request) (res *http.Response, err error) {
    var b []byte
    // save body for retries
    if req.Body != nil {
        b, err = ioutil.ReadAll(req.Body)
        if err != nil {
            return nil, err
        }
    }

    for try := 0; try < t.MaxTries; try++ {
        // reset req.body
        r := bytes.NewReader(b)
        req.Body = ioutil.NopCloser(r)
        res, err = t.transport.RoundTrip(req)
        if !t.ShouldRetry(req, res, err) {
            break
        }
        if res != nil {
            res.Body.Close()
        }
        if t.Wait != nil {
            t.Wait(try)
        }
    }
    return
}

If there's a better way to do this I'd love to know ( I ran into this same issue trying to implement a retrying transport for s3gof3r and ended up retrying outside the client where I could seek in request bodies). Otherwise, I can open a pull request with the above code.

mwhooker commented 9 years ago

very good catch! I think we should be fine here because all of our requests are GETs. However, I'm just reorienting myself in the code, so I may be mistaken.

If we did need to preserve the body, I think your approach is the most sensible.