bpo-29302: Implement contextlib.AsyncExitStack. by Kentzo · Pull Request #4790 · python/cpython · GitHub

This change is entirely based on the work of @thehesiod discussed at https://bugs.python.org/issue29302

I only changed implementation of __aexit__:

except BaseException as e:
    gen.throw(e)

to:

except:
    gen.throw(*sys.exc_info())

and added tests.

https://bugs.python.org/issue29302

thanks @Kentzo !

@1st1 I'd like to take a further look at this one before we merge it, but won't have time until Jan 6th or so (just before the final alpha). Does that timeline work for you?

The alternative plan would be to merge this version based on the existing reviews, and if I have any subsequent changes I want to make, I can do that any time before the first beta. That's probably a better approach, since it's more resilient against my being otherwise occupied in the first week of January.

@1st1 I'd like to take a further look at this one before we merge it, but
won't have time until Jan 6th or so (just before the final alpha). Does
that timeline work for you?

Sure, I think this is worth another look so let's wait. Let's just make sure we don't forget about this before the feature freeze.

I like the patch overall, but there's one design concern I have, as well as a backwards compatibility concern:

for the backwards compatibility issue: calls using keyword arguments mean we can't arbitrarily change parameter names in public APIs
for the design question, implicitly supporting synchronous context managers in the asynchronous exit stack seems questionable to me.

The design question is one @1st1 and I talked about on the issue tracker (see https://bugs.python.org/issue29302#msg288735 and the next couple of comments), and after seeing the code in PR form, I definitely want to go with the more explicit API:

add separate *_sync variants of all the callback registration APIs in the async exit stack variant rather than trying to guess the user's intent based on the type of the object they passed in (if guessing proves desirable, we can add it later, but if we start with it, we're likely stuck with it even if it proves confusing)
avoid checking for awaitables when unwinding the stack in the async case (either add an awaitable wrapper to the synchronous callbacks as I suggest in this review, or else store a 2-tuple as @1st1 suggested on the tracker)

Folks may be calling stack.push(exit=obj), so we shouldn't gratuitously change parameter names.

This introduces a dependency I'd prefer to avoid - inspect pulls in a lot of other modules, and contextlib is used frequently enough that we should aim to minimise its dependency tree.

This comment is oddly placed, and doesn't seem especially accurate - the check is being made against __aexit__, not __aenter__.

More generally, implicitly interleaving synchronous with asynchronous context managers seems dubious to me - while it might be a good idea to allow it, doing it automagically rather than explicitly seems like a recipe for confusion when reading and debugging code.

If we do decide to allow synchronous context managers in asynchronous exit stacks, I'd suggest wrapping their exit callbacks in an awaitable, so this check can be avoided.

inspect is also used by _create_exit_wrapper and _create_cb_wrapper.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Couple of thoughts:

I think that AsyncExitStack must support both sync and async context managers, otherwise it would be impossible to express code like below:

with Foo:
    async with Bar:
        with Ham:
            ...

I'd it like to have enter_context methods for sync context managers, and enter_async_context for async context managers. This is explicit and verbose, yes, but it handles the weird case when an object implements both sync- and async- context managers. Python allows to do that, so we should handle this case instead of guessing.
+1 once for removing inspect.isawaitable call. Instead, we should push tuples like (is_sync, callback) to _exit_callbacks. Otherwise if someone submits a callable that for whatever reason implements __await__, using isawaitable would result in a wrong guess and the callable would be awaited instead of being called.

Also, we need to get this merged before Jan 29. @Kentzo if you don't have time to work on this in the next few days I can handle this PR myself.

@1st1 So the differences between AsyncExitStack and ExitStack would be:

close() becomes a coroutine instead of a synchronous method
it defines __aenter__ and __aexit__ instead of __enter__ and __exit__
it has 3 new methods for callback registration (one coroutine, enter_async_context, and two synchronous methods, push_async_exit and push_async_callback)

