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

Introduce task groups to asyncio and change task cancellation semantics #90908

Closed
gvanrossum opened this issue Feb 14, 2022 · 16 comments
Closed
Assignees

Comments

@gvanrossum
Copy link
Member

BPO 46752
Nosy @gvanrossum, @njsmith, @asvetlov, @dhalbert, @1st1, @Tinche, @iritkatriel, @YvesDup
PRs
  • bpo-46752: Introduce task groups in asyncio #31270
  • bpo-46752: Slight improvements to TaskGroup API #31398
  • bpo-46752: Uniform TaskGroup.__repr__ #31409
  • bpo-46752: Reduce flakiness of taskgroups test 13 #31411
  • bpo-46771: Implement task cancel requests #31513
  • bpo-46752: Taskgroup tweaks #31559
  • 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 = 'https://github.com/gvanrossum'
    closed_at = None
    created_at = <Date 2022-02-14.20:44:29.619>
    labels = ['3.11', 'expert-asyncio']
    title = 'Introduce task groups to asyncio and change task cancellation semantics'
    updated_at = <Date 2022-02-25.15:33:21.072>
    user = 'https://github.com/gvanrossum'

    bugs.python.org fields:

    activity = <Date 2022-02-25.15:33:21.072>
    actor = 'yduprat'
    assignee = 'gvanrossum'
    closed = False
    closed_date = None
    closer = None
    components = ['asyncio']
    creation = <Date 2022-02-14.20:44:29.619>
    creator = 'gvanrossum'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 46752
    keywords = ['patch', 'needs review']
    message_count = 10.0
    messages = ['413260', '413305', '413306', '413335', '413344', '413346', '413348', '413353', '413467', '413577']
    nosy_count = 8.0
    nosy_names = ['gvanrossum', 'njs', 'asvetlov', 'dhalbert', 'yselivanov', 'tinchester', 'iritkatriel', 'yduprat']
    pr_nums = ['31270', '31398', '31409', '31411', '31513', '31559']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue46752'
    versions = ['Python 3.11']

    @gvanrossum
    Copy link
    Member Author

    After some conversations with Yury, and encouraged by the SC's approval of PEP-654, I am proposing to add a new class, asyncio.TaskGroup, which introduces structured concurrency similar to nurseries in Trio.

    I started with EdgeDb's TaskGroup implementation (https://github.com/edgedb/edgedb/blob/master/edb/common/taskgroup.py) and tweaked it only slightly. I also changed a few things in asyncio.Task (see below).

    The key change I made to EdgeDb's TaskGroup is that subtasks can keep spawning more subtasks while __aexit__ is running; __aexit__ exits once the last subtask is done. I made this change after consulting some Trio folks, who knew of real-world use cases for this behavior, and did not know of real-world code in need of prohibiting task creation as soon as __aexit__ starts running. I added some tests for the new behavior; none of the existing tests needed to be adjusted to accommodate this change.

    (For other changes relative to the EdgeDb's TaskGroup, see #75453.)

    In order to avoid the need to monkey-patch the parent task, I added two new methods to asyncio.Task, .cancelled() and .uncancel(), that manage a flag corresponding to __cancel_requested__ in EdgeDb's TaskGroup.

    **This introduces a change in behavior around task cancellation:**

    • A task that catches CancelledError is allowed to run undisturbed (ignoring further .cancel() calls and allowing any number of await calls!) until it either exits or calls .uncancel().

    This change in semantics did not cause any asyncio unittests to fail. However, it may be surprising (especially to Trio folks, where the semantics are pretty much the opposite, once a Trio task is cancelled all further await calls in that task fail unless explicitly shielded).

    For the TaskGroup tests to pass, we require a flag that is not cleared. However, it is probably not really required to ignore subsequent .cancel() calls until .uncancel() is called. This just seemed more consistent, and it is what @asvetlov proposed above and implemented in #75494 (using a property .__cancel_requested__ as the API).

    @gvanrossum gvanrossum added the 3.11 only security fixes label Feb 14, 2022
    @gvanrossum gvanrossum self-assigned this Feb 14, 2022
    @gvanrossum gvanrossum added topic-asyncio 3.11 only security fixes labels Feb 14, 2022
    @gvanrossum gvanrossum self-assigned this Feb 14, 2022
    @gvanrossum
    Copy link
    Member Author

    New changeset 602630a by Guido van Rossum in branch 'main':
    bpo-46752: Add TaskGroup; add Task..cancelled(),.uncancel() (GH-31270)
    602630a

    @gvanrossum
    Copy link
    Member Author

    Remaining TODO list:

    • Add a test showing the need for the .uncancel() call in __aexit__()
      (currently on line 97). Dropping that line does not cause any tests
      to fail.
    • Ensure the taskgroup tests are run with the C and Python Task
      implementations.
    • Rename tests to have meaningful names.
    • I have a few ideas for minor cleanups that I will do later.
    • Documentation and What's New entry (in a separate PR, probably).
    • Update the docs in a few places to de-prioritize asyncio.gather()
      and steer people towards TaskGroups.

    (We could also add something like Trio's cancel scopes, e.g. based on
    Andrew Svetlov's async-timeout, which has a mature API.
    But that should be a separate bpo issue.)

    @dhalbert
    Copy link
    Mannequin

    dhalbert mannequin commented Feb 16, 2022

    For your TODO list (not sure how else to communicate this):

    I agree with the de-emphasis of gather(). I think adding another version of gather() that cancels all the remaining tasks if one fails would also be good, unless you think it is completely redundant due to TaskGroups. This idea was originally mentioned in https://bugs.python.org/issue31452 as a bug, and determined to be "works as designed". So now making an all-cancel() version of gather() is an idiom that people keep recoding, e.g. https://stackoverflow.com/questions/59073556/how-to-cancel-all-remaining-tasks-in-gather-if-one-fails.

    @gvanrossum
    Copy link
    Member Author

    @dhalbert, it's probably better to file a new issue if you want changes to gather(). Although I suppose that if we want to deemphasize it, we shouldn't be adding new features to it. My own new feature idea would be to have it wait for all tasks and then if there are any exceptions, raise an ExceptionGroup. That (like any new gather() behaviors) would require a new keyword-only flag to gather(). If we're going to deemphasize it I might not bother though.

    There's one thing that gather() does that TaskGroup doesn't: it gives us the return values from the tasks. The question is whether that's useful. If it is maybe we should *not* deepmhasize gather() quite as much and then adding new features would be okay.

    @gvanrossum
    Copy link
    Member Author

    I've created a separate issue for cancel scopes: bpo-46771.

    @1st1
    Copy link
    Member

    1st1 commented Feb 16, 2022

    There's one thing that gather() does that TaskGroup doesn't: it gives us the return values from the tasks.

    That's easy to do with task groups too:

      async with TaskGroup() as g:
          r1 = g.create_task(coro1())
          r2 = g.create_task(coro2())
    
      print(r1.result())
    
      # or
      print(await r2)  # I *think* this should work

    @gvanrossum
    Copy link
    Member Author

    I have a PR up to typeshed to add the new Task methods and a new stub file taskgroups.pyi: python/typeshed#7240

    @gvanrossum
    Copy link
    Member Author

    New changeset d851216 by Guido van Rossum in branch 'main':
    bpo-46752: Slight improvements to TaskGroup API (GH-31398)
    d851216

    @asvetlov
    Copy link
    Contributor

    New changeset e7130c2 by Andrew Svetlov in branch 'main':
    bpo-46752: Uniform TaskGroup.__repr__ (GH-31409)
    e7130c2

    @warsaw
    Copy link
    Member

    warsaw commented Apr 27, 2022

    Documentation and What's New entry (in a separate PR, probably).

    Is there a plan or PR yet for documenting TaskGroup before 3.11b1? I can find it and there is no documentation in main AFAICT.

    @gvanrossum
    Copy link
    Member Author

    No, that’s why this issue is still open.

    @kumaraditya303
    Copy link
    Contributor

    Adding deferred blocker as it would be nice to have docs before the rc.

    @gvanrossum
    Copy link
    Member Author

    I'll try to write docs for taskgroups.

    @gvanrossum
    Copy link
    Member Author

    We should also document the new Task.uncancel() method, and the subtly new cancellation semantics in general. I'd like to do that in a separate PR: unlike TaskGroup, few people will care, and those that do can read the code for now.

    ambv pushed a commit that referenced this issue Jun 30, 2022
    miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jun 30, 2022
    Co-authored-by: CAM Gerlach <[email protected]>
    (cherry picked from commit b6ec6d4)
    
    Co-authored-by: Guido van Rossum <[email protected]>
    ambv pushed a commit that referenced this issue Jun 30, 2022
    Co-authored-by: CAM Gerlach <[email protected]>
    (cherry picked from commit b6ec6d4)
    
    Co-authored-by: Guido van Rossum <[email protected]>
    @gvanrossum
    Copy link
    Member Author

    Now the docs are up we can close this.

    @gvanrossum gvanrossum moved this to Done in asyncio Jun 30, 2022
    gvanrossum added a commit to gvanrossum/cpython that referenced this issue Jun 30, 2022
    ambv added a commit to ambv/cpython that referenced this issue Jul 25, 2022
    gvanrossum pushed a commit that referenced this issue Oct 1, 2022
    miss-islington pushed a commit to miss-islington/cpython that referenced this issue Oct 1, 2022
    …ncancel() (pythonGH-95253)
    
    Co-authored-by: Thomas Grainger <[email protected]>
    (cherry picked from commit f00645d)
    
    Co-authored-by: Łukasz Langa <[email protected]>
    miss-islington added a commit that referenced this issue Oct 1, 2022
    …l() (GH-95253)
    
    Co-authored-by: Thomas Grainger <[email protected]>
    (cherry picked from commit f00645d)
    
    Co-authored-by: Łukasz Langa <[email protected]>
    serhiy-storchaka pushed a commit to serhiy-storchaka/cpython that referenced this issue Oct 2, 2022
    pablogsal pushed a commit that referenced this issue Oct 24, 2022
    …l() (GH-95253)
    
    Co-authored-by: Thomas Grainger <[email protected]>
    (cherry picked from commit f00645d)
    
    Co-authored-by: Łukasz Langa <[email protected]>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    Status: Done
    Development

    No branches or pull requests

    6 participants
    @warsaw @1st1 @asvetlov @gvanrossum @kumaraditya303 and others