added pubsub.run_in_thread as it is implemented in redis-py #214
Conversation
|
Sorry for the delay on this. I haven't used run_in_thread myself, so I need to get around to reading up on what it does. I'm concerned that this might not be thread-safe given that fakeredis in general is not thread-safe at all. |
There was a problem hiding this comment.
This won't be thread safe because the worker thread calls get_message, which in turn manipulates the pubsub object.
I'm planning to do some work to generally improve thread safety, so I'm going to hold off on this PR until I've done that (unfortunately it may take a while to get the time for it). In the meantime I've left some comments with nitpicks.
test_fakeredis.py
Outdated
| q.put(msg) | ||
|
|
||
| pubsub = self.redis.pubsub() | ||
| pubsub.subscribe(**{"channel": lambda e: _queue_msg(e, q)}) |
There was a problem hiding this comment.
Instead of the lambda and _queue_msg, you could just pass q.put as the callback.
test_fakeredis.py
Outdated
| msg = b"Hello World" | ||
| self.redis.publish("channel", msg) | ||
|
|
||
| sleep(1) |
There was a problem hiding this comment.
Is the sleep necessary? q.get should block until the message arrives.
test_fakeredis.py
Outdated
|
|
||
| pubsub.psubscribe(**{"channel": None}) | ||
| with self.assertRaises(redis.exceptions.PubSubError): | ||
| pubsub_thread = pubsub.run_in_thread() |
There was a problem hiding this comment.
Not sure this test will be effective, since you'll still have the normal subscribe to the channel so it will be bailing out due to that rather than the psubscribe without handler.
There was a problem hiding this comment.
unsubscribed first.
test_fakeredis.py
Outdated
| with self.assertRaises(redis.exceptions.PubSubError): | ||
| pubsub_thread = pubsub.run_in_thread() | ||
|
|
||
| pubsub.psubscribe(**{"channel": None}) |
There was a problem hiding this comment.
Rather than constructing a dict then **ing it, just pass channel=None.
|
I've just noticed that redis-py says "It is not safe to pass PubSub or Pipeline objects between threads." So I'm no longer worried about thread safety, and once my other comments are addressed I'll be happy to merge this. It also looks like FakePubSubWorkerThread is identical to the redis PubSubWorkerThread (by eye - I didn't diff them). In that case, we could save some code by just importing PubSubWorkerThread. |
|
I agree passing the pubsub objects between threads is not safe, it should be up to the user to decide if what they are doing is safe to do, but in many cases subscriptions can be responded to in a separate thread without needing to modify the pubsub object. And yes it is a carbon copy other than the name, so importing from redis-py should be fine. I'll see if I can make some edits later this week. |
|
Thanks for the updates. It all looks good, but just needs a fix for a merge conflict with some thread-safety improvements I merged this morning. |
Closes #213