I like that, since it means registering synchronous context managers and callbacks stays the same regardless of which kind of stack you're using.

A note regarding the method names: the synchronous method names are just push and callback. I think those are slightly problematic, since they're both push operations (making the first one ambiguous), and the latter isn't a verb (which is dubious for a method name). Since push_async and callback_async aren't clear on whether the operation itself is asynchronous, I see value in switching to the more explicit method names before adding the async qualifier: push_async_exit, push_async_callback.

@1st1 So the differences between AsyncExitStack and ExitStack would be: [..]

Right.

I like that, since it means registering synchronous context managers and callbacks stays the same regardless of which kind of stack you're using.

Yeah, the API consistency for synchronous CMs is great. To be honest I haven't thought about it until you highlighted it, but now I see it as a requirement!

Since push_async and callback_async aren't clear on whether the operation itself is asynchronous, I see value in switching to the more explicit method names before adding the async qualifier: push_async_exit, push_async_callback.

+1.

Please name the close method aclose, for consistency with async generators. (This will also make AsyncExitStack a trio.abc.AsyncResource so long as all the items in it are themselves AsyncResources. But that's just because AsyncResource was designed to be consistent with async generators :-).)

Regarding the bikeshed, I think my preference FWIW is to make both sync and async explicitly marked, so it's e.g. enter_sync_context and enter_async_context, and there is no enter_context. This adds a small amount of hassle in the case where you have an existing ExitStack and then realize you want to mix in some async contexts as well, because you have to update existing calls. But it makes things much less confusing when you start out thinking "hmm, I have some async contexts that I need to put in a stack", so you make an AsyncExitStack and then are confused that AsyncExitStack.enter_context wants a synchronous context.

@njsmith following this way we should add a prefix to all async functions everywhere: aclose(), asend(), arecv() etc.
The logic is weird, isn't it? I don't see how context manager is related to generator.

The semantics around closing things are much more general and subtle than sending/receiving. Obviously send/recv are inherently semantically blocking operations, but close() is non-blocking on a regular socket and blocking on an SSL socket. Plus in general there are just a lot more kinds of things that get closed so it's useful to have general cross-type conventions here. You can't use contextlib.closing with an object whose close is async (or rather, you can if it calls the method 'close', and it will claim to succeed without doing anything!). You *can* use async_generator.aclosing on any object with an async close, but it assumes that the method is called aclose(), because it's not specific to async generators but that is one of the original use cases. In some cases you might want to provide both a synchronous close() and an async aclose() on the same object, e.g. if the object's native close semantics are synchronous, but the way it's used also needs to match some generic ABC. (Probably it's *better* it you can handle this via composition – in trio, trio.socket.socket has a synchronous close() while trio.SocketStream and trio.SocketListener have async aclose() methods – but sometimes stuff happens.) So personally at least I find it very useful to have a conventional protocol for async close that doesn't overlap with the existing convention for sync close.

…

On Jan 20, 2018 04:42, "Andrew Svetlov" ***@***.***> wrote: @njsmith <https://github.com/njsmith> following this way we should add a prefix to all async functions everywhere: aclose(), asend(), arecv() etc. The logic is weird, isn't it? I don't see how context manager is related to generator. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4790 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAlOaDSFF_kyr6I6me86pr7c6e5W8fUjks5tMd8zgaJpZM4Q85Ru> .

@njsmith Good point regarding close() vs aclose() - I'd forgotten that part of PEP 525. Given that, I agree the method on AsyncExitStack should aclose(), and the synchronous close API should just be missing entirely (as with the other synchronous context management methods).

I'm not especially worried about folks having to sprinkle extra "async" mentions through their code to get async context managers to work as expected - that's the default language wide. For example, even though you've written stack = contextlib.AsyncExitStack(), you also have to write async with stack: - you can't just write with stack: the way you can with the synchronous version and have Python automatically figure out you must have wanted to use the async variant of the with statement.

