From 87c7e9e7a4fe15ff3507df244c8af5b3fddbce23 Mon Sep 17 00:00:00 2001 From: Nick Pillitteri <56quarters@users.noreply.github.com> Date: Tue, 11 Jun 2024 13:17:34 -0400 Subject: [PATCH] Memcached: Fix issue where non-zero TTLs were trunctated to zero (#530) Fixes an issue where if a non-zero TTL smaller than a full second was passed to the `SetAsync` or `SetMultiAsync` methods, the TTL would be truncated to zero seconds which results in the item being cached forever. Specifically, this fixes an issue in Mimir where items that were supposed to have a several minute TTL computed as `$ttl - time.Since($start)` ended up being cached forever. They were never evicted by Memcached because there was no need to free space in this particular cache due to low usage. Signed-off-by: Nick Pillitteri --- CHANGELOG.md | 1 + cache/memcached_client.go | 31 ++++++++++++++----------- cache/memcached_client_test.go | 41 ++++++++++++++++++++++++++++++---- 3 files changed, 56 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3b05bd7cb..94ceba6c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -243,3 +243,4 @@ * [BUGFIX] ring: don't mark trace spans as failed if `DoUntilQuorum` terminates due to cancellation. #449 * [BUGFIX] middleware: fix issue where applications that used the httpgrpc tracing middleware would generate duplicate spans for incoming HTTP requests. #451 * [BUGFIX] httpgrpc: store headers in canonical form when converting from gRPC to HTTP. #518 +* [BUGFIX] Memcached: Don't truncate sub-second TTLs to 0 which results in them being cached forever. #530 diff --git a/cache/memcached_client.go b/cache/memcached_client.go index 5a5cdc23e..a22f80354 100644 --- a/cache/memcached_client.go +++ b/cache/memcached_client.go @@ -315,22 +315,27 @@ func (c *MemcachedClient) Name() string { } func (c *MemcachedClient) SetMultiAsync(data map[string][]byte, ttl time.Duration) { - c.setMultiAsync(data, ttl, func(key string, buf []byte, ttl time.Duration) error { - return c.client.Set(&memcache.Item{ - Key: key, - Value: buf, - Expiration: int32(ttl.Seconds()), - }) - }) + c.setMultiAsync(data, ttl, c.setSingleItem) } func (c *MemcachedClient) SetAsync(key string, value []byte, ttl time.Duration) { - c.setAsync(key, value, ttl, func(key string, buf []byte, ttl time.Duration) error { - return c.client.Set(&memcache.Item{ - Key: key, - Value: buf, - Expiration: int32(ttl.Seconds()), - }) + c.setAsync(key, value, ttl, c.setSingleItem) +} + +func (c *MemcachedClient) setSingleItem(key string, value []byte, ttl time.Duration) error { + ttlSeconds := int32(ttl.Seconds()) + // If a TTL of exactly 0 is passed, we honor it and pass it to Memcached which will + // interpret it as an infinite TTL. However, if we get a non-zero TTL that is truncated + // to 0 seconds, we discard the update since the caller didn't intend to set an infinite + // TTL. + if ttl != 0 && ttlSeconds <= 0 { + return nil + } + + return c.client.Set(&memcache.Item{ + Key: key, + Value: value, + Expiration: ttlSeconds, }) } diff --git a/cache/memcached_client_test.go b/cache/memcached_client_test.go index 93d5e82f7..9b71cec1d 100644 --- a/cache/memcached_client_test.go +++ b/cache/memcached_client_test.go @@ -61,11 +61,44 @@ func TestMemcachedClientConfig_Validate(t *testing.T) { } } -func TestMemcachedClient_GetMulti(t *testing.T) { - setup := setupDefaultMemcachedClient +func TestMemcachedClient_SetAsync(t *testing.T) { + t.Run("with non-zero TTL", func(t *testing.T) { + client, _, err := setupDefaultMemcachedClient() + require.NoError(t, err) + client.SetAsync("foo", []byte("bar"), 10*time.Second) + require.NoError(t, client.wait()) + + ctx := context.Background() + res := client.GetMulti(ctx, []string{"foo"}) + require.Equal(t, map[string][]byte{"foo": []byte("bar")}, res) + }) + + t.Run("with truncated zero TTL", func(t *testing.T) { + client, _, err := setupDefaultMemcachedClient() + require.NoError(t, err) + client.SetAsync("foo", []byte("bar"), 100*time.Millisecond) + require.NoError(t, client.wait()) + + ctx := context.Background() + res := client.GetMulti(ctx, []string{"foo"}) + require.Empty(t, res) + }) + t.Run("with zero TTL", func(t *testing.T) { + client, _, err := setupDefaultMemcachedClient() + require.NoError(t, err) + client.SetAsync("foo", []byte("bar"), 0) + require.NoError(t, client.wait()) + + ctx := context.Background() + res := client.GetMulti(ctx, []string{"foo"}) + require.Equal(t, map[string][]byte{"foo": []byte("bar")}, res) + }) +} + +func TestMemcachedClient_GetMulti(t *testing.T) { t.Run("no allocator", func(t *testing.T) { - client, backend, err := setup() + client, backend, err := setupDefaultMemcachedClient() require.NoError(t, err) client.SetAsync("foo", []byte("bar"), 10*time.Second) require.NoError(t, client.wait()) @@ -77,7 +110,7 @@ func TestMemcachedClient_GetMulti(t *testing.T) { }) t.Run("with allocator", func(t *testing.T) { - client, backend, err := setup() + client, backend, err := setupDefaultMemcachedClient() require.NoError(t, err) client.SetAsync("foo", []byte("bar"), 10*time.Second) require.NoError(t, client.wait())