AsyncIO Race Condition Fix

[chayim]

closes #2624
closes #2579

[codecov-commenter]

Codecov Report

Patch coverage: 44.73% and project coverage change: -14.06 ⚠️

Comparison is base (318b114) 92.28% compared to head (900e2c0) 78.22%.

❗ Current head 900e2c0 differs from pull request most recent head 5330257. Consider uploading reports for the commit 5330257 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #2641       +/-   ##
===========================================
- Coverage   92.28%   78.22%   -14.06%     
===========================================
  Files         115      115               
  Lines       29751    29787       +36     
===========================================
- Hits        27455    23301     -4154     
- Misses       2296     6486     +4190     

Impacted Files	Coverage Δ
setup.py	0.00% <ø> (ø)
tests/test_asyncio/test_cluster.py	18.42% <7.69%> (-79.06%)	⬇️
redis/asyncio/cluster.py	16.99% <16.66%> (-74.69%)	⬇️
redis/asyncio/client.py	92.80% <50.00%> (-0.47%)	⬇️
tests/test_asyncio/test_connection.py	96.47% <86.66%> (-1.60%)	⬇️

... and 38 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[drago-balto]

@chayim I think this only fixed the pipeline, while the race condition still exists in the Redis class:

redis-py/redis/asyncio/client.py

Line 514 in 66a4d6b

return await conn.retry.call_with_retry(

I can still reproduce #2624 using the latest redis-py 4.5.3.

Keep in mind that you need slow enough Redis connection to reproduce this, otherwise the cancellation does not happen simply because the entire RESP exchange with Redis happens before there is a chance to cancel the task.

Also, are you sure that shielding from cancellation is the right approach? For example, this means that there would be no way to cancel a blocking BLPOP.

[so1n]

@chayim I think this only fixed the pipeline, while the race condition still exists in the Redis class:

redis-py/redis/asyncio/client.py

Line 514 in 66a4d6b

return await conn.retry.call_with_retry(

I can still reproduce #2624 using the latest redis-py 4.5.3.

Keep in mind that you need slow enough Redis connection to reproduce this, otherwise the cancellation does not happen simply because the entire RESP exchange with Redis happens before there is a chance to cancel the task.

Also, are you sure that shielding from cancellation is the right approach? For example, this means that there would be no way to cancel a blocking BLPOP.

I found the same problem. Just install the Redis service locally and run the following code and it will definitely reproduce:

import asyncio
from redis.asyncio import Redis


async def main():
    async with Redis(single_connection_client=True) as r:

        await r.set('foo', 'foo')
        await r.set('bar', 'bar')

        t = asyncio.create_task(r.get('foo'))
        await asyncio.sleep(0)  # <--- must 0 
        t.cancel()
        try:
            await t
            print('try again, we did not cancel the task in time')
        except asyncio.CancelledError as e:
            print('managed to cancel the task, connection is left open with unread response')

        print('bar:', await r.get('bar'))
        print('ping:', await r.ping())
        print('foo:', await r.get('foo'))


if __name__ == '__main__':
    asyncio.run(main())