Skip to content

fix: use one window for queued rate-limit requests - #67

Open
2748109647 wants to merge 1 commit into
go-chi:masterfrom
2748109647:fix/consistent-request-window
Open

2748109647 wants to merge 1 commit into
go-chi:masterfrom
2748109647:fix/consistent-request-window

Conversation

@2748109647

Copy link
Copy Markdown

Fixes #66.

Problem

OnLimit computes the increment window before acquiring its mutex, while calculateRate samples the clock again after acquiring it. A request queued across a window boundary therefore reads one window and increments another. With the local counter, the stale increment can move the counter backwards and clear both maps, losing counts for other keys. The reset header can also describe the old window.

Change

  • Sample the time after acquiring the limiter mutex.
  • Use that same instant for the rate calculation, increment window and reset header.
  • Extract a private calculateRateAt helper; Status keeps sampling its own time through the existing helper.
  • Add a regression test using the actual local backend, both as the default counter and through WithLimitCounter. It holds the limiter mutex, queues a request, crosses a real window boundary, seeds another key in the new window, and checks read/write window consistency, the reset header and preservation of that key's count.

The public API and sliding-window weighting formula are unchanged.

Validation

Windows/amd64, Go 1.27.1:

  • Before the fix, the regression test failed in both configurations: read/write windows differed and the other key's count became zero (see OnLimit can discard local counters when a queued request crosses a window boundary #66 for actual output).
  • After the fix, go test -run TestOnLimitUsesSameWindowAfterWaitingForLock -count=5 -v passed in both configurations.
  • go test ./... -count=1 passed in the root module, including the final test robustness adjustment.
  • cd _example; go test ./... -count=1 passed for the chi integration examples.
  • go vet ./... passed; gofmt and git diff --check are clean.

The regression test coordinates the mutex directly to reproduce the queued-request schedule with the real local counter. No live Redis backend or load benchmark was run. The race detector was not run because a C compiler is unavailable in this environment.

@github-actions

Copy link
Copy Markdown

Benchmark Results

goos: linux
goarch: amd64
pkg: github.com/go-chi/httprate
cpu: AMD EPYC 7763 64-Core Processor                
               │ master.txt  │            pr.txt             │
               │   sec/op    │   sec/op     vs base          │
LocalCounter-4   19.69m ± 1%   19.86m ± 1%  ~ (p=0.105 n=10)

               │  master.txt  │             pr.txt             │
               │     B/op     │     B/op      vs base          │
LocalCounter-4   2.847Mi ± 0%   2.846Mi ± 0%  ~ (p=0.684 n=10)

               │ master.txt  │            pr.txt             │
               │  allocs/op  │  allocs/op   vs base          │
LocalCounter-4   121.5k ± 0%   121.5k ± 0%  ~ (p=0.698 n=10)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OnLimit can discard local counters when a queued request crosses a window boundary

1 participant