cunla / fakeredis-py

Implementation of Redis in python without having a Redis server running. Fully compatible with using redis-py.
https://fakeredis.moransoftware.ca/
BSD 3-Clause "New" or "Revised" License
298 stars 48 forks source link

Unique `FakeServer` breaks singleton connection in RQ tests #204

Closed RealOrangeOne closed 1 year ago

RealOrangeOne commented 1 year ago

Describe the bug

Since #142, released on 2.11.2, fake redis instances have separate state, based on a uuid. This causes issues when using as an RQ stub.

To Reproduce

queue_name = "default"
queue = get_queue(queue_name)
self.assertEqual(queue.count, 0)
queue.enqueue(_noop)
self.assertEqual(queue.count, 1)
get_worker(queue_name).work(burst=True)
self.assertEqual(queue.count, 0)

Expected behavior

The above test case to pass. Which it does on version 2.11.1.

Instead, it fails on the final line, claiming that the job still exists. The method itself is also never called, so the issue is the job not running.

Desktop (please complete the following information):

Additional context

I have a fix, but I'm not sure it's quite correct. Passing dummy values for host and port into the fake Redis instances results in a stable key, and thus the connection working correctly:

class FakeRedisConnSingleton:
    """Singleton FakeRedis connection."""

    def __init__(self):
        self.conn = None

    def __call__(self, _, strict):
        if not self.conn:
            self.conn = FakeStrictRedis(host="redis", port="1234") if strict else FakeRedis(host="redis", port="1234")
        return self.conn

As the class is designed to be a singleton, perhaps this is sensible. The arguments could be passed through from the singleton's constructor, but that again puts the onus of remembering the fix on the user, which isn't right.

Randomly generating the port number would also work, as at least then each "singleton" would be its own isolated server.

If that approach makes sense, I'm happy to submit a PR.

cunla commented 1 year ago

The question is how come it creates another self.conn? I think probably it creates connections through another path for workers and queues.

RealOrangeOne commented 1 year ago

From debugging, it doesn't create another self.conn, it's something else inside which causes the issues. Reverting the change to server_key from #142 does fix the issue, so I think it's something there, I've just not dove further in to work out why.

cunla commented 1 year ago

So it is getting a connection from another place. I'll look into it.

cunla commented 1 year ago

Which fakeredis version are you using? I am having a tough time replicating

RealOrangeOne commented 1 year ago

In 2.11.1 it works, broken in 2.11.2.

I'll try and get a minimum reproducible setup early in the week if you can't reproduce it.

cunla commented 1 year ago

Can you provide the full failing test? I am not using django-rq usually.

cunla commented 1 year ago

Based on my understanding, django_rq has logic in get_redis_connection which is the cause for this. Instead of overriding get_redis_connection, you use override redis.Redis so the logic in get_redis_connection will run.

Here is a full working test:

import fakeredis
import redis
from django.test import TestCase
from django_rq import get_worker, get_queue

redis.StrictRedis = fakeredis.FakeStrictRedis
redis.Redis = fakeredis.FakeRedis

def _noop():
    pass

class DjangoRqTest(TestCase):
    def test_django_rq(self):
        queue_name = "default"
        queue = get_queue(queue_name)
        self.assertEqual(queue.count, 0)
        queue.enqueue(_noop)
        self.assertEqual(queue.count, 1)
        get_worker(queue_name).work(burst=True)
        self.assertEqual(queue.count, 0)