dragonflydb / dragonfly

A modern replacement for Redis and Memcached
https://www.dragonflydb.io/
Other
25.85k stars 948 forks source link

`SADDEX` is broken #3915

Open zidokobik opened 1 month ago

zidokobik commented 1 month ago

Describe the bug SADDEX fails to expire members occasionally. Leaving them not having an expiration.

To Reproduce Here's a test run in python with the redis-py package:

from asyncio import sleep, Runner, gather
from random import random
from time import time_ns
from redis.asyncio import Redis

TIMEOUT = 10
KEY = 'concurrentRequest:user:1'

async def simulate_request(redis):
    member = time_ns()
    await redis.execute_command('SADDEX', KEY, TIMEOUT, member)
    await sleep(random())  # simulate handling the request

    if random() < 0.05: # 5% chance to fail, members should be expired automatically
        raise Exception
    await redis.srem(KEY, member)

async def main():
    # simulate 5000 requests
    async with Redis(decode_responses=True) as redis:
        requests = [simulate_request(redis) for _ in range(5000)]
        await gather(*requests, return_exceptions=True)

        # wait for the remaining failed requests to expire
        await sleep(TIMEOUT + 1) 

        # actively expire by reading the key
        await redis.smembers(KEY)

        # assert the set is empty after running the test
        cardinality = await redis.scard(KEY)
        print(cardinality)
        assert cardinality == 0

if __name__ == "__main__":
    with Runner() as runner:
        runner.run(main())

Expected behavior The test runs without error, i.e. the set should be empty.

Environment (please complete the following information):

Additional context I have a service in which I need to keep track of client's requests concurrency. I use a set to store the request starting-time as members, then it's SREM when the response is returned, therefore keeping the cardinality as the number of on-flight requests. I use SADDEX (add members with expiration to a set) instead of the normal SADD so that if any error occurs, the member will get removed automatically after a certain timeout. I'm aware that SADDEX is dragonfly-only and experimental, it's the one of the reason I'm using dragonfly over redis.

zidokobik commented 1 month ago

Previously closed thinking it was due to passive expiration, but the bug still persists.

dranikpg commented 4 weeks ago

Hi! Can you please try replacing

cardinality = await redis.scard(KEY)

with

members = await redis.smembers()
assert members == []

Our set expiration logic is lazy, i.e. it invalidates only when a specific member is accessed.

zidokobik commented 4 weeks ago

Yes, that's why the issue was closed before. I edited the snipped to add

# actively expire by reading the key
await redis.smembers(KEY)

but the problem still persists, hence I reopened the issue. Inspecting with redis-cli confirms there are still some values left.