diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml deleted file mode 100644 index be5ef12..0000000 --- a/.github/workflows/benchmark.yml +++ /dev/null @@ -1,57 +0,0 @@ -name: Bechmark - -on: - pull_request: - -jobs: - benchmark: - name: Benchmark - runs-on: ubuntu-latest - - services: - redis: - image: redis:6 - ports: - - 6379:6379 - - steps: - - name: Set up Go - uses: actions/setup-go@v5 - with: - go-version: ^1.17 - - - name: Git clone (master) - uses: actions/checkout@v4 - with: - ref: master - - - name: Run benchmark (master) - run: go test -bench=. -count=10 -benchmem | tee /tmp/master.txt - - - name: Git clone (PR) - uses: actions/checkout@v4 - - - name: Run benchmark (PR) - run: go test -bench=. -count=10 -benchmem | tee /tmp/pr.txt - - - name: Install benchstat - run: go install golang.org/x/perf/cmd/benchstat@latest - - - name: Run benchstat - run: cd /tmp && benchstat master.txt pr.txt | tee /tmp/result.txt - - - name: Comment on PR with benchmark results - uses: actions/github-script@v6 - with: - script: | - const fs = require('fs'); - const results = fs.readFileSync('/tmp/result.txt', 'utf8'); - const issue_number = context.payload.pull_request.number; - const { owner, repo } = context.repo; - - await github.rest.issues.createComment({ - owner, - repo, - issue_number, - body: `### Benchmark Results\n\n\`\`\`\n${results}\n\`\`\`` - }); diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0afc611..57c8df2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -11,7 +11,7 @@ jobs: services: redis: - image: redis:6 + image: redis:7 ports: - 6379:6379 diff --git a/config.go b/config.go index 7ce25e9..2796d4e 100644 --- a/config.go +++ b/config.go @@ -20,7 +20,7 @@ type Config struct { // Timeout for each Redis command after which we fall back to a local // in-memory counter. If Redis does not respond within this duration, // the system will use the local counter unless it is explicitly disabled. - FallbackTimeout time.Duration `toml:"fallback_timeout"` // default: 50ms + FallbackTimeout time.Duration `toml:"fallback_timeout"` // default: 100ms // Client if supplied will be used and the below fields will be ignored. // @@ -31,6 +31,6 @@ type Config struct { Port uint16 `toml:"port"` Password string `toml:"password"` // optional DBIndex int `toml:"db_index"` // default: 0 - MaxIdle int `toml:"max_idle"` // default: 4 - MaxActive int `toml:"max_active"` // default: 8 + MaxIdle int `toml:"max_idle"` // default: 5 + MaxActive int `toml:"max_active"` // default: 10 } diff --git a/httprateredis.go b/httprateredis.go index a9ca6f0..28b0fda 100644 --- a/httprateredis.go +++ b/httprateredis.go @@ -2,9 +2,7 @@ package httprateredis import ( "context" - "errors" "fmt" - "net" "os" "path/filepath" "strconv" @@ -40,8 +38,13 @@ func NewRedisLimitCounter(cfg *Config) (*redisCounter, error) { cfg.PrefixKey = "httprate" } if cfg.FallbackTimeout == 0 { - // Activate local in-memory fallback fairly quickly, as this would slow down all requests. - cfg.FallbackTimeout = 100 * time.Millisecond + if cfg.FallbackDisabled { + cfg.FallbackTimeout = time.Second + } else { + // Activate local in-memory fallback fairly quickly, + // so we don't slow down incoming requests too much. + cfg.FallbackTimeout = 100 * time.Millisecond + } } rc := &redisCounter{ @@ -54,10 +57,10 @@ func NewRedisLimitCounter(cfg *Config) (*redisCounter, error) { if cfg.Client == nil { maxIdle, maxActive := cfg.MaxIdle, cfg.MaxActive if maxIdle < 1 { - maxIdle = 20 + maxIdle = 5 } if maxActive < 1 { - maxActive = 50 + maxActive = 10 } rc.client = redis.NewClient(&redis.Options{ @@ -107,13 +110,8 @@ func (c *redisCounter) IncrementBy(key string, currentWindow time.Time, amount i return c.fallbackCounter.IncrementBy(key, currentWindow, amount) } defer func() { - if err != nil { - // On redis network error, fallback to local in-memory counter. - var netErr net.Error - if errors.As(err, &netErr) || errors.Is(err, redis.ErrClosed) { - c.fallback() - err = c.fallbackCounter.IncrementBy(key, currentWindow, amount) - } + if c.shouldFallback(err) { + err = c.fallbackCounter.IncrementBy(key, currentWindow, amount) } }() } @@ -147,13 +145,8 @@ func (c *redisCounter) Get(key string, currentWindow, previousWindow time.Time) return c.fallbackCounter.Get(key, currentWindow, previousWindow) } defer func() { - if err != nil { - // On redis network error, fallback to local in-memory counter. - var netErr net.Error - if errors.As(err, &netErr) || errors.Is(err, redis.ErrClosed) { - c.fallback() - curr, prev, err = c.fallbackCounter.Get(key, currentWindow, previousWindow) - } + if c.shouldFallback(err) { + curr, prev, err = c.fallbackCounter.Get(key, currentWindow, previousWindow) } }() } @@ -189,25 +182,34 @@ func (c *redisCounter) IsFallbackActivated() bool { return c.fallbackActivated.Load() } -func (c *redisCounter) fallback() { - // Activate the in-memory counter fallback, unless activated by some other goroutine. - fallbackAlreadyActivated := c.fallbackActivated.Swap(true) - if fallbackAlreadyActivated { - return +func (c *redisCounter) Close() error { + return c.client.Close() +} + +func (c *redisCounter) shouldFallback(err error) bool { + if err == nil { + return false + } + + // Activate the local in-memory counter fallback, unless activated by some other goroutine. + alreadyActivated := c.fallbackActivated.Swap(true) + if !alreadyActivated { + go c.reconnect() } - go c.reconnect() + return true } func (c *redisCounter) reconnect() { // Try to re-connect to redis every 200ms. for { + time.Sleep(200 * time.Millisecond) + err := c.client.Ping(context.Background()).Err() if err == nil { c.fallbackActivated.Store(false) return } - time.Sleep(200 * time.Millisecond) } } diff --git a/httprateredis_test.go b/httprateredis_test.go index 36fefbb..5a0a5ab 100644 --- a/httprateredis_test.go +++ b/httprateredis_test.go @@ -15,8 +15,8 @@ func TestRedisCounter(t *testing.T) { limitCounter, err := httprateredis.NewRedisLimitCounter(&httprateredis.Config{ Host: "localhost", Port: 6379, - MaxIdle: 100, - MaxActive: 200, + MaxIdle: 0, + MaxActive: 2, DBIndex: 0, ClientName: "httprateredis_test", PrefixKey: fmt.Sprintf("httprate:test:%v", rand.Int31n(100000)), // Unique Redis key for each test @@ -26,6 +26,7 @@ func TestRedisCounter(t *testing.T) { if err != nil { t.Fatalf("redis not available: %v", err) } + defer limitCounter.Close() limitCounter.Config(1000, time.Minute) @@ -156,23 +157,28 @@ func TestRedisCounter(t *testing.T) { func BenchmarkLocalCounter(b *testing.B) { limitCounter, err := httprateredis.NewRedisLimitCounter(&httprateredis.Config{ - Host: "localhost", - Port: 6379, - MaxIdle: 500, - MaxActive: 500, - DBIndex: 0, - ClientName: "httprateredis_test", - PrefixKey: fmt.Sprintf("httprate:test:%v", rand.Int31n(100000)), // Unique key for each test + Host: "localhost", + Port: 6379, + DBIndex: 0, + ClientName: "httprateredis_test", + PrefixKey: fmt.Sprintf("httprate:test:%v", rand.Int31n(100000)), // Unique key for each test + MaxActive: 10, + MaxIdle: 0, + FallbackDisabled: true, + FallbackTimeout: 5 * time.Second, }) if err != nil { b.Fatalf("redis not available: %v", err) } + defer limitCounter.Close() limitCounter.Config(1000, time.Minute) currentWindow := time.Now().UTC().Truncate(time.Minute) previousWindow := currentWindow.Add(-time.Minute) + concurrentRequests := 100 + b.ResetTimer() for i := 0; i < b.N; i++ { @@ -182,14 +188,14 @@ func BenchmarkLocalCounter(b *testing.B) { previousWindow.Add(time.Duration(i) * time.Minute) wg := sync.WaitGroup{} - wg.Add(1000) - for i := 0; i < 1000; i++ { + wg.Add(concurrentRequests) + for i := 0; i < concurrentRequests; i++ { // Simulate concurrent requests with different rate-limit keys. go func(i int) { defer wg.Done() _, _, _ = limitCounter.Get(fmt.Sprintf("key:%v", i), currentWindow, previousWindow) - _ = limitCounter.IncrementBy(fmt.Sprintf("key:%v", i), currentWindow, rand.Intn(100)) + _ = limitCounter.IncrementBy(fmt.Sprintf("key:%v", i), currentWindow, rand.Intn(20)) }(i) } wg.Wait()