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

[Bug] - CancelledError swallowed (with solution) #2551

Closed
svaraborut opened this issue Jan 11, 2023 · 2 comments
Closed

[Bug] - CancelledError swallowed (with solution) #2551

svaraborut opened this issue Jan 11, 2023 · 2 comments
Labels

Comments

@svaraborut
Copy link

Version: 4.2.2

Platform: any

Description:

I had various issues related to task cancellation when using Redis due to asyncio.CancelledError disappearing sometimes. Probably related to #2028

The bug is difficult to reproduce as cancel() needs to be called on a task exactly when is waiting within an async with async_timeout.timeout(timeout): block.

I tracked down the issue to async_timeout used inside redis.asyncio.connection. This module has this known issue that is not being addressed.

I patched the library externally with this code

import async_timeout


class _Timeout(async_timeout.Timeout):

    RANDOM_TOKEN = '0f0dd596-373b-42df-aa0b-682d046c5d24'

    def __exit__(self, exc_type, exc_val, exc_tb):
        self._do_exit(exc_type, exc_val)
        return None

    async def __aexit__(self, exc_type, exc_val, exc_tb):
        self._do_exit(exc_type, exc_val)
        return None

    def _do_exit(self, exc_type, exc_val):
        if exc_type is asyncio.CancelledError and str(exc_val) == _Timeout.RANDOM_TOKEN \
                and self._state == async_timeout._State.TIMEOUT:
            self._timeout_handler = None
            raise asyncio.TimeoutError
        # timeout has not expired
        self._state = async_timeout._State.EXIT
        self._reject()

    def _on_timeout(self, task: "asyncio.Task[None]") -> None:
        task.cancel(_Timeout.RANDOM_TOKEN)
        self._state = async_timeout._State.TIMEOUT
        # drop the reference early
        self._timeout_handler = None


async_timeout.Timeout = _Timeout

but connection.py should be rewritten using asyncio.timeout as this library may lead to strange behavior as mentioned here.

sileht added a commit to sileht/redis-py that referenced this issue Mar 2, 2023
async_timeout does not support python 3.11
aio-libs/async-timeout#295

And have two years old annoying bugs:
aio-libs/async-timeout#229
redis#2551

Since asyncio.timeout has been shipped in python 3.11, we should start
using it.

Partially fixes 2551
sileht added a commit to sileht/redis-py that referenced this issue Mar 2, 2023
async_timeout does not support python 3.11
aio-libs/async-timeout#295

And have two years old annoying bugs:
aio-libs/async-timeout#229
redis#2551

Since asyncio.timeout has been shipped in python 3.11, we should start
using it.

Partially fixes 2551
sileht added a commit to sileht/redis-py that referenced this issue Mar 2, 2023
async_timeout does not support python 3.11
aio-libs/async-timeout#295

And have two years old annoying bugs:
aio-libs/async-timeout#229
redis#2551

Since asyncio.timeout has been shipped in python 3.11, we should start
using it.

Partially fixes 2551
dvora-h pushed a commit that referenced this issue Mar 16, 2023
async_timeout does not support python 3.11
aio-libs/async-timeout#295

And have two years old annoying bugs:
aio-libs/async-timeout#229
#2551

Since asyncio.timeout has been shipped in python 3.11, we should start
using it.

Partially fixes 2551
@dvora-h
Copy link
Collaborator

dvora-h commented Jul 16, 2023

Closing as I assume this issue was fixed with all the async and CancelledError fixes.

@svaraborut Feel free to re-open it if this still happen to you.

@dvora-h dvora-h closed this as completed Jul 16, 2023
@svaraborut
Copy link
Author

svaraborut commented Nov 9, 2023

The fix does only work on py >= 3.11. Will the underlying issue ever be addressed? Currently it is even worse as the patch does not work with async_timeout=4.0.3 but only with async_timeout=4.0.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants