Skip to content

Commit

Permalink
fix(httpext): return error on unsupported 101 response
Browse files Browse the repository at this point in the history
The hanging behavior described in #971 happens after this Golang change:
golang/go@5416204
, released in 1.12. If a `101 Switching Protocols` response is received,
`response.Body` will be replaced by an underlying `net.Conn`, and
reading it blocks indefinitely without a protocol implementation.

This is not a big problem for k6 in this case, since we don't support
protocol upgrades with the current HTTP API as shown in the example, so
the agreed solution was to detect it and return an error.

This Golang change is currently incompatible with client timeouts[1],
though this will hopefully be resolved by the time we add support for
h2c (#970) or other protocols.

[1]: golang/go#31391

Closes #971
  • Loading branch information
Ivan Mirić authored and cuonglm committed Oct 15, 2019
1 parent 6ad8bbc commit 34670f4
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 0 deletions.
10 changes: 10 additions & 0 deletions lib/netext/httpext/request.go
Expand Up @@ -23,6 +23,7 @@ package httpext
import (
"bytes"
"context"
"fmt"
"io"
"io/ioutil"
"net"
Expand Down Expand Up @@ -314,6 +315,15 @@ func MakeRequest(ctx context.Context, preq *ParsedHTTPRequest) (*Response, error
mreq := preq.Req.WithContext(ctx)
res, resErr := client.Do(mreq)

// TODO(imiric): It would be safer to check for a writeable
// response body here instead of status code, but those are
// wrapped in a read-only body when using client timeouts and are
// unusable until https://github.com/golang/go/issues/31391 is fixed.
if res != nil && res.StatusCode == http.StatusSwitchingProtocols {
_ = res.Body.Close()
return nil, fmt.Errorf("unsupported response status: %s", res.Status)
}

resp.Body, resErr = readResponseBody(state, preq.ResponseType, res, resErr)
finishedReq := tracerTransport.processLastSavedRequest(wrapDecompressionError(resErr))
if finishedReq != nil {
Expand Down
27 changes: 27 additions & 0 deletions lib/netext/httpext/request_test.go
Expand Up @@ -6,10 +6,14 @@ import (
"io"
"io/ioutil"
"net/http"
"net/http/httptest"
"net/url"
"testing"

"github.com/loadimpact/k6/lib"
"github.com/loadimpact/k6/stats"
"github.com/pkg/errors"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -82,6 +86,29 @@ func TestMakeRequestError(t *testing.T) {
require.Error(t, err)
require.Equal(t, err.Error(), "unknown compressionType CompressionType(13)")
})

t.Run("invalid upgrade response", func(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Header().Add("Connection", "Upgrade")
w.Header().Add("Upgrade", "h2c")
w.WriteHeader(http.StatusSwitchingProtocols)
}))
defer srv.Close()
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
state := &lib.State{
Options: lib.Options{RunTags: &stats.SampleTags{}},
Transport: srv.Client().Transport,
}
ctx = lib.WithState(ctx, state)
req, _ := http.NewRequest("GET", srv.URL, nil)
var preq = &ParsedHTTPRequest{Req: req, URL: &URL{u: req.URL}, Body: new(bytes.Buffer)}

res, err := MakeRequest(ctx, preq)

assert.Nil(t, res)
assert.EqualError(t, err, "unsupported response status: 101 Switching Protocols")
})
}

func TestURL(t *testing.T) {
Expand Down

0 comments on commit 34670f4

Please sign in to comment.