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

Add @contextlib.asynccontextmanager #73865

Closed
JelleZijlstra opened this issue Feb 28, 2017 · 13 comments
Closed

Add @contextlib.asynccontextmanager #73865

JelleZijlstra opened this issue Feb 28, 2017 · 13 comments
Labels
3.7 stdlib type-feature

Comments

@JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented Feb 28, 2017

BPO 29679
Nosy @rhettinger, @ncoghlan, @giampaolo, @1st1, @JelleZijlstra
PRs
  • #360
  • 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 2017-06-12.10:40:58.865>
    created_at = <Date 2017-02-28.23:21:09.119>
    labels = ['3.7', 'type-feature', 'library']
    title = 'Add @contextlib.asynccontextmanager'
    updated_at = <Date 2017-06-12.10:40:58.864>
    user = 'https://github.com/JelleZijlstra'

    bugs.python.org fields:

    activity = <Date 2017-06-12.10:40:58.864>
    actor = 'yselivanov'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-06-12.10:40:58.865>
    closer = 'yselivanov'
    components = ['Library (Lib)']
    creation = <Date 2017-02-28.23:21:09.119>
    creator = 'JelleZijlstra'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 29679
    keywords = []
    message_count = 13.0
    messages = ['288729', '288732', '288733', '288734', '288736', '288738', '288759', '288777', '288781', '288850', '288851', '289335', '292647']
    nosy_count = 5.0
    nosy_names = ['rhettinger', 'ncoghlan', 'giampaolo.rodola', 'yselivanov', 'JelleZijlstra']
    pr_nums = ['360']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue29679'
    versions = ['Python 3.7']

    @JelleZijlstra
    Copy link
    Member Author

    @JelleZijlstra JelleZijlstra commented Feb 28, 2017

    An async equivalent of @contextmanager would be an obvious use case for async generators and the async with statement. In my own code, I have several async context objects that could have been written more concisely if @asynccontextmanager was available.

    I'll be working on a PR to implement this.

    @JelleZijlstra JelleZijlstra added 3.7 stdlib labels Feb 28, 2017
    @ncoghlan
    Copy link
    Contributor

    @ncoghlan ncoghlan commented Mar 1, 2017

    The general idea seems reasonable to me, and I've added some specific code level feedback on the PR.

    A related question to this one would be whether or not it may make sense to develop a more async-friendly variant of contextlib.ExitStack (I don't currently write enough async code to design that myself, but I'm open to the idea of adding something along those lines).

    @JelleZijlstra
    Copy link
    Member Author

    @JelleZijlstra JelleZijlstra commented Mar 1, 2017

    Thanks, I'll address your PR comments.

    bpo-29302 is asking for AsyncExitStack, but that can be an independent change. I haven't personally felt the need for AsyncExitStack.

    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented Mar 1, 2017

    Nick, do you think this belongs in contextlib to be side-by-side with @contextmanager or would it be better to group all async tools in their own module.

    It seems to me that people are going to ask for an async version of everything, effectively shadowing the core grammar and the library. Before we go very far down that road, I think we should decide where it all should go.

    @ncoghlan
    Copy link
    Contributor

    @ncoghlan ncoghlan commented Mar 1, 2017

    It's probably worth a python-dev discussion, but I personally draw the line at "Does this need to import 'asyncio'?". If it does, then that's a clear dependency inversion, and the functionality doesn't belong in a relatively low level module like contextlib.

    If it doesn't, then I think the potentially tricky part of this kind of code is the way it interacts with the execution stack and the context management machinery, so it makes sense for it to live in contextlib and have a design and review process that's closely aligned with that for the corresponding synchronous APIs.

    That said, one of my review comments on the PR was that the new test cases should be split out to their own file to avoiding making the existing tests depend on asyncio, which I'd consider a point in favour of adding an asyncio.contextlib module instead of adding these APIs directly to contextlib.

    @ncoghlan
    Copy link
    Contributor

    @ncoghlan ncoghlan commented Mar 1, 2017

    python-dev thread: https://mail.python.org/pipermail/python-dev/2017-March/147501.html

    In formulating my question for the list, it occurred to me that while asynccontextmanager doesn't need to depend on asyncio, the AsyncExitStack proposed in bpo-29302 likely will, which pushes me towards favouring the asyncio.contextlib.asynccontextmanager approach.

    @1st1
    Copy link
    Member

    @1st1 1st1 commented Mar 1, 2017

    In formulating my question for the list, it occurred to me that while asynccontextmanager doesn't need to depend on asyncio, the AsyncExitStack proposed in bpo-29302 likely will, which pushes me towards favouring the asyncio.contextlib.asynccontextmanager approach.

    As I said in [1], I think asynccontextmanager and AsyncExitStack should be framework agnostic and thus stay in the top level contextlib package.

    IMO AsyncExitStack should not be dependent on asyncio, and if it is it means we would need to tweak its design to make it not to. I'll take a look at the other issue to see if I can help.

    [1] https://mail.python.org/pipermail/python-dev/2017-March/147505.html

    @rhettinger
    Copy link
    Contributor

    @rhettinger rhettinger commented Mar 1, 2017

    At third alternative is to create an asynctools module to collect together tools that work with the async keyword but aren't dependent on asyncio.

    @ncoghlan
    Copy link
    Contributor

    @ncoghlan ncoghlan commented Mar 2, 2017

    In the specific case of contextlib, most of the APIs should be able to transparently support async/await without negatively impacting their synchronous behaviour, so after the python-dev discussion, I think one module with separate sync and async test suites is a good way to go for contextlib specifically: https://mail.python.org/pipermail/python-dev/2017-March/147520.html

    However, as noted in that message, I *don't* think we can conclude that's going to be the right answer for the standard library in general - for many modules, a separate a<module> or aio<module> published via PyPI and potentially considered for stdlib inclusion later is going to make more sense.

    @ncoghlan
    Copy link
    Contributor

    @ncoghlan ncoghlan commented Mar 3, 2017

    Something that occurred to me as being a bit tricky to handle here is the backport to contextlib2: that maintains compatibility with 2.6+, so it would need to split any code using "async def" and "await" out to a separate file that only gets imported on 3.5+ (and similarly only run the corresponding test cases on 3.5+).

    A potentially simpler alternative to that would be to create a new "backports.contextlib" package that only supports 3.5+ and explicitly restrict contextlib2 itself to code that runs in the common subset of Python 2 & 3.

    @1st1
    Copy link
    Member

    @1st1 1st1 commented Mar 3, 2017

    so it would need to split any code using "async def" and "await" out to a separate file that only gets imported on 3.5+ (and similarly only run the corresponding test cases on 3.5+).

    This seems doable. I'd only vote to keep contextlib.py as one file in CPython though.

    @ncoghlan
    Copy link
    Contributor

    @ncoghlan ncoghlan commented Mar 10, 2017

    For anyone interested in the question of backports, I moved that discussion over to the contextlib2 tracker: jazzband/contextlib2#12

    @1st1
    Copy link
    Member

    @1st1 1st1 commented May 1, 2017

    New changeset 2e62469 by Yury Selivanov (Jelle Zijlstra) in branch 'master':
    bpo-29679: Implement @contextlib.asynccontextmanager (#360)
    2e62469

    @1st1 1st1 closed this as completed Jun 12, 2017
    @1st1 1st1 added the type-feature label Jun 12, 2017
    @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.7 stdlib type-feature
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants