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

Cancellation ignored by asyncio.wait_for can hang application #87555

Closed
nmatravolgyi mannequin opened this issue Mar 3, 2021 · 4 comments
Closed

Cancellation ignored by asyncio.wait_for can hang application #87555

nmatravolgyi mannequin opened this issue Mar 3, 2021 · 4 comments
Labels
3.8 only security fixes 3.9 only security fixes topic-asyncio type-bug An unexpected behavior, bug, or error

Comments

@nmatravolgyi
Copy link
Mannequin

nmatravolgyi mannequin commented Mar 3, 2021

BPO 43389
Nosy @asvetlov, @1st1
Files
  • aio_wait_for_me.py: Demonstration of stuck task.
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2021-05-09.20:19:02.723>
    created_at = <Date 2021-03-03.16:50:58.584>
    labels = ['type-bug', '3.8', '3.9', 'expert-asyncio']
    title = 'Cancellation ignored by asyncio.wait_for can hang application'
    updated_at = <Date 2021-05-09.20:19:02.723>
    user = 'https://bugs.python.org/nmatravolgyi'

    bugs.python.org fields:

    activity = <Date 2021-05-09.20:19:02.723>
    actor = 'nmatravolgyi'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-05-09.20:19:02.723>
    closer = 'nmatravolgyi'
    components = ['asyncio']
    creation = <Date 2021-03-03.16:50:58.584>
    creator = 'nmatravolgyi'
    dependencies = []
    files = ['49849']
    hgrepos = []
    issue_num = 43389
    keywords = []
    message_count = 4.0
    messages = ['388032', '388059', '388061', '393335']
    nosy_count = 3.0
    nosy_names = ['asvetlov', 'yselivanov', 'nmatravolgyi']
    pr_nums = []
    priority = 'normal'
    resolution = 'duplicate'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue43389'
    versions = ['Python 3.8', 'Python 3.9']

    @nmatravolgyi
    Copy link
    Mannequin Author

    nmatravolgyi mannequin commented Mar 3, 2021

    I have found myself debugging a *very* not intuitive behavior regarding asyncio.wait_for that I'd consider a bug/deficiency. The problem very simply put: wait_for will return the wrapped future's result even when it is being cancelled, ignoring the cancellation as it has never existed.

    This will make parallel execution-waits hang forever if some simple conditions are met. From the perspective of this snippet every task must exit so it just needs to wait. I know cancellation *can* be ignored, but it is discouraged by the documentation for this reason exactly.

    tasks = [...]
    for t in tasks:
        t.cancel()
    results = await asyncio.gather(*tasks, return_exceptions=True)

    I already know that this behavior has been chosen because otherwise the returned value would be lost. But for many applications, losing an explicit cancellation error/event is just as bad.

    The reason why ignoring the cancellation is critical is because the cancelling (arbiter) task cannot reliably solve it. In most cases having repeated cancellations in a polling wait can solve this, but it is ugly and does not work if the original wait_for construct is in a loop and will always ignore the cancellation.

    The most sensible solution would be to allow the user to handle both the return value and the cancellation if they do happen at once. This can be done by subclassing the CancelledError as CancelledWithResultError and raising that instead. If the user code does not handle that exception specifically then the user "chose" to ignore the result. Even if this is not intuitive, it would give the user the control over what really is happening. Right now, the user cannot prefer to handle the cancellation or both.

    Lastly, I may have overlooked something trivial to make this work well. Right now I'm considering replacing all of the asyncio.wait_for constructs with asyncio.wait constructs. I can fully control all tasks and cancellations with that. I've made a simple demonstration of my problem, maybe someone can shed some light onto it.

    @nmatravolgyi nmatravolgyi mannequin added 3.8 only security fixes 3.9 only security fixes topic-asyncio type-bug An unexpected behavior, bug, or error labels Mar 3, 2021
    @nmatravolgyi
    Copy link
    Mannequin Author

    nmatravolgyi mannequin commented Mar 3, 2021

    I've quickly wanted to create a suitable solution for myself. I made a small library with a asyncio.wait_for()-like function using asyncio.wait(). The prototype worked, so I put together a small project. When I ran tox and realized that this issue with wait_for is only present on py38 and py39 (possibly py310). The wait_for does not get stuck with py36, py37 and pypy3.

    The repo is a little bare bones, but you can run tox after checkout: https://github.com/Traktormaster/wait-for2

    Right now the tests are set-up that they expect wait_for to get stuck so only py38 and py39 passes.

    I'm pretty sure the side-effect of returning the future's result when handling cancellation is not desired. However I'm not sure how to handle it correctly. The repo holds a demo of what I suggested in the beginning of this thread (CancelledWithResultError). It works but it is limited.

    @nmatravolgyi
    Copy link
    Mannequin Author

    nmatravolgyi mannequin commented Mar 3, 2021

    One more thing. I've figured out that I can fix the cancellation around the asyncio.wait_for() with asyncio.shield() like:

    try:
        await asyncio.shield(wf := asyncio.ensure_future(asyncio.wait_for(self.event.wait(), timeout=60.0)))
    except asyncio.CancelledError:
        wf.cancel()
        result = await asyncio.gather(wf, return_exceptions=True)
        # here I know there is a cancellation AND I might have a result as well!
        raise

    However I don't like the idea of writing all that boilerplate for every wait_for usage. I still might be overlooking something, but at least I have adequate workarounds.

    I'm curious what the consensus will be on this issue. I'm certain it should be documented though. Right now there is no mention of ignoring/eating a cancellation.

    @nmatravolgyi
    Copy link
    Mannequin Author

    nmatravolgyi mannequin commented May 9, 2021

    Closing as duplicate, because there was already an issue for this bug: https://bugs.python.org/issue42130

    @nmatravolgyi nmatravolgyi mannequin closed this as completed May 9, 2021
    @nmatravolgyi nmatravolgyi mannequin closed this as completed May 9, 2021
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes 3.9 only security fixes topic-asyncio type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    0 participants