retry timeout

buggyhttp/buggyhttp.go

@@ -4,9 +4,11 @@ package buggyhttp
 import (
 	"context"
 	"fmt"
+	"net"
 	"net/http"
 	"strconv"
 	"strings"
+	"sync/atomic"
 	"time"
 )
 
@@ -133,18 +135,87 @@ 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 {
 			successAfter = i
 		}
 	}
 
-	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()
 	}
 }

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)
 }

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:
 		}
 	}