Fix TTL in SetIfNotExistsWithTTL #67
Conversation
|
I believe CI failure is unrelated to this PR. I believe it's related to a change in one the dependencies. Should we pin dependency versions? They're currently not pinned in make get-deps |
|
@SimonBarendse Thanks! Good find, and thanks for the Go playground link — very helpful.
Hmm, so the long term "right" move is probably to package-ify and then pin versions that way. For this instance though: it looks like most of the errors are related to minor go-redis tweaks in the API — e.g. addition of I was thinking about a CI fix that basically just looks like this: diff --git rate.go rate.go
index a020541..502a164 100644
--- rate.go
+++ rate.go
diff --git store/goredisstore/goredisstore.go store/goredisstore/goredisstore.go
index 6b82af7..ed4e54a 100644
--- store/goredisstore/goredisstore.go
+++ store/goredisstore/goredisstore.go
@@ -2,6 +2,7 @@
package goredisstore // import "github.com/throttled/throttled/store/goredisstore"
import (
+ "context"
"strings"
"time"
@@ -46,12 +47,16 @@ func New(client *redis.Client, keyPrefix string) (*GoRedisStore, error) {
// or -1 if it does not exist. It also returns the current time at
// the redis server to microsecond precision.
func (r *GoRedisStore) GetWithTime(key string) (int64, time.Time, error) {
+ // TODO: Throttled should probably provide a Context-based version of its
+ // API.
+ context := context.Background()
+
key = r.prefix + key
pipe := r.client.Pipeline()
- timeCmd := pipe.Time()
- getKeyCmd := pipe.Get(key)
- _, err := pipe.Exec()
+ timeCmd := pipe.Time(context)
+ getKeyCmd := pipe.Get(context, key)
+ _, err := pipe.Exec(context)
now, err := timeCmd.Result()
if err != nil {
@@ -73,9 +78,13 @@ func (r *GoRedisStore) GetWithTime(key string) (int64, time.Time, error) {
// If a new value was set, the ttl in the key is also set, though this
// operation is not performed atomically.
func (r *GoRedisStore) SetIfNotExistsWithTTL(key string, value int64, ttl time.Duration) (bool, error) {
+ // TODO: Throttled should probably provide a Context-based version of its
+ // API.
+ context := context.Background()
+
key = r.prefix + key
- updated, err := r.client.SetNX(key, value, 0).Result()
+ updated, err := r.client.SetNX(context, key, value, 0).Result()
if err != nil {
return false, err
}
@@ -89,7 +98,7 @@ func (r *GoRedisStore) SetIfNotExistsWithTTL(key string, value int64, ttl time.D
ttlSeconds = 1
}
- err = r.client.Expire(key, ttlSeconds*time.Second).Err()
+ err = r.client.Expire(context, key, ttlSeconds*time.Second).Err()
return updated, err
}
@@ -99,6 +108,10 @@ func (r *GoRedisStore) SetIfNotExistsWithTTL(key string, value int64, ttl time.D
// store, it returns false with no error. If the swap succeeds, the
// ttl for the key is updated atomically.
func (r *GoRedisStore) CompareAndSwapWithTTL(key string, old, new int64, ttl time.Duration) (bool, error) {
+ // TODO: Throttled should probably provide a Context-based version of its
+ // API.
+ context := context.Background()
+
key = r.prefix + key
ttlSeconds := int(ttl.Seconds())
@@ -111,7 +124,7 @@ func (r *GoRedisStore) CompareAndSwapWithTTL(key string, old, new int64, ttl tim
}
// result will be 0 or 1
- result, err := r.client.Eval(redisCASScript, []string{key}, old, new, ttlSeconds).Result()
+ result, err := r.client.Eval(context, redisCASScript, []string{key}, old, new, ttlSeconds).Result()
var swapped bool
if s, ok := result.(int64); ok {
diff --git store/goredisstore/goredisstore_test.go store/goredisstore/goredisstore_test.go
index abbada5..b83fc1f 100644
--- store/goredisstore/goredisstore_test.go
+++ store/goredisstore/goredisstore_test.go
@@ -1,6 +1,7 @@
package goredisstore_test
import (
+ "context"
"log"
"testing"
"time"
@@ -62,12 +63,14 @@ func BenchmarkRedisStore(b *testing.B) {
}
func clearRedis(c *redis.Client) error {
- keys, err := c.Keys(redisTestPrefix + "*").Result()
+ context := context.Background()
+
+ keys, err := c.Keys(context, redisTestPrefix + "*").Result()
if err != nil {
return err
}
- return c.Del(keys...).Err()
+ return c.Del(context, keys...).Err()
}
func setupRedis(tb testing.TB, ttl time.Duration) (*redis.Client, *goredisstore.GoRedisStore) {
@@ -79,7 +82,7 @@ func setupRedis(tb testing.TB, ttl time.Duration) (*redis.Client, *goredisstore.
DB: redisTestDB, // use default DB
})
- if err := client.Ping().Err(); err != nil {
+ if err := client.Ping(context.Background()).Err(); err != nil {
client.Close()
tb.Skip("redis server not available on localhost port 6379")
}But I notice that touches code that you've directly modified above, which means that your project must still be on the older version of go-redis too. How much churn would upgrading be for you? |
|
@SimonBarendse Just in the interest of getting the build fixed, I timed out and merged #68. Would you mind rebasing what you have here against that? Thanks! |
A time.Duration represents the time as an int64 nanosecond count. By creating a time.Duration from the amount of seconds in the ttl we effectively create a time.Duration 1,000,000,000 times smaller than the orginal ttl, as we're then interpreting the seconds as nanoseconds. The following playground example illustrates this: https://play.golang.org/p/8w15MOkAfC3
|
Sorry for the late reply @brandur Thanks for fixing the build! Upgrading I've now rebased this against |
|
No worries, and thanks again! |
|
@SimonBarendse Sorry about the churn here, but I ended up reverting the changes I mentioned above that upgraded the go-redis store to use V8 in #78. After playing around with it a bit locally, I think the upgrade is more likely to cause problems than not because users need to inject the same version via the API. I upgraded this project to use Go Modules, and with that on, it's able to automatically pin itself to go-redis V6, which is the same API that we were using before, so it seems slightly better to continue using that as it's most likely what existing Throttled users were on. I'm still not exactly sure what the ideal long term answer is for us, but we may end up having to ship a separate store implementation to support newer versions of go-redis. |
|
Thanks for the heads up! Nice that the project now uses Go modules. Using pinned versions of dependencies will prevent similar issues in the future. As for the upgrade of go-redis, I believe that offers most value if we change throttled to become context-aware. We'd have to create a new store interface which accepts a context for all of its functions and then change the ratelimiters so that a context is passed. Line 63 in d85267f For backwards compatibility, we'd add three new context-aware interfaces (http ratelimiter, ratelimiter and store) and their corresponding implementations, while keeping the existing interfaces. We can move the existing implementations in a.o. The current redis store implementation can keep using What do you think? |
|
@SimonBarendse Yep, all of that seems roughly correct to me. The other option to a new struct might be to just add a new
Yep, that sounds like it's the way to go. |
Yes that could also work 👍 The reason I suggested to make two structs would be to help with naming. We'd be able to re-use the same function names. My reasoning was that it's already clear from the function signature that it's using a context, so having it in the function name as well would be a bit redundant. Especially in the store, which already has names ending with We'd have: type GCRARateLimiter interface {
RateLimit(key string, quantity int) (bool, RateLimitResult, error)
}
type GCRARateLimiterWithContext interface {
RateLimit(ctx context.Context, key string, quantity int) (bool, RateLimitResult, error)
}
type GCRAStore interface {
GetWithTime(key string) (int64, time.Time, error)
SetIfNotExistsWithTTL(key string, value int64, ttl time.Duration) (bool, error)
CompareAndSwapWithTTL(key string, old, new int64, ttl time.Duration) (bool, error)
}
type GCRAStoreWithContext interface {
GetWithTime(ctx context.Context, key string) (int64, time.Time, error)
SetIfNotExistsWithTTL(ctx context.Context, key string, value int64, ttl time.Duration) (bool, error)
CompareAndSwapWithTTL(ctx context.Context, key string, old, new int64, ttl time.Duration) (bool, error)
}or type GCRARateLimiter interface {
RateLimit(key string, quantity int) (bool, RateLimitResult, error)
}
type GCRARateLimiterWithContext interface {
RateLimitWithContext(ctx context.Context, key string, quantity int) (bool, RateLimitResult, error)
}
type GCRAStore interface {
GetWithTime(key string) (int64, time.Time, error)
SetIfNotExistsWithTTL(key string, value int64, ttl time.Duration) (bool, error)
CompareAndSwapWithTTL(key string, old, new int64, ttl time.Duration) (bool, error)
}
type GCRAStoreWithContext interface {
GetWithTimeWithContext(ctx context.Context, key string) (int64, time.Time, error)
SetIfNotExistsWithTTLWithContext(ctx context.Context, key string, value int64, ttl time.Duration) (bool, error)
CompareAndSwapWithTTLWithContext(ctx context.Context, key string, old, new int64, ttl time.Duration) (bool, error)
} |
|
@SimonBarendse Yep, good point! Are you interested in taking on this project? I think we should do it, but TBH, I probably wouldn't get around to it myself until at least a few more people complained about the lack of context awareness. |
|
Yes, I can take on this project. At the moment there's a couple of other projects that require my priority. I think I can get around to taking up this project in a month or so. We'll also be changing our setup to use a Redis cluster rather than a single instance then. In order to support this for throttled, we'd need a store that accepts the |
Awesome. Thank you!
Yep, definitely. Redis Cluster is key for anyone who needs to scale up rate limiting. |
Hi 👋
I think I've found a bug in the
SetIfNotExistsWithTTLfunction.A
time.Durationrepresents the time as anint64nanosecond count (godoc). However, we're currently creating atime.Durationfromttl.Seconds. Effectively, creating a new time.Duration 1,000,000,000 times smaller than the orginal ttl, as we're interpreting the seconds as nanoseconds.The following playground example illustrates this: https://play.golang.org/p/8w15MOkAfC3