diff --git a/csc_commands.go b/csc_commands.go index 4664181f..2b9d1d57 100644 --- a/csc_commands.go +++ b/csc_commands.go @@ -71,7 +71,7 @@ func isCacheable(cmd Cmder) bool { if cmd.Name() == "sort_ro" && sortROHasByGet(cmd) { return false } - return cmdFirstKeyPosWithInfo(cmd, nil) != 0 + return cmdFirstKeyPosWithInfo(cmd, nil) > 0 } // sortROHasByGet reports whether a SORT_RO invocation uses BY or GET @@ -200,109 +200,97 @@ func buildCacheKeyNS(cmd Cmder, ns string) (string, bool) { // cacheKeyScratchMaxCap bounds the buffer capacity kept in the pool. const cacheKeyScratchMaxCap = 64 << 10 -// keyArg renders the key argument at pos exactly as proto.Writer sends it to -// the server, so invalidation lookups match the key names in the server's -// "invalidate" pushes. Only types whose stringArg rendering is byte-identical -// to the wire encoding are accepted (fmt.Sprint of any integer matches the +// isWireKeyType reports whether a key argument of v's type renders, through +// stringArg, exactly as proto.Writer sends it to the server, so invalidation +// lookups match the key names in the server's "invalidate" pushes. Only +// byte-identical types are accepted (fmt.Sprint of any integer matches the // writer's base-10 strconv output); for anything else — pointers, bools, // times, durations, floats, BinaryMarshaler values — the rendering can // diverge, the invalidation would never match, and the entry would be served -// stale forever, so ok=false and the caller skips caching (see processCached). -func keyArg(cmd Cmder, pos int) (string, bool) { - args := cmd.Args() - if pos < 0 || pos >= len(args) { - return "", false - } - switch args[pos].(type) { +// stale forever, so the caller skips caching (see processCached). +func isWireKeyType(v any) bool { + switch v.(type) { case string, []byte, int, int8, int16, int32, int64, uint, uint8, uint16, uint32, uint64: - return cmd.stringArg(pos), true + return true } - return "", false + return false } -// extractRedisKeys returns the Redis key arguments from cmd. The result lets -// the cache map incoming invalidations back to affected entries. Returns nil -// (caller skips caching) when any key -// argument cannot be rendered in its wire form (see keyArg). -func extractRedisKeys(cmd Cmder) []string { +// cscKeySpan returns the inclusive argument range [lo, hi] that holds cmd's +// Redis keys. For every cacheable command shape the keys are contiguous. +// ok is false when cmd has no key. +func cscKeySpan(cmd Cmder) (lo, hi int, ok bool) { firstKey := cmdFirstKeyPosWithInfo(cmd, nil) - if firstKey == 0 { - return nil + // SetFirstKeyPos is public and takes any int8, so a negative position is + // possible. It is not a key: reject it here rather than index args with it. + if firstKey <= 0 { + return 0, 0, false } - argsLen := len(cmd.Args()) if firstKey >= argsLen { - return nil + return 0, 0, false } switch cmd.Name() { // All remaining args from firstKeyPos are keys. case "mget", "exists", "sdiff", "sinter", "sunion": - keys := make([]string, 0, argsLen-firstKey) - for i := firstKey; i < argsLen; i++ { - k, ok := keyArg(cmd, i) - if !ok { - return nil - } - keys = append(keys, k) - } - return keys + return firstKey, argsLen - 1, true // Numkeys pattern: numkeys at args[1], keys from args[2]. case "sintercard", "zdiff", "zinter", "zunion": if argsLen < 3 { - return nil + return 0, 0, false } - numKeys, err := strconv.Atoi(cmd.stringArg(1)) - if err != nil || numKeys <= 0 { - return nil + numKeys, ok := cscNumKeys(cmd) + if !ok || numKeys <= 0 { + return 0, 0, false } - keys := make([]string, 0, numKeys) - for i := 2; i < 2+numKeys && i < argsLen; i++ { - k, ok := keyArg(cmd, i) - if !ok { - return nil - } - keys = append(keys, k) - } - return keys + return 2, min(2+numKeys, argsLen) - 1, true // LCS: exactly two consecutive keys starting at firstKeyPos. case "lcs": if firstKey+1 >= argsLen { - return nil + return 0, 0, false } - k1, ok1 := keyArg(cmd, firstKey) - k2, ok2 := keyArg(cmd, firstKey+1) - if !ok1 || !ok2 { - return nil - } - return []string{k1, k2} + return firstKey, firstKey + 1, true // JSON.MGET: keys from firstKeyPos to second-to-last (last arg is the // JSON path, not a key). case "json.mget": - lastKey := argsLen - 2 - if lastKey < firstKey { - return nil + if argsLen-2 < firstKey { + return 0, 0, false } - keys := make([]string, 0, lastKey-firstKey+1) - for i := firstKey; i <= lastKey; i++ { - k, ok := keyArg(cmd, i) - if !ok { - return nil - } - keys = append(keys, k) - } - return keys + return firstKey, argsLen - 2, true } // Single key at firstKeyPos (GET, HGET, LRANGE, ...). - k, ok := keyArg(cmd, firstKey) - if !ok { - return nil + return firstKey, firstKey, true +} + +// cscNumKeys parses the numkeys argument (args[1]). The typed commands pass +// an int, which is read without rendering it to a string. +func cscNumKeys(cmd Cmder) (int, bool) { + switch v := cmd.Args()[1].(type) { + case int: + return v, true + case int64: + return int(v), true } - return []string{k} + n, err := strconv.Atoi(cmd.stringArg(1)) + return n, err == nil +} + +// cscKeysRenderable reports whether every key in [lo, hi] is a type +// isWireKeyType accepts. It allocates nothing, so processCached runs it on every read, +// before it builds the cache key. +func cscKeysRenderable(cmd Cmder, lo, hi int) bool { + args := cmd.Args() + for i := lo; i <= hi; i++ { + if !isWireKeyType(args[i]) { + return false + } + } + return true } diff --git a/csc_extractkeys_test.go b/csc_extractkeys_test.go new file mode 100644 index 00000000..9438e749 --- /dev/null +++ b/csc_extractkeys_test.go @@ -0,0 +1,20 @@ +package redis + +// extractRedisKeys returns the Redis key arguments from cmd. The result lets +// the cache map incoming invalidations back to affected entries. Returns nil +// (caller skips caching) when any key argument cannot be rendered in its wire +// form (see isWireKeyType). +// +// Production code reads the key span in processCached and builds the strings +// only on a miss (cscMissKeys), so this helper has no non-test caller. +func extractRedisKeys(cmd Cmder) []string { + lo, hi, ok := cscKeySpan(cmd) + if !ok || !cscKeysRenderable(cmd, lo, hi) { + return nil + } + keys := make([]string, 0, hi-lo+1) + for i := lo; i <= hi; i++ { + keys = append(keys, cmd.stringArg(i)) + } + return keys +} diff --git a/csc_integration.go b/csc_integration.go index a2ab8946..e1e57abe 100644 --- a/csc_integration.go +++ b/csc_integration.go @@ -129,6 +129,21 @@ func cscNamespacedKey(prefix, key string) string { return prefix + key } +// cscMissKeys returns the namespaced redis keys in args [lo, hi] for Reserve. +// processCached has already checked the span with cscKeySpan and +// cscKeysRenderable. +// +// This is called on the MISS path only. It used to run before the cache-hit +// check, which meant a hit -- the 94-99% case -- allocated a raw key slice, +// this slice, and one string per key, then discarded all three. +func cscMissKeys(cmd Cmder, keyPrefix string, lo, hi int) []string { + ns := make([]string, 0, hi-lo+1) + for i := lo; i <= hi; i++ { + ns = append(ns, cscNamespacedKey(keyPrefix, cmd.stringArg(i))) + } + return ns +} + // invalidateHandler propagates RESP3 "invalidate" push notifications into the // shared client-side cache. keyPrefix scopes incoming key names so a shared // cache cannot collide across databases or fixed ACL identities. @@ -1538,29 +1553,36 @@ func (c *baseClient) processCached(ctx context.Context, cmd Cmder, state *proces return c.processWithRetry(ctx, cmd, nil, state, startAttempt) } - redisKeys := extractRedisKeys(cmd) - if len(redisKeys) == 0 { - // Without a key list we cannot react to invalidations for this command. - return c.processWithRetry(ctx, cmd, nil, state, startAttempt) - } - // Read the namespace BEFORE building the key: the key is now built with // the namespace already in it, in one allocation, so the prefix has to be - // known first. Both checks are cheap and neither depends on the other. + // known first. keyPrefix := c.cscKeyPrefix if keyPrefix == "" { // A successfully attached client always has a namespace. Fail closed if // an incomplete custom baseClient reaches this path. return c.processWithRetry(ctx, cmd, nil, state, startAttempt) } + // Check the key arguments BEFORE building the cache key. Building it + // encodes every argument, which calls MarshalBinary on a marshaler key; an + // uncacheable command would then marshal its key twice, once here and once + // for the request. The check allocates nothing. + keyLo, keyHi, ok := cscKeySpan(cmd) + if !ok || !cscKeysRenderable(cmd, keyLo, keyHi) { + // Without a key list we cannot react to invalidations for this + // command, so it must not be cached. + return c.processWithRetry(ctx, cmd, nil, state, startAttempt) + } key, ok := buildCacheKeyNS(cmd, keyPrefix) if !ok { return c.processWithRetry(ctx, cmd, nil, state, startAttempt) } - nsRedisKeys := make([]string, len(redisKeys)) - for i, k := range redisKeys { - nsRedisKeys[i] = cscNamespacedKey(keyPrefix, k) - } + + // The key strings are deliberately NOT built here. Only Reserve needs + // them, and Reserve is on the miss path -- so at a 94-99% hit rate this + // used to allocate a key slice, a namespaced slice and one string per key + // on every read and discard all of it. By object count that was ~18% of + // everything the client allocated. They are built in cscMissKeys below, + // after the hit check. // Serve hits straight from the cache. if data, ok := c.cscGet(ctx, key); ok { @@ -1582,6 +1604,9 @@ func (c *baseClient) processCached(ctx context.Context, cmd Cmder, state *proces // refreshes); only the early-flush latency is client-local. c.cscRefreshQueue.signalDemand(key) + // Miss path: now the key list is actually needed. + nsRedisKeys := cscMissKeys(cmd, keyPrefix, keyLo, keyHi) + token, shouldFetch := c.csc.Reserve(key, nsRedisKeys) if !shouldFetch { // Another goroutine is fetching; Get below waits until it completes. diff --git a/csc_keygate_test.go b/csc_keygate_test.go new file mode 100644 index 00000000..77b9b3e3 --- /dev/null +++ b/csc_keygate_test.go @@ -0,0 +1,76 @@ +package redis_test + +import ( + "context" + "strconv" + "testing" + "time" + + "github.com/redis/go-redis/v9" +) + +// countingKeyArg is a key argument the cache cannot render in wire form, so +// the command is uncacheable. It counts MarshalBinary calls. +type countingKeyArg struct { + key string + calls int +} + +func (a *countingKeyArg) MarshalBinary() ([]byte, error) { + a.calls++ + return []byte(a.key), nil +} + +// TestCSCUncacheableKeyMarshaledOnce pins the key-type gate order in +// processCached: a command whose key the cache cannot render must fall back +// before the cache key is built. Building the cache key encodes every +// argument, so a BinaryMarshaler key would be marshaled once for the cache +// key and again for the request. +func TestCSCUncacheableKeyMarshaledOnce(t *testing.T) { + if err := probeRedis(cscNativeAddr()); err != nil { + t.Skipf("redis not available at %s: %v", cscNativeAddr(), err) + } + cache := redis.NewLocalCache(redis.CacheConfig{MaxEntries: 32}) + c := redis.NewClient(&redis.Options{ + Addr: cscNativeAddr(), + Protocol: 3, + ClientSideCache: cache, + PoolSize: 1, + }) + t.Cleanup(func() { _ = c.Close() }) + + ctx := context.Background() + if err := c.Ping(ctx).Err(); err != nil { + t.Skipf("redis not available at %s: %v", cscNativeAddr(), err) + } + plain := redis.NewClient(&redis.Options{Addr: cscNativeAddr()}) + t.Cleanup(func() { _ = plain.Close() }) + unavailable, err := probeClientTracking(ctx, plain) + if unavailable { + t.Skipf("CLIENT TRACKING is unavailable: %v", err) + } + if err != nil { + t.Fatalf("probe CLIENT TRACKING: %v", err) + } + + key := "csc-keygate:" + strconv.FormatInt(time.Now().UnixNano(), 10) + if err := plain.Set(ctx, key, "v", 0).Err(); err != nil { + t.Fatalf("SET: %v", err) + } + t.Cleanup(func() { _ = plain.Del(context.Background(), key).Err() }) + + arg := &countingKeyArg{key: key} + cmd := redis.NewStringCmd(ctx, "get", arg) + if err := c.Process(ctx, cmd); err != nil { + t.Fatalf("GET: %v", err) + } + if got := cmd.Val(); got != "v" { + t.Fatalf("GET = %q, want v", got) + } + if arg.calls != 1 { + t.Fatalf("MarshalBinary called %d times, want 1 (the request only)", arg.calls) + } + if n := cache.Len(); n != 0 { + t.Fatalf("cache.Len = %d, want 0: an unrenderable key must not be cached", n) + } +} diff --git a/csc_keypos_negative_test.go b/csc_keypos_negative_test.go new file mode 100644 index 00000000..0c6112af --- /dev/null +++ b/csc_keypos_negative_test.go @@ -0,0 +1,35 @@ +package redis + +import ( + "context" + "testing" +) + +// A cacheable command whose public key position was set to a negative value +// is not cacheable, and the key gate does not index args with it. The span +// let -1 through (only 0 was rejected), and cscKeysRenderable then read +// args[-1] and panicked the process; the old keyArg bounds check had sent such +// a command normally. +func TestCSCNegativeKeyPositionIsUncacheable(t *testing.T) { + ctx := context.Background() + get := NewStringCmd(ctx, "get", "k") + get.SetFirstKeyPos(-1) + mget := NewSliceCmd(ctx, "mget", "a", "b") + mget.SetFirstKeyPos(-3) + for _, cmd := range []Cmder{get, mget} { + if isCacheable(cmd) { + t.Errorf("%s with a negative key position reported cacheable", cmd.Name()) + } + func() { + defer func() { + if r := recover(); r != nil { + t.Errorf("%s: key gate panicked: %v", cmd.Name(), r) + } + }() + if lo, hi, ok := cscKeySpan(cmd); ok { + t.Errorf("%s: span (%d, %d) accepted for a negative key position", cmd.Name(), lo, hi) + _ = cscKeysRenderable(cmd, lo, hi) + } + }() + } +} diff --git a/csc_keyspan_test.go b/csc_keyspan_test.go new file mode 100644 index 00000000..8d4b4a91 --- /dev/null +++ b/csc_keyspan_test.go @@ -0,0 +1,32 @@ +package redis + +import ( + "context" + "testing" +) + +// TestCSCKeyGateAllocatesNothing pins that the key check processCached runs +// on every cached read, hits included, does not allocate. +func TestCSCKeyGateAllocatesNothing(t *testing.T) { + ctx := context.Background() + // A no-op cmdable builds the typed commands without any I/O. + c := cmdable(func(context.Context, Cmder) error { return nil }) + cmds := map[string]Cmder{ + "get": c.Get(ctx, "k"), + "mget": c.MGet(ctx, "a", "b", "c"), + "zinter": c.ZInter(ctx, &ZStore{Keys: []string{"a", "b"}}), + "lcs": c.LCS(ctx, &LCSQuery{Key1: "a", Key2: "b"}), + } + for name, cmd := range cmds { + lo, hi, ok := cscKeySpan(cmd) + if !ok || !cscKeysRenderable(cmd, lo, hi) { + t.Fatalf("%s: span (%d, %d, %v) not renderable", name, lo, hi, ok) + } + if n := testing.AllocsPerRun(100, func() { + lo, hi, ok := cscKeySpan(cmd) + _ = ok && cscKeysRenderable(cmd, lo, hi) + }); n != 0 { + t.Errorf("%s: key gate allocates %v objects per call, want 0", name, n) + } + } +}