From 760fb8b09105859010be6a9c48460ca47beab899 Mon Sep 17 00:00:00 2001 From: Vojtech Vitek Date: Wed, 7 Aug 2024 16:13:20 +0200 Subject: [PATCH 1/8] Fallback on all Redis errors --- httprateredis.go | 39 ++++++++++++++++----------------------- 1 file changed, 16 insertions(+), 23 deletions(-) diff --git a/httprateredis.go b/httprateredis.go index a9ca6f0..ebf321d 100644 --- a/httprateredis.go +++ b/httprateredis.go @@ -2,9 +2,7 @@ package httprateredis import ( "context" - "errors" "fmt" - "net" "os" "path/filepath" "strconv" @@ -107,13 +105,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 +140,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 +177,30 @@ 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) shouldFallback(err error) bool { + if err == nil { + return false } - go c.reconnect() + // Activate the local in-memory counter fallback, unless activated by some other goroutine. + alreadyActivated := c.fallbackActivated.Swap(true) + if !alreadyActivated { + 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) } } From 236d48d4e3a17b99fb0e7f8f2515860ac4208eb8 Mon Sep 17 00:00:00 2001 From: Vojtech Vitek Date: Wed, 7 Aug 2024 17:01:37 +0200 Subject: [PATCH 2/8] Benchmark: Fail on test error (tee ate the pipe error) --- .github/workflows/benchmark.yml | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index be5ef12..17f0152 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -26,13 +26,17 @@ jobs: ref: master - name: Run benchmark (master) - run: go test -bench=. -count=10 -benchmem | tee /tmp/master.txt + run: | + set -eo pipefail + 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 + run: | + set -eo pipefail + go test -bench=. -count=10 -benchmem | tee /tmp/pr.txt - name: Install benchstat run: go install golang.org/x/perf/cmd/benchstat@latest From 0ef2c4a052cdad45d84f115360f2affb4f91f027 Mon Sep 17 00:00:00 2001 From: Vojtech Vitek Date: Thu, 8 Aug 2024 11:34:36 +0200 Subject: [PATCH 3/8] Disable fallback on benchmark test --- httprateredis_test.go | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/httprateredis_test.go b/httprateredis_test.go index 36fefbb..f8563b6 100644 --- a/httprateredis_test.go +++ b/httprateredis_test.go @@ -156,13 +156,14 @@ 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, + MaxIdle: 500, + MaxActive: 500, + DBIndex: 0, + ClientName: "httprateredis_test", + PrefixKey: fmt.Sprintf("httprate:test:%v", rand.Int31n(100000)), // Unique key for each test + FallbackDisabled: true, }) if err != nil { b.Fatalf("redis not available: %v", err) From 408208f2da519a24bc8123d8552f5452a39316f0 Mon Sep 17 00:00:00 2001 From: Vojtech Vitek Date: Thu, 8 Aug 2024 11:46:14 +0200 Subject: [PATCH 4/8] Tweak redis pool settings for benchmark --- httprateredis_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/httprateredis_test.go b/httprateredis_test.go index f8563b6..d6ce605 100644 --- a/httprateredis_test.go +++ b/httprateredis_test.go @@ -158,8 +158,8 @@ func BenchmarkLocalCounter(b *testing.B) { limitCounter, err := httprateredis.NewRedisLimitCounter(&httprateredis.Config{ Host: "localhost", Port: 6379, - MaxIdle: 500, - MaxActive: 500, + MaxIdle: 100, + MaxActive: 200, DBIndex: 0, ClientName: "httprateredis_test", PrefixKey: fmt.Sprintf("httprate:test:%v", rand.Int31n(100000)), // Unique key for each test From c16478442573947516c7b662a94c8c2e3b39f174 Mon Sep 17 00:00:00 2001 From: Vojtech Vitek Date: Thu, 8 Aug 2024 16:01:47 +0200 Subject: [PATCH 5/8] Chill the defaults, we don't need that many connections --- config.go | 6 +++--- httprateredis.go | 13 +++++++++---- httprateredis_test.go | 2 -- 3 files changed, 12 insertions(+), 9 deletions(-) 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 ebf321d..f36e6cb 100644 --- a/httprateredis.go +++ b/httprateredis.go @@ -38,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{ @@ -52,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{ diff --git a/httprateredis_test.go b/httprateredis_test.go index d6ce605..3818c5a 100644 --- a/httprateredis_test.go +++ b/httprateredis_test.go @@ -158,8 +158,6 @@ func BenchmarkLocalCounter(b *testing.B) { limitCounter, err := httprateredis.NewRedisLimitCounter(&httprateredis.Config{ Host: "localhost", Port: 6379, - MaxIdle: 100, - MaxActive: 200, DBIndex: 0, ClientName: "httprateredis_test", PrefixKey: fmt.Sprintf("httprate:test:%v", rand.Int31n(100000)), // Unique key for each test From a2a09ff2750eaf445c07f20aa41abb1e69f94a89 Mon Sep 17 00:00:00 2001 From: Vojtech Vitek Date: Thu, 8 Aug 2024 16:44:14 +0200 Subject: [PATCH 6/8] Benchmark: Close redis client on each test iteration (-count=10) --- httprateredis.go | 4 ++++ httprateredis_test.go | 2 ++ 2 files changed, 6 insertions(+) diff --git a/httprateredis.go b/httprateredis.go index f36e6cb..28b0fda 100644 --- a/httprateredis.go +++ b/httprateredis.go @@ -182,6 +182,10 @@ func (c *redisCounter) IsFallbackActivated() bool { return c.fallbackActivated.Load() } +func (c *redisCounter) Close() error { + return c.client.Close() +} + func (c *redisCounter) shouldFallback(err error) bool { if err == nil { return false diff --git a/httprateredis_test.go b/httprateredis_test.go index 3818c5a..a6b4820 100644 --- a/httprateredis_test.go +++ b/httprateredis_test.go @@ -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) @@ -166,6 +167,7 @@ func BenchmarkLocalCounter(b *testing.B) { if err != nil { b.Fatalf("redis not available: %v", err) } + defer limitCounter.Close() limitCounter.Config(1000, time.Minute) From 34e20156a923193f445acb10f99f09f199c0bf84 Mon Sep 17 00:00:00 2001 From: Vojtech Vitek Date: Thu, 8 Aug 2024 16:58:36 +0200 Subject: [PATCH 7/8] Update Redis in CI, lower #no of requests --- .github/workflows/ci.yml | 2 +- httprateredis_test.go | 15 ++++++++++----- 2 files changed, 11 insertions(+), 6 deletions(-) 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/httprateredis_test.go b/httprateredis_test.go index a6b4820..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 @@ -162,7 +162,10 @@ func BenchmarkLocalCounter(b *testing.B) { 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) @@ -174,6 +177,8 @@ func BenchmarkLocalCounter(b *testing.B) { currentWindow := time.Now().UTC().Truncate(time.Minute) previousWindow := currentWindow.Add(-time.Minute) + concurrentRequests := 100 + b.ResetTimer() for i := 0; i < b.N; i++ { @@ -183,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() From 9ed1ad396d904ad145f2ee4b3d18f0b8de632c1e Mon Sep 17 00:00:00 2001 From: Vojtech Vitek Date: Thu, 8 Aug 2024 17:37:29 +0200 Subject: [PATCH 8/8] Remove benchmark from CI, as Github Actions limit us too much https://github.com/go-chi/httprate-redis/pull/16#issuecomment-2276127184 --- .github/workflows/benchmark.yml | 61 --------------------------------- 1 file changed, 61 deletions(-) delete mode 100644 .github/workflows/benchmark.yml diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml deleted file mode 100644 index 17f0152..0000000 --- a/.github/workflows/benchmark.yml +++ /dev/null @@ -1,61 +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: | - set -eo pipefail - go test -bench=. -count=10 -benchmem | tee /tmp/master.txt - - - name: Git clone (PR) - uses: actions/checkout@v4 - - - name: Run benchmark (PR) - run: | - set -eo pipefail - 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\`\`\`` - });