Potential data race condition when setting tags for in-memory stores?

[mikestefanello]

When using in-memory stores such as go-cache, it seems possible that a data race condition exists when setting cache tags. The code in question can be found here: https://github.com/eko/gocache/blob/master/store/go_cache/go_cache.go#L82-L110

As the tags are iterated, the list of keys for each tag is fetched without any locking:

        if result, err := s.Get(ctx, tagKey); err == nil {
            if bytes, ok := result.(map[string]struct{}); ok {
                cacheKeys = bytes
            }
        }

A write lock is then acquired, and the new key is added to the list of keys for the given tag:

        s.mu.Lock()
        cacheKeys[key.(string)] = struct{}{}
        s.mu.Unlock()

        s.client.Set(tagKey, cacheKeys, 720*time.Hour)

If this is being called concurrently, the final list of tags can be incorrect.

For example, let's say for the tag tag1, it already contains keyA and keyB. Two concurrent requests come in: Request 1 sets keyC with tag1, and request 2 sets keyD with tag1.

Request 1 will fetch the existing list of keyA and keyB.

Request 2 will fetch the existing list of keyA and keyB.

Request 1 acquires the lock and writes a final list of keyA, keyB, keyC.

Request 2 then acquires the lock and writes a final list of keyA, keyB, keyD. (and keyC is now missing)

This is not a problem for stores like Redis, since you're using sets and the read-modify-write pattern is done atomically. It looks like you need to acquire the write lock before you read.