By contrast, I'm definitely not keen on having the correctness of code like f = stack.enter_context(open(file_of_interest)) depend on whether or not "stack" is a regular exit stack or an asynchronous one.

This does mean that stack.push(async_cm_or_exit) and stack.callback(async_callback) may run into the "coroutine never awaited" scenario, but that's why we have the other issue about detecting unawaited coroutines. I'd also be amenable to having a check (potentially __debug__-only) in AsyncContextManager.__aexit__ itself for nominally synchronous callbacks returning awaitables, as that scenario is likely to inadvertently suppress exceptions as well (since a lot of awaitables are likely to be "True" by default).

I agree with everything Nick said here.

👍

This version looks good to me, but leaving to @1st1 to merge after his review :)

The Appveyor failures look unrelated to me - closing & reopening to check if they're transient errors.

I'll take a look today/tomorrow, thanks Nick!

@ncoghlan What do you think about adding push_exit and push_callback aliases for API symmetry?

@Kentzo Worth considering as a separate issue for 3.8, but I wouldn't rush into it for 3.7.

There're multiple formatting issues with the PR. Please fix the code to follow PEP 8. Reformat docstrings to have a short first line ended with a period, optionally followed by an empty line with more paragraphs.

Most importantly, I don't like the design of _shutdown_loop. I think we shouldn't use generator.send/throw here, I'd prefer to copy/paste the code which will make the code easier to read and maintain (generators are complicated!)

/cc @ncoghlan

Please trim the lines in your to be max 79 chars wide.

I'd rewrite to:

An :ref:`asynchronous context manager <async-context-managers>`, similar
to :class:`ExitStack`, that supports combining both synchronous and 
asynchronous context managers, as well as having coroutines for 
cleanup logic.

Same as -> Similar to

"or a coroutine function" -> "or a coroutine".

ditto

First line of the docstring should not be empty. Reformat to

"""A base class for ExitStack and AsyncExitStack."""

Please keep and empty line after the first line of the docstring. I also recommend you to read https://www.python.org/dev/peps/pep-0257/.

@1st1 In that PEP there is no empty line after a doctstring inside def. Should I add this extra empty line for all functions in my PR?

Ilya, I mean this:

def foo():
    """First line
    A paragraph
    """
    code

should be formatted as this

def foo():
    """First line.

    A paragraph.
    """
    code

The first line should be a complete sentence.

I'm not making these rules up -- there're tons of tools out there that expect this specific code style to extract/parse docstrings.

Ah I see.

I don't like using generator.send/throw for this. I'd better copy/paste the code. Generators have a pretty complicated semantics (especially around closing) and sometimes have issues with keeping exceptions' tracebacks in tact.

Put periods . at the end of all your sentences. Throughout the patch.

Tabulation is off and many lines are longer than 79. Please make sure that your PR follows PEP 8.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

I have made the requested changes; please review again

Thanks for making the requested changes!

@1st1, @ncoghlan: please review the changes made to this pull request.

Merged. Thanks so much Ilya!

the-knights-who-say-ni added the CLA signed label Dec 11, 2017

bedevere-bot added the awaiting review label Dec 11, 2017

Kentzo force-pushed the bpo-29302 branch from 6a0f2bc to 6cbc803 Compare December 11, 2017 06:24

asvetlov added the topic-asyncio label Dec 11, 2017

asvetlov requested a review from 1st1 December 11, 2017 07:16

Kentzo force-pushed the bpo-29302 branch from 6cbc803 to 17c503b Compare December 11, 2017 21:50

Kentzo changed the title bpo-29302: Implement contextlib.AsyncContextManager. bpo-29302: Implement contextlib.AsyncExitStack. Dec 11, 2017

Kentzo force-pushed the bpo-29302 branch from 17c503b to 48019b2 Compare December 11, 2017 22:53

Kentzo mentioned this pull request Dec 11, 2017

