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.AsyncExitStack #73488

Closed
thehesiod mannequin opened this issue Jan 17, 2017 · 23 comments
Closed

add contextlib.AsyncExitStack #73488

thehesiod mannequin opened this issue Jan 17, 2017 · 23 comments
Labels
3.7 stdlib type-feature

Comments

@thehesiod
Copy link
Mannequin

@thehesiod thehesiod mannequin commented Jan 17, 2017

BPO 29302
Nosy @ncoghlan, @giampaolo, @benjaminp, @njsmith, @1st1, @thehesiod, @vedgar, @Kentzo, @csabella, @privatwolke, @miss-islington
PRs
  • #4790
  • #17130
  • #17138
  • 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 2018-01-25.20:52:36.005>
    created_at = <Date 2017-01-17.22:49:42.279>
    labels = ['3.7', 'type-feature', 'library']
    title = 'add contextlib.AsyncExitStack'
    updated_at = <Date 2019-11-13.02:40:34.018>
    user = 'https://github.com/thehesiod'

    bugs.python.org fields:

    activity = <Date 2019-11-13.02:40:34.018>
    actor = 'miss-islington'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-01-25.20:52:36.005>
    closer = 'yselivanov'
    components = ['Library (Lib)']
    creation = <Date 2017-01-17.22:49:42.279>
    creator = 'thehesiod'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 29302
    keywords = ['patch']
    message_count = 23.0
    messages = ['285687', '286380', '286411', '288735', '288744', '288761', '288762', '290674', '293138', '300572', '300586', '300588', '300589', '300595', '301004', '307970', '308014', '308046', '310353', '310705', '310706', '356510', '356512']
    nosy_count = 11.0
    nosy_names = ['ncoghlan', 'giampaolo.rodola', 'benjamin.peterson', 'njs', 'yselivanov', 'thehesiod', 'veky', 'Ilya.Kulakov', 'cheryl.sabella', 'privatwolke', 'miss-islington']
    pr_nums = ['4790', '17130', '17138']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue29302'
    versions = ['Python 3.7']

    @thehesiod
    Copy link
    Mannequin Author

    @thehesiod thehesiod mannequin commented Jan 17, 2017

    ExitStack is a really useful class and would be a great to have an async version. I've gone ahead and created an implementation based on the existing Python 3.5.2 implementation. Let me know what you guys think. I think it would be possible to combine most of the two classes together if you guys think it would be useful. Let me know if I can/should create a github PR and where to do that.

    @thehesiod thehesiod mannequin added 3.7 stdlib type-feature labels Jan 17, 2017
    @thehesiod
    Copy link
    Mannequin Author

    @thehesiod thehesiod mannequin commented Jan 27, 2017

    created gist: https://gist.github.com/thehesiod/b8442ed50e27a23524435a22f10c04a0

    I've now updated the imple to support both __aenter__/aexit and __enter__/exit so I don't need two ExitStacks

    @vedgar
    Copy link
    Mannequin

    @vedgar vedgar mannequin commented Jan 28, 2017

    An example from "real life": I recently needed this when implementing a demo of dining philosophers using asyncio (when the order of locks should depend on comparison of fork ids, not be static). Of course, it wasn't hard to implement it directly, but still, it would be nice if I had it in stdlib.

    Also, yes, it would probably be nicer if there was only one ExitStack with push_async and similar methods.

    @ncoghlan
    Copy link
    Contributor

    @ncoghlan ncoghlan commented Mar 1, 2017

    I quite like the idea of enhancing the existing ExitStack to also handle asynchronous operations rather than having completely independent implementations. However, a separate object may end up being simpler in practice as it allows developers to more explicitly declare their intent of writing async-friendly code.

    Sketching out what a combined implementation based on the current ExitStack might look like:

    • add suitable __aenter__ and __aexit__ implementations
    • add enter_context_async (async def, called with await)
    • add push_aexit (normal def, but given function is called with await)
    • add coroutine_callback (normal def, but given coroutine is called with await)
    • add close_async (async def, called with await)
    • add a new internal state flag _allow_async. This would default to True, but be set to False when the synchronous __enter__ is called. When it's false, attempting to register an async callback would raise RuntimeError. __enter__ would also need to check any previously register contexts and callbacks, and complain if any of them require the use of Await
    • keep track of which callbacks should be invoked as synchronous calls and which should be called via await
    • update close to raise RuntimeError if it's asked to clean up asynchronous resources

    That's really quite messy and hard to explain, and makes async code look quite different from the corresponding synchronous code.

    The repeated checks of self._allow_async and iscoroutinefunction would also slow down existing synchronous code for no specific end user benefit.

    By contrast, a dedicated AsyncExitStack object can:

    • assume everything passed to it is a coroutine or an async context manager by default
    • always require close() to be called via await
    • either add synchronous variants of the default-to-async methods (enter_context_sync, push_sync, callback_sync), or else make them auto-adapt to handle both synchronous and asynchronous inputs
    • rather than using iscoroutinefunction, instead always invoke callbacks with await, and wrap synchronous callbacks supplied using the above methods in simple one-shot iterators

    @thehesiod
    Copy link
    Mannequin Author

    @thehesiod thehesiod mannequin commented Mar 1, 2017

    Thanks for the feedback Nick! If I get a chance I'll see about refactoring my gist into a base class and two sub-classes with the async supporting non-async but not vice-versa. I think it will be cleaner. Sorry I didn't spend too much effort on the existing gist as I tried quickly layering on async support to move on to my primary task. After doing that I noticed that the code could use some refactoring, but only after taking the time deconstructing the impl to understand how the code works.

    @1st1
    Copy link
    Member

    @1st1 1st1 commented Mar 1, 2017

    Alexander,

    You don't need asyncio.isocoroutinefunction. Please use inspect.iscoroutinefunction and inspect.iscoroutine.

    Nick,

    I'm +1 to have a separate class AsyncExitStack.

    • assume everything passed to it is a coroutine or an async context manager by default

    I would add a separate set of methods prefixed with 'a' to handle async context managers.

    • always require close() to be called via await

    I'd only have one coroutine 'aclose()' that would internally close sync and async context managers in the right order.

    • either add synchronous variants of the default-to-async methods (enter_context_sync, push_sync, callback_sync), or else make them auto-adapt to handle both synchronous and asynchronous inputs

    I'd use the 'a' prefix. We use it already to name magic methods: __aenter__, __aexit__, __aiter__.

    • rather than using iscoroutinefunction, instead always invoke callbacks with await, and wrap synchronous callbacks supplied using the above methods in simple one-shot iterators

    I'd favour a more explicit approach: separate methods for handling sync and async.

    Also, speaking about asyncio dependancy -- we shouldn't have it. Using asyncio.iscoroutinefunction is unnecessary, inspect.iscoroutinefunction should be used instead.

    @1st1
    Copy link
    Member

    @1st1 1st1 commented Mar 1, 2017

    Looking at the code: we don't need to use iscoroutinefunction at all. If we had separate methods for sync and async context managers, you can store the flag if a function is async or sync along with it.

    So _exit_callbacks should store tuples (is_async, cb), instead of just cb.

    asyncio.iscoroutinefunction differs from inspect.iscoroutinefunction a little bit to support asyncio-specific debug coroutine wrapper functions. We should avoid using any version of iscoroutinefunction here.

    @thehesiod
    Copy link
    Mannequin Author

    @thehesiod thehesiod mannequin commented Mar 28, 2017

    ok I've updated the gist with a base class and sync + async sub-classes. The way it worked out I think is nice because we can have the same method names across both sync+async. Let me know what you guys think! btw, it seems the test_dont_reraise_RuntimeError test hangs even with the release version.

    @1st1
    Copy link
    Member

    @1st1 1st1 commented May 5, 2017

    Overall, the approach looks fine. Alexander, do you want to make a PR to start working on adding this to 3.7?

    @Kentzo
    Copy link
    Mannequin

    @Kentzo Kentzo mannequin commented Aug 19, 2017

    I'm not sure about type() to get a class object and calling __aenter__, __aexit__ through it: that makes it hard to mock these classes as Mock's spec= relies on __class__ and type() seem to ignore it (learned it a hard way.

    Yury, I could take a second look and try to change this into a patch if that's OK.

    @1st1
    Copy link
    Member

    @1st1 1st1 commented Aug 19, 2017

    I'm not sure about type() to get a class object and calling __aenter__, __aexit__ through it: that makes it hard to mock these classes as Mock's spec= relies on __class__ and type() seem to ignore it (learned it a hard way.

    Looking up __dunder__ methods on the class is how it should be done as that's how the rest of Python works. And that's how ExitStack is currently implemented for synchronous "with" blocks. We won't be able to change this behaviour even if we wanted to, so it stays.

    Here's an example:

        class Foo:
            def __init__(self):
                self.__aenter__ = ...
                self.__aexit__ = ...

    If we implement AsyncExitStack to lookup __anter__ directly on the object, then the above Foo class would be supported by it, but at the same time rejected by the 'async with' statement.

    Yury, I could take a second look and try to change this into a patch if that's OK.

    By all means you can submit a PR!

    @Kentzo
    Copy link
    Mannequin

    @Kentzo Kentzo mannequin commented Aug 19, 2017

    but at the same time rejected by the 'async with' statement.

    Perhaps unittest.mock (or type) needs to be adjusted to allow mocking via spec= without subclassing?

    By all means you can submit a PR!

    I'll take a look then.

    @1st1
    Copy link
    Member

    @1st1 1st1 commented Aug 19, 2017

    > but at the same time rejected by the 'async with' statement.

    Perhaps unittest.mock (or type) needs to be adjusted to allow mocking via spec= without subclassing?

    Maybe. You should try to find discussions around this topic on python mailing lists and this tracker. If you find nothing then feel free to open an issue and add Michael Foord to nosy list.

    @ncoghlan
    Copy link
    Contributor

    @ncoghlan ncoghlan commented Aug 20, 2017

    While it *may* be possible to do something simpler for test purposes where performance isn't a major concern, fully supporting type() level mocking basically requires bringing the equivalent of wrapt object proxies into the standard library: https://wrapt.readthedocs.io/en/latest/wrappers.html#object-proxy

    I actually think we *should* do that, and occasionally bug Graham Dumpleton about it, but while he's not opposed to the idea, it's also something that would take quite a bit of work (since odd edge cases that are tolerable in an opt-in third party module would probably need to be addressed for a standard library version).

    For test cases like AsyncExitStack though, we instead just use custom type definitions, rather than the unittest.mock module.

    Autospec'ed mocks are most attractive when we're dealing with object interfaces that are subject to a high rate of churn (since they make the tests more self-adjusting), and that isn't the case here: Python's syntactic support protocols rarely change, and when they do, preserving backwards compatibility with existing classes is typically a key requirement.

    @thehesiod
    Copy link
    Mannequin Author

    @thehesiod thehesiod mannequin commented Aug 30, 2017

    let me know if I need to do anything

    @csabella
    Copy link
    Contributor

    @csabella csabella commented Dec 10, 2017

    Alexander,

    Did you want to submit a PR for this?

    @Kentzo
    Copy link
    Mannequin

    @Kentzo Kentzo mannequin commented Dec 11, 2017

    Charyl, I made the PR.

    Where is the AbstractAsyncContextManager? I see that typing.py references it, but there is no actual implementation.

    @csabella
    Copy link
    Contributor

    @csabella csabella commented Dec 11, 2017

    Looks like AbstractAsyncContextManager is defined in issue bpo-30241.

    @ncoghlan
    Copy link
    Contributor

    @ncoghlan ncoghlan commented Jan 20, 2018

    Explicitly noting some API design decisions from the review of #4790:

    1. AsyncExitStack will define __aenter__ and __aexit__ but *not* __enter__ and __exit. This makes it unambiguous which form of with statement you need to be using for a given stack.
    2. AsyncExitStack will use the same methods to add synchronous context managers as ExitStack does
    3. The only common method with clearly distinct external behaviour will be stack.close(), which will be a coroutine in the AsyncExitStack case (the other common methods will all still be synchronous in AsyncExitStack)
    4. AsyncExitStack will gain 3 new methods for working with async context managers:
    • enter_async_context(cm): coroutine to enter an async context manager and adds its __aexit__ to the context stack if __aenter__ succeeds
    • push_async_exit(cm_or_exit): push an exit callback directly onto the stack. The argument must either be an object implementing __aexit__, or else a callback with the same signature as __aexit__ (use stack.push() to register a synchronous CM or exit callback)
    • push_async_callback(callback): push the given callback onto the stack. The callback should return an awaitable (use stack.callback() to register synchronous callbacks)

    @1st1
    Copy link
    Member

    @1st1 1st1 commented Jan 25, 2018

    New changeset 1aa094f by Yury Selivanov (Ilya Kulakov) in branch 'master':
    bpo-29302: Implement contextlib.AsyncExitStack. (bpo-4790)
    1aa094f

    @1st1
    Copy link
    Member

    @1st1 1st1 commented Jan 25, 2018

    Thank you Alexander and Ilya!

    @1st1 1st1 closed this as completed Jan 25, 2018
    @benjaminp
    Copy link
    Contributor

    @benjaminp benjaminp commented Nov 13, 2019

    New changeset d6d6e2a by Benjamin Peterson (Ilya Kulakov) in branch 'master':
    Add Ilya Kulakov to Misc/ACKS. (GH-17130)
    d6d6e2a

    @miss-islington
    Copy link
    Contributor

    @miss-islington miss-islington commented Nov 13, 2019

    New changeset e5125f7 by Miss Islington (bot) in branch '3.8':
    Add Ilya Kulakov to Misc/ACKS. (GH-17130)
    e5125f7

    @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

    5 participants