diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..fe23078 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,24 @@ +# Changelog + +All notable changes to this project are documented in this file. + +The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), +and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). + +## [0.1.0] - 2026-07-25 + +First public release. + +### Added + +- Middleware-based HTTP client (`New` returning `*Client`, `WithMiddleware`, `WithTransport`) built on `http.RoundTripper`, plus the exported `RoundTripperFunc` adapter that makes a custom middleware a one-liner. +- Resiliency middleware: `Timeout`, `Retry` with pluggable backoff, `CircuitBreaker`, and `RateLimit` (token bucket behind the `RateLimiter` interface: `TryAcquire` plus `WaitContext`). +- `SharedCircuitBreaker` (`NewCircuitBreaker`) for circuit state shared across multiple clients, with observable `State()` and `CircuitState.String()`. +- Observability middleware: `Logging` and `Metrics`. `MetricsConfig.PathNormalizer` bounds metrics label cardinality (raw path is omitted by default). +- Backoff strategies: constant, linear, exponential, Fibonacci, and their jitter variants — all zero-alloc and overflow-safe. `BackoffFunc` receives the response that triggered the retry, and the `WithRetryAfter` decorator honors the `Retry-After` header (delay-seconds or HTTP-date) on 429/503. +- Fluent request builder (`Client.R`) with JSON, XML, form and reader bodies, path parameters, and query parameters. Reader bodies up to 10 MB are buffered so retries can rewind them; larger bodies stream and are sent exactly once. +- `DecodeJSON` response helper: always drains and closes the body, fails on status >= 300. +- Error classification: `Classify`, `IsRetryable`, `IsTimeout`, `IsConnection`, and related helpers. +- Zero external dependencies; Go standard library only. + +[0.1.0]: https://github.com/oswaldom-code/rhttp/releases/tag/v0.1.0 diff --git a/CLAUDE.md b/CLAUDE.md index 281ed60..d9f3208 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -7,8 +7,8 @@ Production-grade HTTP client for Go with built-in resiliency patterns. Zero exte ``` rhttp/ # Package rhttp lives at the module root -├── client.go # Client interface and New() constructor -├── middleware.go # Middleware type and chain() function +├── client.go # Client struct and New() constructor +├── middleware.go # Middleware and RoundTripperFunc types, chain() ├── transport.go # Optimized DefaultTransport() ├── options.go # Functional options pattern ├── errors.go # Sentinel errors @@ -22,8 +22,6 @@ rhttp/ # Package rhttp lives at the module root ├── metrics.go # Metrics middleware ├── request.go # Fluent API (RequestBuilder) ├── pool.go # Object pooling with sync.Pool -├── internal/ # Internal package -│ └── roundtripper.go ├── examples/ # Runnable examples ├── .github/workflows/ # CI with GitHub Actions ├── Makefile # Development commands @@ -66,7 +64,7 @@ func MyMiddleware(cfg Config) Middleware { ### Tests - Use standard `testing` package (project does NOT use ginkgo/gomega by design - zero deps) - Name files `*_test.go` -- Use `internal.MockRoundTripper` for transport mocks +- Use `rhttp.RoundTripperFunc` for transport mocks - Respect context in mocks with `select { case <-req.Context().Done(): ... }` ### Errors @@ -90,9 +88,12 @@ func MyMiddleware(cfg Config) Middleware { ## Recommended Middleware Order ```go -Logging → Metrics → Timeout → RateLimit → CircuitBreaker → Retry +Logging → Metrics → Timeout → RateLimit → Retry → CircuitBreaker ``` +Retry sits outside CircuitBreaker so every attempt consults the circuit: a +tripped breaker short-circuits the remaining attempts. + ## Pre-Commit Checklist 1. `make fmt` - Code formatted diff --git a/Makefile b/Makefile index 75881fc..f65378b 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: help test test-race test-coverage coverage-summary bench lint fmt vet docs check clean install-tools +.PHONY: help test test-race test-short test-coverage coverage-summary bench bench-compare lint fmt fmt-check vet docs check check-all clean install-tools deps ci version info .DEFAULT_GOAL := help @@ -49,7 +49,7 @@ test-coverage: @echo "" @echo "$(GREEN)Coverage report generated: $(COVERAGE_HTML)$(NC)" -coverage-summary: +coverage-summary: test-coverage @echo "$(GREEN)=== Total Coverage ===$(NC)" @$(GOCMD) tool cover -func=$(COVERAGE_FILE) | tail -1 @echo "" @@ -62,7 +62,7 @@ test-short: bench: @echo "$(GREEN)Running benchmarks...$(NC)" - $(GOTEST) -bench=. -benchmem $(PACKAGES) + $(GOTEST) -bench=. -benchmem -count=5 $(PACKAGES) bench-compare: @echo "$(GREEN)Running benchmarks for comparison...$(NC)" @@ -122,6 +122,10 @@ install-tools: go install golang.org/x/pkgsite/cmd/pkgsite@latest @echo "$(GREEN)Done! Make sure $(GOPATH)/bin is in your PATH.$(NC)" +deps: + @echo "$(GREEN)Downloading dependencies...$(NC)" + $(GOMOD) download + ci: deps fmt-check vet lint test-race @echo "$(GREEN)CI pipeline passed!$(NC)" diff --git a/README.md b/README.md index d76d006..53645aa 100644 --- a/README.md +++ b/README.md @@ -11,37 +11,36 @@ Production-grade HTTP client for Go with built-in resiliency patterns. ## Motivation -Después de implementar clientes HTTP con patrones de resiliencia en múltiples proyectos -de microservicios, identificé un patrón recurrente: +After building HTTP clients with resiliency patterns across multiple microservice +projects, a recurring pattern emerged: -1. **La stdlib no es suficiente** - `net/http` es potente pero no incluye retry, - circuit breaker ni rate limiting -2. **Las dependencias son un problema** - Librerías como Resty traen dependencias - transitivas que complican auditorías de seguridad y aumentan el tamaño del binario -3. **Reinventar la rueda es costoso** - Cada equipo termina escribiendo su propio - wrapper con bugs sutiles en manejo de contextos, timeouts y connection pooling +1. **The stdlib is not enough** - `net/http` is powerful but ships no retry, + circuit breaker, or rate limiting +2. **Dependencies are a liability** - Libraries like Resty pull in transitive + dependencies that complicate security audits and grow the binary size +3. **Reinventing the wheel is costly** - Every team ends up writing its own + wrapper with subtle bugs in context handling, timeouts, and connection pooling -Esta librería resuelve ese problema: **resiliencia production-ready con cero dependencias**. +This library solves that: **production-ready resiliency with zero dependencies**. ### Usage Modes -| Modo | Cuándo usarlo | -|------|---------------| -| `go get` | Proyectos que aceptan dependencias externas | -| Copiar a `pkg/rhttp` | Políticas estrictas de zero-deps, vendor everything | +| Mode | When to use it | +|------|----------------| +| `go get` | Projects that accept external dependencies | +| Copy into `pkg/rhttp` | Strict zero-deps policies, vendor everything | -El código está diseñado para funcionar en ambos escenarios sin modificaciones. +The code is designed to work in both scenarios without modification. ## Features - **Zero dependencies** - Only Go standard library -- **Faster than net/http** - 35% faster than `http.Client` baseline +- **Low overhead** - The full middleware stack adds ~1 μs per request - **Middleware architecture** - Composable, testable, extensible - **Fluent API** - Resty-style request builder - **Resiliency patterns** - Retry, circuit breaker, rate limiting, timeout - **Multiple backoff strategies** - Constant, linear, exponential, Fibonacci, jitter variants -- **Object pooling** - Reduced allocations via `sync.Pool` -- **100% test coverage** - 101 tests +- **Well tested** - Race-clean suite; live coverage in the Codecov badge above ## Installation @@ -98,14 +97,14 @@ func main() { client := rhttp.New() // GET request with query params -resp, err := rhttp.R(client). +resp, err := client.R(). SetHeader("Authorization", "Bearer token"). SetQueryParam("page", "1"). SetQueryParam("limit", "10"). Get("https://api.example.com/users") // POST request with JSON body -resp, err := rhttp.R(client). +resp, err := client.R(). SetAuthToken("my-token"). SetBodyJSON(map[string]string{ "name": "John", @@ -114,7 +113,7 @@ resp, err := rhttp.R(client). Post("https://api.example.com/users") // Path parameters -resp, err := rhttp.R(client). +resp, err := client.R(). SetPathParam("org", "acme"). SetPathParam("repo", "api"). Get("https://api.github.com/repos/{org}/{repo}") @@ -161,7 +160,8 @@ client := rhttp.New( | `ExponentialBackoffEqualJitter(base, max)` | `base * 2^attempt / 2 + random(0, half)` | | `DecorrelatedJitterBackoff(base, max)` | AWS-style decorrelated jitter | -Composable with `WithJitter()`, `WithMin()`, `WithMax()`. +Composable with `WithJitter()`, `WithMin()`, `WithMax()`, and `WithRetryAfter()` +(honors the `Retry-After` header on 429/503 responses). ### Circuit Breaker @@ -196,9 +196,6 @@ client := rhttp.New( }), ), ) - -// Per-host rate limiting -perHostLimiter := rhttp.NewPerHostRateLimiter(50, 5) // 50 req/s per host ``` ### Logging @@ -236,6 +233,22 @@ client := rhttp.New( `MetricEvent` fields: `Method`, `Host`, `Path`, `StatusCode`, `Duration`, `BytesSent`, `BytesReceived`, `Error`, `Success` +#### Path cardinality + +Exporting a raw request path (`/users/8f3a.../orders/2941`) as a metrics label creates one time series per ID, which grows Prometheus memory without bound. To prevent this, `Path` is **empty by default** and is only populated when you provide a `PathNormalizer` that collapses high-cardinality segments to a template: + +```go +rhttp.Metrics(rhttp.MetricsConfig{ + Recorder: recorder, + PathNormalizer: func(p string) string { + // /users/8f3a/orders/2941 -> /users/:id/orders/:id + return idSegment.ReplaceAllString(p, "/:id") + }, +}) +``` + +To emit the raw path anyway (not recommended as a metrics label), use `func(p string) string { return p }`. + ## Error Classification ```go @@ -267,7 +280,7 @@ if err != nil { ## Middleware Order -Middleware executes in the order specified: +The **first middleware in the list is the outermost**: it runs first on the way in and last on the way out. Each subsequent middleware wraps the ones after it, and the transport sits at the center. ```go client := rhttp.New( @@ -276,13 +289,31 @@ client := rhttp.New( rhttp.Metrics(...), // 2. Start timing rhttp.Timeout(...), // 3. Apply timeout rhttp.RateLimit(...), // 4. Check rate limit - rhttp.CircuitBreaker(...), // 5. Check circuit - rhttp.Retry(...), // 6. Retry on failure + rhttp.Retry(...), // 5. Retry on failure + rhttp.CircuitBreaker(...), // 6. Check circuit per attempt ), ) ``` -Recommended order: `Logging → Metrics → Timeout → RateLimit → CircuitBreaker → Retry` +Recommended order: `Logging → Metrics → Timeout → RateLimit → Retry → CircuitBreaker` + +### Timeout placement changes its meaning + +Where you put `Timeout` relative to `Retry` selects one of two semantics — both valid, but very different: + +| Pattern | Order | Meaning | +|---------|-------|---------| +| **Total budget** | `Timeout → Retry` | The timeout covers **all attempts and their backoffs combined**. Once it expires, no further retries happen. | +| **Per-attempt timeout** | `Retry → Timeout` | Each attempt gets its **own fresh timeout**; the total wall-clock time is roughly `attempts × timeout` plus backoffs. | + +See the runnable `ExampleRetry_totalBudget` and `ExampleRetry_perAttemptTimeout` for both wirings. + +### Retry vs CircuitBreaker + +| Order | Effect | +|-------|--------| +| `Retry → CircuitBreaker` (retry outer) **— recommended** | Each attempt consults the circuit; a tripped breaker short-circuits the remaining attempts. The circuit counts every attempt. | +| `CircuitBreaker → Retry` (breaker outer) | The circuit sees one fully-retried request as a single call; retries are not individually gated by the breaker. | ## Custom Transport @@ -300,39 +331,67 @@ client := rhttp.New( transport := rhttp.DefaultTransport() // HTTP/2 enabled, optimized pool ``` -## Object Pooling +## Benchmarks -Reduce allocations with buffer pooling: +Two suites, measured 2026-07-25 on linux/amd64 (Intel Core i7-1255U, Go 1.24): +the in-repo microbenchmarks (`make bench`) measure client and middleware +overhead against a no-op transport, and a standalone comparison harness +([`benchmarks/`](benchmarks/), `make report`) measures rhttp against Resty +v2.17.2, go-retryablehttp v0.7.8 and Heimdall v7.0.3 with equivalent +configuration (5s timeout, 3 attempts, exponential backoff 100ms-2s). -```go -// Get a buffer from the pool -buf := rhttp.GetBuffer() -defer rhttp.PutBuffer(buf) +### Middleware overhead (no network) -buf.WriteString("request body") -``` - -## Benchmarks +Minimum of 5 runs: ``` -goos: linux -goarch: amd64 -cpu: Intel Core i7-1255U - -BenchmarkClient_Baseline-12 235 ns/op 656 B/op 4 allocs/op -BenchmarkStdHttpClient_Baseline-12 317 ns/op 600 B/op 7 allocs/op (+35%) -BenchmarkClient_WithRetry-12 265 ns/op 656 B/op 4 allocs/op -BenchmarkClient_WithCircuitBreaker-12 271 ns/op 656 B/op 4 allocs/op -BenchmarkClient_AllMiddleware-12 1143 ns/op 1472 B/op 12 allocs/op -BenchmarkTokenBucket_TryAcquire-12 52 ns/op 0 B/op 0 allocs/op -BenchmarkBackoff_Exponential-12 7 ns/op 0 B/op 0 allocs/op +BenchmarkMiddlewareOverhead_Baseline-12 240 ns/op 656 B/op 4 allocs/op +BenchmarkMiddlewareOverhead_WithRetry-12 272 ns/op 656 B/op 4 allocs/op +BenchmarkMiddlewareOverhead_WithCircuitBreaker-12 266 ns/op 656 B/op 4 allocs/op +BenchmarkMiddlewareOverhead_AllMiddleware-12 1030 ns/op 1589 B/op 13 allocs/op +BenchmarkStdHttpClient_Baseline-12 256 ns/op 552 B/op 5 allocs/op +BenchmarkTokenBucket_TryAcquire-12 48 ns/op 0 B/op 0 allocs/op +BenchmarkBackoffStrategies/Exponential-12 8 ns/op 0 B/op 0 allocs/op ``` -**Key results:** -- 35% faster than `net/http` client baseline -- All middleware stack: ~1μs overhead (negligible vs network latency) -- Rate limiter: 52ns per check, zero allocations -- Backoff strategies: <10ns, zero allocations +- Full middleware stack: ~1 μs and ~1.5 KB per request — negligible against network latency (0.5–500 ms) +- Rate limiter: 48 ns per check, zero allocations +- Backoff strategies: <10 ns, zero allocations + +### Comparison with other clients + +Wrapper overhead (no-op transport, timeout + 3-attempt retry configured everywhere, min of 5 runs): + +| Client | ns/op | allocs/op | vs best | +|---|---:|---:|---:| +| rhttp (Timeout+Retry) | 910 | 12 | 1.00x | +| rhttp (Timeout+Retry+CircuitBreaker) | 946 | 12 | 1.04x | +| net/http (Timeout only, no retry) | 1750 | 26 | 1.92x | +| go-retryablehttp | 1775 | 26 | 1.95x | +| Heimdall (retry) | 2555 | 32 | 2.81x | +| Resty (retry) | 5818 | 48 | 6.39x | + +End-to-end (~1 KB JSON over loopback): + +| Client | ns/op | allocs/op | vs best | +|---|---:|---:|---:| +| go-retryablehttp | 58309 | 74 | 1.00x | +| net/http (Timeout only, no retry) | 60995 | 75 | 1.05x | +| Heimdall (retry) | 61009 | 80 | 1.05x | +| rhttp (Timeout+Retry) | 61923 | 76 | 1.06x | +| rhttp (Timeout+Retry+CircuitBreaker) | 62817 | 76 | 1.08x | +| Resty (retry) | 71696 | 96 | 1.23x | + +**Read the caveats before quoting these numbers:** + +- All clients are configured equivalently and fully consume and close each response body. +- `net/http` does not retry: it is the floor, not a symmetric competitor. +- Heimdall runs without its Hystrix circuit breaker (retry only, for feature symmetry). +- Resty buffers the full response body by design. +- Loopback amplifies relative overhead: against a real network (0.5-500 ms per + request) every client in the table performs the same for practical purposes. + +Full methodology and reproduction steps: [`benchmarks/REPORT.md`](benchmarks/REPORT.md). ## Design Principles @@ -413,7 +472,6 @@ All PRs must pass CI checks before merging. - [x] **Metrics middleware** - Pluggable `MetricsRecorder` interface - [x] **Error classification** - Timeout, connection, DNS, TLS, temporary - [x] **Fluent API** - Resty-style `RequestBuilder` -- [x] **Object pooling** - Reduced allocations via `sync.Pool` - [x] **Zero dependencies** - Only Go standard library ### Phase 2: Advanced Resiliency diff --git a/backoff.go b/backoff.go index 3e656bc..fe679f6 100644 --- a/backoff.go +++ b/backoff.go @@ -1,18 +1,24 @@ package rhttp import ( + "math" "math/rand" + "net/http" + "strconv" "sync" "time" ) // BackoffFunc returns the duration to wait before the nth retry attempt. // attempt is 0-indexed (0 = first retry, 1 = second retry, etc.) -type BackoffFunc func(attempt int) time.Duration +// resp is the response of the attempt that triggered the retry; it is nil when +// that attempt produced no response (e.g. a transport error). Pure strategies +// ignore it; decorators like [WithRetryAfter] use it. +type BackoffFunc func(attempt int, resp *http.Response) time.Duration // ConstantBackoff returns a backoff function that always returns the same duration. func ConstantBackoff(d time.Duration) BackoffFunc { - return func(_ int) time.Duration { + return func(_ int, _ *http.Response) time.Duration { return d } } @@ -20,7 +26,7 @@ func ConstantBackoff(d time.Duration) BackoffFunc { // LinearBackoff returns a backoff function with linear growth. // The wait time is: base * (attempt + 1), capped at maxDuration. func LinearBackoff(base, maxDuration time.Duration) BackoffFunc { - return func(attempt int) time.Duration { + return func(attempt int, _ *http.Response) time.Duration { backoff := base * time.Duration(attempt+1) if backoff > maxDuration { return maxDuration @@ -29,10 +35,19 @@ func LinearBackoff(base, maxDuration time.Duration) BackoffFunc { } } +// overflowsExp reports whether base * 2^attempt overflows int64. +func overflowsExp(base time.Duration, attempt int) bool { + return attempt >= 63 || base > math.MaxInt64>>attempt +} + // ExponentialBackoff returns a backoff function with exponential growth and jitter. // The wait time is: base * 2^attempt with ±20% jitter, capped at maxDuration. +// Attempts whose product no longer fits in int64 saturate at maxDuration. func ExponentialBackoff(base, maxDuration time.Duration) BackoffFunc { - return func(attempt int) time.Duration { + return func(attempt int, _ *http.Response) time.Duration { + if overflowsExp(base, attempt) { + return maxDuration + } backoff := base * (1 << attempt) backoff = min(backoff, maxDuration) // Add jitter: ±20% (not crypto, just randomization for backoff distribution) @@ -45,7 +60,7 @@ func ExponentialBackoff(base, maxDuration time.Duration) BackoffFunc { // The wait time is: base * fib(attempt + 1), capped at maxDuration. // Fibonacci: 1, 1, 2, 3, 5, 8, 13, 21, 34, 55... func FibonacciBackoff(base, maxDuration time.Duration) BackoffFunc { - return func(attempt int) time.Duration { + return func(attempt int, _ *http.Response) time.Duration { fib := fibonacci(attempt + 1) backoff := base * time.Duration(fib) if backoff > maxDuration { @@ -70,13 +85,16 @@ func fibonacci(n int) int { // DecorrelatedJitterBackoff returns a backoff with decorrelated jitter. // This algorithm provides better distribution than exponential backoff with jitter. // See: https://aws.amazon.com/blogs/architecture/exponential-backoff-and-jitter/ -// Note: This function returns a stateful BackoffFunc that is safe for concurrent use. +// +// The returned BackoffFunc carries shared state guarded by a mutex: it is +// race-free, but concurrent retry sequences feed the same last-backoff value +// and correlate their delays. Use one instance per sequence when that matters. func DecorrelatedJitterBackoff(base, maxDuration time.Duration) BackoffFunc { var ( mu sync.Mutex lastBackoff time.Duration ) - return func(attempt int) time.Duration { + return func(attempt int, _ *http.Response) time.Duration { mu.Lock() defer mu.Unlock() @@ -100,7 +118,10 @@ func DecorrelatedJitterBackoff(base, maxDuration time.Duration) BackoffFunc { // The wait time is: random(0, base * 2^attempt), capped at maxDuration. // This provides the best spread for avoiding thundering herd. func ExponentialBackoffFullJitter(base, maxDuration time.Duration) BackoffFunc { - return func(attempt int) time.Duration { + return func(attempt int, _ *http.Response) time.Duration { + if overflowsExp(base, attempt) { + return maxDuration + } ceiling := base * (1 << attempt) ceiling = min(ceiling, maxDuration) return time.Duration(rand.Float64() * float64(ceiling)) //nolint:gosec @@ -110,7 +131,10 @@ func ExponentialBackoffFullJitter(base, maxDuration time.Duration) BackoffFunc { // ExponentialBackoffEqualJitter returns exponential backoff with equal jitter. // The wait time is: (base * 2^attempt)/2 + random(0, (base * 2^attempt)/2) func ExponentialBackoffEqualJitter(base, maxDuration time.Duration) BackoffFunc { - return func(attempt int) time.Duration { + return func(attempt int, _ *http.Response) time.Duration { + if overflowsExp(base, attempt) { + return maxDuration + } ceiling := base * (1 << attempt) ceiling = min(ceiling, maxDuration) half := ceiling / 2 @@ -128,8 +152,8 @@ func WithJitter(backoff BackoffFunc, jitterFraction float64) BackoffFunc { jitterFraction = 1 } - return func(attempt int) time.Duration { - d := backoff(attempt) + return func(attempt int, resp *http.Response) time.Duration { + d := backoff(attempt, resp) jitter := float64(d) * jitterFraction * (rand.Float64()*2 - 1) //nolint:gosec result := d + time.Duration(jitter) if result < 0 { @@ -141,8 +165,8 @@ func WithJitter(backoff BackoffFunc, jitterFraction float64) BackoffFunc { // WithMax wraps a backoff function and caps the maximum duration. func WithMax(backoff BackoffFunc, maxDuration time.Duration) BackoffFunc { - return func(attempt int) time.Duration { - d := backoff(attempt) + return func(attempt int, resp *http.Response) time.Duration { + d := backoff(attempt, resp) if d > maxDuration { return maxDuration } @@ -152,11 +176,51 @@ func WithMax(backoff BackoffFunc, maxDuration time.Duration) BackoffFunc { // WithMin wraps a backoff function and ensures a minimum duration. func WithMin(backoff BackoffFunc, minDuration time.Duration) BackoffFunc { - return func(attempt int) time.Duration { - d := backoff(attempt) + return func(attempt int, resp *http.Response) time.Duration { + d := backoff(attempt, resp) if d < minDuration { return minDuration } return d } } + +// parseRetryAfter extracts the wait hinted by a 429 or 503 response's +// Retry-After header, either as delay-seconds or as an HTTP-date. +func parseRetryAfter(resp *http.Response) (time.Duration, bool) { + if resp == nil { + return 0, false + } + if resp.StatusCode != http.StatusTooManyRequests && resp.StatusCode != http.StatusServiceUnavailable { + return 0, false + } + value := resp.Header.Get("Retry-After") + if value == "" { + return 0, false + } + if seconds, err := strconv.Atoi(value); err == nil { + if seconds <= 0 { + return 0, false + } + return time.Duration(seconds) * time.Second, true + } + if t, err := http.ParseTime(value); err == nil { + if d := time.Until(t); d > 0 { + return d, true + } + } + return 0, false +} + +// WithRetryAfter wraps a backoff function and honors the Retry-After header +// on 429 and 503 responses: the wait is max(base backoff, server hint). +// The wait can still be cut short by canceling the request context. +func WithRetryAfter(backoff BackoffFunc) BackoffFunc { + return func(attempt int, resp *http.Response) time.Duration { + d := backoff(attempt, resp) + if hint, ok := parseRetryAfter(resp); ok && hint > d { + return hint + } + return d + } +} diff --git a/backoff_test.go b/backoff_test.go index 8f65b30..e56b146 100644 --- a/backoff_test.go +++ b/backoff_test.go @@ -1,6 +1,7 @@ package rhttp_test import ( + "net/http" "testing" "time" @@ -11,7 +12,7 @@ func TestConstantBackoff(t *testing.T) { backoff := rhttp.ConstantBackoff(100 * time.Millisecond) for attempt := 0; attempt < 10; attempt++ { - d := backoff(attempt) + d := backoff(attempt, nil) if d != 100*time.Millisecond { t.Errorf("attempt %d: expected 100ms, got %v", attempt, d) } @@ -31,7 +32,7 @@ func TestLinearBackoff(t *testing.T) { } for attempt, exp := range expected { - d := backoff(attempt) + d := backoff(attempt, nil) if d != exp { t.Errorf("attempt %d: expected %v, got %v", attempt, exp, d) } @@ -50,7 +51,7 @@ func TestExponentialBackoff_Growth(t *testing.T) { } for attempt, exp := range expectedBase { - d := backoff(attempt) + d := backoff(attempt, nil) // Allow 25% tolerance for jitter minExpected := time.Duration(float64(exp) * 0.75) maxExpected := time.Duration(float64(exp) * 1.25) @@ -64,7 +65,7 @@ func TestExponentialBackoff_Max(t *testing.T) { backoff := rhttp.ExponentialBackoff(100*time.Millisecond, 500*time.Millisecond) // After a few attempts, should be capped at max - d := backoff(10) + d := backoff(10, nil) // With jitter, should be within ±25% of 500ms if d > 625*time.Millisecond { t.Errorf("expected capped at ~500ms, got %v", d) @@ -85,7 +86,7 @@ func TestFibonacciBackoff(t *testing.T) { } for attempt, exp := range expected { - d := backoff(attempt) + d := backoff(attempt, nil) if d != exp { t.Errorf("attempt %d: expected %v, got %v", attempt, exp, d) } @@ -96,7 +97,7 @@ func TestFibonacciBackoff_Max(t *testing.T) { backoff := rhttp.FibonacciBackoff(100*time.Millisecond, 500*time.Millisecond) // Should cap at 500ms - d := backoff(10) + d := backoff(10, nil) if d != 500*time.Millisecond { t.Errorf("expected capped at 500ms, got %v", d) } @@ -106,14 +107,14 @@ func TestDecorrelatedJitterBackoff(t *testing.T) { backoff := rhttp.DecorrelatedJitterBackoff(100*time.Millisecond, 10*time.Second) // First attempt should be base - d0 := backoff(0) + d0 := backoff(0, nil) if d0 != 100*time.Millisecond { t.Errorf("attempt 0: expected 100ms, got %v", d0) } // Subsequent attempts should vary and be within bounds for i := 1; i < 5; i++ { - d := backoff(i) + d := backoff(i, nil) // Should be positive and not exceed max if d <= 0 || d > 10*time.Second { t.Errorf("attempt %d: unexpected duration %v", i, d) @@ -125,7 +126,7 @@ func TestExponentialBackoffFullJitter(t *testing.T) { backoff := rhttp.ExponentialBackoffFullJitter(100*time.Millisecond, 10*time.Second) for attempt := 0; attempt < 5; attempt++ { - d := backoff(attempt) + d := backoff(attempt, nil) ceiling := 100 * time.Millisecond * (1 << attempt) if ceiling > 10*time.Second { ceiling = 10 * time.Second @@ -142,7 +143,7 @@ func TestExponentialBackoffEqualJitter(t *testing.T) { backoff := rhttp.ExponentialBackoffEqualJitter(100*time.Millisecond, 10*time.Second) for attempt := 0; attempt < 5; attempt++ { - d := backoff(attempt) + d := backoff(attempt, nil) ceiling := 100 * time.Millisecond * (1 << attempt) if ceiling > 10*time.Second { ceiling = 10 * time.Second @@ -163,7 +164,7 @@ func TestWithJitter(t *testing.T) { // Run multiple times and check variance var minD, maxD time.Duration = time.Hour, 0 for i := 0; i < 100; i++ { - d := withJitter(0) + d := withJitter(0, nil) if d < minD { minD = d } @@ -186,7 +187,7 @@ func TestWithMax(t *testing.T) { capped := rhttp.WithMax(linear, 300*time.Millisecond) // attempt 5 would be 600ms without cap - d := capped(5) + d := capped(5, nil) if d != 300*time.Millisecond { t.Errorf("expected capped at 300ms, got %v", d) } @@ -196,7 +197,7 @@ func TestWithMin(t *testing.T) { constant := rhttp.ConstantBackoff(10 * time.Millisecond) withMin := rhttp.WithMin(constant, 100*time.Millisecond) - d := withMin(0) + d := withMin(0, nil) if d != 100*time.Millisecond { t.Errorf("expected min 100ms, got %v", d) } @@ -215,8 +216,96 @@ func BenchmarkBackoffStrategies(b *testing.B) { b.Run(name, func(b *testing.B) { b.ReportAllocs() for i := 0; i < b.N; i++ { - _ = backoff(i % 10) + _ = backoff(i%10, nil) } }) } } + +func retryAfterResponse(status int, value string) *http.Response { + resp := &http.Response{StatusCode: status, Header: make(http.Header)} + if value != "" { + resp.Header.Set("Retry-After", value) + } + return resp +} + +func TestWithRetryAfter_HeaderWinsOverBase(t *testing.T) { + backoff := rhttp.WithRetryAfter(rhttp.ConstantBackoff(10 * time.Millisecond)) + + d := backoff(0, retryAfterResponse(http.StatusTooManyRequests, "2")) + if d != 2*time.Second { + t.Errorf("expected 2s from Retry-After, got %v", d) + } +} + +func TestWithRetryAfter_BaseWinsWhenLarger(t *testing.T) { + backoff := rhttp.WithRetryAfter(rhttp.ConstantBackoff(5 * time.Second)) + + d := backoff(0, retryAfterResponse(http.StatusServiceUnavailable, "1")) + if d != 5*time.Second { + t.Errorf("expected base 5s to win, got %v", d) + } +} + +func TestWithRetryAfter_NilResponseUsesBase(t *testing.T) { + backoff := rhttp.WithRetryAfter(rhttp.ConstantBackoff(10 * time.Millisecond)) + + d := backoff(0, nil) + if d != 10*time.Millisecond { + t.Errorf("expected base 10ms, got %v", d) + } +} + +func TestWithRetryAfter_IgnoresOtherStatuses(t *testing.T) { + backoff := rhttp.WithRetryAfter(rhttp.ConstantBackoff(10 * time.Millisecond)) + + for _, status := range []int{http.StatusOK, http.StatusInternalServerError, http.StatusBadGateway} { + d := backoff(0, retryAfterResponse(status, "2")) + if d != 10*time.Millisecond { + t.Errorf("status %d: expected base 10ms, got %v", status, d) + } + } +} + +func TestWithRetryAfter_HTTPDateFormat(t *testing.T) { + backoff := rhttp.WithRetryAfter(rhttp.ConstantBackoff(10 * time.Millisecond)) + + future := time.Now().Add(2 * time.Second).UTC().Format(http.TimeFormat) + d := backoff(0, retryAfterResponse(http.StatusTooManyRequests, future)) + + if d < 1*time.Second || d > 3*time.Second { + t.Errorf("expected ~2s from HTTP-date, got %v", d) + } +} + +func TestWithRetryAfter_InvalidHeaderUsesBase(t *testing.T) { + backoff := rhttp.WithRetryAfter(rhttp.ConstantBackoff(10 * time.Millisecond)) + + for _, value := range []string{"", "garbage", "-5"} { + d := backoff(0, retryAfterResponse(http.StatusTooManyRequests, value)) + if d != 10*time.Millisecond { + t.Errorf("value %q: expected base 10ms, got %v", value, d) + } + } +} + +func TestExponentialVariants_OverflowReturnsMax(t *testing.T) { + const base = 100 * time.Millisecond + const maxDur = 10 * time.Second + + variants := map[string]rhttp.BackoffFunc{ + "Exponential": rhttp.ExponentialBackoff(base, maxDur), + "FullJitter": rhttp.ExponentialBackoffFullJitter(base, maxDur), + "EqualJitter": rhttp.ExponentialBackoffEqualJitter(base, maxDur), + } + + for name, backoff := range variants { + for _, attempt := range []int{37, 63, 64, 100} { + d := backoff(attempt, nil) + if d != maxDur { + t.Errorf("%s attempt %d: expected exactly maxDuration %v, got %v", name, attempt, maxDur, d) + } + } + } +} diff --git a/benchmark_test.go b/benchmark_test.go index 7dd1e6d..c67bb68 100644 --- a/benchmark_test.go +++ b/benchmark_test.go @@ -7,12 +7,11 @@ import ( "time" "github.com/oswaldom-code/rhttp" - "github.com/oswaldom-code/rhttp/internal" ) // noopRoundTripper returns immediately with a 200 OK response. // This isolates the benchmark to measure only client/middleware overhead. -var noopRoundTripper = internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { +var noopRoundTripper = rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { return &http.Response{ StatusCode: http.StatusOK, Body: http.NoBody, @@ -26,7 +25,7 @@ var noopLogger = rhttp.LoggerFunc(func(rhttp.LogEntry) {}) // noopRecorder discards all metric events var noopRecorder = rhttp.MetricsRecorderFunc(func(rhttp.MetricEvent) {}) -func BenchmarkClient_Baseline(b *testing.B) { +func BenchmarkMiddlewareOverhead_Baseline(b *testing.B) { c := rhttp.New(rhttp.WithTransport(noopRoundTripper)) req, _ := http.NewRequest(http.MethodGet, "http://example.com", http.NoBody) ctx := context.Background() @@ -39,7 +38,7 @@ func BenchmarkClient_Baseline(b *testing.B) { } } -func BenchmarkClient_WithTimeout(b *testing.B) { +func BenchmarkMiddlewareOverhead_WithTimeout(b *testing.B) { c := rhttp.New( rhttp.WithTransport(noopRoundTripper), rhttp.WithMiddleware(rhttp.Timeout(5*time.Second)), @@ -55,7 +54,7 @@ func BenchmarkClient_WithTimeout(b *testing.B) { } } -func BenchmarkClient_WithRetry(b *testing.B) { +func BenchmarkMiddlewareOverhead_WithRetry(b *testing.B) { c := rhttp.New( rhttp.WithTransport(noopRoundTripper), rhttp.WithMiddleware(rhttp.Retry(rhttp.RetryConfig{ @@ -73,7 +72,7 @@ func BenchmarkClient_WithRetry(b *testing.B) { } } -func BenchmarkClient_WithCircuitBreaker(b *testing.B) { +func BenchmarkMiddlewareOverhead_WithCircuitBreaker(b *testing.B) { c := rhttp.New( rhttp.WithTransport(noopRoundTripper), rhttp.WithMiddleware(rhttp.CircuitBreaker(rhttp.CircuitBreakerConfig{ @@ -92,7 +91,7 @@ func BenchmarkClient_WithCircuitBreaker(b *testing.B) { } } -func BenchmarkClient_WithLogging(b *testing.B) { +func BenchmarkMiddlewareOverhead_WithLogging(b *testing.B) { c := rhttp.New( rhttp.WithTransport(noopRoundTripper), rhttp.WithMiddleware(rhttp.Logging(rhttp.LoggingConfig{ @@ -110,7 +109,7 @@ func BenchmarkClient_WithLogging(b *testing.B) { } } -func BenchmarkClient_WithMetrics(b *testing.B) { +func BenchmarkMiddlewareOverhead_WithMetrics(b *testing.B) { c := rhttp.New( rhttp.WithTransport(noopRoundTripper), rhttp.WithMiddleware(rhttp.Metrics(rhttp.MetricsConfig{ @@ -128,7 +127,7 @@ func BenchmarkClient_WithMetrics(b *testing.B) { } } -func BenchmarkClient_AllMiddleware(b *testing.B) { +func BenchmarkMiddlewareOverhead_AllMiddleware(b *testing.B) { c := rhttp.New( rhttp.WithTransport(noopRoundTripper), rhttp.WithMiddleware( @@ -153,7 +152,7 @@ func BenchmarkClient_AllMiddleware(b *testing.B) { } } -func BenchmarkClient_Parallel(b *testing.B) { +func BenchmarkMiddlewareOverhead_Parallel(b *testing.B) { c := rhttp.New( rhttp.WithTransport(noopRoundTripper), rhttp.WithMiddleware( diff --git a/benchmarks/Makefile b/benchmarks/Makefile new file mode 100644 index 0000000..d958f25 --- /dev/null +++ b/benchmarks/Makefile @@ -0,0 +1,7 @@ +.PHONY: report bench + +report: + go run ./report -count 5 + +bench: + go test -bench=. -benchmem -count=5 -timeout=30m . diff --git a/benchmarks/README.md b/benchmarks/README.md new file mode 100644 index 0000000..11f71ed --- /dev/null +++ b/benchmarks/README.md @@ -0,0 +1,31 @@ +# Comparison benchmarks + +Standalone benchmark suite comparing rhttp against popular Go HTTP clients +(net/http, Resty, go-retryablehttp, Heimdall) under equivalent configuration. + +This is a **separate Go module** so the root rhttp module stays zero-dependency. +It is excluded automatically from root builds and tests. + +## Usage + +```bash +cd benchmarks + +# Full run + Markdown report (writes REPORT.md) +make report + +# Raw benchmark output only +make bench +``` + +`go run ./report` accepts `-count N` (samples per benchmark, default 5), +`-bench REGEX` and `-out FILE`. + +## Scenarios + +- **Overhead**: no-op transport, isolates client/middleware cost per request. +- **E2E**: local `httptest.Server` returning ~1 KB JSON over loopback. + +All clients: 5s timeout, 3 total attempts, exponential backoff 100ms-2s, body +fully consumed and closed. See the Methodology section of the generated report +for fairness caveats. diff --git a/benchmarks/REPORT.md b/benchmarks/REPORT.md new file mode 100644 index 0000000..185a1b4 --- /dev/null +++ b/benchmarks/REPORT.md @@ -0,0 +1,57 @@ +# HTTP client comparison report + +Generated: 2026-07-25 13:04 CEST + +## Environment + +| | | +|---|---| +| CPU | 12th Gen Intel(R) Core(TM) i7-1255U (12 threads) | +| OS/arch | linux/amd64 | +| Go | go1.24.1 | +| Samples per benchmark | 5 (min reported as typical cost) | + +## Tool versions + +- github.com/oswaldom-code/rhttp (local, via replace) +- github.com/go-resty/resty/v2 v2.17.2 +- github.com/hashicorp/go-retryablehttp v0.7.8 +- github.com/gojek/heimdall/v7 v7.0.3 + +## Methodology + +All clients are configured equivalently: 5s timeout, 3 total attempts, exponential backoff 100ms-2s. Every client fully consumes and closes the response body. + +- **Overhead**: a no-op transport returns 200 OK without touching the network, isolating client/middleware cost per request. +- **E2E**: a local httptest.Server returns ~1 KB of JSON over loopback, measuring total request cost including a real HTTP round trip. + +Caveats: net/http does not retry (it is the floor, not a symmetric competitor); Heimdall runs without its Hystrix circuit breaker (retry only, for feature symmetry); Resty buffers the full response body by design; loopback amplifies relative overhead — against a real network (0.5-500 ms) these differences are negligible. + +## Results: wrapper overhead (no network) + +| Client | ns/op (min) | ns/op (mean) | B/op | allocs/op | vs best | +|---|---:|---:|---:|---:|---:| +| rhttp (Timeout+Retry+CircuitBreaker) | 1012 | 1058 | 1468 | 12 | 1.00x | +| rhttp (Timeout+Retry) | 1019 | 1099 | 1468 | 12 | 1.01x | +| net/http (Timeout only, no retry) | 1784 | 1854 | 1594 | 26 | 1.76x | +| go-retryablehttp | 1909 | 1965 | 1595 | 26 | 1.89x | +| Heimdall (retry) | 2797 | 2969 | 2221 | 32 | 2.76x | +| Resty (retry) | 6486 | 6787 | 4885 | 48 | 6.41x | + +## Results: end-to-end (loopback, ~1 KB JSON) + +| Client | ns/op (min) | ns/op (mean) | B/op | allocs/op | vs best | +|---|---:|---:|---:|---:|---:| +| go-retryablehttp | 59892 | 63859 | 6357 | 74 | 1.00x | +| Heimdall (retry) | 61990 | 67502 | 6999 | 80 | 1.04x | +| net/http (Timeout only, no retry) | 62209 | 67362 | 6584 | 75 | 1.04x | +| rhttp (Timeout+Retry) | 63382 | 66040 | 7224 | 76 | 1.06x | +| rhttp (Timeout+Retry+CircuitBreaker) | 66125 | 67722 | 7185 | 76 | 1.10x | +| Resty (retry) | 72794 | 78349 | 10916 | 96 | 1.22x | + +## Reproduce + +```bash +cd benchmarks +go run ./report -count 5 +``` diff --git a/benchmarks/bench_test.go b/benchmarks/bench_test.go new file mode 100644 index 0000000..09f02d5 --- /dev/null +++ b/benchmarks/bench_test.go @@ -0,0 +1,220 @@ +package benchmarks + +import ( + "context" + "io" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/go-resty/resty/v2" + "github.com/gojek/heimdall/v7/httpclient" + "github.com/hashicorp/go-retryablehttp" + "github.com/oswaldom-code/rhttp" +) + +type rtFunc func(*http.Request) (*http.Response, error) + +func (f rtFunc) RoundTrip(req *http.Request) (*http.Response, error) { return f(req) } + +var noopTransport = rtFunc(func(req *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusOK, + Body: http.NoBody, + Request: req, + }, nil +}) + +func drain(resp *http.Response) { + _, _ = io.Copy(io.Discard, resp.Body) + _ = resp.Body.Close() +} + +func newRhttpFull(rt http.RoundTripper) *rhttp.Client { + return rhttp.New( + rhttp.WithTransport(rt), + rhttp.WithMiddleware( + rhttp.Timeout(5*time.Second), + rhttp.Retry(rhttp.RetryConfig{ + MaxAttempts: 3, + Backoff: rhttp.ExponentialBackoff(100*time.Millisecond, 2*time.Second), + }), + rhttp.CircuitBreaker(rhttp.CircuitBreakerConfig{ + FailureThreshold: 5, + ResetTimeout: 30 * time.Second, + }), + ), + ) +} + +func newRhttpRetryOnly(rt http.RoundTripper) *rhttp.Client { + return rhttp.New( + rhttp.WithTransport(rt), + rhttp.WithMiddleware( + rhttp.Timeout(5*time.Second), + rhttp.Retry(rhttp.RetryConfig{ + MaxAttempts: 3, + Backoff: rhttp.ExponentialBackoff(100*time.Millisecond, 2*time.Second), + }), + ), + ) +} + +func newResty(rt http.RoundTripper) *resty.Client { + return resty.New(). + SetTransport(rt). + SetTimeout(5 * time.Second). + SetRetryCount(2). + SetRetryWaitTime(100 * time.Millisecond). + SetRetryMaxWaitTime(2 * time.Second) +} + +func newRetryable(rt http.RoundTripper) *retryablehttp.Client { + c := retryablehttp.NewClient() + c.HTTPClient = &http.Client{Transport: rt, Timeout: 5 * time.Second} + c.RetryMax = 2 + c.RetryWaitMin = 100 * time.Millisecond + c.RetryWaitMax = 2 * time.Second + c.Logger = nil + return c +} + +func newHeimdall(rt http.RoundTripper) *httpclient.Client { + return httpclient.NewClient( + httpclient.WithHTTPClient(&http.Client{Transport: rt, Timeout: 5 * time.Second}), + httpclient.WithRetryCount(2), + ) +} + +func benchRhttp(b *testing.B, c *rhttp.Client, url string) { + req, _ := http.NewRequest(http.MethodGet, url, http.NoBody) + ctx := context.Background() + b.ResetTimer() + b.ReportAllocs() + for i := 0; i < b.N; i++ { + resp, err := c.Do(ctx, req) + if err != nil { + b.Fatal(err) + } + drain(resp) + } +} + +func benchNetHTTP(b *testing.B, c *http.Client, url string) { + req, _ := http.NewRequest(http.MethodGet, url, http.NoBody) + b.ResetTimer() + b.ReportAllocs() + for i := 0; i < b.N; i++ { + resp, err := c.Do(req) + if err != nil { + b.Fatal(err) + } + drain(resp) + } +} + +func benchResty(b *testing.B, c *resty.Client, url string) { + b.ResetTimer() + b.ReportAllocs() + for i := 0; i < b.N; i++ { + _, err := c.R().Get(url) + if err != nil { + b.Fatal(err) + } + } +} + +func benchRetryable(b *testing.B, c *retryablehttp.Client, url string) { + req, _ := retryablehttp.NewRequest(http.MethodGet, url, nil) + b.ResetTimer() + b.ReportAllocs() + for i := 0; i < b.N; i++ { + resp, err := c.Do(req) + if err != nil { + b.Fatal(err) + } + drain(resp) + } +} + +func benchHeimdall(b *testing.B, c *httpclient.Client, url string) { + b.ResetTimer() + b.ReportAllocs() + for i := 0; i < b.N; i++ { + resp, err := c.Get(url, nil) + if err != nil { + b.Fatal(err) + } + drain(resp) + } +} + +func BenchmarkOverhead_NetHTTP_Bare(b *testing.B) { + benchNetHTTP(b, &http.Client{Transport: noopTransport, Timeout: 5 * time.Second}, "http://example.com/users") +} + +func BenchmarkOverhead_Rhttp_TimeoutRetry(b *testing.B) { + benchRhttp(b, newRhttpRetryOnly(noopTransport), "http://example.com/users") +} + +func BenchmarkOverhead_Rhttp_FullStack(b *testing.B) { + benchRhttp(b, newRhttpFull(noopTransport), "http://example.com/users") +} + +func BenchmarkOverhead_Resty_Retry(b *testing.B) { + benchResty(b, newResty(noopTransport), "http://example.com/users") +} + +func BenchmarkOverhead_Retryablehttp(b *testing.B) { + benchRetryable(b, newRetryable(noopTransport), "http://example.com/users") +} + +func BenchmarkOverhead_Heimdall_Retry(b *testing.B) { + benchHeimdall(b, newHeimdall(noopTransport), "http://example.com/users") +} + +var payload = []byte(`{"users":[{"id":1,"name":"Ada Lovelace","email":"ada@example.com","active":true},{"id":2,"name":"Grace Hopper","email":"grace@example.com","active":true},{"id":3,"name":"Alan Turing","email":"alan@example.com","active":false},{"id":4,"name":"Dennis Ritchie","email":"dennis@example.com","active":true},{"id":5,"name":"Ken Thompson","email":"ken@example.com","active":true},{"id":6,"name":"Rob Pike","email":"rob@example.com","active":true},{"id":7,"name":"Robert Griesemer","email":"robert@example.com","active":true},{"id":8,"name":"Russ Cox","email":"russ@example.com","active":true},{"id":9,"name":"Brad Fitzpatrick","email":"brad@example.com","active":false},{"id":10,"name":"Ian Lance Taylor","email":"ian@example.com","active":true}],"total":10,"page":1,"per_page":10,"has_more":false}`) + +func newServer() *httptest.Server { + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write(payload) + })) +} + +func BenchmarkE2E_NetHTTP_Bare(b *testing.B) { + srv := newServer() + defer srv.Close() + benchNetHTTP(b, &http.Client{Timeout: 5 * time.Second}, srv.URL) +} + +func BenchmarkE2E_Rhttp_TimeoutRetry(b *testing.B) { + srv := newServer() + defer srv.Close() + benchRhttp(b, newRhttpRetryOnly(http.DefaultTransport), srv.URL) +} + +func BenchmarkE2E_Rhttp_FullStack(b *testing.B) { + srv := newServer() + defer srv.Close() + benchRhttp(b, newRhttpFull(http.DefaultTransport), srv.URL) +} + +func BenchmarkE2E_Resty_Retry(b *testing.B) { + srv := newServer() + defer srv.Close() + benchResty(b, newResty(http.DefaultTransport), srv.URL) +} + +func BenchmarkE2E_Retryablehttp(b *testing.B) { + srv := newServer() + defer srv.Close() + benchRetryable(b, newRetryable(http.DefaultTransport), srv.URL) +} + +func BenchmarkE2E_Heimdall_Retry(b *testing.B) { + srv := newServer() + defer srv.Close() + benchHeimdall(b, newHeimdall(http.DefaultTransport), srv.URL) +} diff --git a/benchmarks/go.mod b/benchmarks/go.mod new file mode 100644 index 0000000..0aef904 --- /dev/null +++ b/benchmarks/go.mod @@ -0,0 +1,23 @@ +module github.com/oswaldom-code/rhttp/benchmarks + +go 1.24.1 + +require ( + github.com/go-resty/resty/v2 v2.17.2 + github.com/gojek/heimdall/v7 v7.0.3 + github.com/hashicorp/go-retryablehttp v0.7.8 + github.com/oswaldom-code/rhttp v0.0.0 +) + +require ( + github.com/davecgh/go-spew v1.1.1 // indirect + github.com/gojek/valkyrie v0.0.0-20180215180059-6aee720afcdf // indirect + github.com/hashicorp/go-cleanhttp v0.5.2 // indirect + github.com/pkg/errors v0.9.1 // indirect + github.com/pmezard/go-difflib v1.0.0 // indirect + github.com/stretchr/objx v0.3.0 // indirect + github.com/stretchr/testify v1.3.0 // indirect + golang.org/x/net v0.43.0 // indirect +) + +replace github.com/oswaldom-code/rhttp => ../ diff --git a/benchmarks/go.sum b/benchmarks/go.sum new file mode 100644 index 0000000..fb6b512 --- /dev/null +++ b/benchmarks/go.sum @@ -0,0 +1,49 @@ +github.com/DataDog/datadog-go v3.7.1+incompatible/go.mod h1:LButxg5PwREeZtORoXG3tL4fMGNddJ+vMq1mwgfaqoQ= +github.com/afex/hystrix-go v0.0.0-20180502004556-fa1af6a1f4f5/go.mod h1:SkGFH1ia65gfNATL8TAiHDNxPzPdmEL5uirI2Uyuz6c= +github.com/cactus/go-statsd-client/statsd v0.0.0-20200423205355-cb0885a1018c/go.mod h1:l/bIBLeOl9eX+wxJAzxS4TveKRtAqlyDpHjhkfO0MEI= +github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= +github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/fatih/color v1.16.0 h1:zmkK9Ngbjj+K0yRhTVONQh1p/HknKYSlNT+vZCzyokM= +github.com/fatih/color v1.16.0/go.mod h1:fL2Sau1YI5c0pdGEVCbKQbLXB6edEj1ZgiY4NijnWvE= +github.com/go-resty/resty/v2 v2.17.2 h1:FQW5oHYcIlkCNrMD2lloGScxcHJ0gkjshV3qcQAyHQk= +github.com/go-resty/resty/v2 v2.17.2/go.mod h1:kCKZ3wWmwJaNc7S29BRtUhJwy7iqmn+2mLtQrOyQlVA= +github.com/gojek/heimdall/v7 v7.0.3 h1:+5sAhl8S0m+qRRL8IVeHCJudFh/XkG3wyO++nvOg+gc= +github.com/gojek/heimdall/v7 v7.0.3/go.mod h1:Z43HtMid7ysSjmsedPTXAki6jcdcNVnjn5pmsTyiMic= +github.com/gojek/valkyrie v0.0.0-20180215180059-6aee720afcdf h1:5xRGbUdOmZKoDXkGx5evVLehuCMpuO1hl701bEQqXOM= +github.com/gojek/valkyrie v0.0.0-20180215180059-6aee720afcdf/go.mod h1:QzhUKaYKJmcbTnCYCAVQrroCOY7vOOI8cSQ4NbuhYf0= +github.com/gopherjs/gopherjs v0.0.0-20181017120253-0766667cb4d1/go.mod h1:wJfORRmW1u3UXTncJ5qlYoELFm8eSnnEO6hX4iZ3EWY= +github.com/hashicorp/go-cleanhttp v0.5.2 h1:035FKYIWjmULyFRBKPs8TBQoi0x6d9G4xc9neXJWAZQ= +github.com/hashicorp/go-cleanhttp v0.5.2/go.mod h1:kO/YDlP8L1346E6Sodw+PrpBSV4/SoxCXGY6BqNFT48= +github.com/hashicorp/go-hclog v1.6.3 h1:Qr2kF+eVWjTiYmU7Y31tYlP1h0q/X3Nl3tPGdaB11/k= +github.com/hashicorp/go-hclog v1.6.3/go.mod h1:W4Qnvbt70Wk/zYJryRzDRU/4r0kIg0PVHBcfoyhpF5M= +github.com/hashicorp/go-retryablehttp v0.7.8 h1:ylXZWnqa7Lhqpk0L1P1LzDtGcCR0rPVUrx/c8Unxc48= +github.com/hashicorp/go-retryablehttp v0.7.8/go.mod h1:rjiScheydd+CxvumBsIrFKlx3iS0jrZ7LvzFGFmuKbw= +github.com/jtolds/gls v4.20.0+incompatible/go.mod h1:QJZ7F/aHp+rZTRtaJ1ow/lLfFfVYBRgL+9YlvaHOwJU= +github.com/mattn/go-colorable v0.1.13 h1:fFA4WZxdEF4tXPZVKMLwD8oUnCTTo08duU7wxecdEvA= +github.com/mattn/go-colorable v0.1.13/go.mod h1:7S9/ev0klgBDR4GtXTXX8a3vIGJpMovkB8vQcUbaXHg= +github.com/mattn/go-isatty v0.0.20 h1:xfD0iDuEKnDkl03q4limB+vH+GxLEtL/jb4xVJSWWEY= +github.com/mattn/go-isatty v0.0.20/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y= +github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= +github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= +github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= +github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +github.com/rcrowley/go-metrics v0.0.0-20201227073835-cf1acfcdf475/go.mod h1:bCqnVzQkZxMG4s8nGwiZ5l3QUCyqpo9Y+/ZMZ9VjZe4= +github.com/smartystreets/assertions v0.0.0-20180927180507-b2de0cb4f26d/go.mod h1:OnSkiWE9lh6wB0YB77sQom3nweQdgAjqCqsofrRNTgc= +github.com/smartystreets/goconvey v1.6.4/go.mod h1:syvi0/a8iFYH4r/RixwvyeAJjdLS9QV7WQ/tjFTllLA= +github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= +github.com/stretchr/objx v0.3.0 h1:NGXK3lHquSN08v5vWalVI/L8XU9hdzE/G6xsrze47As= +github.com/stretchr/objx v0.3.0/go.mod h1:qt09Ya8vawLte6SNmTgCsAVtYtaKzEcn8ATUoHMkEqE= +github.com/stretchr/testify v1.3.0 h1:TivCn/peBQ7UY8ooIcPgZFpTNSz0Q2U6UrFlUfqbe0Q= +github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= +golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= +golang.org/x/net v0.0.0-20190311183353-d8887717615a/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg= +golang.org/x/net v0.43.0 h1:lat02VYK2j4aLzMzecihNvTlJNQUq316m2Mr9rnM6YE= +golang.org/x/net v0.43.0/go.mod h1:vhO1fvI4dGsIjh73sWfUVjj3N7CA9WkKJNQm2svM6Jg= +golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= +golang.org/x/sys v0.35.0 h1:vz1N37gP5bs89s7He8XuIYXpyY0+QlsKmzipCbUtyxI= +golang.org/x/sys v0.35.0/go.mod h1:BJP2sWEmIv4KK5OTEluFJCKSidICx8ciO85XgH3Ak8k= +golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= +golang.org/x/time v0.12.0 h1:ScB/8o8olJvc+CQPWrK3fPZNfh7qgwCrY0zJmoEQLSE= +golang.org/x/time v0.12.0/go.mod h1:CDIdPxbZBQxdj6cxyCIdrNogrJKMJ7pr37NYpMcMDSg= +golang.org/x/tools v0.0.0-20190328211700-ab21143f2384/go.mod h1:LCzVGOaR6xXOjkQ3onu1FJEFr0SW1gC7cKk1uF8kGRs= diff --git a/benchmarks/report/main.go b/benchmarks/report/main.go new file mode 100644 index 0000000..2ceb0f8 --- /dev/null +++ b/benchmarks/report/main.go @@ -0,0 +1,230 @@ +package main + +import ( + "bufio" + "flag" + "fmt" + "os" + "os/exec" + "regexp" + "runtime" + "sort" + "strconv" + "strings" + "time" +) + +type result struct { + name string + ns []float64 + bytes int64 + allocs int64 +} + +var benchLine = regexp.MustCompile(`^(Benchmark\S+?)(?:-\d+)?\s+\d+\s+([\d.]+) ns/op\s+([\d.]+) B/op\s+([\d.]+) allocs/op`) + +var labels = map[string]string{ + "Overhead_NetHTTP_Bare": "net/http (Timeout only, no retry)", + "Overhead_Rhttp_TimeoutRetry": "rhttp (Timeout+Retry)", + "Overhead_Rhttp_FullStack": "rhttp (Timeout+Retry+CircuitBreaker)", + "Overhead_Resty_Retry": "Resty (retry)", + "Overhead_Retryablehttp": "go-retryablehttp", + "Overhead_Heimdall_Retry": "Heimdall (retry)", + "E2E_NetHTTP_Bare": "net/http (Timeout only, no retry)", + "E2E_Rhttp_TimeoutRetry": "rhttp (Timeout+Retry)", + "E2E_Rhttp_FullStack": "rhttp (Timeout+Retry+CircuitBreaker)", + "E2E_Resty_Retry": "Resty (retry)", + "E2E_Retryablehttp": "go-retryablehttp", + "E2E_Heimdall_Retry": "Heimdall (retry)", +} + +func cpuModel() string { + data, err := os.ReadFile("/proc/cpuinfo") + if err != nil { + return "unknown" + } + for _, line := range strings.Split(string(data), "\n") { + if strings.HasPrefix(line, "model name") { + if i := strings.Index(line, ":"); i >= 0 { + return strings.TrimSpace(line[i+1:]) + } + } + } + return "unknown" +} + +func depVersions() []string { + deps := []string{ + "github.com/go-resty/resty/v2", + "github.com/hashicorp/go-retryablehttp", + "github.com/gojek/heimdall/v7", + } + args := append([]string{"list", "-m", "-f", "{{.Path}} {{.Version}}"}, deps...) + out, err := exec.Command("go", args...).Output() + if err != nil { + return []string{"unavailable"} + } + var versions []string + for _, line := range strings.Split(strings.TrimSpace(string(out)), "\n") { + if line != "" { + versions = append(versions, line) + } + } + return versions +} + +func runBenchmarks(pattern string, count int) ([]string, error) { + cmd := exec.Command("go", "test", + "-bench="+pattern, "-benchmem", + "-count="+strconv.Itoa(count), + "-timeout=30m", ".") + stdout, err := cmd.StdoutPipe() + if err != nil { + return nil, err + } + cmd.Stderr = os.Stderr + if err := cmd.Start(); err != nil { + return nil, err + } + var lines []string + scanner := bufio.NewScanner(stdout) + for scanner.Scan() { + line := scanner.Text() + fmt.Fprintln(os.Stderr, line) + if benchLine.MatchString(line) { + lines = append(lines, line) + } + } + if err := cmd.Wait(); err != nil { + return nil, err + } + return lines, nil +} + +func parse(lines []string) map[string]*result { + results := make(map[string]*result) + for _, line := range lines { + m := benchLine.FindStringSubmatch(line) + if m == nil { + continue + } + name := strings.TrimPrefix(m[1], "Benchmark") + ns, _ := strconv.ParseFloat(m[2], 64) + bytes, _ := strconv.ParseFloat(m[3], 64) + allocs, _ := strconv.ParseFloat(m[4], 64) + r, ok := results[name] + if !ok { + r = &result{name: name} + results[name] = r + } + r.ns = append(r.ns, ns) + r.bytes = int64(bytes) + r.allocs = int64(allocs) + } + return results +} + +func minMean(xs []float64) (float64, float64) { + minV := xs[0] + sum := 0.0 + for _, x := range xs { + if x < minV { + minV = x + } + sum += x + } + return minV, sum / float64(len(xs)) +} + +func writeTable(sb *strings.Builder, prefix string, results map[string]*result) { + type row struct { + label string + minNs, meanNs float64 + bytes, allocs int64 + } + var rows []row + for name, r := range results { + if !strings.HasPrefix(name, prefix) { + continue + } + label, ok := labels[name] + if !ok { + label = name + } + minV, mean := minMean(r.ns) + rows = append(rows, row{label, minV, mean, r.bytes, r.allocs}) + } + if len(rows) == 0 { + sb.WriteString("_no results_\n\n") + return + } + sort.Slice(rows, func(i, j int) bool { return rows[i].minNs < rows[j].minNs }) + best := rows[0].minNs + sb.WriteString("| Client | ns/op (min) | ns/op (mean) | B/op | allocs/op | vs best |\n") + sb.WriteString("|---|---:|---:|---:|---:|---:|\n") + for _, r := range rows { + fmt.Fprintf(sb, "| %s | %.0f | %.0f | %d | %d | %.2fx |\n", + r.label, r.minNs, r.meanNs, r.bytes, r.allocs, r.minNs/best) + } + sb.WriteString("\n") +} + +func buildReport(results map[string]*result, count int) string { + var sb strings.Builder + sb.WriteString("# HTTP client comparison report\n\n") + fmt.Fprintf(&sb, "Generated: %s\n\n", time.Now().Format("2006-01-02 15:04 MST")) + sb.WriteString("## Environment\n\n") + sb.WriteString("| | |\n|---|---|\n") + fmt.Fprintf(&sb, "| CPU | %s (%d threads) |\n", cpuModel(), runtime.NumCPU()) + fmt.Fprintf(&sb, "| OS/arch | %s/%s |\n", runtime.GOOS, runtime.GOARCH) + fmt.Fprintf(&sb, "| Go | %s |\n", runtime.Version()) + fmt.Fprintf(&sb, "| Samples per benchmark | %d (min reported as typical cost) |\n\n", count) + sb.WriteString("## Tool versions\n\n") + sb.WriteString("- github.com/oswaldom-code/rhttp (local, via replace)\n") + for _, v := range depVersions() { + fmt.Fprintf(&sb, "- %s\n", v) + } + sb.WriteString("\n## Methodology\n\n") + sb.WriteString("All clients are configured equivalently: 5s timeout, 3 total attempts, ") + sb.WriteString("exponential backoff 100ms-2s. Every client fully consumes and closes the ") + sb.WriteString("response body.\n\n") + sb.WriteString("- **Overhead**: a no-op transport returns 200 OK without touching the ") + sb.WriteString("network, isolating client/middleware cost per request.\n") + sb.WriteString("- **E2E**: a local httptest.Server returns ~1 KB of JSON over loopback, ") + sb.WriteString("measuring total request cost including a real HTTP round trip.\n\n") + sb.WriteString("Caveats: net/http does not retry (it is the floor, not a symmetric ") + sb.WriteString("competitor); Heimdall runs without its Hystrix circuit breaker (retry ") + sb.WriteString("only, for feature symmetry); Resty buffers the full response body by ") + sb.WriteString("design; loopback amplifies relative overhead — against a real network ") + sb.WriteString("(0.5-500 ms) these differences are negligible.\n\n") + sb.WriteString("## Results: wrapper overhead (no network)\n\n") + writeTable(&sb, "Overhead_", results) + sb.WriteString("## Results: end-to-end (loopback, ~1 KB JSON)\n\n") + writeTable(&sb, "E2E_", results) + sb.WriteString("## Reproduce\n\n") + sb.WriteString("```bash\ncd benchmarks\ngo run ./report -count 5\n```\n") + return sb.String() +} + +func main() { + count := flag.Int("count", 5, "runs per benchmark") + pattern := flag.String("bench", ".", "benchmark regex") + out := flag.String("out", "REPORT.md", "output file") + flag.Parse() + + lines, err := runBenchmarks(*pattern, *count) + if err != nil { + fmt.Fprintln(os.Stderr, "error:", err) + os.Exit(1) + } + results := parse(lines) + if len(results) == 0 { + fmt.Fprintln(os.Stderr, "error: no benchmark results parsed") + os.Exit(1) + } + if err := os.WriteFile(*out, []byte(buildReport(results, *count)), 0o644); err != nil { + fmt.Fprintln(os.Stderr, "error:", err) + os.Exit(1) + } + fmt.Println("report written to", *out) +} diff --git a/circuitbreaker.go b/circuitbreaker.go index 06ddff5..18f19d9 100644 --- a/circuitbreaker.go +++ b/circuitbreaker.go @@ -15,6 +15,20 @@ const ( CircuitHalfOpen ) +// String returns the lowercase name of the state. +func (s CircuitState) String() string { + switch s { + case CircuitClosed: + return "closed" + case CircuitOpen: + return "open" + case CircuitHalfOpen: + return "half-open" + default: + return "unknown" + } +} + // CircuitBreakerConfig configures the circuit breaker middleware. type CircuitBreakerConfig struct { // FailureThreshold is the number of consecutive failures before opening the circuit. @@ -38,16 +52,13 @@ type CircuitBreakerConfig struct { func DefaultIsFailure(resp *http.Response, err error) bool { if err != nil { - return true - } - if resp != nil && resp.StatusCode >= 500 { - return true + return Classify(err).Kind != ErrKindCanceled } - return false + return resp != nil && resp.StatusCode >= 500 } -// CircuitBreaker returns a middleware that implements the circuit breaker pattern. -func CircuitBreaker(cfg CircuitBreakerConfig) Middleware { +// newCircuitBreaker applies defaults and returns a circuit-breaker state machine. +func newCircuitBreaker(cfg CircuitBreakerConfig) *circuitBreaker { if cfg.FailureThreshold <= 0 { cfg.FailureThreshold = 5 } @@ -64,55 +75,58 @@ func CircuitBreaker(cfg CircuitBreakerConfig) Middleware { cfg.SuccessThreshold = 1 } - cb := &circuitBreaker{ - cfg: cfg, - state: CircuitClosed, - } + return &circuitBreaker{cfg: cfg, state: CircuitClosed} +} +func CircuitBreaker(cfg CircuitBreakerConfig) Middleware { return func(next http.RoundTripper) http.RoundTripper { - cb.next = next - return cb + return circuitBreakerRoundTripper{next: next, cb: newCircuitBreaker(cfg)} } } type circuitBreaker struct { - next http.RoundTripper - cfg CircuitBreakerConfig + cfg CircuitBreakerConfig mu sync.Mutex state CircuitState + generation uint64 failures int lastFailureTime time.Time halfOpenInFlight int halfOpenSuccess int } -func (cb *circuitBreaker) allowRequest() bool { +// allowRequest reports whether the request is admitted and returns the +// generation under which it was admitted. Every state transition bumps the +// generation, so recordResult can discard results from requests that outlived +// the state in which they were admitted. +func (cb *circuitBreaker) allowRequest() (admitted bool, gen uint64) { cb.mu.Lock() defer cb.mu.Unlock() switch cb.state { case CircuitClosed: - return true + return true, cb.generation case CircuitOpen: if time.Since(cb.lastFailureTime) >= cb.cfg.ResetTimeout { cb.state = CircuitHalfOpen + cb.generation++ cb.halfOpenSuccess = 0 cb.halfOpenInFlight = 1 - return true + return true, cb.generation } - return false + return false, cb.generation case CircuitHalfOpen: if cb.halfOpenInFlight < cb.cfg.MaxHalfOpenRequests { cb.halfOpenInFlight++ - return true + return true, cb.generation } - return false + return false, cb.generation default: - return true + return true, cb.generation } } @@ -126,6 +140,7 @@ func (cb *circuitBreaker) recordClosedResult(isFailure bool) { cb.lastFailureTime = time.Now() if cb.failures >= cb.cfg.FailureThreshold { cb.state = CircuitOpen + cb.generation++ } } @@ -136,9 +151,11 @@ func (cb *circuitBreaker) recordHalfOpenResult(isFailure bool) { if isFailure { cb.state = CircuitOpen + cb.generation++ cb.lastFailureTime = time.Now() cb.failures = cb.cfg.FailureThreshold cb.halfOpenSuccess = 0 + cb.halfOpenInFlight = 0 return } @@ -148,16 +165,24 @@ func (cb *circuitBreaker) recordHalfOpenResult(isFailure bool) { } cb.state = CircuitClosed + cb.generation++ cb.failures = 0 cb.halfOpenSuccess = 0 cb.halfOpenInFlight = 0 } -func (cb *circuitBreaker) recordResult(resp *http.Response, err error) { +func (cb *circuitBreaker) recordResult(resp *http.Response, err error, gen uint64) { + isFailure := cb.cfg.IsFailure(resp, err) + cb.mu.Lock() defer cb.mu.Unlock() - isFailure := cb.cfg.IsFailure(resp, err) + // Discard results from a bygone episode: the state under which the request + // was admitted no longer exists, so counting it would corrupt the current + // one (e.g. a slow Closed request closing a Half-Open circuit). + if gen != cb.generation { + return + } switch cb.state { case CircuitClosed: @@ -167,27 +192,65 @@ func (cb *circuitBreaker) recordResult(resp *http.Response, err error) { cb.recordHalfOpenResult(isFailure) case CircuitOpen: - // Unreachable: allowRequest rejects requests while Open, so a result - // is never recorded in this state. Handled to keep the switch exhaustive. + // Unreachable: a request is only admitted while Closed or on the + // transition into Half-Open, and every transition bumps the generation. + // A result observed while the breaker sits in Open therefore carries a + // stale generation and was already discarded above. Kept for switch + // exhaustiveness. } } -func (cb *circuitBreaker) RoundTrip(req *http.Request) (*http.Response, error) { - if !cb.allowRequest() { +// State returns the current state of the circuit breaker. +// Useful for monitoring and testing. +func (cb *circuitBreaker) State() CircuitState { + cb.mu.Lock() + defer cb.mu.Unlock() + return cb.state +} + +type circuitBreakerRoundTripper struct { + next http.RoundTripper + cb *circuitBreaker +} + +func (rt circuitBreakerRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { + allowed, gen := rt.cb.allowRequest() + if !allowed { + closeRequestBody(req) return nil, ErrCircuitOpen } - resp, err := cb.next.RoundTrip(req) + resp, err := rt.next.RoundTrip(req) - cb.recordResult(resp, err) + rt.cb.recordResult(resp, err, gen) return resp, err } -// State returns the current state of the circuit breaker. -// Useful for monitoring and testing. -func (cb *circuitBreaker) State() CircuitState { - cb.mu.Lock() - defer cb.mu.Unlock() - return cb.state +// SharedCircuitBreaker is a circuit breaker whose state can be shared across +// multiple middleware applications or clients. Unlike the CircuitBreaker +// middleware, which creates an independent breaker per application, all +// middleware derived from the same SharedCircuitBreaker observe the same state. +type SharedCircuitBreaker struct { + cb *circuitBreaker +} + +// NewCircuitBreaker creates a SharedCircuitBreaker with the given configuration, +// applying defaults for any zero-valued fields. Use it when several clients must +// trip together against the same dependency. +func NewCircuitBreaker(cfg CircuitBreakerConfig) *SharedCircuitBreaker { + return &SharedCircuitBreaker{cb: newCircuitBreaker(cfg)} +} + +// Middleware returns a Middleware backed by this shared breaker. Applying it to +// multiple clients makes them share a single circuit state. +func (s *SharedCircuitBreaker) Middleware() Middleware { + return func(next http.RoundTripper) http.RoundTripper { + return circuitBreakerRoundTripper{next: next, cb: s.cb} + } +} + +// State returns the current state of the shared circuit breaker. +func (s *SharedCircuitBreaker) State() CircuitState { + return s.cb.State() } diff --git a/circuitbreaker_test.go b/circuitbreaker_test.go index 576fbd7..a044543 100644 --- a/circuitbreaker_test.go +++ b/circuitbreaker_test.go @@ -4,18 +4,19 @@ import ( "context" "errors" "net/http" + "net/url" + "strings" "sync" "sync/atomic" "testing" "time" "github.com/oswaldom-code/rhttp" - "github.com/oswaldom-code/rhttp/internal" ) func TestCircuitBreaker_ClosedState_AllowsRequests(t *testing.T) { var calls int32 - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { atomic.AddInt32(&calls, 1) return &http.Response{StatusCode: http.StatusOK, Request: req}, nil }) @@ -46,7 +47,7 @@ func TestCircuitBreaker_ClosedState_AllowsRequests(t *testing.T) { func TestCircuitBreaker_OpensAfterFailureThreshold(t *testing.T) { var calls int32 - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { atomic.AddInt32(&calls, 1) return nil, errors.New("connection refused") }) @@ -86,7 +87,7 @@ func TestCircuitBreaker_OpensAfterFailureThreshold(t *testing.T) { func TestCircuitBreaker_TransitionsToHalfOpenAfterTimeout(t *testing.T) { var calls int32 shouldSucceed := false - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { atomic.AddInt32(&calls, 1) if shouldSucceed { return &http.Response{StatusCode: http.StatusOK, Request: req}, nil @@ -116,7 +117,7 @@ func TestCircuitBreaker_TransitionsToHalfOpenAfterTimeout(t *testing.T) { } // Wait for reset timeout - time.Sleep(60 * time.Millisecond) + time.Sleep(110 * time.Millisecond) // Now circuit should be half-open, next request goes through shouldSucceed = true @@ -133,7 +134,7 @@ func TestCircuitBreaker_TransitionsToHalfOpenAfterTimeout(t *testing.T) { func TestCircuitBreaker_HalfOpenSuccessCloses(t *testing.T) { callCount := 0 - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { callCount++ if callCount <= 2 { return nil, errors.New("connection refused") @@ -156,7 +157,7 @@ func TestCircuitBreaker_HalfOpenSuccessCloses(t *testing.T) { } // Wait for half-open - time.Sleep(15 * time.Millisecond) + time.Sleep(60 * time.Millisecond) // Success in half-open should close circuit req, _ := http.NewRequest(http.MethodGet, "http://example.com", http.NoBody) @@ -176,7 +177,7 @@ func TestCircuitBreaker_HalfOpenSuccessCloses(t *testing.T) { } func TestCircuitBreaker_HalfOpenFailureReopens(t *testing.T) { - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { return nil, errors.New("connection refused") }) @@ -195,7 +196,7 @@ func TestCircuitBreaker_HalfOpenFailureReopens(t *testing.T) { } // Wait for half-open - time.Sleep(15 * time.Millisecond) + time.Sleep(60 * time.Millisecond) // Failure in half-open should reopen circuit req, _ := http.NewRequest(http.MethodGet, "http://example.com", http.NoBody) @@ -211,8 +212,32 @@ func TestCircuitBreaker_HalfOpenFailureReopens(t *testing.T) { } func TestCircuitBreaker_SuccessResetsFailureCount(t *testing.T) { + // Control case: with threshold 3 and no intermediate success, the third + // consecutive failure must open the circuit. Without this, the assertion + // below would also pass if failures were never counted at all. + var failCalls int32 + failRT := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + atomic.AddInt32(&failCalls, 1) + return nil, errors.New("connection refused") + }) + cFail := rhttp.New( + rhttp.WithTransport(failRT), + rhttp.WithMiddleware(rhttp.CircuitBreaker(rhttp.CircuitBreakerConfig{ + FailureThreshold: 3, + ResetTimeout: 1 * time.Hour, + })), + ) + for i := 0; i < 3; i++ { + req, _ := http.NewRequest(http.MethodGet, "http://example.com", http.NoBody) + _, _ = cFail.Do(context.Background(), req) + } + req, _ := http.NewRequest(http.MethodGet, "http://example.com", http.NoBody) + if _, err := cFail.Do(context.Background(), req); !errors.Is(err, rhttp.ErrCircuitOpen) { + t.Fatalf("control case: expected open circuit after 3 straight failures, got %v", err) + } + callCount := 0 - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { callCount++ // Fail on calls 1, 2, then succeed, then fail on 4, 5 if callCount <= 2 || callCount >= 4 && callCount <= 5 { @@ -236,7 +261,7 @@ func TestCircuitBreaker_SuccessResetsFailureCount(t *testing.T) { } // 1 success - should reset counter - req, _ := http.NewRequest(http.MethodGet, "http://example.com", http.NoBody) + req, _ = http.NewRequest(http.MethodGet, "http://example.com", http.NoBody) _, _ = c.Do(context.Background(), req) // 2 more failures - should not open circuit (counter was reset) @@ -257,7 +282,7 @@ func TestCircuitBreaker_SuccessResetsFailureCount(t *testing.T) { func TestCircuitBreaker_5xxStatusCountsAsFailure(t *testing.T) { var calls int32 - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { atomic.AddInt32(&calls, 1) return &http.Response{StatusCode: http.StatusInternalServerError, Request: req}, nil }) @@ -290,7 +315,7 @@ func TestCircuitBreaker_5xxStatusCountsAsFailure(t *testing.T) { func TestCircuitBreaker_ThreadSafety(t *testing.T) { var calls int64 - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { atomic.AddInt64(&calls, 1) return &http.Response{StatusCode: http.StatusOK, Request: req}, nil }) @@ -337,7 +362,7 @@ func newBlockingProbe() *blockingProbe { } } -func (b *blockingProbe) rt() internal.RoundTripperFunc { +func (b *blockingProbe) rt() rhttp.RoundTripperFunc { return func(req *http.Request) (*http.Response, error) { if !b.halfOpen.Load() { return nil, errors.New("connection refused") @@ -349,7 +374,7 @@ func (b *blockingProbe) rt() internal.RoundTripperFunc { } } -func openCircuit(t *testing.T, c rhttp.Client, times int) { +func openCircuit(t *testing.T, c *rhttp.Client, times int) { t.Helper() for i := 0; i < times; i++ { req, _ := http.NewRequest(http.MethodGet, "http://example.com", http.NoBody) @@ -371,7 +396,7 @@ func TestCircuitBreaker_HalfOpenAdmitsSingleProbeByDefault(t *testing.T) { ) openCircuit(t, c, 2) - time.Sleep(15 * time.Millisecond) + time.Sleep(60 * time.Millisecond) bp.halfOpen.Store(true) // One probe transitions to half-open and blocks inside the transport. @@ -414,7 +439,7 @@ func TestCircuitBreaker_HalfOpenRespectsMaxHalfOpenRequests(t *testing.T) { ) openCircuit(t, c, 2) - time.Sleep(15 * time.Millisecond) + time.Sleep(60 * time.Millisecond) bp.halfOpen.Store(true) // Admit maxProbes concurrent probes; hold them all in flight. @@ -447,7 +472,7 @@ func TestCircuitBreaker_HalfOpenRespectsMaxHalfOpenRequests(t *testing.T) { func TestCircuitBreaker_OneSuccessDoesNotCloseWithThreshold(t *testing.T) { var succeed atomic.Bool - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { if succeed.Load() { return &http.Response{StatusCode: http.StatusOK, Request: req}, nil } @@ -464,7 +489,7 @@ func TestCircuitBreaker_OneSuccessDoesNotCloseWithThreshold(t *testing.T) { ) openCircuit(t, c, 2) - time.Sleep(15 * time.Millisecond) + time.Sleep(60 * time.Millisecond) // First half-open probe succeeds (1 of 2 required). succeed.Store(true) @@ -488,7 +513,7 @@ func TestCircuitBreaker_OneSuccessDoesNotCloseWithThreshold(t *testing.T) { func TestCircuitBreaker_ClosesAfterSuccessThreshold(t *testing.T) { var succeed atomic.Bool var calls int32 - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { atomic.AddInt32(&calls, 1) if succeed.Load() { return &http.Response{StatusCode: http.StatusOK, Request: req}, nil @@ -506,7 +531,7 @@ func TestCircuitBreaker_ClosesAfterSuccessThreshold(t *testing.T) { ) openCircuit(t, c, 2) - time.Sleep(15 * time.Millisecond) + time.Sleep(60 * time.Millisecond) succeed.Store(true) // Two sequential half-open successes close the circuit. @@ -537,9 +562,105 @@ func TestCircuitBreaker_ClosesAfterSuccessThreshold(t *testing.T) { } } +func TestCircuitBreaker_MiddlewareApplicationsAreIndependent(t *testing.T) { + mw := rhttp.CircuitBreaker(rhttp.CircuitBreakerConfig{ + FailureThreshold: 1, + ResetTimeout: time.Hour, + }) + + backendErr := errors.New("backend down") + failing := rhttp.RoundTripperFunc(func(*http.Request) (*http.Response, error) { + return nil, backendErr + }) + healthy := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + return &http.Response{StatusCode: http.StatusOK, Body: http.NoBody, Request: req}, nil + }) + + chainA := mw(failing) + chainB := mw(healthy) + + reqA, _ := http.NewRequest(http.MethodGet, "http://a.example", http.NoBody) + if _, err := chainA.RoundTrip(reqA); !errors.Is(err, backendErr) { + t.Fatalf("chain A reached the wrong transport (next overwritten by chain B): err=%v", err) + } + + reqB, _ := http.NewRequest(http.MethodGet, "http://b.example", http.NoBody) + resp, err := chainB.RoundTrip(reqB) + if errors.Is(err, rhttp.ErrCircuitOpen) { + t.Fatal("chain B's circuit opened due to chain A's failures: shared state") + } + if resp == nil || resp.StatusCode != http.StatusOK { + t.Fatalf("chain B did not reach its own transport: resp=%v err=%v", resp, err) + } +} + +func TestCircuitBreaker_SharedInstanceSharesState(t *testing.T) { + shared := rhttp.NewCircuitBreaker(rhttp.CircuitBreakerConfig{ + FailureThreshold: 1, + ResetTimeout: time.Hour, + }) + + backendErr := errors.New("backend down") + failing := rhttp.RoundTripperFunc(func(*http.Request) (*http.Response, error) { + return nil, backendErr + }) + healthy := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + return &http.Response{StatusCode: http.StatusOK, Body: http.NoBody, Request: req}, nil + }) + + chainA := shared.Middleware()(failing) + chainB := shared.Middleware()(healthy) + + reqA, _ := http.NewRequest(http.MethodGet, "http://a.example", http.NoBody) + if _, err := chainA.RoundTrip(reqA); !errors.Is(err, backendErr) { + t.Fatalf("chain A should reach its failing transport, got %v", err) + } + + reqB, _ := http.NewRequest(http.MethodGet, "http://b.example", http.NoBody) + if _, err := chainB.RoundTrip(reqB); !errors.Is(err, rhttp.ErrCircuitOpen) { + t.Fatalf("shared breaker: chain B should see the circuit opened by chain A, got %v", err) + } +} + +func TestCircuitBreaker_ClientCancellationsDoNotOpenCircuit(t *testing.T) { + rt := rhttp.CircuitBreaker(rhttp.CircuitBreakerConfig{ + FailureThreshold: 3, + ResetTimeout: time.Hour, + })(rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + return nil, &url.Error{Op: "Get", URL: req.URL.String(), Err: context.Canceled} + })) + + req, _ := http.NewRequest(http.MethodGet, "http://example.com", http.NoBody) + for i := 0; i < 5; i++ { + _, _ = rt.RoundTrip(req) + } + + if _, err := rt.RoundTrip(req); errors.Is(err, rhttp.ErrCircuitOpen) { + t.Fatal("client cancellations opened the circuit against a healthy upstream") + } +} + +func TestCircuitBreaker_TimeoutsOpenCircuit(t *testing.T) { + rt := rhttp.CircuitBreaker(rhttp.CircuitBreakerConfig{ + FailureThreshold: 3, + ResetTimeout: time.Hour, + })(rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + return nil, &url.Error{Op: "Get", URL: req.URL.String(), Err: context.DeadlineExceeded} + })) + + req, _ := http.NewRequest(http.MethodGet, "http://example.com", http.NoBody) + for i := 0; i < 3; i++ { + _, _ = rt.RoundTrip(req) + } + + if _, err := rt.RoundTrip(req); !errors.Is(err, rhttp.ErrCircuitOpen) { + t.Fatal("timeouts should count as failures and open the circuit") + } +} + func TestCircuitBreaker_CustomIsFailure(t *testing.T) { var calls int32 - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { atomic.AddInt32(&calls, 1) // Return 429 which is not a 5xx return &http.Response{StatusCode: http.StatusTooManyRequests, Request: req}, nil @@ -579,3 +700,203 @@ func TestCircuitBreaker_CustomIsFailure(t *testing.T) { t.Fatalf("expected circuit to open with custom IsFailure, got %v", err) } } + +func TestCircuitBreaker_IsFailureMayCallState(t *testing.T) { + var scb *rhttp.SharedCircuitBreaker + scb = rhttp.NewCircuitBreaker(rhttp.CircuitBreakerConfig{ + FailureThreshold: 2, + IsFailure: func(_ *http.Response, err error) bool { + // A user callback that inspects the breaker must not deadlock. + _ = scb.State() + return err != nil + }, + }) + + rt := rhttp.RoundTripperFunc(func(_ *http.Request) (*http.Response, error) { + return nil, errors.New("boom") + }) + c := rhttp.New( + rhttp.WithTransport(rt), + rhttp.WithMiddleware(scb.Middleware()), + ) + + done := make(chan struct{}) + go func() { + req, _ := http.NewRequest(http.MethodGet, "http://example.com", http.NoBody) + _, _ = c.Do(context.Background(), req) + close(done) + }() + + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("IsFailure calling State() deadlocked recordResult") + } +} + +// Regression (C2): a slow request admitted while Closed must not have its late +// result counted against a later Half-Open episode. Without generation gating, +// its success runs recordHalfOpenResult, closing the circuit and freeing the +// real probe's budget. +func TestCircuitBreaker_StaleResultDoesNotCloseHalfOpen(t *testing.T) { + enteredA := make(chan struct{}) + relA := make(chan struct{}) + enteredC := make(chan struct{}) + relC := make(chan struct{}) + + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + switch req.Header.Get("X-Role") { + case "fail": + return nil, errors.New("connection refused") + case "slow-closed": + close(enteredA) + <-relA + return &http.Response{StatusCode: http.StatusOK, Request: req}, nil + case "probe": + close(enteredC) + <-relC + return &http.Response{StatusCode: http.StatusOK, Request: req}, nil + default: + return &http.Response{StatusCode: http.StatusOK, Request: req}, nil + } + }) + + scb := rhttp.NewCircuitBreaker(rhttp.CircuitBreakerConfig{ + FailureThreshold: 1, + ResetTimeout: 10 * time.Millisecond, + }) + c := rhttp.New( + rhttp.WithTransport(rt), + rhttp.WithMiddleware(scb.Middleware()), + ) + + do := func(role string) { + req, _ := http.NewRequest(http.MethodGet, "http://example.com", http.NoBody) + req.Header.Set("X-Role", role) + _, _ = c.Do(context.Background(), req) + } + + var wg sync.WaitGroup + + // 1. Admit a slow request while Closed; hold it in flight. + doneA := make(chan struct{}) + wg.Add(1) + go func() { defer wg.Done(); do("slow-closed"); close(doneA) }() + <-enteredA + + // 2. A failure opens the circuit (threshold 1). + do("fail") + if got := scb.State(); got != rhttp.CircuitOpen { + t.Fatalf("expected Open after failure, got %v", got) + } + + // 3. After the reset timeout, admit a probe; hold it in flight (Half-Open). + time.Sleep(60 * time.Millisecond) + wg.Add(1) + go func() { defer wg.Done(); do("probe") }() + <-enteredC + if got := scb.State(); got != rhttp.CircuitHalfOpen { + t.Fatalf("expected Half-Open with a probe in flight, got %v", got) + } + + // 4. The stale Closed-era request completes successfully and is recorded. + close(relA) + <-doneA + + // 5. The circuit must stay Half-Open and the probe budget must remain taken. + if got := scb.State(); got != rhttp.CircuitHalfOpen { + t.Fatalf("stale Closed result closed the circuit: state=%v", got) + } + req, _ := http.NewRequest(http.MethodGet, "http://example.com", http.NoBody) + req.Header.Set("X-Role", "check") + if _, err := c.Do(context.Background(), req); !errors.Is(err, rhttp.ErrCircuitOpen) { + t.Fatalf("stale result freed the half-open budget: got %v", err) + } + + close(relC) + wg.Wait() +} + +func TestCircuitState_String(t *testing.T) { + cases := map[rhttp.CircuitState]string{ + rhttp.CircuitClosed: "closed", + rhttp.CircuitOpen: "open", + rhttp.CircuitHalfOpen: "half-open", + rhttp.CircuitState(99): "unknown", + } + for state, want := range cases { + if got := state.String(); got != want { + t.Errorf("state %d: expected %q, got %q", int(state), want, got) + } + } +} + +func TestCircuitBreaker_OpenClosesRequestBody(t *testing.T) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + return nil, errors.New("connection refused") + }) + c := rhttp.New( + rhttp.WithTransport(rt), + rhttp.WithMiddleware(rhttp.CircuitBreaker(rhttp.CircuitBreakerConfig{ + FailureThreshold: 1, + ResetTimeout: time.Hour, + })), + ) + + req, _ := http.NewRequest(http.MethodGet, "http://example.com", http.NoBody) + _, _ = c.Do(context.Background(), req) + + rec := &closeRecorder{Reader: strings.NewReader("payload")} + req, _ = http.NewRequest(http.MethodPut, "http://example.com", http.NoBody) + req.Body = rec + _, err := c.Do(context.Background(), req) + + if !errors.Is(err, rhttp.ErrCircuitOpen) { + t.Fatalf("expected ErrCircuitOpen, got %v", err) + } + if !rec.closed { + t.Error("request body was not closed on circuit-open short-circuit") + } +} + +func TestSharedCircuitBreaker_StateObservesTransitions(t *testing.T) { + bp := newBlockingProbe() + shared := rhttp.NewCircuitBreaker(rhttp.CircuitBreakerConfig{ + FailureThreshold: 2, + ResetTimeout: 10 * time.Millisecond, + }) + c := rhttp.New( + rhttp.WithTransport(bp.rt()), + rhttp.WithMiddleware(shared.Middleware()), + ) + + if got := shared.State(); got != rhttp.CircuitClosed { + t.Fatalf("expected initial state closed, got %v", got) + } + + openCircuit(t, c, 2) + if got := shared.State(); got != rhttp.CircuitOpen { + t.Fatalf("expected open after %d failures, got %v", 2, got) + } + + time.Sleep(60 * time.Millisecond) + bp.halfOpen.Store(true) + + done := make(chan struct{}) + go func() { + defer close(done) + req, _ := http.NewRequest(http.MethodGet, "http://example.com", http.NoBody) + _, _ = c.Do(context.Background(), req) + }() + + <-bp.entered + if got := shared.State(); got != rhttp.CircuitHalfOpen { + t.Fatalf("expected half-open while the probe is in flight, got %v", got) + } + + close(bp.release) + <-done + if got := shared.State(); got != rhttp.CircuitClosed { + t.Fatalf("expected closed after a successful probe, got %v", got) + } +} diff --git a/client.go b/client.go index 19dced6..5f6fbc5 100644 --- a/client.go +++ b/client.go @@ -5,17 +5,14 @@ import ( "net/http" ) -// Client defines the interface for executing HTTP requests. -type Client interface { - Do(ctx context.Context, req *http.Request) (*http.Response, error) -} - -type client struct { +// Client executes HTTP requests through the configured middleware chain. +// It is safe for concurrent use by multiple goroutines. +type Client struct { rt http.RoundTripper } // New creates a new Client with the given options. -func New(opts ...Option) Client { +func New(opts ...Option) *Client { cfg := defaultConfig() for _, opt := range opts { opt(cfg) @@ -30,14 +27,17 @@ func New(opts ...Option) Client { rt = chain(rt, cfg.middleware...) } - return &client{rt: rt} + return &Client{rt: rt} } // Do executes the request with the configured middleware chain. -func (c *client) Do(ctx context.Context, req *http.Request) (*http.Response, error) { +func (c *Client) Do(ctx context.Context, req *http.Request) (*http.Response, error) { if req == nil { return nil, ErrInvalidRequest } + if ctx == nil { + ctx = context.Background() + } req = req.Clone(ctx) return c.rt.RoundTrip(req) diff --git a/client_test.go b/client_test.go index 175c157..af627b2 100644 --- a/client_test.go +++ b/client_test.go @@ -6,11 +6,10 @@ import ( "testing" "github.com/oswaldom-code/rhttp" - "github.com/oswaldom-code/rhttp/internal" ) func TestClient_Do(t *testing.T) { - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { return &http.Response{ StatusCode: http.StatusOK, Request: req, @@ -43,20 +42,20 @@ func TestClient_MiddlewareChain(t *testing.T) { var order []int mw1 := func(next http.RoundTripper) http.RoundTripper { - return internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + return rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { order = append(order, 1) return next.RoundTrip(req) }) } mw2 := func(next http.RoundTripper) http.RoundTripper { - return internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + return rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { order = append(order, 2) return next.RoundTrip(req) }) } - base := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + base := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { order = append(order, 0) return &http.Response{StatusCode: http.StatusOK, Request: req}, nil }) @@ -74,3 +73,19 @@ func TestClient_MiddlewareChain(t *testing.T) { t.Fatalf("unexpected middleware order: %v", order) } } + +func TestDo_NilContextDoesNotPanic(t *testing.T) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + return &http.Response{StatusCode: http.StatusOK, Body: http.NoBody, Request: req}, nil + }) + c := rhttp.New(rhttp.WithTransport(rt)) + + req, _ := http.NewRequest(http.MethodGet, "http://example.com", http.NoBody) + resp, err := c.Do(nil, req) //nolint:staticcheck // nil ctx is the case under test + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if resp.StatusCode != http.StatusOK { + t.Fatalf("expected 200, got %d", resp.StatusCode) + } +} diff --git a/decode.go b/decode.go new file mode 100644 index 0000000..dec8410 --- /dev/null +++ b/decode.go @@ -0,0 +1,25 @@ +package rhttp + +import ( + "encoding/json" + "fmt" + "net/http" +) + +// DecodeJSON decodes a JSON response body into v and always closes the body, +// draining any remainder so the connection can be reused. +// +// It is opinionated: a status code >= 300 is an error and the body is not +// decoded. Callers that need the payload of error responses should read +// resp.Body directly instead. +func DecodeJSON(resp *http.Response, v any) error { + defer drainAndClose(resp) + + if resp.StatusCode >= 300 { + return fmt.Errorf("rhttp: unexpected status %d", resp.StatusCode) + } + if err := json.NewDecoder(resp.Body).Decode(v); err != nil { + return fmt.Errorf("rhttp: decode json: %w", err) + } + return nil +} diff --git a/decode_test.go b/decode_test.go new file mode 100644 index 0000000..8b32c36 --- /dev/null +++ b/decode_test.go @@ -0,0 +1,87 @@ +package rhttp_test + +import ( + "io" + "net/http" + "strings" + "testing" + + "github.com/oswaldom-code/rhttp" +) + +type closeRecorder struct { + io.Reader + closed bool +} + +func (c *closeRecorder) Close() error { + c.closed = true + return nil +} + +func jsonResponse(status int, body string) (*http.Response, *closeRecorder) { + rec := &closeRecorder{Reader: strings.NewReader(body)} + return &http.Response{StatusCode: status, Body: rec}, rec +} + +func TestDecodeJSON_Success(t *testing.T) { + resp, rec := jsonResponse(http.StatusOK, `{"name":"John","age":30}`) + + var out struct { + Name string `json:"name"` + Age int `json:"age"` + } + if err := rhttp.DecodeJSON(resp, &out); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if out.Name != "John" || out.Age != 30 { + t.Errorf("unexpected decode result: %+v", out) + } + if !rec.closed { + t.Error("body was not closed") + } +} + +func TestDecodeJSON_ErrorStatusDoesNotDecode(t *testing.T) { + resp, rec := jsonResponse(http.StatusInternalServerError, `{"name":"John"}`) + + var out struct { + Name string `json:"name"` + } + err := rhttp.DecodeJSON(resp, &out) + if err == nil { + t.Fatal("expected error for status 500") + } + if out.Name != "" { + t.Errorf("expected no decode on error status, got %+v", out) + } + if !rec.closed { + t.Error("body was not closed") + } +} + +func TestDecodeJSON_RedirectStatusIsError(t *testing.T) { + resp, rec := jsonResponse(http.StatusMovedPermanently, "") + + var out any + if err := rhttp.DecodeJSON(resp, &out); err == nil { + t.Fatal("expected error for status 301") + } + if !rec.closed { + t.Error("body was not closed") + } +} + +func TestDecodeJSON_MalformedBodyIsError(t *testing.T) { + resp, rec := jsonResponse(http.StatusOK, `{"name":`) + + var out struct { + Name string `json:"name"` + } + if err := rhttp.DecodeJSON(resp, &out); err == nil { + t.Fatal("expected decode error for malformed JSON") + } + if !rec.closed { + t.Error("body was not closed") + } +} diff --git a/doc.go b/doc.go index 2de37ed..428777a 100644 --- a/doc.go +++ b/doc.go @@ -27,7 +27,7 @@ // Middleware wraps http.RoundTripper to add cross-cutting concerns. The recommended // order from outermost to innermost is: // -// Logging -> Metrics -> Timeout -> RateLimit -> CircuitBreaker -> Retry +// Logging -> Metrics -> Timeout -> RateLimit -> Retry -> CircuitBreaker // // Available middleware: // - [Timeout]: Enforces request timeouts @@ -41,7 +41,7 @@ // // For a more ergonomic API, use the RequestBuilder: // -// resp, err := rhttp.R(client). +// resp, err := client.R(). // SetHeader("Authorization", "Bearer token"). // SetQueryParam("page", "1"). // SetBodyJSON(payload). @@ -58,6 +58,9 @@ // - [ExponentialBackoffFullJitter]: Full jitter for thundering herd prevention // - [ExponentialBackoffEqualJitter]: Equal jitter variant // +// Strategies compose with [WithJitter], [WithMin], [WithMax], and +// [WithRetryAfter], which honors the Retry-After header on 429/503 responses. +// // # Error Classification // // Errors are automatically classified using [Classify] to help with retry decisions: @@ -74,7 +77,8 @@ // // All types in this package are safe for concurrent use unless otherwise noted. // The [Client] can be shared across goroutines, and middleware implementations -// are designed to be thread-safe. +// are designed to be thread-safe. The exception is [RequestBuilder]: each +// builder is meant for a single request from a single goroutine. // // # Zero Dependencies // diff --git a/errorclass.go b/errorclass.go index 748429b..e9dc64b 100644 --- a/errorclass.go +++ b/errorclass.go @@ -3,10 +3,11 @@ package rhttp import ( "context" "crypto/tls" + "crypto/x509" "errors" "net" "net/url" - "strings" + "syscall" ) // ErrorKind represents the category of an HTTP client error. @@ -30,9 +31,6 @@ const ( // ErrKindTLS indicates a TLS/SSL error. ErrKindTLS - - // ErrKindTemporary indicates a temporary error that may resolve on retry. - ErrKindTemporary ) // String returns a human-readable name for the error kind. @@ -48,8 +46,6 @@ func (k ErrorKind) String() string { return "dns" case ErrKindTLS: return "tls" - case ErrKindTemporary: - return "temporary" default: return "unknown" } @@ -58,7 +54,7 @@ func (k ErrorKind) String() string { // IsRetryable returns true if the error kind is typically safe to retry. func (k ErrorKind) IsRetryable() bool { switch k { - case ErrKindTimeout, ErrKindConnection, ErrKindDNS, ErrKindTemporary: + case ErrKindTimeout, ErrKindConnection, ErrKindDNS: return true default: return false @@ -97,13 +93,53 @@ func Classify(err error) *ClassifiedError { } } -//nolint:gocognit,gocyclo // error classification inherently requires multiple checks +func classifyTLS(err error) ErrorKind { + var certErr *tls.CertificateVerificationError + if errors.As(err, &certErr) { + return ErrKindTLS + } + + var unknownAuthErr x509.UnknownAuthorityError + if errors.As(err, &unknownAuthErr) { + return ErrKindTLS + } + + var invalidCertErr x509.CertificateInvalidError + if errors.As(err, &invalidCertErr) { + return ErrKindTLS + } + + var hostnameErr x509.HostnameError + if errors.As(err, &hostnameErr) { + return ErrKindTLS + } + + return ErrKindUnknown +} + +func classifyConnection(err error) ErrorKind { + if errors.Is(err, syscall.ECONNREFUSED) || + errors.Is(err, syscall.ECONNRESET) || + errors.Is(err, syscall.EHOSTUNREACH) || + errors.Is(err, syscall.ENETUNREACH) { + return ErrKindConnection + } + + var opErr *net.OpError + if errors.As(err, &opErr) { + if opErr.Op == "dial" || opErr.Op == "read" || opErr.Op == "write" { + return ErrKindConnection + } + } + + return ErrKindUnknown +} + func classifyError(err error) ErrorKind { if err == nil { return ErrKindUnknown } - // Check for context errors first if errors.Is(err, context.DeadlineExceeded) { return ErrKindTimeout } @@ -111,69 +147,31 @@ func classifyError(err error) ErrorKind { return ErrKindCanceled } - // Check for URL errors (often wrap other errors) var urlErr *url.Error if errors.As(err, &urlErr) { if urlErr.Timeout() { return ErrKindTimeout } - // Classify the wrapped error if urlErr.Err != nil { return classifyError(urlErr.Err) } } - // Check for network errors var netErr net.Error - if errors.As(err, &netErr) { - if netErr.Timeout() { - return ErrKindTimeout - } + if errors.As(err, &netErr) && netErr.Timeout() { + return ErrKindTimeout } - // Check for DNS errors var dnsErr *net.DNSError if errors.As(err, &dnsErr) { return ErrKindDNS } - // Check for operation errors (connection refused, etc.) - var opErr *net.OpError - if errors.As(err, &opErr) { - if opErr.Timeout() { - return ErrKindTimeout - } - // Connection errors - if opErr.Op == "dial" || opErr.Op == "read" || opErr.Op == "write" { - return ErrKindConnection - } - } - - // Check for TLS errors - var tlsErr *tls.CertificateVerificationError - if errors.As(err, &tlsErr) { - return ErrKindTLS - } - - // Check error message for common patterns - errMsg := strings.ToLower(err.Error()) - if strings.Contains(errMsg, "connection refused") || - strings.Contains(errMsg, "connection reset") || - strings.Contains(errMsg, "no route to host") || - strings.Contains(errMsg, "network is unreachable") { - return ErrKindConnection - } - if strings.Contains(errMsg, "tls") || - strings.Contains(errMsg, "certificate") || - strings.Contains(errMsg, "x509") { - return ErrKindTLS - } - if strings.Contains(errMsg, "no such host") || - strings.Contains(errMsg, "lookup") { - return ErrKindDNS + if kind := classifyTLS(err); kind != ErrKindUnknown { + return kind } - return ErrKindUnknown + return classifyConnection(err) } // IsTimeout returns true if the error is a timeout error. diff --git a/errorclass_test.go b/errorclass_test.go index 331a4a4..417ffaa 100644 --- a/errorclass_test.go +++ b/errorclass_test.go @@ -2,9 +2,11 @@ package rhttp_test import ( "context" + "crypto/x509" "errors" "net" "net/url" + "syscall" "testing" "github.com/oswaldom-code/rhttp" @@ -42,7 +44,7 @@ func TestClassify_DNSError(t *testing.T) { } func TestClassify_ConnectionRefused(t *testing.T) { - err := errors.New("dial tcp 127.0.0.1:8080: connection refused") + err := &net.OpError{Op: "dial", Net: "tcp", Err: syscall.ECONNREFUSED} classified := rhttp.Classify(err) if classified.Kind != rhttp.ErrKindConnection { @@ -51,7 +53,7 @@ func TestClassify_ConnectionRefused(t *testing.T) { } func TestClassify_ConnectionReset(t *testing.T) { - err := errors.New("read tcp: connection reset by peer") + err := &net.OpError{Op: "read", Net: "tcp", Err: syscall.ECONNRESET} classified := rhttp.Classify(err) if classified.Kind != rhttp.ErrKindConnection { @@ -60,7 +62,7 @@ func TestClassify_ConnectionReset(t *testing.T) { } func TestClassify_TLSError(t *testing.T) { - err := errors.New("tls: certificate signed by unknown authority") + err := x509.UnknownAuthorityError{} classified := rhttp.Classify(err) if classified.Kind != rhttp.ErrKindTLS { @@ -69,7 +71,7 @@ func TestClassify_TLSError(t *testing.T) { } func TestClassify_X509Error(t *testing.T) { - err := errors.New("x509: certificate has expired") + err := x509.CertificateInvalidError{Reason: x509.Expired} classified := rhttp.Classify(err) if classified.Kind != rhttp.ErrKindTLS { @@ -128,10 +130,9 @@ func TestClassify_UnknownError(t *testing.T) { } func TestClassifiedError_Error(t *testing.T) { - err := errors.New("connection refused") - classified := rhttp.Classify(err) + classified := rhttp.Classify(syscall.ECONNREFUSED) - expected := "connection: connection refused" + expected := "connection: " + syscall.ECONNREFUSED.Error() if classified.Error() != expected { t.Errorf("expected %q, got %q", expected, classified.Error()) } @@ -156,7 +157,6 @@ func TestErrorKind_String(t *testing.T) { {rhttp.ErrKindConnection, "connection"}, {rhttp.ErrKindDNS, "dns"}, {rhttp.ErrKindTLS, "tls"}, - {rhttp.ErrKindTemporary, "temporary"}, {rhttp.ErrKindUnknown, "unknown"}, } @@ -172,7 +172,6 @@ func TestErrorKind_IsRetryable(t *testing.T) { rhttp.ErrKindTimeout, rhttp.ErrKindConnection, rhttp.ErrKindDNS, - rhttp.ErrKindTemporary, } for _, k := range retryable { if !k.IsRetryable() { @@ -217,9 +216,8 @@ func TestIsCanceled(t *testing.T) { } func TestIsConnection(t *testing.T) { - err := errors.New("connection refused") - if !rhttp.IsConnection(err) { - t.Error("expected IsConnection to be true for connection refused") + if !rhttp.IsConnection(syscall.ECONNREFUSED) { + t.Error("expected IsConnection to be true for ECONNREFUSED") } if rhttp.IsConnection(context.Canceled) { t.Error("expected IsConnection to be false for Canceled") @@ -237,8 +235,7 @@ func TestIsDNS(t *testing.T) { } func TestIsTLS(t *testing.T) { - err := errors.New("tls: handshake failure") - if !rhttp.IsTLS(err) { + if !rhttp.IsTLS(x509.UnknownAuthorityError{}) { t.Error("expected IsTLS to be true for TLS error") } if rhttp.IsTLS(context.Canceled) { @@ -246,12 +243,38 @@ func TestIsTLS(t *testing.T) { } } +func TestClassify_AllKindsAreReachable(t *testing.T) { + producers := map[rhttp.ErrorKind]error{ + rhttp.ErrKindUnknown: errors.New("something completely unexpected"), + rhttp.ErrKindTimeout: context.DeadlineExceeded, + rhttp.ErrKindCanceled: context.Canceled, + rhttp.ErrKindConnection: syscall.ECONNREFUSED, + rhttp.ErrKindDNS: &net.DNSError{Err: "no such host"}, + rhttp.ErrKindTLS: x509.UnknownAuthorityError{}, + } + + for kind, err := range producers { + if got := rhttp.Classify(err).Kind; got != kind { + t.Errorf("expected %v to classify as %v, got %v", err, kind, got) + } + } + + for k := rhttp.ErrKindUnknown; ; k++ { + if k != rhttp.ErrKindUnknown && k.String() == "unknown" { + break + } + if _, ok := producers[k]; !ok { + t.Errorf("ErrorKind %d (%s) has no producing error: orphaned kind", k, k) + } + } +} + func TestIsRetryable(t *testing.T) { // Retryable if !rhttp.IsRetryable(context.DeadlineExceeded) { t.Error("expected timeout to be retryable") } - if !rhttp.IsRetryable(errors.New("connection refused")) { + if !rhttp.IsRetryable(syscall.ECONNREFUSED) { t.Error("expected connection error to be retryable") } @@ -259,7 +282,7 @@ func TestIsRetryable(t *testing.T) { if rhttp.IsRetryable(context.Canceled) { t.Error("expected canceled to not be retryable") } - if rhttp.IsRetryable(errors.New("tls: certificate error")) { + if rhttp.IsRetryable(x509.UnknownAuthorityError{}) { t.Error("expected TLS error to not be retryable") } if rhttp.IsRetryable(nil) { diff --git a/example_test.go b/example_test.go index 27e08e5..80b4259 100644 --- a/example_test.go +++ b/example_test.go @@ -51,11 +51,11 @@ func ExampleNew_withMiddleware() { fmt.Println("Status:", resp.StatusCode) } -func ExampleR() { +func ExampleClient_R() { client := rhttp.New() // Use the fluent API to build and execute requests - resp, err := rhttp.R(client). + resp, err := client.R(). SetHeader("Authorization", "Bearer token"). SetQueryParam("page", "1"). Get("https://api.example.com/users") @@ -79,7 +79,7 @@ func ExampleRequestBuilder_SetBodyJSON() { user := User{Name: "John", Email: "john@example.com"} - resp, err := rhttp.R(client). + resp, err := client.R(). SetBodyJSON(user). Post("https://api.example.com/users") @@ -96,7 +96,7 @@ func ExampleRequestBuilder_SetPathParam() { client := rhttp.New() // Path parameters are replaced in the URL template - resp, err := rhttp.R(client). + resp, err := client.R(). SetPathParam("id", "123"). Get("https://api.example.com/users/{id}") @@ -131,9 +131,9 @@ func ExampleExponentialBackoff() { backoff := rhttp.ExponentialBackoff(100*time.Millisecond, 10*time.Second) // Backoff durations increase exponentially with jitter - fmt.Println("Attempt 0:", backoff(0)) // ~100ms - fmt.Println("Attempt 1:", backoff(1)) // ~200ms - fmt.Println("Attempt 2:", backoff(2)) // ~400ms + fmt.Println("Attempt 0:", backoff(0, nil)) // ~100ms + fmt.Println("Attempt 1:", backoff(1, nil)) // ~200ms + fmt.Println("Attempt 2:", backoff(2, nil)) // ~400ms } func ExampleNewTokenBucket() { @@ -186,13 +186,76 @@ func ExampleLogging() { _ = client // Use client for requests } -func ExampleGetBuffer() { - // Get a buffer from the pool - buf := rhttp.GetBuffer() +func ExampleRetry_totalBudget() { + client := rhttp.New( + rhttp.WithMiddleware( + rhttp.Timeout(5*time.Second), + rhttp.Retry(rhttp.RetryConfig{ + MaxAttempts: 3, + Backoff: rhttp.ExponentialBackoff(100*time.Millisecond, 2*time.Second), + }), + rhttp.CircuitBreaker(rhttp.CircuitBreakerConfig{ + FailureThreshold: 5, + ResetTimeout: 30 * time.Second, + }), + ), + ) - // Use the buffer - buf.WriteString("Hello, World!") + req, _ := http.NewRequest("GET", "https://api.example.com/users", http.NoBody) + resp, err := client.Do(context.Background(), req) + if err != nil { + fmt.Println("request failed:", err) + return + } + defer resp.Body.Close() + + fmt.Println("Status:", resp.StatusCode) +} - // Return to pool when done - rhttp.PutBuffer(buf) +func ExampleRetry_perAttemptTimeout() { + client := rhttp.New( + rhttp.WithMiddleware( + rhttp.Retry(rhttp.RetryConfig{ + MaxAttempts: 3, + Backoff: rhttp.ExponentialBackoff(100*time.Millisecond, 2*time.Second), + }), + rhttp.Timeout(2*time.Second), + rhttp.CircuitBreaker(rhttp.CircuitBreakerConfig{ + FailureThreshold: 5, + ResetTimeout: 30 * time.Second, + }), + ), + ) + + req, _ := http.NewRequest("GET", "https://api.example.com/users", http.NoBody) + resp, err := client.Do(context.Background(), req) + if err != nil { + fmt.Println("request failed:", err) + return + } + defer resp.Body.Close() + + fmt.Println("Status:", resp.StatusCode) +} + +func ExampleRoundTripperFunc() { + // A custom middleware is a function over RoundTripperFunc: five lines. + withRequestID := func(next http.RoundTripper) http.RoundTripper { + return rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + req.Header.Set("X-Request-ID", "abc-123") + return next.RoundTrip(req) + }) + } + + client := rhttp.New(rhttp.WithMiddleware(withRequestID)) + + req, _ := http.NewRequest("GET", "https://api.example.com/users", http.NoBody) + resp, err := client.Do(context.Background(), req) + if err != nil { + fmt.Println("request failed:", err) + return + } + defer resp.Body.Close() + + fmt.Println("Status:", resp.StatusCode) } diff --git a/examples/basic/main.go b/examples/basic/main.go index 72b98e6..a4e216f 100644 --- a/examples/basic/main.go +++ b/examples/basic/main.go @@ -50,8 +50,8 @@ func main() { customHeaders(client) } -func simpleGet(client rhttp.Client) { - resp, err := rhttp.R(client). +func simpleGet(client *rhttp.Client) { + resp, err := client.R(). Get("https://httpbin.org/get") if err != nil { log.Printf("Error: %v", err) @@ -63,8 +63,8 @@ func simpleGet(client rhttp.Client) { printBody(resp.Body) } -func getWithQueryParams(client rhttp.Client) { - resp, err := rhttp.R(client). +func getWithQueryParams(client *rhttp.Client) { + resp, err := client.R(). SetQueryParam("page", "1"). SetQueryParam("limit", "10"). SetQueryParams(map[string]string{ @@ -82,14 +82,14 @@ func getWithQueryParams(client rhttp.Client) { printBody(resp.Body) } -func postJSON(client rhttp.Client) { +func postJSON(client *rhttp.Client) { payload := map[string]any{ "name": "rhttp", "type": "library", "tags": []string{"http", "resilience", "go"}, } - resp, err := rhttp.R(client). + resp, err := client.R(). SetBodyJSON(payload). Post("https://httpbin.org/post") if err != nil { @@ -102,9 +102,9 @@ func postJSON(client rhttp.Client) { printBody(resp.Body) } -func pathParams(client rhttp.Client) { +func pathParams(client *rhttp.Client) { // Simulates: GET /users/123/posts/456 - resp, err := rhttp.R(client). + resp, err := client.R(). SetPathParam("userId", "123"). SetPathParam("postId", "456"). Get("https://httpbin.org/anything/users/{userId}/posts/{postId}") @@ -118,11 +118,11 @@ func pathParams(client rhttp.Client) { printBody(resp.Body) } -func customHeaders(client rhttp.Client) { +func customHeaders(client *rhttp.Client) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - resp, err := rhttp.R(client). + resp, err := client.R(). Context(ctx). SetHeader("X-Custom-Header", "custom-value"). SetHeader("X-Request-ID", "req-12345"). diff --git a/interaction_test.go b/interaction_test.go new file mode 100644 index 0000000..3f00ac5 --- /dev/null +++ b/interaction_test.go @@ -0,0 +1,110 @@ +package rhttp_test + +import ( + "context" + "errors" + "net/http" + "sync/atomic" + "syscall" + "testing" + "time" + + "github.com/oswaldom-code/rhttp" +) + +func TestTimeoutOuterRetry_TotalBudget(t *testing.T) { + var attempts int32 + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + atomic.AddInt32(&attempts, 1) + return nil, syscall.ECONNREFUSED + }) + + c := rhttp.New( + rhttp.WithTransport(rt), + rhttp.WithMiddleware( + rhttp.Timeout(300*time.Millisecond), + rhttp.Retry(rhttp.RetryConfig{ + MaxAttempts: 3, + Backoff: rhttp.ConstantBackoff(200 * time.Millisecond), + }), + ), + ) + + req, _ := http.NewRequest(http.MethodGet, "http://example.com", http.NoBody) + _, err := c.Do(context.Background(), req) + + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("expected DeadlineExceeded from the outer timeout, got %v", err) + } + if got := atomic.LoadInt32(&attempts); got != 2 { + t.Fatalf("expected the 300ms budget to cut the run at 2 attempts, got %d", got) + } +} + +func TestRetryOuterTimeout_PerAttempt(t *testing.T) { + var attempts int32 + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + atomic.AddInt32(&attempts, 1) + select { + case <-req.Context().Done(): + return nil, req.Context().Err() + case <-time.After(150 * time.Millisecond): + return &http.Response{StatusCode: http.StatusOK, Body: http.NoBody, Request: req}, nil + } + }) + + c := rhttp.New( + rhttp.WithTransport(rt), + rhttp.WithMiddleware( + rhttp.Retry(rhttp.RetryConfig{ + MaxAttempts: 3, + Backoff: rhttp.ConstantBackoff(time.Millisecond), + }), + rhttp.Timeout(100*time.Millisecond), + ), + ) + + req, _ := http.NewRequest(http.MethodGet, "http://example.com", http.NoBody) + _, err := c.Do(context.Background(), req) + + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("expected DeadlineExceeded, got %v", err) + } + if got := atomic.LoadInt32(&attempts); got != 3 { + t.Fatalf("expected 3 attempts, each with a fresh per-attempt deadline, got %d", got) + } +} + +func TestRetryOuterCircuitBreaker_OpenCutsAttempts(t *testing.T) { + var attempts int32 + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + atomic.AddInt32(&attempts, 1) + return nil, syscall.ECONNREFUSED + }) + + c := rhttp.New( + rhttp.WithTransport(rt), + rhttp.WithMiddleware( + rhttp.Retry(rhttp.RetryConfig{ + MaxAttempts: 5, + Backoff: rhttp.ConstantBackoff(time.Millisecond), + }), + rhttp.CircuitBreaker(rhttp.CircuitBreakerConfig{ + FailureThreshold: 2, + ResetTimeout: time.Hour, + }), + ), + ) + + req, _ := http.NewRequest(http.MethodGet, "http://example.com", http.NoBody) + _, err := c.Do(context.Background(), req) + + // This pins today's behavior: ErrCircuitOpen is not retryable, so the + // tripped breaker short-circuits the remaining retry budget. + if !errors.Is(err, rhttp.ErrCircuitOpen) { + t.Fatalf("expected ErrCircuitOpen, got %v", err) + } + if got := atomic.LoadInt32(&attempts); got != 2 { + t.Fatalf("expected the transport to see only the 2 attempts that tripped the breaker, got %d", got) + } +} diff --git a/internal/roundtripper.go b/internal/roundtripper.go deleted file mode 100644 index 0f9465d..0000000 --- a/internal/roundtripper.go +++ /dev/null @@ -1,13 +0,0 @@ -// Package internal provides internal utilities for the rhttp package. -package internal - -import "net/http" - -// RoundTripperFunc is an adapter that allows ordinary functions to be used -// as http.RoundTripper. This is useful for creating mock transports in tests. -type RoundTripperFunc func(*http.Request) (*http.Response, error) - -// RoundTrip implements the http.RoundTripper interface. -func (f RoundTripperFunc) RoundTrip(req *http.Request) (*http.Response, error) { - return f(req) -} diff --git a/logging_test.go b/logging_test.go index 7fe9bb3..92e7d3b 100644 --- a/logging_test.go +++ b/logging_test.go @@ -9,7 +9,6 @@ import ( "time" "github.com/oswaldom-code/rhttp" - "github.com/oswaldom-code/rhttp/internal" ) func TestLogging_LogsSuccessfulRequest(t *testing.T) { @@ -18,7 +17,7 @@ func TestLogging_LogsSuccessfulRequest(t *testing.T) { captured = entry }) - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { return &http.Response{StatusCode: http.StatusOK, Request: req}, nil }) @@ -56,7 +55,7 @@ func TestLogging_LogsFailedRequest(t *testing.T) { }) expectedErr := errors.New("connection refused") - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { return nil, expectedErr }) @@ -87,7 +86,7 @@ func TestLogging_MeasuresDuration(t *testing.T) { captured = entry }) - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { time.Sleep(50 * time.Millisecond) return &http.Response{StatusCode: http.StatusOK, Request: req}, nil }) @@ -114,7 +113,7 @@ func TestLogging_ShouldLogFilters(t *testing.T) { }) callCount := 0 - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { callCount++ if callCount%2 == 0 { return &http.Response{StatusCode: http.StatusOK, Request: req}, nil @@ -146,7 +145,7 @@ func TestLogging_ShouldLogFilters(t *testing.T) { } func TestLogging_NilLoggerIsNoOp(t *testing.T) { - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { return &http.Response{StatusCode: http.StatusOK, Request: req}, nil }) @@ -178,7 +177,7 @@ func TestLogging_ThreadSafety(t *testing.T) { mu.Unlock() }) - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { return &http.Response{StatusCode: http.StatusOK, Request: req}, nil }) diff --git a/metrics.go b/metrics.go index ad72ad4..6896a80 100644 --- a/metrics.go +++ b/metrics.go @@ -40,6 +40,13 @@ func (f MetricsRecorderFunc) RecordRequest(event MetricEvent) { type MetricsConfig struct { // Recorder is the metrics recorder. Required. Recorder MetricsRecorder + + // PathNormalizer maps a request path to the value emitted as MetricEvent.Path. + // It exists to bound label cardinality: raw paths like /users/8f3a/orders/2941 + // would create one time series per ID. If nil, Path is emitted empty; to collapse + // high-cardinality segments, provide a function that returns a template such as + // /users/:id/orders/:id. + PathNormalizer func(path string) string } // Metrics returns a middleware that records HTTP client metrics. @@ -52,15 +59,24 @@ func Metrics(cfg MetricsConfig) Middleware { return func(next http.RoundTripper) http.RoundTripper { return metricsRoundTripper{ - next: next, - recorder: cfg.Recorder, + next: next, + recorder: cfg.Recorder, + pathNormalizer: cfg.PathNormalizer, } } } type metricsRoundTripper struct { - next http.RoundTripper - recorder MetricsRecorder + next http.RoundTripper + recorder MetricsRecorder + pathNormalizer func(path string) string +} + +func (m metricsRoundTripper) normalizePath(path string) string { + if m.pathNormalizer == nil { + return "" + } + return m.pathNormalizer(path) } func (m metricsRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { @@ -73,7 +89,7 @@ func (m metricsRoundTripper) RoundTrip(req *http.Request) (*http.Response, error event := MetricEvent{ Method: req.Method, Host: req.URL.Host, - Path: req.URL.Path, + Path: m.normalizePath(req.URL.Path), Duration: duration, Error: err, Success: err == nil && resp != nil && resp.StatusCode < 500, diff --git a/metrics_test.go b/metrics_test.go index 89130df..46c1165 100644 --- a/metrics_test.go +++ b/metrics_test.go @@ -5,12 +5,12 @@ import ( "context" "errors" "net/http" + "strings" "sync" "testing" "time" "github.com/oswaldom-code/rhttp" - "github.com/oswaldom-code/rhttp/internal" ) func TestMetrics_RecordsSuccessfulRequest(t *testing.T) { @@ -19,7 +19,7 @@ func TestMetrics_RecordsSuccessfulRequest(t *testing.T) { captured = event }) - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { return &http.Response{ StatusCode: http.StatusOK, ContentLength: 1024, @@ -30,7 +30,8 @@ func TestMetrics_RecordsSuccessfulRequest(t *testing.T) { c := rhttp.New( rhttp.WithTransport(rt), rhttp.WithMiddleware(rhttp.Metrics(rhttp.MetricsConfig{ - Recorder: recorder, + Recorder: recorder, + PathNormalizer: func(p string) string { return p }, })), ) @@ -63,6 +64,67 @@ func TestMetrics_RecordsSuccessfulRequest(t *testing.T) { } } +func TestMetrics_NilPathNormalizerEmitsEmptyPath(t *testing.T) { + var captured rhttp.MetricEvent + recorder := rhttp.MetricsRecorderFunc(func(event rhttp.MetricEvent) { + captured = event + }) + + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + return &http.Response{StatusCode: http.StatusOK, Request: req}, nil + }) + + c := rhttp.New( + rhttp.WithTransport(rt), + rhttp.WithMiddleware(rhttp.Metrics(rhttp.MetricsConfig{ + Recorder: recorder, + })), + ) + + req, _ := http.NewRequest(http.MethodGet, "http://api.example.com/users/8f3a/orders/2941", http.NoBody) + _, _ = c.Do(context.Background(), req) + + if captured.Path != "" { + t.Errorf("expected empty path without normalizer, got %q", captured.Path) + } + if captured.Host != "api.example.com" { + t.Errorf("expected host to still be emitted, got %q", captured.Host) + } +} + +func TestMetrics_PathNormalizerTransformsPath(t *testing.T) { + var captured rhttp.MetricEvent + recorder := rhttp.MetricsRecorderFunc(func(event rhttp.MetricEvent) { + captured = event + }) + + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + return &http.Response{StatusCode: http.StatusOK, Request: req}, nil + }) + + normalizer := func(p string) string { + if strings.HasPrefix(p, "/users/") { + return "/users/:id" + } + return p + } + + c := rhttp.New( + rhttp.WithTransport(rt), + rhttp.WithMiddleware(rhttp.Metrics(rhttp.MetricsConfig{ + Recorder: recorder, + PathNormalizer: normalizer, + })), + ) + + req, _ := http.NewRequest(http.MethodGet, "http://api.example.com/users/8f3a/profile", http.NoBody) + _, _ = c.Do(context.Background(), req) + + if captured.Path != "/users/:id" { + t.Errorf("expected normalized path /users/:id, got %q", captured.Path) + } +} + func TestMetrics_RecordsFailedRequest(t *testing.T) { var captured rhttp.MetricEvent recorder := rhttp.MetricsRecorderFunc(func(event rhttp.MetricEvent) { @@ -70,7 +132,7 @@ func TestMetrics_RecordsFailedRequest(t *testing.T) { }) expectedErr := errors.New("connection refused") - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { return nil, expectedErr }) @@ -101,7 +163,7 @@ func TestMetrics_5xxIsNotSuccess(t *testing.T) { captured = event }) - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { return &http.Response{StatusCode: http.StatusInternalServerError, Request: req}, nil }) @@ -129,7 +191,7 @@ func TestMetrics_4xxIsSuccess(t *testing.T) { captured = event }) - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { return &http.Response{StatusCode: http.StatusNotFound, Request: req}, nil }) @@ -150,7 +212,7 @@ func TestMetrics_4xxIsSuccess(t *testing.T) { } func TestMetrics_NilRecorderIsNoOp(t *testing.T) { - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { return &http.Response{StatusCode: http.StatusOK, Request: req}, nil }) @@ -178,7 +240,7 @@ func TestMetrics_RecordsBytesSent(t *testing.T) { captured = event }) - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { return &http.Response{StatusCode: http.StatusOK, Request: req}, nil }) @@ -205,7 +267,7 @@ func TestMetrics_MeasuresDuration(t *testing.T) { captured = event }) - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { time.Sleep(50 * time.Millisecond) return &http.Response{StatusCode: http.StatusOK, Request: req}, nil }) @@ -234,7 +296,7 @@ func TestMetrics_ThreadSafety(t *testing.T) { mu.Unlock() }) - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { return &http.Response{StatusCode: http.StatusOK, Request: req}, nil }) diff --git a/middleware.go b/middleware.go index cf9cd79..eba5b48 100644 --- a/middleware.go +++ b/middleware.go @@ -5,6 +5,31 @@ import "net/http" // Middleware wraps an http.RoundTripper to add behavior. type Middleware func(http.RoundTripper) http.RoundTripper +// RoundTripperFunc adapts an ordinary function to an [http.RoundTripper], +// which makes writing a custom [Middleware] a one-liner: +// +// func withRequestID(next http.RoundTripper) http.RoundTripper { +// return rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { +// req.Header.Set("X-Request-ID", newID()) +// return next.RoundTrip(req) +// }) +// } +type RoundTripperFunc func(*http.Request) (*http.Response, error) + +// RoundTrip implements the [http.RoundTripper] interface. +func (f RoundTripperFunc) RoundTrip(req *http.Request) (*http.Response, error) { + return f(req) +} + +// closeRequestBody releases the request body when a middleware short-circuits +// and the request will never reach the transport, honoring the RoundTripper +// contract that the body is always closed. +func closeRequestBody(req *http.Request) { + if req.Body != nil && req.Body != http.NoBody { + _ = req.Body.Close() + } +} + // chain applies middleware in reverse order so the first middleware // in the slice is the outermost wrapper (executes first). func chain(base http.RoundTripper, mws ...Middleware) http.RoundTripper { diff --git a/pool.go b/pool.go deleted file mode 100644 index 5ec977e..0000000 --- a/pool.go +++ /dev/null @@ -1,97 +0,0 @@ -package rhttp - -import ( - "bytes" - "sync" -) - -// BufferPool provides reusable byte buffers to reduce allocations. -var BufferPool = &sync.Pool{ - New: func() any { - return bytes.NewBuffer(make([]byte, 0, 4096)) - }, -} - -// GetBuffer retrieves a buffer from the pool. -func GetBuffer() *bytes.Buffer { - buf, _ := BufferPool.Get().(*bytes.Buffer) //nolint:errcheck // type is guaranteed by pool's New func - buf.Reset() - return buf -} - -// PutBuffer returns a buffer to the pool. -func PutBuffer(buf *bytes.Buffer) { - if buf == nil { - return - } - // Don't pool oversized buffers (>64KB) to prevent memory bloat - if buf.Cap() > 65536 { - return - } - buf.Reset() - BufferPool.Put(buf) -} - -// responsePool provides reusable response wrappers. -var responsePool = &sync.Pool{ - New: func() any { - return &Response{} - }, -} - -// Response wraps http.Response with pooling support and convenience methods. -type Response struct { - StatusCode int - Headers map[string][]string - Body []byte - ContentLength int64 - pooled bool -} - -// Reset clears the response for reuse. -func (r *Response) Reset() { - r.StatusCode = 0 - r.Headers = nil - r.Body = nil - r.ContentLength = 0 - r.pooled = false -} - -// Release returns the response to the pool. -// After calling Release, the Response must not be used. -func (r *Response) Release() { - if r == nil || !r.pooled { - return - } - r.Reset() - responsePool.Put(r) -} - -// acquireResponse gets a response from the pool. -// Currently unused but kept for future RequestBuilder enhancements. -func acquireResponse() *Response { //nolint:unused - r, _ := responsePool.Get().(*Response) //nolint:errcheck // type is guaranteed by pool's New func - r.Reset() - r.pooled = true - return r -} - -// IsSuccess returns true if status code is 2xx. -func (r *Response) IsSuccess() bool { - return r.StatusCode >= 200 && r.StatusCode < 300 -} - -// IsError returns true if status code is 4xx or 5xx. -func (r *Response) IsError() bool { - return r.StatusCode >= 400 -} - -// IsServerError returns true if status code is 5xx. -func (r *Response) IsServerError() bool { - return r.StatusCode >= 500 -} - -// IsClientError returns true if status code is 4xx. -func (r *Response) IsClientError() bool { - return r.StatusCode >= 400 && r.StatusCode < 500 -} diff --git a/pool_test.go b/pool_test.go deleted file mode 100644 index 39f2014..0000000 --- a/pool_test.go +++ /dev/null @@ -1,148 +0,0 @@ -package rhttp_test - -import ( - "sync" - "testing" - - "github.com/oswaldom-code/rhttp" -) - -func TestBufferPool_GetAndPut(t *testing.T) { - buf := rhttp.GetBuffer() - if buf == nil { - t.Fatal("expected non-nil buffer") - } - - buf.WriteString("test data") - if buf.Len() != 9 { - t.Errorf("expected length 9, got %d", buf.Len()) - } - - rhttp.PutBuffer(buf) - - // Get another buffer - should be reset - buf2 := rhttp.GetBuffer() - if buf2.Len() != 0 { - t.Errorf("expected reset buffer with length 0, got %d", buf2.Len()) - } - rhttp.PutBuffer(buf2) -} - -func TestBufferPool_NilSafe(_ *testing.T) { - // Should not panic - rhttp.PutBuffer(nil) -} - -func TestBufferPool_Concurrent(_ *testing.T) { - var wg sync.WaitGroup - for i := 0; i < 100; i++ { - wg.Add(1) - go func() { - defer wg.Done() - buf := rhttp.GetBuffer() - buf.WriteString("concurrent test") - rhttp.PutBuffer(buf) - }() - } - wg.Wait() -} - -func TestResponse_IsSuccess(t *testing.T) { - tests := []struct { - status int - expected bool - }{ - {200, true}, - {201, true}, - {204, true}, - {299, true}, - {300, false}, - {400, false}, - {500, false}, - } - - for _, tt := range tests { - r := &rhttp.Response{StatusCode: tt.status} - if r.IsSuccess() != tt.expected { - t.Errorf("IsSuccess(%d) = %v, want %v", tt.status, r.IsSuccess(), tt.expected) - } - } -} - -func TestResponse_IsError(t *testing.T) { - tests := []struct { - status int - expected bool - }{ - {200, false}, - {399, false}, - {400, true}, - {404, true}, - {500, true}, - {503, true}, - } - - for _, tt := range tests { - r := &rhttp.Response{StatusCode: tt.status} - if r.IsError() != tt.expected { - t.Errorf("IsError(%d) = %v, want %v", tt.status, r.IsError(), tt.expected) - } - } -} - -func TestResponse_IsServerError(t *testing.T) { - tests := []struct { - status int - expected bool - }{ - {499, false}, - {500, true}, - {502, true}, - {503, true}, - } - - for _, tt := range tests { - r := &rhttp.Response{StatusCode: tt.status} - if r.IsServerError() != tt.expected { - t.Errorf("IsServerError(%d) = %v, want %v", tt.status, r.IsServerError(), tt.expected) - } - } -} - -func TestResponse_IsClientError(t *testing.T) { - tests := []struct { - status int - expected bool - }{ - {399, false}, - {400, true}, - {404, true}, - {499, true}, - {500, false}, - } - - for _, tt := range tests { - r := &rhttp.Response{StatusCode: tt.status} - if r.IsClientError() != tt.expected { - t.Errorf("IsClientError(%d) = %v, want %v", tt.status, r.IsClientError(), tt.expected) - } - } -} - -func BenchmarkBufferPool(b *testing.B) { - b.ReportAllocs() - for i := 0; i < b.N; i++ { - buf := rhttp.GetBuffer() - buf.WriteString("benchmark test data") - rhttp.PutBuffer(buf) - } -} - -func BenchmarkBufferPool_NoPool(b *testing.B) { - b.ReportAllocs() - for i := 0; i < b.N; i++ { - buf := make([]byte, 0, 4096) - buf = append(buf, "benchmark test data"...) - _ = buf - } -} diff --git a/ratelimit.go b/ratelimit.go index c3efa46..482c061 100644 --- a/ratelimit.go +++ b/ratelimit.go @@ -10,10 +10,6 @@ import ( // RateLimiter controls the rate of HTTP requests. type RateLimiter interface { - // Wait blocks until a token is available. - // Deprecated: Use WaitContext for proper cancellation support. - Wait() error - // WaitContext blocks until a token is available or context is canceled. // Returns an error if the context is canceled. WaitContext(ctx context.Context) error @@ -30,26 +26,31 @@ type TokenBucket struct { maxTokens float64 refillRate float64 // tokens per second lastRefill time.Time + waitTime time.Duration // time for one token to refill; immutable + unlimited bool } // NewTokenBucket creates a new token bucket rate limiter. // rate: requests per second allowed // burst: maximum burst size (bucket capacity) +// +// A non-positive rate or a burst below 1 is invalid configuration: the returned +// bucket does not limit (it allows every request), following the project +// convention that invalid config becomes a no-op rather than a busy-loop or a +// permanent block. func NewTokenBucket(rate float64, burst int) *TokenBucket { + if rate <= 0 || burst < 1 { + return &TokenBucket{unlimited: true} + } return &TokenBucket{ tokens: float64(burst), maxTokens: float64(burst), refillRate: rate, lastRefill: time.Now(), + waitTime: time.Duration(float64(time.Second) / rate), } } -// Wait blocks until a token is available. -// Deprecated: Use WaitContext for proper cancellation support. -func (tb *TokenBucket) Wait() error { - return tb.WaitContext(context.Background()) -} - // WaitContext blocks until a token is available or context is canceled. func (tb *TokenBucket) WaitContext(ctx context.Context) error { for { @@ -57,21 +58,22 @@ func (tb *TokenBucket) WaitContext(ctx context.Context) error { return nil } - // Calculate wait time for next token - tb.mu.Lock() - waitTime := time.Duration((1.0 / tb.refillRate) * float64(time.Second)) - tb.mu.Unlock() - + timer := time.NewTimer(tb.waitTime) select { case <-ctx.Done(): + timer.Stop() return ctx.Err() - case <-time.After(waitTime): + case <-timer.C: } } } // TryAcquire attempts to acquire a token without blocking. func (tb *TokenBucket) TryAcquire() bool { + if tb.unlimited { + return true + } + tb.mu.Lock() defer tb.mu.Unlock() @@ -147,10 +149,13 @@ func (r *rateLimitRoundTripper) RoundTrip(req *http.Request) (*http.Response, er waitTime := time.Until(r.retryAt) r.retryLock.Unlock() + timer := time.NewTimer(waitTime) select { case <-req.Context().Done(): + timer.Stop() + closeRequestBody(req) return nil, req.Context().Err() - case <-time.After(waitTime): + case <-timer.C: } } else { r.retryLock.Unlock() @@ -160,9 +165,11 @@ func (r *rateLimitRoundTripper) RoundTrip(req *http.Request) (*http.Response, er // Acquire rate limit token if r.cfg.WaitOnLimit { if err := r.cfg.Limiter.WaitContext(req.Context()); err != nil { + closeRequestBody(req) return nil, err } } else if !r.cfg.Limiter.TryAcquire() { + closeRequestBody(req) return nil, ErrRateLimited } @@ -185,47 +192,3 @@ func (r *rateLimitRoundTripper) RoundTrip(req *http.Request) (*http.Response, er return resp, err } - -// PerHostRateLimiter provides separate rate limiters for each host. -// It lazily creates a TokenBucket for each unique host on first access. -// This is useful when making requests to multiple APIs with different rate limits. -type PerHostRateLimiter struct { - mu sync.RWMutex - limiters map[string]*TokenBucket - rate float64 - burst int -} - -// NewPerHostRateLimiter creates a rate limiter that applies limits per host. -func NewPerHostRateLimiter(rate float64, burst int) *PerHostRateLimiter { - return &PerHostRateLimiter{ - limiters: make(map[string]*TokenBucket), - rate: rate, - burst: burst, - } -} - -// GetLimiter returns the rate limiter for a specific host. -// If no limiter exists for the host, a new one is created with the configured -// rate and burst values. This method is safe for concurrent use. -func (p *PerHostRateLimiter) GetLimiter(host string) *TokenBucket { - p.mu.RLock() - limiter, ok := p.limiters[host] - p.mu.RUnlock() - - if ok { - return limiter - } - - p.mu.Lock() - defer p.mu.Unlock() - - // Double-check after acquiring write lock - if limiter, ok = p.limiters[host]; ok { - return limiter - } - - limiter = NewTokenBucket(p.rate, p.burst) - p.limiters[host] = limiter - return limiter -} diff --git a/ratelimit_test.go b/ratelimit_test.go index 704a9de..ea6fe54 100644 --- a/ratelimit_test.go +++ b/ratelimit_test.go @@ -4,13 +4,13 @@ import ( "context" "errors" "net/http" + "strings" "sync" "sync/atomic" "testing" "time" "github.com/oswaldom-code/rhttp" - "github.com/oswaldom-code/rhttp/internal" ) func TestTokenBucket_Basic(t *testing.T) { @@ -29,6 +29,39 @@ func TestTokenBucket_Basic(t *testing.T) { } } +func TestNewTokenBucket_ZeroRateIsUnlimited(t *testing.T) { + tb := rhttp.NewTokenBucket(0, 1) + + // An invalid rate must not limit: without the guard, only the initial burst + // token is granted and WaitContext then busy-loops on a negative wait time. + for i := 0; i < 10; i++ { + if !tb.TryAcquire() { + t.Fatalf("attempt %d: invalid rate must not limit", i) + } + } + + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + if err := tb.WaitContext(ctx); err != nil { + t.Fatalf("WaitContext on unlimited bucket returned error: %v", err) + } +} + +func TestNewTokenBucket_ZeroBurstIsUnlimited(t *testing.T) { + tb := rhttp.NewTokenBucket(10, 0) + + // Zero burst must not block forever (maxTokens == 0 → TryAcquire never true). + if !tb.TryAcquire() { + t.Fatal("zero burst must not block forever") + } + + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + if err := tb.WaitContext(ctx); err != nil { + t.Fatalf("WaitContext on unlimited bucket returned error: %v", err) + } +} + func TestTokenBucket_Refill(t *testing.T) { tb := rhttp.NewTokenBucket(100, 1) // 100 req/s, burst of 1 @@ -51,14 +84,14 @@ func TestTokenBucket_Refill(t *testing.T) { } } -func TestTokenBucket_Wait(t *testing.T) { +func TestTokenBucket_WaitContextBlocksUntilToken(t *testing.T) { tb := rhttp.NewTokenBucket(100, 1) // 100 req/s, burst of 1 // Consume the token tb.TryAcquire() start := time.Now() - err := tb.Wait() + err := tb.WaitContext(context.Background()) elapsed := time.Since(start) if err != nil { @@ -71,6 +104,38 @@ func TestTokenBucket_Wait(t *testing.T) { } } +// stubLimiter mirrors the method set of x/time/rate.Limiter without importing it. +type stubLimiter struct{} + +func (stubLimiter) Allow() bool { return true } +func (stubLimiter) Wait(_ context.Context) error { return nil } + +// xRateAdapter shows that adapting an x/time/rate style limiter to +// rhttp.RateLimiter takes a struct and two one-line methods. +type xRateAdapter struct{ l stubLimiter } + +func (a xRateAdapter) TryAcquire() bool { return a.l.Allow() } +func (a xRateAdapter) WaitContext(ctx context.Context) error { return a.l.Wait(ctx) } + +func TestRateLimiter_XTimeRateAdapter(t *testing.T) { + var limiter rhttp.RateLimiter = xRateAdapter{} + + rt := rhttp.RoundTripperFunc(func(*http.Request) (*http.Response, error) { + return &http.Response{StatusCode: http.StatusOK, Body: http.NoBody}, nil + }) + c := rhttp.New( + rhttp.WithTransport(rt), + rhttp.WithMiddleware(rhttp.RateLimit(rhttp.RateLimitConfig{Limiter: limiter})), + ) + + req, _ := http.NewRequest(http.MethodGet, "http://example.com", http.NoBody) + resp, err := c.Do(context.Background(), req) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + resp.Body.Close() +} + func TestTokenBucket_Concurrent(t *testing.T) { tb := rhttp.NewTokenBucket(1000, 100) @@ -96,7 +161,7 @@ func TestTokenBucket_Concurrent(t *testing.T) { func TestRateLimit_Middleware(t *testing.T) { var calls int32 - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { atomic.AddInt32(&calls, 1) return &http.Response{StatusCode: http.StatusOK, Request: req}, nil }) @@ -125,7 +190,7 @@ func TestRateLimit_Middleware(t *testing.T) { } func TestRateLimit_NoWait(t *testing.T) { - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { return &http.Response{StatusCode: http.StatusOK, Request: req}, nil }) @@ -155,7 +220,7 @@ func TestRateLimit_NoWait(t *testing.T) { func TestRateLimit_RespectRetryAfter(t *testing.T) { callCount := 0 - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { callCount++ if callCount == 1 { resp := &http.Response{ @@ -203,7 +268,7 @@ func TestRateLimit_RespectRetryAfter(t *testing.T) { } func TestRateLimit_NilLimiter(t *testing.T) { - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { return &http.Response{StatusCode: http.StatusOK, Request: req}, nil }) @@ -225,37 +290,6 @@ func TestRateLimit_NilLimiter(t *testing.T) { } } -func TestPerHostRateLimiter(t *testing.T) { - phl := rhttp.NewPerHostRateLimiter(10, 5) - - limiter1 := phl.GetLimiter("api.example.com") - limiter2 := phl.GetLimiter("api.other.com") - limiter3 := phl.GetLimiter("api.example.com") // same as limiter1 - - if limiter1 == limiter2 { - t.Error("expected different limiters for different hosts") - } - - if limiter1 != limiter3 { - t.Error("expected same limiter for same host") - } - - // Drain limiter1 - for i := 0; i < 5; i++ { - limiter1.TryAcquire() - } - - // limiter2 should still have tokens - if !limiter2.TryAcquire() { - t.Error("expected limiter2 to have tokens") - } - - // limiter1 should be empty - if limiter1.TryAcquire() { - t.Error("expected limiter1 to be empty") - } -} - func BenchmarkTokenBucket_TryAcquire(b *testing.B) { tb := rhttp.NewTokenBucket(1000000, 1000000) // high limits @@ -279,3 +313,177 @@ func BenchmarkTokenBucket_Concurrent(b *testing.B) { } }) } + +func TestRateLimit_FailFastClosesRequestBody(t *testing.T) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + return &http.Response{StatusCode: http.StatusOK, Body: http.NoBody, Request: req}, nil + }) + limiter := rhttp.NewTokenBucket(1, 1) + limiter.TryAcquire() + + c := rhttp.New( + rhttp.WithTransport(rt), + rhttp.WithMiddleware(rhttp.RateLimit(rhttp.RateLimitConfig{Limiter: limiter})), + ) + + rec := &closeRecorder{Reader: strings.NewReader("payload")} + req, _ := http.NewRequest(http.MethodPut, "http://example.com", http.NoBody) + req.Body = rec + _, err := c.Do(context.Background(), req) + + if !errors.Is(err, rhttp.ErrRateLimited) { + t.Fatalf("expected ErrRateLimited, got %v", err) + } + if !rec.closed { + t.Error("request body was not closed on rate-limit short-circuit") + } +} + +func TestTokenBucket_TokensReportsAvailability(t *testing.T) { + tb := rhttp.NewTokenBucket(1, 5) // 1 token/s: refill drift is negligible + + if got := tb.Tokens(); got != 5 { + t.Fatalf("expected a full bucket of 5 tokens, got %v", got) + } + + tb.TryAcquire() + tb.TryAcquire() + + got := tb.Tokens() + if got < 3 || got >= 4 { + t.Fatalf("expected ~3 tokens after two acquires, got %v", got) + } +} + +func TestTokenBucket_WaitContextAlreadyCanceled(t *testing.T) { + tb := rhttp.NewTokenBucket(1, 1) + tb.TryAcquire() + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + start := time.Now() + err := tb.WaitContext(ctx) + if !errors.Is(err, context.Canceled) { + t.Fatalf("expected context.Canceled, got %v", err) + } + if elapsed := time.Since(start); elapsed > 100*time.Millisecond { + t.Errorf("expected immediate return on canceled ctx, took %v", elapsed) + } +} + +func TestRateLimit_RetryAfterHTTPDate(t *testing.T) { + var calls int32 + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + if atomic.AddInt32(&calls, 1) == 1 { + resp := &http.Response{ + StatusCode: http.StatusTooManyRequests, + Header: make(http.Header), + Request: req, + } + resp.Header.Set("Retry-After", time.Now().Add(2*time.Second).UTC().Format(http.TimeFormat)) + return resp, nil + } + return &http.Response{StatusCode: http.StatusOK, Request: req}, nil + }) + + limiter := rhttp.NewTokenBucket(1000, 100) + c := rhttp.New( + rhttp.WithTransport(rt), + rhttp.WithMiddleware(rhttp.RateLimit(rhttp.RateLimitConfig{ + Limiter: limiter, + RespectRetryAfter: true, + })), + ) + + req, _ := http.NewRequest(http.MethodGet, "http://example.com", http.NoBody) + _, _ = c.Do(context.Background(), req) + + start := time.Now() + req, _ = http.NewRequest(http.MethodGet, "http://example.com", http.NoBody) + resp, err := c.Do(context.Background(), req) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if resp.StatusCode != http.StatusOK { + t.Fatalf("expected 200, got %d", resp.StatusCode) + } + if elapsed := time.Since(start); elapsed < 900*time.Millisecond { + t.Errorf("expected to honor the HTTP-date Retry-After (~1-2s), waited %v", elapsed) + } +} + +func TestRateLimit_CtxCanceledDuringRetryAfterWait(t *testing.T) { + var calls int32 + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + atomic.AddInt32(&calls, 1) + resp := &http.Response{ + StatusCode: http.StatusTooManyRequests, + Header: make(http.Header), + Request: req, + } + resp.Header.Set("Retry-After", "2") + return resp, nil + }) + + limiter := rhttp.NewTokenBucket(1000, 100) + c := rhttp.New( + rhttp.WithTransport(rt), + rhttp.WithMiddleware(rhttp.RateLimit(rhttp.RateLimitConfig{ + Limiter: limiter, + RespectRetryAfter: true, + })), + ) + + req, _ := http.NewRequest(http.MethodGet, "http://example.com", http.NoBody) + _, _ = c.Do(context.Background(), req) + + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + start := time.Now() + req, _ = http.NewRequest(http.MethodGet, "http://example.com", http.NoBody) + _, err := c.Do(ctx, req) + + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("expected DeadlineExceeded during Retry-After wait, got %v", err) + } + if elapsed := time.Since(start); elapsed > 1*time.Second { + t.Errorf("expected the canceled ctx to cut the 2s wait, took %v", elapsed) + } +} + +func TestRateLimit_RespectRetryAfterConcurrent(t *testing.T) { + var calls int32 + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + if atomic.AddInt32(&calls, 1)%3 == 0 { + resp := &http.Response{ + StatusCode: http.StatusTooManyRequests, + Header: make(http.Header), + Request: req, + } + resp.Header.Set("Retry-After", "0") + return resp, nil + } + return &http.Response{StatusCode: http.StatusOK, Request: req}, nil + }) + + limiter := rhttp.NewTokenBucket(100000, 1000) + c := rhttp.New( + rhttp.WithTransport(rt), + rhttp.WithMiddleware(rhttp.RateLimit(rhttp.RateLimitConfig{ + Limiter: limiter, + RespectRetryAfter: true, + })), + ) + + var wg sync.WaitGroup + for i := 0; i < 50; i++ { + wg.Add(1) + go func() { + defer wg.Done() + req, _ := http.NewRequest(http.MethodGet, "http://example.com", http.NoBody) + _, _ = c.Do(context.Background(), req) + }() + } + wg.Wait() +} diff --git a/request.go b/request.go index 0c5a9e7..552e7a2 100644 --- a/request.go +++ b/request.go @@ -14,8 +14,13 @@ import ( ) // RequestBuilder provides a fluent interface for building HTTP requests. +// +// A builder is meant for a single request and is not safe for concurrent use. +// Bodies set from bytes (SetBodyBytes, SetBodyString, SetBodyJSON, SetBodyXML, +// SetBodyForm) survive re-execution; a body set from a reader via SetBody is +// consumed by the first execution. type RequestBuilder struct { - client Client + client *Client ctx context.Context method string url string @@ -28,30 +33,13 @@ type RequestBuilder struct { err error } -// R creates a new RequestBuilder. -func (c *client) R() *RequestBuilder { +// R creates a new RequestBuilder bound to the client. +// Query and path parameter maps are initialized lazily on first use. +func (c *Client) R() *RequestBuilder { return &RequestBuilder{ - client: c, - ctx: context.Background(), - headers: make(http.Header), - queryParams: make(url.Values), - pathParams: make(map[string]string), - } -} - -// R creates a new RequestBuilder from a Client interface. -// Returns nil if the client doesn't support RequestBuilder. -func R(c Client) *RequestBuilder { - if rc, ok := c.(interface{ R() *RequestBuilder }); ok { - return rc.R() - } - // Fallback: create a basic builder - return &RequestBuilder{ - client: c, - ctx: context.Background(), - headers: make(http.Header), - queryParams: make(url.Values), - pathParams: make(map[string]string), + client: c, + ctx: context.Background(), + headers: make(http.Header), } } @@ -113,14 +101,22 @@ func (rb *RequestBuilder) SetBasicAuth(username, password string) *RequestBuilde return rb } +func (rb *RequestBuilder) ensureQueryParams() { + if rb.queryParams == nil { + rb.queryParams = make(url.Values) + } +} + // SetQueryParam sets a single query parameter. func (rb *RequestBuilder) SetQueryParam(key, value string) *RequestBuilder { + rb.ensureQueryParams() rb.queryParams.Set(key, value) return rb } // SetQueryParams sets multiple query parameters from a map. func (rb *RequestBuilder) SetQueryParams(params map[string]string) *RequestBuilder { + rb.ensureQueryParams() for k, v := range params { rb.queryParams.Set(k, v) } @@ -129,19 +125,28 @@ func (rb *RequestBuilder) SetQueryParams(params map[string]string) *RequestBuild // AddQueryParam adds a query parameter (allows multiple values for same key). func (rb *RequestBuilder) AddQueryParam(key, value string) *RequestBuilder { + rb.ensureQueryParams() rb.queryParams.Add(key, value) return rb } +func (rb *RequestBuilder) ensurePathParams() { + if rb.pathParams == nil { + rb.pathParams = make(map[string]string) + } +} + // SetPathParam sets a path parameter to be replaced in the URL. // Example: SetPathParam("id", "123") replaces {id} in "/users/{id}". func (rb *RequestBuilder) SetPathParam(key, value string) *RequestBuilder { + rb.ensurePathParams() rb.pathParams[key] = value return rb } // SetPathParams sets multiple path parameters from a map. func (rb *RequestBuilder) SetPathParams(params map[string]string) *RequestBuilder { + rb.ensurePathParams() for k, v := range params { rb.pathParams[k] = v } @@ -149,6 +154,10 @@ func (rb *RequestBuilder) SetPathParams(params map[string]string) *RequestBuilde } // SetBody sets the request body from a reader. +// +// The reader is buffered up to 10 MB so the body can be rewound and the request +// retried. If the body exceeds 10 MB it is streamed instead: the request is sent +// once and is not retried, since the reader cannot be replayed. func (rb *RequestBuilder) SetBody(body io.Reader) *RequestBuilder { rb.body = body return rb @@ -254,6 +263,36 @@ func (rb *RequestBuilder) Execute(method, url string) (*http.Response, error) { return rb.execute() } +const maxBufferBytes = 10 << 20 + +func bufferBody(r io.Reader) ([]byte, io.Reader, error) { + buf, err := io.ReadAll(io.LimitReader(r, maxBufferBytes+1)) + if err != nil { + return nil, nil, err + } + if len(buf) > maxBufferBytes { + return nil, io.MultiReader(bytes.NewReader(buf), r), nil + } + return buf, nil, nil +} + +func (rb *RequestBuilder) resolveBody() (io.Reader, []byte, error) { + if rb.body == nil { + return nil, rb.bodyBytes, nil + } + if rb.bodyBytes != nil { + return bytes.NewReader(rb.bodyBytes), rb.bodyBytes, nil + } + buf, stream, err := bufferBody(rb.body) + if err != nil { + return nil, nil, err + } + if stream != nil { + return stream, nil, nil + } + return bytes.NewReader(buf), buf, nil +} + func (rb *RequestBuilder) execute() (*http.Response, error) { if rb.err != nil { return nil, rb.err @@ -275,9 +314,9 @@ func (rb *RequestBuilder) execute() (*http.Response, error) { } // Create body reader - var bodyReader io.Reader - if rb.body != nil { - bodyReader = rb.body + bodyReader, bodyBytes, err := rb.resolveBody() + if err != nil { + return nil, err } // Create request @@ -287,29 +326,34 @@ func (rb *RequestBuilder) execute() (*http.Response, error) { } // Set GetBody for retry support - if rb.bodyBytes != nil { + if bodyBytes != nil { req.GetBody = func() (io.ReadCloser, error) { - return io.NopCloser(bytes.NewReader(rb.bodyBytes)), nil + return io.NopCloser(bytes.NewReader(bodyBytes)), nil } - req.ContentLength = int64(len(rb.bodyBytes)) + req.ContentLength = int64(len(bodyBytes)) } - // Apply headers - for k, vals := range rb.headers { - for _, v := range vals { - req.Header.Add(k, v) - } - } + // Client.Do clones the request, so sharing the builder's header map is safe. + req.Header = rb.headers // Apply timeout ctx := rb.ctx - if rb.timeout > 0 { - var cancel context.CancelFunc - ctx, cancel = context.WithTimeout(ctx, rb.timeout) - defer cancel() + if rb.timeout <= 0 { + return rb.client.Do(ctx, req) } - return rb.client.Do(ctx, req) + ctx, cancel := context.WithTimeout(ctx, rb.timeout) + resp, err := rb.client.Do(ctx, req) + if err != nil { + cancel() + return resp, err + } + if resp.Body == nil { + cancel() + return resp, nil + } + resp.Body = &cancelBody{ReadCloser: resp.Body, cancel: cancel} + return resp, nil } // basicAuth encodes username and password for Basic authentication. diff --git a/request_test.go b/request_test.go index 43a4a9d..c52d775 100644 --- a/request_test.go +++ b/request_test.go @@ -1,28 +1,32 @@ package rhttp_test import ( + "bytes" "context" + "encoding/base64" "encoding/json" + "errors" "io" "net/http" + "net/http/httptest" "strings" + "sync/atomic" "testing" "time" "github.com/oswaldom-code/rhttp" - "github.com/oswaldom-code/rhttp/internal" ) func TestRequestBuilder_Get(t *testing.T) { var capturedReq *http.Request - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { capturedReq = req return &http.Response{StatusCode: http.StatusOK, Request: req}, nil }) c := rhttp.New(rhttp.WithTransport(rt)) - resp, err := rhttp.R(c).Get("http://example.com/api") + resp, err := c.R().Get("http://example.com/api") if err != nil { t.Fatalf("unexpected error: %v", err) @@ -40,14 +44,14 @@ func TestRequestBuilder_Get(t *testing.T) { func TestRequestBuilder_Post(t *testing.T) { var capturedReq *http.Request - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { capturedReq = req return &http.Response{StatusCode: http.StatusCreated, Request: req}, nil }) c := rhttp.New(rhttp.WithTransport(rt)) - resp, err := rhttp.R(c). + resp, err := c.R(). SetBodyString("test body"). Post("http://example.com/api") @@ -64,14 +68,14 @@ func TestRequestBuilder_Post(t *testing.T) { func TestRequestBuilder_Headers(t *testing.T) { var capturedReq *http.Request - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { capturedReq = req return &http.Response{StatusCode: http.StatusOK, Request: req}, nil }) c := rhttp.New(rhttp.WithTransport(rt)) - _, _ = rhttp.R(c). + _, _ = c.R(). SetHeader("X-Custom", "value1"). SetHeaders(map[string]string{ "X-Another": "value2", @@ -108,14 +112,14 @@ func TestRequestBuilder_Headers(t *testing.T) { func TestRequestBuilder_QueryParams(t *testing.T) { var capturedReq *http.Request - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { capturedReq = req return &http.Response{StatusCode: http.StatusOK, Request: req}, nil }) c := rhttp.New(rhttp.WithTransport(rt)) - _, _ = rhttp.R(c). + _, _ = c.R(). SetQueryParam("page", "1"). SetQueryParams(map[string]string{ "limit": "10", @@ -144,14 +148,14 @@ func TestRequestBuilder_QueryParams(t *testing.T) { func TestRequestBuilder_PathParams(t *testing.T) { var capturedReq *http.Request - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { capturedReq = req return &http.Response{StatusCode: http.StatusOK, Request: req}, nil }) c := rhttp.New(rhttp.WithTransport(rt)) - _, _ = rhttp.R(c). + _, _ = c.R(). SetPathParam("org", "acme"). SetPathParams(map[string]string{ "repo": "api", @@ -168,7 +172,7 @@ func TestRequestBuilder_PathParams(t *testing.T) { func TestRequestBuilder_SetBodyJSON(t *testing.T) { var capturedReq *http.Request var capturedBody []byte - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { capturedReq = req capturedBody, _ = io.ReadAll(req.Body) return &http.Response{StatusCode: http.StatusOK, Request: req}, nil @@ -177,7 +181,7 @@ func TestRequestBuilder_SetBodyJSON(t *testing.T) { c := rhttp.New(rhttp.WithTransport(rt)) payload := map[string]string{"name": "test", "value": "123"} - _, _ = rhttp.R(c). + _, _ = c.R(). SetBodyJSON(payload). Post("http://example.com/api") @@ -197,7 +201,7 @@ func TestRequestBuilder_SetBodyJSON(t *testing.T) { func TestRequestBuilder_SetBodyForm(t *testing.T) { var capturedReq *http.Request var capturedBody string - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { capturedReq = req body, _ := io.ReadAll(req.Body) capturedBody = string(body) @@ -206,7 +210,7 @@ func TestRequestBuilder_SetBodyForm(t *testing.T) { c := rhttp.New(rhttp.WithTransport(rt)) - _, _ = rhttp.R(c). + _, _ = c.R(). SetBodyForm(map[string]string{ "username": "test", "password": "secret", @@ -227,14 +231,14 @@ func TestRequestBuilder_SetBodyForm(t *testing.T) { func TestRequestBuilder_SetAuthToken(t *testing.T) { var capturedReq *http.Request - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { capturedReq = req return &http.Response{StatusCode: http.StatusOK, Request: req}, nil }) c := rhttp.New(rhttp.WithTransport(rt)) - _, _ = rhttp.R(c). + _, _ = c.R(). SetAuthToken("my-token-123"). Get("http://example.com/api") @@ -246,25 +250,26 @@ func TestRequestBuilder_SetAuthToken(t *testing.T) { func TestRequestBuilder_SetBasicAuth(t *testing.T) { var capturedReq *http.Request - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { capturedReq = req return &http.Response{StatusCode: http.StatusOK, Request: req}, nil }) c := rhttp.New(rhttp.WithTransport(rt)) - _, _ = rhttp.R(c). + _, _ = c.R(). SetBasicAuth("user", "pass"). Get("http://example.com/api") auth := capturedReq.Header.Get("Authorization") - if !strings.HasPrefix(auth, "Basic ") { - t.Errorf("expected Authorization to start with 'Basic ', got %s", auth) + want := "Basic " + base64.StdEncoding.EncodeToString([]byte("user:pass")) + if auth != want { + t.Errorf("expected Authorization %q, got %q", want, auth) } } func TestRequestBuilder_Timeout(t *testing.T) { - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { // Respect context cancellation select { case <-req.Context().Done(): @@ -276,20 +281,54 @@ func TestRequestBuilder_Timeout(t *testing.T) { c := rhttp.New(rhttp.WithTransport(rt)) - _, err := rhttp.R(c). + _, err := c.R(). SetTimeout(50 * time.Millisecond). Get("http://example.com/api") if err == nil { t.Fatal("expected timeout error") } - if !strings.Contains(err.Error(), "context deadline exceeded") { - t.Errorf("expected deadline exceeded error, got %v", err) + if !errors.Is(err, context.DeadlineExceeded) { + t.Errorf("expected context.DeadlineExceeded, got %v", err) + } +} + +func TestRequestBuilder_SetTimeoutBodyReadableAfterReturn(t *testing.T) { + const head, tail = "first-chunk-", "second-chunk" + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + fl, ok := w.(http.Flusher) + if !ok { + t.Error("ResponseWriter is not a Flusher") + return + } + _, _ = io.WriteString(w, head) + fl.Flush() + time.Sleep(50 * time.Millisecond) + _, _ = io.WriteString(w, tail) + })) + defer srv.Close() + + c := rhttp.New() + + resp, err := c.R(). + SetTimeout(5 * time.Second). + Get(srv.URL) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + defer resp.Body.Close() + + body, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("reading body after builder execute returned: %v", err) + } + if string(body) != head+tail { + t.Fatalf("expected body %q, got %q", head+tail, body) } } func TestRequestBuilder_Context(t *testing.T) { - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { // Respect context cancellation select { case <-req.Context().Done(): @@ -304,7 +343,7 @@ func TestRequestBuilder_Context(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) defer cancel() - _, err := rhttp.R(c). + _, err := c.R(). Context(ctx). Get("http://example.com/api") @@ -331,13 +370,13 @@ func TestRequestBuilder_AllMethods(t *testing.T) { for _, m := range methods { t.Run(m.name, func(t *testing.T) { var capturedMethod string - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { capturedMethod = req.Method return &http.Response{StatusCode: http.StatusOK, Request: req}, nil }) c := rhttp.New(rhttp.WithTransport(rt)) - _, _ = m.fn(rhttp.R(c), "http://example.com") + _, _ = m.fn(c.R(), "http://example.com") if capturedMethod != m.expect { t.Errorf("expected %s, got %s", m.expect, capturedMethod) @@ -346,8 +385,45 @@ func TestRequestBuilder_AllMethods(t *testing.T) { } } +type opaqueReader struct{ r io.Reader } + +func (o *opaqueReader) Read(p []byte) (int, error) { return o.r.Read(p) } + +func TestRequestBuilder_ReaderBodyIsRetryable(t *testing.T) { + attempts := 0 + var bodies []string + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + attempts++ + b, _ := io.ReadAll(req.Body) + bodies = append(bodies, string(b)) + return &http.Response{StatusCode: http.StatusServiceUnavailable, Body: http.NoBody}, nil + }) + + c := rhttp.New( + rhttp.WithTransport(rt), + rhttp.WithMiddleware(rhttp.Retry(rhttp.RetryConfig{ + MaxAttempts: 3, + RetryAllMethods: true, + Backoff: rhttp.ConstantBackoff(0), + })), + ) + + _, _ = c.R(). + SetBody(&opaqueReader{r: strings.NewReader("payload")}). + Post("http://example.com") + + if attempts != 3 { + t.Fatalf("opaque reader body disabled retries: got %d attempts, want 3", attempts) + } + for i, b := range bodies { + if b != "payload" { + t.Errorf("attempt %d body = %q, want %q", i+1, b, "payload") + } + } +} + func BenchmarkRequestBuilder_Simple(b *testing.B) { - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { return &http.Response{StatusCode: http.StatusOK, Request: req}, nil }) @@ -357,12 +433,12 @@ func BenchmarkRequestBuilder_Simple(b *testing.B) { b.ReportAllocs() for i := 0; i < b.N; i++ { - _, _ = rhttp.R(c).Get("http://example.com") + _, _ = c.R().Get("http://example.com") } } func BenchmarkRequestBuilder_WithOptions(b *testing.B) { - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { return &http.Response{StatusCode: http.StatusOK, Request: req}, nil }) @@ -372,10 +448,199 @@ func BenchmarkRequestBuilder_WithOptions(b *testing.B) { b.ReportAllocs() for i := 0; i < b.N; i++ { - _, _ = rhttp.R(c). + _, _ = c.R(). SetHeader("Authorization", "Bearer token"). SetQueryParam("page", "1"). SetPathParam("id", "123"). Get("http://example.com/users/{id}") } } + +func TestRequestBuilder_SecondExecuteResendsFullBody(t *testing.T) { + var bodies []string + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + data, err := io.ReadAll(req.Body) + if err != nil { + return nil, err + } + bodies = append(bodies, string(data)) + return &http.Response{StatusCode: http.StatusOK, Body: http.NoBody, Request: req}, nil + }) + + c := rhttp.New(rhttp.WithTransport(rt)) + rb := c.R().SetBodyBytes([]byte("payload")) + + for i := 0; i < 2; i++ { + resp, err := rb.Post("http://example.com") + if err != nil { + t.Fatalf("execute %d: unexpected error: %v", i, err) + } + resp.Body.Close() + } + + if len(bodies) != 2 || bodies[0] != "payload" || bodies[1] != "payload" { + t.Fatalf("expected both executions to send the full body, got %q", bodies) + } +} + +func TestRequestBuilder_LargeBodyStreamsWithoutRetry(t *testing.T) { + // One byte over the 10 MB buffering limit forces the streaming path. + const size = 10<<20 + 1 + + var attempts int32 + var received int64 + var sawGetBody bool + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + atomic.AddInt32(&attempts, 1) + sawGetBody = req.GetBody != nil + n, err := io.Copy(io.Discard, req.Body) + if err != nil { + return nil, err + } + atomic.StoreInt64(&received, n) + return &http.Response{StatusCode: http.StatusServiceUnavailable, Body: http.NoBody, Request: req}, nil + }) + + c := rhttp.New( + rhttp.WithTransport(rt), + rhttp.WithMiddleware(rhttp.Retry(rhttp.RetryConfig{ + MaxAttempts: 3, + Backoff: rhttp.ConstantBackoff(time.Millisecond), + })), + ) + + opaque := &nonReplayableReader{r: bytes.NewReader(make([]byte, size))} + resp, err := c.R(). + SetBody(opaque). + Put("http://example.com/upload") + + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + resp.Body.Close() + + if sawGetBody { + t.Error("expected GetBody to be nil on the streaming path") + } + if received != size { + t.Errorf("expected the transport to receive %d bytes, got %d", size, received) + } + if got := atomic.LoadInt32(&attempts); got != 1 { + t.Errorf("expected a single attempt for a non-replayable streamed body, got %d", got) + } +} + +func TestRequestBuilder_MalformedURL(t *testing.T) { + var calls int32 + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + atomic.AddInt32(&calls, 1) + return &http.Response{StatusCode: http.StatusOK, Request: req}, nil + }) + c := rhttp.New(rhttp.WithTransport(rt)) + + _, err := c.R().Get("http://exa mple.com/api") + if err == nil { + t.Fatal("expected error for malformed URL") + } + if calls != 0 { + t.Errorf("expected the transport to never run, got %d calls", calls) + } +} + +func TestRequestBuilder_SetBodyJSONMarshalError(t *testing.T) { + var calls int32 + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + atomic.AddInt32(&calls, 1) + return &http.Response{StatusCode: http.StatusOK, Request: req}, nil + }) + c := rhttp.New(rhttp.WithTransport(rt)) + + _, err := c.R(). + SetBodyJSON(make(chan int)). + Post("http://example.com") + if err == nil { + t.Fatal("expected marshal error for unsupported JSON type") + } + if calls != 0 { + t.Errorf("expected the transport to never run, got %d calls", calls) + } +} + +func TestRequestBuilder_SetBodyXML(t *testing.T) { + var capturedReq *http.Request + var capturedBody string + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + capturedReq = req + body, _ := io.ReadAll(req.Body) + capturedBody = string(body) + return &http.Response{StatusCode: http.StatusOK, Request: req}, nil + }) + c := rhttp.New(rhttp.WithTransport(rt)) + + type User struct { + Name string `xml:"name"` + } + _, err := c.R(). + SetBodyXML(User{Name: "John"}). + Post("http://example.com") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got := capturedReq.Header.Get("Content-Type"); got != "application/xml" { + t.Errorf("expected Content-Type=application/xml, got %s", got) + } + if !strings.Contains(capturedBody, "John") { + t.Errorf("unexpected XML body: %s", capturedBody) + } +} + +func TestRequestBuilder_SetBodyXMLMarshalError(t *testing.T) { + c := rhttp.New(rhttp.WithTransport(rhttp.RoundTripperFunc( + func(req *http.Request) (*http.Response, error) { + return &http.Response{StatusCode: http.StatusOK, Request: req}, nil + }))) + + _, err := c.R(). + SetBodyXML(map[string]string{"k": "v"}). + Post("http://example.com") + if err == nil { + t.Fatal("expected marshal error: xml does not support maps") + } +} + +func TestRequestBuilder_ExecuteCustomMethod(t *testing.T) { + var capturedReq *http.Request + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + capturedReq = req + return &http.Response{StatusCode: http.StatusOK, Request: req}, nil + }) + c := rhttp.New(rhttp.WithTransport(rt)) + + _, err := c.R().Execute("TRACE", "http://example.com/api") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if capturedReq.Method != "TRACE" { + t.Errorf("expected method TRACE, got %s", capturedReq.Method) + } +} + +func TestRequestBuilder_PathParamIsEscaped(t *testing.T) { + var capturedReq *http.Request + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + capturedReq = req + return &http.Response{StatusCode: http.StatusOK, Request: req}, nil + }) + c := rhttp.New(rhttp.WithTransport(rt)) + + _, err := c.R(). + SetPathParam("id", "a/b c"). + Get("http://example.com/items/{id}") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + want := "http://example.com/items/a%2Fb%20c" + if got := capturedReq.URL.String(); got != want { + t.Errorf("expected escaped path param URL %s, got %s", want, got) + } +} diff --git a/retry.go b/retry.go index d533ad6..0f6c05e 100644 --- a/retry.go +++ b/retry.go @@ -12,8 +12,9 @@ type RetryConfig struct { MaxAttempts int // Backoff returns the duration to wait before the nth retry (0-indexed). - // If nil, exponential backoff is used. - Backoff func(attempt int) time.Duration + // It receives the response of the attempt that triggered the retry (nil if + // it produced no response). If nil, exponential backoff is used. + Backoff BackoffFunc // IsRetryable determines if a request should be retried based on the response and error. // If nil, default retry logic is used. @@ -59,12 +60,19 @@ func (r retryRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) for attempt := 0; attempt < r.cfg.MaxAttempts; attempt++ { if attempt > 0 { - if err := r.prepareRetry(req, attempt); err != nil { + if err := r.waitBackoff(req, attempt, resp); err != nil { + closeRequestBody(req) return nil, err } } - resp, err = r.next.RoundTrip(req) + attemptReq, prepErr := r.prepareRequest(req, attempt) + if prepErr != nil { + closeRequestBody(req) + return nil, prepErr + } + + resp, err = r.next.RoundTrip(attemptReq) if !r.cfg.IsRetryable(resp, err) { return resp, err @@ -86,28 +94,45 @@ func (r retryRoundTripper) canRetry(req *http.Request) bool { return req.Body == nil || req.Body == http.NoBody || req.GetBody != nil } -func (r retryRoundTripper) prepareRetry(req *http.Request, attempt int) error { +func (r retryRoundTripper) prepareRequest(req *http.Request, attempt int) (*http.Request, error) { + // The first attempt uses the request as-is: Do already cloned it, so the + // caller's request is never mutated. + if attempt == 0 { + return req, nil + } + + attemptReq := req.Clone(req.Context()) + if req.GetBody != nil { body, err := req.GetBody() if err != nil { - return err + return nil, err } - req.Body = body + attemptReq.Body = body } + return attemptReq, nil +} + +func (r retryRoundTripper) waitBackoff(req *http.Request, attempt int, prev *http.Response) error { + timer := time.NewTimer(r.cfg.Backoff(attempt-1, prev)) + defer timer.Stop() + select { case <-req.Context().Done(): return req.Context().Err() - case <-time.After(r.cfg.Backoff(attempt - 1)): + case <-timer.C: return nil } } +const maxDrainBytes = 256 << 10 + func drainAndClose(resp *http.Response) { if resp == nil || resp.Body == nil { return } - _, _ = io.Copy(io.Discard, resp.Body) + _, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, maxDrainBytes)) _ = resp.Body.Close() } @@ -124,7 +149,7 @@ func isIdempotent(method string) bool { // DefaultIsRetryable returns true for transient errors and retryable status codes. func DefaultIsRetryable(resp *http.Response, err error) bool { if err != nil { - return true + return Classify(err).Kind.IsRetryable() } if resp == nil { return false diff --git a/retry_test.go b/retry_test.go index 1265481..3003f0c 100644 --- a/retry_test.go +++ b/retry_test.go @@ -3,21 +3,24 @@ package rhttp_test import ( "bytes" "context" + "crypto/tls" "errors" "io" + "net" "net/http" + "net/url" "strings" "sync/atomic" + "syscall" "testing" "time" "github.com/oswaldom-code/rhttp" - "github.com/oswaldom-code/rhttp/internal" ) func TestRetry_SuccessOnFirstAttempt(t *testing.T) { var attempts int32 - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { atomic.AddInt32(&attempts, 1) return &http.Response{StatusCode: http.StatusOK, Request: req}, nil }) @@ -43,7 +46,7 @@ func TestRetry_SuccessOnFirstAttempt(t *testing.T) { func TestRetry_SuccessAfterRetry(t *testing.T) { var attempts int32 - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { n := atomic.AddInt32(&attempts, 1) if n < 3 { return &http.Response{ @@ -59,7 +62,7 @@ func TestRetry_SuccessAfterRetry(t *testing.T) { rhttp.WithTransport(rt), rhttp.WithMiddleware(rhttp.Retry(rhttp.RetryConfig{ MaxAttempts: 3, - Backoff: func(int) time.Duration { return time.Millisecond }, + Backoff: func(int, *http.Response) time.Duration { return time.Millisecond }, })), ) @@ -79,8 +82,8 @@ func TestRetry_SuccessAfterRetry(t *testing.T) { func TestRetry_MaxAttemptsExhausted(t *testing.T) { var attempts int32 - expectedErr := errors.New("connection refused") - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + expectedErr := syscall.ECONNREFUSED + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { atomic.AddInt32(&attempts, 1) return nil, expectedErr }) @@ -89,7 +92,7 @@ func TestRetry_MaxAttemptsExhausted(t *testing.T) { rhttp.WithTransport(rt), rhttp.WithMiddleware(rhttp.Retry(rhttp.RetryConfig{ MaxAttempts: 3, - Backoff: func(int) time.Duration { return time.Millisecond }, + Backoff: func(int, *http.Response) time.Duration { return time.Millisecond }, })), ) @@ -106,9 +109,10 @@ func TestRetry_MaxAttemptsExhausted(t *testing.T) { func TestRetry_NonIdempotentMethodNotRetried(t *testing.T) { var attempts int32 - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { atomic.AddInt32(&attempts, 1) - return nil, errors.New("connection refused") + // A genuinely retryable error: proves the method guard is what stops the retry. + return nil, syscall.ECONNREFUSED }) c := rhttp.New( @@ -126,10 +130,10 @@ func TestRetry_NonIdempotentMethodNotRetried(t *testing.T) { func TestRetry_NonIdempotentMethodWithRetryAllMethods(t *testing.T) { var attempts int32 - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { n := atomic.AddInt32(&attempts, 1) if n < 2 { - return nil, errors.New("connection refused") + return nil, syscall.ECONNREFUSED } return &http.Response{StatusCode: http.StatusOK, Request: req}, nil }) @@ -139,7 +143,7 @@ func TestRetry_NonIdempotentMethodWithRetryAllMethods(t *testing.T) { rhttp.WithMiddleware(rhttp.Retry(rhttp.RetryConfig{ MaxAttempts: 3, RetryAllMethods: true, - Backoff: func(int) time.Duration { return time.Millisecond }, + Backoff: func(int, *http.Response) time.Duration { return time.Millisecond }, })), ) @@ -165,16 +169,16 @@ func TestRetry_NonIdempotentMethodWithRetryAllMethods(t *testing.T) { func TestRetry_ContextCancelledDuringBackoff(t *testing.T) { var attempts int32 - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { atomic.AddInt32(&attempts, 1) - return nil, errors.New("connection refused") + return nil, syscall.ECONNREFUSED }) c := rhttp.New( rhttp.WithTransport(rt), rhttp.WithMiddleware(rhttp.Retry(rhttp.RetryConfig{ MaxAttempts: 3, - Backoff: func(int) time.Duration { return 10 * time.Second }, + Backoff: func(int, *http.Response) time.Duration { return 10 * time.Second }, })), ) @@ -203,7 +207,7 @@ func TestRetry_RetryableStatusCodes(t *testing.T) { for _, code := range retryableCodes { t.Run(http.StatusText(code), func(t *testing.T) { var attempts int32 - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { n := atomic.AddInt32(&attempts, 1) if n < 2 { return &http.Response{ @@ -219,7 +223,7 @@ func TestRetry_RetryableStatusCodes(t *testing.T) { rhttp.WithTransport(rt), rhttp.WithMiddleware(rhttp.Retry(rhttp.RetryConfig{ MaxAttempts: 3, - Backoff: func(int) time.Duration { return time.Millisecond }, + Backoff: func(int, *http.Response) time.Duration { return time.Millisecond }, })), ) @@ -239,7 +243,7 @@ func TestRetry_RetryableStatusCodes(t *testing.T) { func TestRetry_LastAttemptBodyReadable(t *testing.T) { const payload = "final-503-body" var attempts int32 - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { atomic.AddInt32(&attempts, 1) return &http.Response{ StatusCode: http.StatusServiceUnavailable, @@ -252,7 +256,7 @@ func TestRetry_LastAttemptBodyReadable(t *testing.T) { rhttp.WithTransport(rt), rhttp.WithMiddleware(rhttp.Retry(rhttp.RetryConfig{ MaxAttempts: 3, - Backoff: func(int) time.Duration { return time.Millisecond }, + Backoff: func(int, *http.Response) time.Duration { return time.Millisecond }, })), ) @@ -277,7 +281,7 @@ func TestRetry_LastAttemptBodyReadable(t *testing.T) { func TestRetry_NonRetryableStatusCode(t *testing.T) { var attempts int32 - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { atomic.AddInt32(&attempts, 1) return &http.Response{ StatusCode: http.StatusBadRequest, @@ -313,16 +317,17 @@ func (n *nonReplayableReader) Read(p []byte) (int, error) { func TestRetry_NonReplayableBodyNotRetried(t *testing.T) { var attempts int32 - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { atomic.AddInt32(&attempts, 1) - return nil, errors.New("connection refused") + // A genuinely retryable error: proves the body guard is what stops the retry. + return nil, syscall.ECONNREFUSED }) c := rhttp.New( rhttp.WithTransport(rt), rhttp.WithMiddleware(rhttp.Retry(rhttp.RetryConfig{ MaxAttempts: 3, - Backoff: func(int) time.Duration { return time.Millisecond }, + Backoff: func(int, *http.Response) time.Duration { return time.Millisecond }, })), ) @@ -338,12 +343,114 @@ func TestRetry_NonReplayableBodyNotRetried(t *testing.T) { } } +type countingBody struct { + io.Reader + read int64 +} + +func (c *countingBody) Read(p []byte) (int, error) { + n, err := c.Reader.Read(p) + c.read += int64(n) + return n, err +} + +func (c *countingBody) Close() error { return nil } + +func TestRetry_DrainIsBounded(t *testing.T) { + body := &countingBody{Reader: strings.NewReader(strings.Repeat("a", 8<<20))} + first := true + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + if first { + first = false + return &http.Response{StatusCode: http.StatusServiceUnavailable, Body: body, Request: req}, nil + } + return &http.Response{StatusCode: http.StatusOK, Body: http.NoBody, Request: req}, nil + }) + wrapped := rhttp.Retry(rhttp.RetryConfig{ + MaxAttempts: 2, + Backoff: func(int, *http.Response) time.Duration { return 0 }, + })(rt) + + req, _ := http.NewRequest(http.MethodGet, "http://example.com", http.NoBody) + _, _ = wrapped.RoundTrip(req) + + const maxDrain = 256 << 10 + if body.read > maxDrain { + t.Fatalf("drain read %d bytes of an 8 MB body; max acceptable: %d", body.read, maxDrain) + } +} + +func TestRetry_RespectsErrorClassification(t *testing.T) { + tlsErr := &url.Error{ + Op: "Get", + URL: "https://example.com", + Err: &tls.CertificateVerificationError{}, + } + + cases := []struct { + name string + err error + attempts int32 + }{ + {"tls_not_retryable", tlsErr, 1}, + {"canceled_not_retryable", context.Canceled, 1}, + {"connection_retryable", syscall.ECONNREFUSED, 3}, + {"dns_retryable", &net.DNSError{Err: "no such host"}, 3}, + {"timeout_retryable", context.DeadlineExceeded, 3}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var attempts int32 + rt := rhttp.RoundTripperFunc(func(*http.Request) (*http.Response, error) { + atomic.AddInt32(&attempts, 1) + return nil, tc.err + }) + wrapped := rhttp.Retry(rhttp.RetryConfig{ + MaxAttempts: 3, + Backoff: func(int, *http.Response) time.Duration { return 0 }, + })(rt) + + req, _ := http.NewRequest(http.MethodGet, "https://example.com", http.NoBody) + _, _ = wrapped.RoundTrip(req) + + if attempts != tc.attempts { + t.Fatalf("%s: got %d attempts, want %d", tc.name, attempts, tc.attempts) + } + }) + } +} + +func TestRetry_DoesNotMutateOriginalRequest(t *testing.T) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + return &http.Response{StatusCode: http.StatusServiceUnavailable, Body: http.NoBody, Request: req}, nil + }) + wrapped := rhttp.Retry(rhttp.RetryConfig{ + MaxAttempts: 3, + RetryAllMethods: true, + Backoff: func(int, *http.Response) time.Duration { return 0 }, + })(rt) + + payload := []byte(`{"x":1}`) + orig, _ := http.NewRequest(http.MethodPost, "http://example.com", bytes.NewReader(payload)) + orig.GetBody = func() (io.ReadCloser, error) { + return io.NopCloser(bytes.NewReader(payload)), nil + } + origBody := orig.Body + + _, _ = wrapped.RoundTrip(orig) + + if orig.Body != origBody { + t.Fatal("RoundTrip mutated req.Body of the original request (http.RoundTripper contract)") + } +} + func TestExponentialBackoff(t *testing.T) { backoff := rhttp.ExponentialBackoff(100*time.Millisecond, 1*time.Second) // Test exponential growth (with some tolerance for jitter) for attempt := 0; attempt < 5; attempt++ { - d := backoff(attempt) + d := backoff(attempt, nil) expected := 100 * time.Millisecond * (1 << attempt) if expected > 1*time.Second { expected = 1 * time.Second @@ -358,3 +465,76 @@ func TestExponentialBackoff(t *testing.T) { } } } + +func TestRetry_BackoffReceivesPreviousResponse(t *testing.T) { + var calls int32 + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + n := atomic.AddInt32(&calls, 1) + if n == 1 { + resp := &http.Response{ + StatusCode: http.StatusTooManyRequests, + Header: make(http.Header), + Body: http.NoBody, + Request: req, + } + resp.Header.Set("Retry-After", "1") + return resp, nil + } + return &http.Response{StatusCode: http.StatusOK, Body: http.NoBody, Request: req}, nil + }) + + c := rhttp.New( + rhttp.WithTransport(rt), + rhttp.WithMiddleware(rhttp.Retry(rhttp.RetryConfig{ + MaxAttempts: 3, + Backoff: rhttp.WithRetryAfter(rhttp.ConstantBackoff(10 * time.Millisecond)), + })), + ) + + start := time.Now() + req, _ := http.NewRequest(http.MethodGet, "http://example.com", http.NoBody) + resp, err := c.Do(context.Background(), req) + elapsed := time.Since(start) + + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if resp.StatusCode != http.StatusOK { + t.Fatalf("expected 200 after retry, got %d", resp.StatusCode) + } + if elapsed < 900*time.Millisecond { + t.Errorf("expected the retry to honor Retry-After (~1s), waited only %v", elapsed) + } +} + +func TestRetry_BackoffCanceledClosesRequestBody(t *testing.T) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + return &http.Response{StatusCode: http.StatusServiceUnavailable, Body: http.NoBody, Request: req}, nil + }) + + c := rhttp.New( + rhttp.WithTransport(rt), + rhttp.WithMiddleware(rhttp.Retry(rhttp.RetryConfig{ + MaxAttempts: 3, + Backoff: rhttp.ConstantBackoff(200 * time.Millisecond), + })), + ) + + rec := &closeRecorder{Reader: strings.NewReader("payload")} + req, _ := http.NewRequest(http.MethodPut, "http://example.com", http.NoBody) + req.Body = rec + req.GetBody = func() (io.ReadCloser, error) { + return io.NopCloser(strings.NewReader("payload")), nil + } + + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + _, err := c.Do(ctx, req) + + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("expected DeadlineExceeded during backoff, got %v", err) + } + if !rec.closed { + t.Error("request body was not closed when backoff was canceled") + } +} diff --git a/timeout.go b/timeout.go index 666e19a..6304544 100644 --- a/timeout.go +++ b/timeout.go @@ -9,7 +9,13 @@ import ( // Timeout returns a middleware that applies a timeout to requests. // If the request's context already has a shorter deadline, it is respected. +// A non-positive duration disables the middleware (it becomes a no-op). func Timeout(d time.Duration) Middleware { + if d <= 0 { + return func(next http.RoundTripper) http.RoundTripper { + return next + } + } return func(next http.RoundTripper) http.RoundTripper { return timeoutRoundTripper{next: next, timeout: d} } diff --git a/timeout_test.go b/timeout_test.go index d4d29e8..e4436c7 100644 --- a/timeout_test.go +++ b/timeout_test.go @@ -10,11 +10,10 @@ import ( "time" "github.com/oswaldom-code/rhttp" - "github.com/oswaldom-code/rhttp/internal" ) func TestTimeout_RequestCompletesBeforeTimeout(t *testing.T) { - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { return &http.Response{StatusCode: http.StatusOK, Request: req}, nil }) @@ -35,7 +34,7 @@ func TestTimeout_RequestCompletesBeforeTimeout(t *testing.T) { } func TestTimeout_RequestExceedsTimeout(t *testing.T) { - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { select { case <-req.Context().Done(): return nil, req.Context().Err() @@ -57,9 +56,38 @@ func TestTimeout_RequestExceedsTimeout(t *testing.T) { } } +func TestTimeout_NonPositiveDurationIsNoOp(t *testing.T) { + for _, d := range []time.Duration{0, -time.Second} { + t.Run(d.String(), func(t *testing.T) { + var hadDeadline bool + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + _, hadDeadline = req.Context().Deadline() + return &http.Response{StatusCode: http.StatusOK, Request: req}, nil + }) + + c := rhttp.New( + rhttp.WithTransport(rt), + rhttp.WithMiddleware(rhttp.Timeout(d)), + ) + + req, _ := http.NewRequest(http.MethodGet, "http://example.com", http.NoBody) + resp, err := c.Do(context.Background(), req) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if resp.StatusCode != http.StatusOK { + t.Fatalf("unexpected status: %d", resp.StatusCode) + } + if hadDeadline { + t.Fatal("expected no deadline for non-positive timeout") + } + }) + } +} + func TestTimeout_RespectsExistingShorterDeadline(t *testing.T) { var capturedDeadline time.Time - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { capturedDeadline, _ = req.Context().Deadline() return &http.Response{StatusCode: http.StatusOK, Request: req}, nil }) @@ -121,7 +149,7 @@ func TestTimeout_StreamingBodyReadableAfterReturn(t *testing.T) { func TestTimeout_AppliesWhenExistingDeadlineLonger(t *testing.T) { var capturedCtx context.Context - rt := internal.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + rt := rhttp.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { capturedCtx = req.Context() return &http.Response{StatusCode: http.StatusOK, Request: req}, nil })