Unexpected side effect of httpexpect to stdlib client retry policy #175

pantafive · 2022-11-26T11:33:07Z

pantafive
Nov 26, 2022

After we make a request with httpexpect, http.DefaultClient starts to retry requests on fails which is unexpected behavior.
Can you please explain why it happens and how to avoid this behavior?

package experiments

import (
	"net/http"
	"testing"

	"github.com/gavv/httpexpect/v2"
	"github.com/stretchr/testify/require"
	"net/http/httptest"
)

func TestUnexpectedRetryBehavior(t *testing.T) {
	var requestCount int

	handler := func(w http.ResponseWriter, r *http.Request) {
		// don't drop request with /pass path
		// httpexpect dummy request will use it
		if r.URL.Path == "/pass" {
			return
		}

		// count each dropped request
		defer func() { requestCount  = 1 }()

		hj, ok := w.(http.Hijacker)
		require.True(t, ok)

		conn, _, err := hj.Hijack()
		require.NoError(t, err)

		err = conn.Close()
		require.NoError(t, err)
	}

	server := httptest.NewServer(http.HandlerFunc(handler))
	defer server.Close()

	_, _ = http.Get(server.URL)
	// ensure that before httpexpect is called stdlib client doesn't make retries
	require.Equal(t, 1, requestCount)

	// make dummy request with httpexpect which changes retry policy of stdlib client
	// NB: same result will be with WithRetryPolicy(0) and WithMaxRetries(0)
	httpexpect.New(t, server.URL).GET("/pass").Expect().Status(http.StatusOK)

	_, _ = http.Get(server.URL)
	// TODO: normally, requestCount should be 2, but it's 3
	require.Equal(t, 3, requestCount)
}

gavv · 2022-11-26T12:55:42Z

gavv
Nov 26, 2022
Maintainer

Hi, interesting question. I don't know the answer, but here is what I can tell:

It's not specific to httpexpect at all
Hijack it first request makes the last request failing.
http.Transport performs the retry.

Here is a test without httpexpect that reproduces your problem:

package httpexpect

import (
	"net/http"
	"net/http/httptest"
	"sync/atomic"
	"testing"

	"github.com/stretchr/testify/assert"
)

func TestUnexpectedRetryBehavior(t *testing.T) {
	var requestCount int32

	handler := func(w http.ResponseWriter, r *http.Request) {
		println("request: "   r.URL.Path)
		if r.URL.Path == "/pass" {
			return
		}

		atomic.AddInt32(&requestCount, 1)

		hj, ok := w.(http.Hijacker)
		assert.True(t, ok)

		conn, _, err := hj.Hijack()
		assert.NoError(t, err)

		err = conn.Close()
		assert.NoError(t, err)
	}

	server := httptest.NewServer(http.HandlerFunc(handler))
	defer server.Close()

	println(">>> sending request 1")
	_, _ = http.Get(server.URL)
	assert.Equal(t, int32(1), atomic.LoadInt32(&requestCount))

	println(">>> sending /pass")
	_, _ = http.Get(server.URL   "/pass")

	println(">>> sending request 2")
	_, _ = http.Get(server.URL)
	assert.Equal(t, int32(2), atomic.LoadInt32(&requestCount))
}

Here is the output:

gotest -v -run TestUnexpectedRetryBehavior
=== RUN   TestUnexpectedRetryBehavior
>>> sending request 1
request: /
>>> sending /pass
request: /pass
>>> sending request 2
request: /
----- HERE -----
request: /
    e2e_test.go:45: 
        	Error Trace:	e2e_test.go:45
        	Error:      	Not equal: 
        	            	expected: 2
        	            	actual  : 3
        	Test:       	TestUnexpectedRetryBehavior
--- FAIL: TestUnexpectedRetryBehavior (0.00s)
FAIL
exit status 1
FAIL	github.com/gavv/httpexpect/v2	0.005s

See that ----- HERE ----- line? I inserted that message into this block in transport.go, like this:

	if _, ok := err.(transportReadFromServerError); ok {
		// We got some non-EOF net.Conn.Read failure reading
		// the 1st response byte from the server.
		println("----- HERE -----")
		return true
	}

0 replies

pantafive · 2022-11-26T23:40:45Z

pantafive
Nov 26, 2022
Author

Disabling KeepAlives solve the problem. @gavv , thank you so much for pointing me in the right direction.

func TestHTTPExpectUnexpected(t *testing.T) {
	var requestCount int32

	transport := http.DefaultTransport.(*http.Transport).Clone()
	transport.DisableKeepAlives = true
	client := &http.Client{Transport: transport}

	handler := func(w http.ResponseWriter, r *http.Request) {
		if r.URL.Path == "/pass" {
			return
		}

		atomic.AddInt32(&requestCount, 1)

		hj, ok := w.(http.Hijacker)
		assert.True(t, ok)

		conn, _, err := hj.Hijack()
		assert.NoError(t, err)

		err = conn.Close()
		assert.NoError(t, err)
	}

	t.Run("test with stdlib client", func(t *testing.T) {
		server := httptest.NewServer(http.HandlerFunc(handler))
		defer server.Close()

		_, _ = client.Get(server.URL)
		assert.Equal(t, int32(1), atomic.LoadInt32(&requestCount))

		_, _ = client.Get(server.URL   "/pass")

		_, _ = client.Get(server.URL)
		assert.Equal(t, int32(2), atomic.LoadInt32(&requestCount))
	})
}

2 replies

gavv Nov 27, 2022
Maintainer

Note that disabling keepalives is usually a bad idea. It forces client to establish new tcp connection for each request. It may be acceptable for tests though.

pantafive Nov 27, 2022
Author

I know. I need to test system behavior in case of connection loss, etc. In this case, we need as straightforward behavior as possible.

gavv · 2022-11-27T00:38:54Z

gavv
Nov 27, 2022
Maintainer

Moved to discussions

0 replies

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unexpected side effect of httpexpect to stdlib client retry policy #175

{{title}}

{{editor}}'s edit

{{editor}}'s edit

Replies: 3 comments 2 replies

{{title}}

{{title}}

{{editor}}'s edit

{{editor}}'s edit

{{title}}

{{editor}}'s edit

{{editor}}'s edit

{{title}}

{{title}}

Select a reply

Unexpected side effect of httpexpect to stdlib client retry policy #175

pantafive Nov 26, 2022

Replies: 3 comments · 2 replies

gavv Nov 26, 2022 Maintainer

pantafive Nov 26, 2022 Author

gavv Nov 27, 2022 Maintainer

pantafive Nov 27, 2022 Author

gavv Nov 27, 2022 Maintainer

pantafive
Nov 26, 2022

Replies: 3 comments 2 replies

gavv
Nov 26, 2022
Maintainer

pantafive
Nov 26, 2022
Author

gavv Nov 27, 2022
Maintainer

pantafive Nov 27, 2022
Author

gavv
Nov 27, 2022
Maintainer