`RedisChannelLayer.receive()` drops events due to `asyncio.shield()` in redis-py 4.5.4 about channels_redis HOT 10 CLOSED

@carltongibson as far as I can tell, #348 is resolved, because when I un-xfail the test_receive_cancel and test_message_expiry__group_send__one_channel_expires_message tests, and run pytest tests/test_core.py with Python 3.11.3 (and redis-py 4.5.4), then all tests pass.

However, in this issue, the test I included still fails with Python 3.11.3 and redis-py 4.5.4, which makes me think that this issue is different from #348. (I'm still not sure I understand #348 though.)

from channels_redis.

redis-py 4.5.5 was released today and both tests above pass when using that version of redis-py. 🎉

from channels_redis.

Hey @dcki. You could open a PR removing those xfails if you like... 🙂 -- it would be good it that has been resolved. (You'd also get the attention of the folks on that issue commenting there.)

I need to have a play to be able to say more, but please do keep digging.

from channels_redis.

Even more confirmation about asyncio.shield(), from contributor chayim who put it in:

Now getting back to this you're bang on the asyncio.shield change is absolutely a bandaid, in order to get things secure, and out. It's not ideal - but at the time, with what was known, I felt (perhaps incorrectly) that this was the right move forward.

redis/redis-py#2695 (comment)

That was a week ago, April 23rd, and that PR looks pretty active, so it seems like a fix for this issue here in channels_redis might be on the way.

One action that might be good to take now would be to just put in my test, above, with xfails. Assuming that test does use channels_redis in a way that is intended to be supported. I'd like to create a PR for that if that seems like a good idea.

from channels_redis.

Hi @dcki — thanks for this. A lot to look at there.

Take a look at #348, and the linked discussions there. It seems related to that I think (at initial read through)

from channels_redis.

Thanks @carltongibson. I think this issue is different, but I'm not sure I understand #348.

Reading that did make me wonder if Python 3.11.3 might help, since it sounds like it has an update related to channels_redis, but the test in the code snippet above still passes and fails in the same ways on Python 3.11.3 (fails unless a 5 second sleep is added or a particular asyncio.shield() is commented out, as described above).

from channels_redis.

Unrelated to your last message about xfails:

Looking at the last couple comments in this redis-py issue, apparently there is at least one person that claims to know what they're talking about, that says asyncio.shield() should be removed. So maybe this is all a redis-py issue. I'm not sure what channels_redis should do to handle that in the meantime...

from channels_redis.

@dcki It would certainly be worth a PR, so that it's easy to play with. 👍

from channels_redis.

That's great @dcki.

Were there any tests you still wanted to add to the test suite here?

from channels_redis.

#377 added explicit testing against multiple redis-py versions:

 redis46: redis>=4.6,<4.7
 redis50: redis>=5.0,<5.1
 redismain: https://github.com/redis/redis-py/archive/master.tar.gz

Any of those should work. Please open a new issue if you hit problems with one of those versions.

from channels_redis.