Skip to content

asyncio.gather should not use special Future #77594

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

Closed
tecki mannequin opened this issue May 2, 2018 · 4 comments
Closed

asyncio.gather should not use special Future #77594

tecki mannequin opened this issue May 2, 2018 · 4 comments
Labels
3.8 (EOL) end of life topic-asyncio type-bug An unexpected behavior, bug, or error

Comments

@tecki
Copy link
Mannequin

tecki mannequin commented May 2, 2018

BPO 33413
Nosy @asvetlov, @1st1, @tecki, @twisteroidambassador
PRs
  • bpo-33413: asyncio.gather without a special Future #6694
  • 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 = None
    created_at = <Date 2018-05-02.18:39:18.059>
    labels = ['type-bug', '3.8', 'expert-asyncio']
    title = 'asyncio.gather should not use special Future'
    updated_at = <Date 2018-05-10.18:11:39.229>
    user = 'https://github.com/tecki'

    bugs.python.org fields:

    activity = <Date 2018-05-10.18:11:39.229>
    actor = 'ned.deily'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['asyncio']
    creation = <Date 2018-05-02.18:39:18.059>
    creator = 'Martin.Teichmann'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 33413
    keywords = ['patch']
    message_count = 3.0
    messages = ['316085', '316281', '316371']
    nosy_count = 4.0
    nosy_names = ['asvetlov', 'yselivanov', 'Martin.Teichmann', 'twisteroid ambassador']
    pr_nums = ['6694']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue33413'
    versions = ['Python 3.8']

    @tecki
    Copy link
    Mannequin Author

    tecki mannequin commented May 2, 2018

    asyncio.gather() returns a _GatheringFuture, which inherits from asyncio.Future. This is weird in current asyncio, as futures are supposed to be created with loop.create_future(). So I tried to reimplement gather() without this weird special future. I succeeded, yet I stumbled over weird inconsistencies with cancellation. There are three cases:

    • coroutines have no special notion of cancellation, they treat CancelledError as any other exception

    • futures have a clear distinction between exceptions and cancellation: future.set_exception(CancelledError()) is different from future.cancel(), as only for the latter future.cancelled() is True. This is used in the _GatheringFuture: it is cancelled() only if it got cancelled via future.cancel(), if its children gets cancelled it may set_exception(CancelledError()), but it will not be cancelled itself.

    • Tasks consider raising a CancelledError always as a cancellation, whether it actually got cancelled or the wrapped coroutine raised CancelledError for whatever other reason. There is one exception: if the coroutine manages to return immediately after being cancelled, it raises a CancelledError, but task.cancelled() is false. So if a coroutine ends in

      current_task().cancel()
      return

    the current task raises a CancelledError, but task.cancelled() is false.

    I consider the last exception actually a bug, but it allows me to make my inheritance-free gather() look to the outside exactly like it used to be.

    @tecki tecki mannequin added 3.8 (EOL) end of life type-bug An unexpected behavior, bug, or error labels May 2, 2018
    @twisteroidambassador
    Copy link
    Mannequin

    twisteroidambassador mannequin commented May 8, 2018

    I would like to comment on the last observation about current_task().cancel(). I also ran into this corner case recently.

    When a task is cancelled from outside, by virtue of there being something outside doing the cancelling, the task being cancelled is not currently running, and that usually means the task is waiting at an await statement, in which case a CancelledError will be raised at this await statement the next time this task runs. The other possibility is that the task has been created but has not had a chance to run yet, and in this case the task is marked cancelled, and code inside the task will not run.

    When one cancels a task from the inside by calling cancel() on the task object, the task will still run as normal until it reaches the next await statement, where a CancelledError will be raised. If there is no await between calling cancel() and the task returning, however, the CancelledError is never raised inside the task, and the task will end up in the state of done() == True, cancelled() == False, exception() == CancelledError. Anyone awaiting for the task will get a CancelledError without a meaningful stack trace, like this:

    Traceback (most recent call last):
      File "cancel_self.py", line 89, in run_one
        loop.run_until_complete(coro)
      File "C:\Program Files\Python36\lib\asyncio\base_events.py", line 467, in run_until_complete
        return future.result()
    concurrent.futures._base.CancelledError

    This is the case described in the original comment.

    I would also consider this a bug or at least undesired behavior. Since CancelledError is never raised inside the task, code in the coroutine cannot catch it, and after the task returns the return value is lost. For a coroutine that acquires and returns some resource (say asyncio.open_connection()), this means that neither the task itself nor the code awaiting the task can release the resource, leading to leakage.

    I guess one should be careful not to cancel the current task from the inside.

    @tecki
    Copy link
    Mannequin Author

    tecki mannequin commented May 10, 2018

    I looked a bit into the details, and found that bpo-30048 created the described weird behavior. There they fixed the problem that a cancel is ignored if a coroutine manages to cancel its own task and return immediately. As shown in the discussion there, this is actually something happening in real code, and is a valid use case.

    They fixed that by setting a CancelledError as an exception raised by the task, but did not cancel that task (they could have, I tested it, it would pass all tests).

    But this is just a side show of the fact that we have now four different beasts that can be awaited, and behave differently: coroutines, Futures, Tasks, and _GatheringFutures. I think we should consolidate that.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @ezio-melotti ezio-melotti moved this to Todo in asyncio Jul 17, 2022
    @kumaraditya303
    Copy link
    Contributor

    I really don't want to touch gather code at this point, it's a outdated API in favor of newer TaskGroup. The code is very complicated and not worth risking breaking for this.

    @kumaraditya303 kumaraditya303 closed this as not planned Won't fix, can't repro, duplicate, stale May 15, 2023
    @github-project-automation github-project-automation bot moved this from Todo to Done in asyncio May 15, 2023
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 (EOL) end of life topic-asyncio type-bug An unexpected behavior, bug, or error
    Projects
    Status: Done
    Development

    No branches or pull requests

    2 participants