Skip to content

Commit

Permalink
fix: use http.Request's GetBody (#1093)
Browse files Browse the repository at this point in the history
This is needed for correct handling of redirects of requests with bodies
and in http2 when the server sends GoAway without an error.

Unfortunately testing the GoAway case turned out to be problematic so it
is left for the future.

Fixes #1091
  • Loading branch information
mstoykov authored Aug 5, 2019
1 parent 77ac332 commit ea694a3
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 12 deletions.
18 changes: 18 additions & 0 deletions js/modules/k6/http/request_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,24 @@ func TestRequestAndBatch(t *testing.T) {
`))
assert.NoError(t, err)
})

t.Run("post body", func(t *testing.T) {

tb.Mux.HandleFunc("/post-redirect", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
require.Equal(t, r.Method, "POST")
_, _ = io.Copy(ioutil.Discard, r.Body)
http.Redirect(w, r, sr("HTTPBIN_URL/post"), http.StatusPermanentRedirect)
}))
_, err := common.RunString(rt, sr(`
let res = http.post("HTTPBIN_URL/post-redirect", "pesho", {redirects: 1});
if (res.status != 200) { throw new Error("wrong status: " + res.status) }
if (res.url != "HTTPBIN_URL/post") { throw new Error("incorrect URL: " + res.url) }
if (res.json().data != "pesho") { throw new Error("incorrect data : " + res.json().data) }
`))
assert.NoError(t, err)
})

})
t.Run("Timeout", func(t *testing.T) {
t.Run("10s", func(t *testing.T) {
Expand Down
28 changes: 19 additions & 9 deletions lib/netext/httpext/request.go
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ func stdCookiesToHTTPRequestCookies(cookies []*http.Cookie) map[string][]*HTTPRe
return result
}

func compressBody(algos []CompressionType, body io.ReadCloser) (io.Reader, int64, string, error) {
func compressBody(algos []CompressionType, body io.ReadCloser) (*bytes.Buffer, string, error) {
var contentEncoding string
var prevBuf io.Reader = body
var buf *bytes.Buffer
Expand All @@ -176,21 +176,21 @@ func compressBody(algos []CompressionType, body io.ReadCloser) (io.Reader, int64
case CompressionTypeBr:
w = brotli.NewWriter(buf)
default:
return nil, 0, "", fmt.Errorf("unknown compressionType %s", compressionType)
return nil, "", fmt.Errorf("unknown compressionType %s", compressionType)
}
// we don't close in defer because zlib will write it's checksum again if it closes twice :(
var _, err = io.Copy(w, prevBuf)
if err != nil {
_ = w.Close()
return nil, 0, "", err
return nil, "", err
}

if err = w.Close(); err != nil {
return nil, 0, "", err
return nil, "", err
}
}

return buf, int64(buf.Len()), contentEncoding, body.Close()
return buf, contentEncoding, body.Close()
}

func readResponseBody(
Expand Down Expand Up @@ -318,22 +318,24 @@ func MakeRequest(ctx context.Context, preq *ParsedHTTPRequest) (*Response, error
}

if preq.Body != nil {
preq.Req.Body = ioutil.NopCloser(preq.Body)

// TODO: maybe hide this behind of flag in order for this to not happen for big post/puts?
// should we set this after the compression? what will be the point ?
respReq.Body = preq.Body.String()

switch {
case len(preq.Compressions) > 0:
compressedBody, length, contentEncoding, err := compressBody(preq.Compressions, preq.Req.Body)
var (
contentEncoding string
err error
)
preq.Body, contentEncoding, err = compressBody(preq.Compressions, ioutil.NopCloser(preq.Body))
if err != nil {
return nil, err
}

preq.Req.Body = ioutil.NopCloser(compressedBody)
if preq.Req.Header.Get("Content-Length") == "" {
preq.Req.ContentLength = length
preq.Req.ContentLength = int64(preq.Body.Len())
} else {
state.Logger.Warningf(compressionHeaderOverwriteMessage, "Content-Length", preq.Req.Method, preq.Req.URL)
}
Expand All @@ -347,6 +349,14 @@ func MakeRequest(ctx context.Context, preq *ParsedHTTPRequest) (*Response, error
}
// TODO: print some message in case we have Content-Length set so that we can warn users
// that setting it manually can lead to bad requests

preq.Req.GetBody = func() (io.ReadCloser, error) {
// using `Bytes()` should reuse the same buffer and as such help with the memory usage. We
// should not be writing to it any way so there shouldn't be way to corrupt it (?)
return ioutil.NopCloser(bytes.NewBuffer(preq.Body.Bytes())), nil
}
// as per the documentation using GetBody still requires setting the Body.
preq.Req.Body, _ = preq.Req.GetBody()
}

tags := state.Options.RunTags.CloneTags()
Expand Down
4 changes: 2 additions & 2 deletions lib/netext/httpext/request_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,13 @@ func badCloseBody() io.ReadCloser {
func TestCompressionBodyError(t *testing.T) {
var algos = []CompressionType{CompressionTypeGzip}
t.Run("bad read body", func(t *testing.T) {
_, _, _, err := compressBody(algos, ioutil.NopCloser(badReadBody()))
_, _, err := compressBody(algos, ioutil.NopCloser(badReadBody()))
require.Error(t, err)
require.Equal(t, err.Error(), badReadMsg)
})

t.Run("bad close body", func(t *testing.T) {
_, _, _, err := compressBody(algos, badCloseBody())
_, _, err := compressBody(algos, badCloseBody())
require.Error(t, err)
require.Equal(t, err.Error(), badCloseMsg)
})
Expand Down
3 changes: 2 additions & 1 deletion release notes/upcoming.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,5 @@ Description of feature.

## Bugs fixed!

* Category: description of bug. (#PR)
* HTTP: Use Request's GetBody in order to be able to get the body multiple times for a single
request as needed in 308 redirects of posts and if the server sends GOAWAY with no error (#1093)

0 comments on commit ea694a3

Please sign in to comment.