GeezerVison

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?

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.

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:

1. 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:
            ...

2. 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.

3. +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:

1. close() becomes a coroutine instead of a synchronous method
2. it defines __aenter__ and __aexit__ instead of __enter__ and __exit__
3. 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.

@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.

👍

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.

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!