diff --git a/buggyhttp/buggyhttp.go b/buggyhttp/buggyhttp.go index 1475484..cc7e3ae 100644 --- a/buggyhttp/buggyhttp.go +++ b/buggyhttp/buggyhttp.go @@ -4,9 +4,11 @@ package buggyhttp import ( "context" "fmt" + "net" "net/http" "strconv" "strings" + "sync/atomic" "time" ) @@ -133,9 +135,79 @@ func foo(w http.ResponseWriter, req *http.Request) { _, _ = fmt.Fprintf(w, "foo") } +// Server is a configurable instance of the buggy test server. Each instance +// keeps its own state (so tests do not share a global counter) and can bind an +// ephemeral port, avoiding conflicts with other services running locally. +type Server struct { + httpServer *http.Server + // count drives the /successAfter endpoint, per instance. + count atomic.Int64 +} + +// New returns a new, not yet listening, buggy server. +func New() *Server { + return &Server{} +} + +func (s *Server) mux() *http.ServeMux { + mux := http.NewServeMux() + mux.HandleFunc("/foo", foo) + mux.HandleFunc("/successAfter", s.successAfter) + mux.HandleFunc("/emptyResponse", emptyResponse) + mux.HandleFunc("/unexpectedEOF", unexpectedEOF) + mux.HandleFunc("/endlessBody", endlessBody) + mux.HandleFunc("/endlessWaitTime", endlessWaitTime) + mux.HandleFunc("/superSlow", superSlow) + mux.HandleFunc("/messyHeaders", messyHeaders) + mux.HandleFunc("/messyEncoding", messyEncoding) + mux.HandleFunc("/infiniteRedirects", infiniteRedirects) + return mux +} + +// Start binds an ephemeral port on 127.0.0.1, serves in the background and +// returns the base URL (e.g. http://127.0.0.1:54321). The listener is bound +// before returning, so the URL is ready to receive requests. +func (s *Server) Start() (string, error) { + return s.start("127.0.0.1:0") +} + +// StartPort binds the given port (all interfaces) and serves in the background. +func (s *Server) StartPort(port int) error { + _, err := s.start(fmt.Sprintf(":%d", port)) + return err +} + +func (s *Server) start(addr string) (string, error) { + ln, err := net.Listen("tcp", addr) + if err != nil { + return "", err + } + s.httpServer = &http.Server{Handler: s.mux()} + go s.httpServer.Serve(ln) //nolint + return "http://" + ln.Addr().String(), nil +} + +// StartTLS binds the given port for TLS and serves in the background. +func (s *Server) StartTLS(port int, certFile, keyFile string) error { + ln, err := net.Listen("tcp", fmt.Sprintf(":%d", port)) + if err != nil { + return err + } + s.httpServer = &http.Server{Handler: s.mux()} + go s.httpServer.ServeTLS(ln, certFile, keyFile) //nolint + return nil +} + +// Close shuts the server down. +func (s *Server) Close() error { + if s.httpServer != nil { + return s.httpServer.Shutdown(context.Background()) + } + return nil +} + // generates recoverable errors until SuccessAfter attempts => after it 200 + body -var count int // as of now a local horrible variable suffice -func successAfter(w http.ResponseWriter, req *http.Request) { +func (s *Server) successAfter(w http.ResponseWriter, req *http.Request) { successAfter := defaultSuccessAfterThreshold if req.FormValue("successAfter") != "" { if i, err := strconv.Atoi(req.FormValue("successAfter")); err == nil { @@ -143,8 +215,7 @@ func successAfter(w http.ResponseWriter, req *http.Request) { } } - count++ - if count <= successAfter { + if s.count.Add(1) <= int64(successAfter) { hj, _ := w.(http.Hijacker) conn, bufrw, _ := hj.Hijack() defer func() { @@ -164,68 +235,36 @@ func successAfter(w http.ResponseWriter, req *http.Request) { } // zeroes attempts and return 200 + valid body - count = 0 + s.count.Store(0) _, _ = fmt.Fprintf(w, "foo") } var ( - server *http.Server - serverTLS *http.Server + defaultServer *Server + defaultServerTLS *Server ) -// Listen on specified port +// Listen on the specified port using a package-level server. +// Deprecated: prefer New().Start() / New().StartPort() for an isolated, port +// configurable instance. func Listen(port int) { - - mux := http.NewServeMux() - mux.HandleFunc("/foo", foo) - mux.HandleFunc("/successAfter", successAfter) - mux.HandleFunc("/emptyResponse", emptyResponse) - mux.HandleFunc("/unexpectedEOF", unexpectedEOF) - mux.HandleFunc("/endlessBody", endlessBody) - mux.HandleFunc("/endlessWaitTime", endlessWaitTime) - mux.HandleFunc("/superSlow", superSlow) - mux.HandleFunc("/messyHeaders", messyHeaders) - mux.HandleFunc("/messyEncoding", messyEncoding) - mux.HandleFunc("/infiniteRedirects", infiniteRedirects) - - server = &http.Server{ - Addr: fmt.Sprintf(":%d", port), - Handler: mux, - } - - go server.ListenAndServe() //nolint + defaultServer = New() + _ = defaultServer.StartPort(port) } -// ListenTLS because buggyhttp also supports bugged TLS +// ListenTLS because buggyhttp also supports bugged TLS. +// Deprecated: prefer New().StartTLS(). func ListenTLS(port int, certFile, keyFile string) { - serverTLS = &http.Server{ - Addr: fmt.Sprintf(":%d", port), - Handler: newMux(), - } - - go serverTLS.ListenAndServeTLS(certFile, keyFile) //nolint -} - -func newMux() *http.ServeMux { - mux := http.NewServeMux() - mux.HandleFunc("/foo", foo) - mux.HandleFunc("/successAfter", successAfter) - mux.HandleFunc("/emptyResponse", emptyResponse) - mux.HandleFunc("/unexpectedEOF", unexpectedEOF) - mux.HandleFunc("/endlessBody", endlessBody) - mux.HandleFunc("/endlessWaitTime", endlessWaitTime) - mux.HandleFunc("/superSlow", superSlow) - mux.HandleFunc("/messyHeaders", messyHeaders) - mux.HandleFunc("/infiniteRedirects", infiniteRedirects) - return mux + defaultServerTLS = New() + _ = defaultServerTLS.StartTLS(port, certFile, keyFile) } -// Stop the server +// Stop the package-level servers started via Listen/ListenTLS. func Stop() { - if server != nil { - _ = server.Shutdown(context.Background()) + if defaultServer != nil { + _ = defaultServer.Close() } - if serverTLS != nil { - _ = serverTLS.Shutdown(context.Background()) + if defaultServerTLS != nil { + _ = defaultServerTLS.Close() } } diff --git a/client_test.go b/client_test.go index fe0d129..b525dfd 100644 --- a/client_test.go +++ b/client_test.go @@ -152,7 +152,7 @@ func TestClient_Do(t *testing.T) { // Request to /foo => 200 + valid body func testClientSuccess_Do(t *testing.T, body interface{}) { // Create a request - req, err := NewRequest("GET", "http://127.0.0.1:8080/foo", body) + req, err := NewRequest("GET", testServerURL+"/foo", body) if err != nil { t.Fatalf("err: %v", err) } @@ -215,7 +215,7 @@ func testClientSuccess_Do(t *testing.T, body interface{}) { func TestClientRetry_Do(t *testing.T) { expectedRetries := 3 // Create a generic request towards /successAfter passing the number of times before the same request is successful - req, err := NewRequest("GET", fmt.Sprintf("http://127.0.0.1:8080/successAfter?successAfter=%d", expectedRetries), nil) + req, err := NewRequest("GET", fmt.Sprintf("%s/successAfter?successAfter=%d", testServerURL, expectedRetries), nil) if err != nil { t.Fatalf("err: %v", err) } @@ -245,7 +245,7 @@ func TestClientRetry_Do(t *testing.T) { func TestClientRetryWithBody_Do(t *testing.T) { expectedRetries := 5 // Create a generic request towards /successAfter passing the number of times before the same request is successful - req, err := NewRequest("GET", fmt.Sprintf("http://127.0.0.1:8080/successAfter?successAfter=%d", expectedRetries), "request with body") + req, err := NewRequest("GET", fmt.Sprintf("%s/successAfter?successAfter=%d", testServerURL, expectedRetries), "request with body") if err != nil { t.Fatalf("err: %v", err) } @@ -275,7 +275,7 @@ func TestClientRetryWithBody_Do(t *testing.T) { // Expected: The library should keep on retrying until the final timeout or maximum retries amount func TestClientEmptyResponse_Do(t *testing.T) { // Create a request - req, err := NewRequest("GET", "http://127.0.0.1:8080/emptyResponse", nil) + req, err := NewRequest("GET", testServerURL+"/emptyResponse", nil) if err != nil { t.Fatalf("err: %v", err) } @@ -299,7 +299,7 @@ func TestClientEmptyResponse_Do(t *testing.T) { // Expected: The library should keep on retrying until the final timeout or maximum retries amount func TestClientUnexpectedEOF_Do(t *testing.T) { // Create a request - req, err := NewRequest("GET", "http://127.0.0.1:8080/unexpectedEOF", nil) + req, err := NewRequest("GET", testServerURL+"/unexpectedEOF", nil) if err != nil { t.Fatalf("err: %v", err) } @@ -323,7 +323,7 @@ func TestClientUnexpectedEOF_Do(t *testing.T) { // Expected: The library should read until a certain limit with return code 200 func TestClientEndlessBody_Do(t *testing.T) { // Create a request - req, err := NewRequest("GET", "http://127.0.0.1:8080/endlessBody", nil) + req, err := NewRequest("GET", testServerURL+"/endlessBody", nil) if err != nil { t.Fatalf("err: %v", err) } @@ -352,7 +352,7 @@ func TestClientEndlessBody_Do(t *testing.T) { // Expected: The library should stop reading headers after a certain amount or go into timeout func TestClientMessyHeaders_Do(t *testing.T) { // Create a request - req, err := NewRequest("GET", "http://127.0.0.1:8080/messyHeaders", nil) + req, err := NewRequest("GET", testServerURL+"/messyHeaders", nil) if err != nil { t.Fatalf("err: %v", err) } @@ -380,7 +380,7 @@ func TestClientMessyHeaders_Do(t *testing.T) { // Expected: The library should be successful as all strings are treated as runes func TestClientMessyEncoding_Do(t *testing.T) { // Create a request - req, err := NewRequest("GET", "http://127.0.0.1:8080/messyEncoding", nil) + req, err := NewRequest("GET", testServerURL+"/messyEncoding", nil) if err != nil { t.Fatalf("err: %v", err) } @@ -451,7 +451,7 @@ func TestWrapTransport_Request(t *testing.T) { client := NewClient(options) - req, err := NewRequest("GET", "http://127.0.0.1:8080/foo", nil) + req, err := NewRequest("GET", testServerURL+"/foo", nil) require.NoError(t, err) resp, err := client.Do(req) @@ -480,7 +480,7 @@ func TestWrapTransport_ModifyRequest(t *testing.T) { client := NewClient(options) - req, err := NewRequest("GET", "http://127.0.0.1:8080/foo", nil) + req, err := NewRequest("GET", testServerURL+"/foo", nil) require.NoError(t, err) resp, err := client.Do(req) @@ -521,7 +521,7 @@ func TestWrapTransport_Chain(t *testing.T) { client := NewClient(options) - req, err := NewRequest("GET", "http://127.0.0.1:8080/foo", nil) + req, err := NewRequest("GET", testServerURL+"/foo", nil) require.NoError(t, err) resp, err := client.Do(req) @@ -562,9 +562,22 @@ func (t *testTransportWrapper) RoundTrip(req *http.Request) (*http.Response, err return t.base.RoundTrip(req) } +// testServerURL is the base URL of the buggyhttp instance shared by the tests. +// It binds an ephemeral port so the suite does not collide with anything already +// listening on a fixed port locally. +var testServerURL string + func TestMain(m *testing.M) { - // start buggyhttp - buggyhttp.Listen(8080) - defer buggyhttp.Stop() - os.Exit(m.Run()) + // start buggyhttp on an ephemeral port + srv := buggyhttp.New() + base, err := srv.Start() + if err != nil { + fmt.Printf("could not start buggyhttp: %s\n", err) + os.Exit(1) + } + testServerURL = base + + code := m.Run() + _ = srv.Close() + os.Exit(code) } diff --git a/do.go b/do.go index 1368865..9b6731e 100644 --- a/do.go +++ b/do.go @@ -25,9 +25,15 @@ func (c *Client) Do(req *Request) (*http.Response, error) { var resp *http.Response var err error - // Create a main context that will be used as the main timeout - mainCtx, cancel := context.WithTimeout(context.Background(), c.options.Timeout) - defer cancel() + // Create a main context that will be used as the main timeout. A zero or + // negative timeout means no overall deadline (otherwise WithTimeout would + // create an already-expired context and abort before the first retry). + mainCtx := context.Background() + if c.options.Timeout > 0 { + var cancel context.CancelFunc + mainCtx, cancel = context.WithTimeout(context.Background(), c.options.Timeout) + defer cancel() + } retryMax := c.options.RetryMax if ctxRetryMax := req.Context().Value(RETRY_MAX); ctxRetryMax != nil { @@ -36,6 +42,7 @@ func (c *Client) Do(req *Request) (*http.Response, error) { } } +retryLoop: for i := 0; ; i++ { // request body can be read multiple times // hence no need to rewind it @@ -105,17 +112,20 @@ func (c *Client) Do(req *Request) (*http.Response, error) { // If the context is cancelled however, return. wait := c.Backoff(c.options.RetryWaitMin, c.options.RetryWaitMax, i, resp) - // Exit if the main context or the request context is done - // Otherwise, wait for the duration and try again. - // use label to explicitly specify what to break - selectstatement: + // Wait for the backoff period, but stop early if either the overall + // timeout (mainCtx) or the request context is done. A timer is used + // instead of time.After so it is released immediately on early exit + // rather than lingering for the full backoff duration. + timer := time.NewTimer(wait) select { case <-mainCtx.Done(): - break selectstatement + timer.Stop() + break retryLoop case <-req.Context().Done(): + timer.Stop() c.closeIdleConnections() return nil, req.Context().Err() - case <-time.After(wait): + case <-timer.C: } } diff --git a/do_retry_test.go b/do_retry_test.go new file mode 100644 index 0000000..2848986 --- /dev/null +++ b/do_retry_test.go @@ -0,0 +1,110 @@ +package retryablehttp + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// alwaysRetry forces the retry loop to keep going so the loop exit conditions +// (overall timeout / request cancellation) can be exercised in isolation from +// any status-code policy. +func alwaysRetry(_ context.Context, _ *http.Response, _ error) (bool, error) { + return true, nil +} + +// TestDoMainTimeoutStopsRetrying verifies that once the overall Options.Timeout +// elapses the retry loop stops instead of breaking only the inner select and +// storming through the remaining retries with no backoff. +func TestDoMainTimeoutStopsRetrying(t *testing.T) { + var hits int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + atomic.AddInt32(&hits, 1) + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + opts := Options{ + RetryWaitMin: 300 * time.Millisecond, + RetryWaitMax: 300 * time.Millisecond, + Timeout: 100 * time.Millisecond, + RetryMax: 5, + CheckRetry: alwaysRetry, + } + client := NewClient(opts) + + req, err := NewRequest("GET", srv.URL, nil) + require.NoError(t, err) + + start := time.Now() + resp, err := client.Do(req) + elapsed := time.Since(start) + if resp != nil { + _ = resp.Body.Close() + } + + require.Error(t, err, "expected an error once the overall timeout elapsed") + // the timeout (100ms) fires during the first backoff (300ms), so exactly one + // request should have been sent. Without the fix all RetryMax+1 requests run. + require.LessOrEqualf(t, atomic.LoadInt32(&hits), int32(2), + "retry storm after timeout: %d requests sent", atomic.LoadInt32(&hits)) + require.Lessf(t, elapsed, 2*time.Second, "did not stop promptly at timeout: %v", elapsed) +} + +// TestDoRequestCancelDuringBackoffReturnsPromptly verifies that cancelling the +// request context while waiting on the backoff returns immediately with the +// context error (the path whose backoff timer is now released instead of left +// to fire after the full RetryWaitMax). +func TestDoRequestCancelDuringBackoffReturnsPromptly(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + opts := Options{ + RetryWaitMin: 30 * time.Second, + RetryWaitMax: 30 * time.Second, + Timeout: 60 * time.Second, + RetryMax: 5, + CheckRetry: alwaysRetry, + } + client := NewClient(opts) + + ctx, cancel := context.WithCancel(context.Background()) + req, err := NewRequestWithContext(ctx, "GET", srv.URL, nil) + require.NoError(t, err) + + type result struct { + resp *http.Response + err error + } + done := make(chan result, 1) + go func() { + resp, err := client.Do(req) + done <- result{resp, err} + }() + + // let the first request complete and the loop enter the long backoff + time.Sleep(200 * time.Millisecond) + start := time.Now() + cancel() + + select { + case res := <-done: + if res.resp != nil { + _ = res.resp.Body.Close() + } + require.Truef(t, errors.Is(res.err, context.Canceled), + "expected context.Canceled, got %v", res.err) + require.Lessf(t, time.Since(start), 5*time.Second, + "cancel did not unblock backoff promptly: %v", time.Since(start)) + case <-time.After(10 * time.Second): + t.Fatal("Do did not return after request context was cancelled") + } +}