Add context.Context to LimitCounter methods that may invoke network calls
- Dominant language
- Go
- Stars
- 476
- Forks
- 25
- PR merge metrics
- No merged PRs in 30d
Description
## Background
This carries over the proposal from #13 ("Add support for context counter"), which was closed as stale after going unmerged for a long time. The proposal itself still holds: `LimitCounter` methods can be backed by network calls (e.g. [httprate-redis](https://github.com/go-chi/httprate-redis)), yet the interface doesn't accept a `context.Context`:
```go
type LimitCounter interface {
Config(requestLimit int, windowLength time.Duration)
Increment(key string, currentWindow time.Time) error
IncrementBy(key string, currentWindow time.Time, amount int) error
Get(key string, currentWindow, previousWindow time.Time) (int, int, error)
}
```
## Use cases (from the #13 discussion)
- **Network-backed counters with transactions** (@creack): a distributed counter implementation needs the request context to scope its network operations.
- **Tracing / context propagation** (@bendrucker): with httprate-redis and a traced Redis client (e.g. dd-trace-go, [redisotel](https://github.com/redis/go-redis/tree/master/extra/redisotel)), the Redis spans don't attach to the parent request span because Go tracers thread the active span through `context.WithValue`. This works only if the request context reaches the counter.
- **Timeouts / fallbacks**: a context deadline would let implementations bound the network call and e.g. fall back to a local in-memory counter when Redis is slow.
## Proposal
Add `ctx context.Context` as the first parameter to the methods that may perform I/O:
```go
type LimitCounter interface {
Config(requestLimit int, windowLength time.Duration)
Increment(ctx context.Context, key string, currentWindow time.Time) error
IncrementBy(ctx context.Context, key string, currentWindow time.Time, amount int) error
Get(ctx context.Context, key string, currentWindow, previousWindow time.Time) (int, int, error)
}
```
The middleware would pass `r.Context()` down from `OnLimit`/`RespondOnLimit`. `RateLimiter.Status(key)` has no request in scope, so it would need a `ctx` parameter too (or a `StatusCtx` variant).
## Design questions to settle
1. **Cancellation semantics vs. abuse**: if thousands of clients connect and disconnect within <1ms (DDoS-shaped traffic), should a canceled request context abort the counter increment — effectively letting the attacker avoid counting toward the 429 limit? One option from the #13 thread: pass the context through [`context.WithoutCancel`](https://pkg.go.dev/context#WithoutCancel) by default (tracing values propagate, cancellation doesn't), with opt-in cancellation.
2. **Breaking change strategy**: changing the interface breaks existing custom `LimitCounter` implementations (including httprate-redis). Since #61 already modernized the key API with a deprecation path, this may be acceptable for a future release — possibly a `LimitCounterCtx` interface upgraded via type assertion for a transition period, or a clean break.
3. **Global timeouts as an alternative**: would a timeout configured inside the counter implementation (ignoring `r.Context()`) be sufficient? Per the discussion above — no, because it doesn't solve tracing propagation.
/cc @creack @bendrucker
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with the LimitCounter interface and trace the OnLimit and RespondOnLimit entry points that would pass request context into counter methods. Review RateLimiter.Status and the prior #13 discussion, then settle cancellation semantics and a breaking-change or compatibility strategy. Done means the context-aware API and its migration approach are clearly agreed.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend-api-design
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Needs clarification
- Newbie friendliness
- 30/100