Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unique FakeServer breaks singleton connection in RQ tests #204

Closed
RealOrangeOne opened this issue Jun 27, 2023 · 7 comments
Closed

Unique FakeServer breaks singleton connection in RQ tests #204

RealOrangeOne opened this issue Jun 27, 2023 · 7 comments
Labels
bug Something isn't working

Comments

@RealOrangeOne
Copy link

RealOrangeOne commented Jun 27, 2023

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):

  • OS: Linux
  • python version: 3.11.4
  • redis-py version: 4.4.4
  • RQ version: 1.15.1
  • RQ version: 2.8.1

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.

@RealOrangeOne RealOrangeOne added the bug Something isn't working label Jun 27, 2023
@cunla
Copy link
Owner

cunla commented Jun 28, 2023

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

@RealOrangeOne
Copy link
Author

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
Copy link
Owner

cunla commented Jul 1, 2023

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

@cunla cunla moved this from To do to Ready in Open-source work Jul 1, 2023
Flix6x added a commit to FlexMeasures/flexmeasures that referenced this issue Jul 1, 2023
Signed-off-by: F.N. Claessen <[email protected]>
@cunla
Copy link
Owner

cunla commented Jul 2, 2023

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

@cunla cunla moved this from Ready to In progress in Open-source work Jul 2, 2023
@RealOrangeOne
Copy link
Author

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
Copy link
Owner

cunla commented Jul 2, 2023

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

@cunla
Copy link
Owner

cunla commented Jul 2, 2023

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)

@cunla cunla closed this as completed in 997fbe7 Jul 2, 2023
@github-project-automation github-project-automation bot moved this from In progress to Done in Open-source work Jul 2, 2023
nhoening added a commit to FlexMeasures/flexmeasures that referenced this issue Jul 7, 2023
* first attempt, use Flask-Classful branch. New blocker is Flask-Login and forecasting/scheduling jobs are failing now

Signed-off-by: Nicolas Höning <[email protected]>

* fix: from cunla/fakeredis-py#204

Signed-off-by: F.N. Claessen <[email protected]>

* prevent sphinx warning

Signed-off-by: F.N. Claessen <[email protected]>

* black

Signed-off-by: F.N. Claessen <[email protected]>

* move to original rq-dashboard in .txt

Signed-off-by: Nicolas Höning <[email protected]>

* add comment

Signed-off-by: Nicolas Höning <[email protected]>

---------

Signed-off-by: Nicolas Höning <[email protected]>
Signed-off-by: F.N. Claessen <[email protected]>
Co-authored-by: F.N. Claessen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

No branches or pull requests

2 participants