bpo-30241: implement contextlib.AbstractAsyncContextManager #1412

Merged

Kentzo force-pushed the bpo-29302 branch 2 times, most recently from 4374de3 to 60000cd Compare December 11, 2017 23:17

1st1 requested a review from ncoghlan December 11, 2017 23:35

ncoghlan requested changes Jan 20, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Jan 20, 2018

bedevere-bot added the awaiting change review label Jan 22, 2018

Kentzo force-pushed the bpo-29302 branch from cf6e8c0 to 5372693 Compare January 22, 2018 17:48

ncoghlan approved these changes Jan 23, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jan 23, 2018

ncoghlan closed this Jan 23, 2018

ncoghlan reopened this Jan 23, 2018

1st1 requested changes Jan 23, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Jan 23, 2018

bpo-29302: Implement contextlib.AsyncExitStack.

7f1d1cb

Kentzo force-pushed the bpo-29302 branch from 5372693 to 7f1d1cb Compare January 24, 2018 07:07

bedevere-bot removed the awaiting changes label Jan 24, 2018

bedevere-bot added the awaiting change review label Jan 24, 2018

1st1 approved these changes Jan 25, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jan 25, 2018

1st1 merged commit 1aa094f into python:master Jan 25, 2018

bedevere-bot removed the awaiting merge label Jan 25, 2018

Kentzo deleted the bpo-29302 branch January 25, 2018 21:45

Kentzo mentioned this pull request Feb 12, 2018

Add stub for AsyncExitContext added by bpo-29302. python/typeshed#1876

Merged

Kentzo mentioned this pull request Nov 12, 2019

Adding missing acknowledgement #17130

Merged

bpo-29302: Implement contextlib.AsyncExitStack. by Kentzo · Pull Request #4790 · python/cpython · GitHub

Repository navigation

bpo-29302: Implement contextlib.AsyncExitStack. - #4790

bpo-29302: Implement contextlib.AsyncExitStack.#47901st1 merged 1 commit intopython:masterpython/cpython:masterfrom GreatFruitOmsk:bpo-29302GreatFruitOmsk/cpython:bpo-29302Copy head branch name to clipboard

Conversation

Kentzo commented Dec 11, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thehesiod commented Dec 11, 2017

Uh oh!

ncoghlan commented Dec 14, 2017 via email

Uh oh!

ncoghlan commented Dec 14, 2017 via email

Uh oh!

1st1 commented Dec 14, 2017

Uh oh!

ncoghlan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jan 20, 2018

Uh oh!

1st1 commented Jan 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

1st1 commented Jan 20, 2018

Uh oh!

ncoghlan commented Jan 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

1st1 commented Jan 20, 2018

Uh oh!

njsmith commented Jan 20, 2018

Uh oh!

asvetlov commented Jan 20, 2018

Uh oh!

njsmith commented Jan 20, 2018 via email

Uh oh!

ncoghlan commented Jan 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

1st1 commented Jan 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

njsmith commented Jan 21, 2018

Uh oh!

ncoghlan left a comment

Choose a reason for hiding this comment

Uh oh!

ncoghlan commented Jan 23, 2018

Uh oh!

1st1 commented Jan 23, 2018

Uh oh!

Kentzo commented Jan 23, 2018

Uh oh!

ncoghlan commented Jan 23, 2018

Uh oh!

1st1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

bpo-29302: Implement contextlib.AsyncExitStack.#4790
1st1 merged 1 commit into
python:masterfrom
GreatFruitOmsk:bpo-29302

Kentzo commented Dec 11, 2017 •
edited by bedevere-bot
Loading

1st1 commented Jan 20, 2018 •
edited
Loading

ncoghlan commented Jan 20, 2018 •
edited
Loading

ncoghlan commented Jan 20, 2018 •
edited
Loading

1st1 commented Jan 20, 2018 •
edited
Loading

1st1 Jan 24, 2018 •
edited
Loading