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 phraseI 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 thatAsyncExitStack must support both sync and async context managers, otherwise it would be impossible to express code like below:

withFoo:asyncwithBar:withHam:            ...

2. I'd it like to haveenter_context methods for sync context managers, andenter_async_context for async context managers. This is explicit and verbose, yes, but it handles the weird case when an object implementsboth sync- and async- context managers. Python allows to do that, so we should handle this case instead of guessing.

3. +1 once for removinginspect.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__, usingisawaitable 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 andpush_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 justpush andcallback. I think those are slightly problematic, since they're bothpush operations (making the first one ambiguous), and the latter isn't a verb (which is dubious for a method name). Sincepush_async andcallback_async aren't clear on whether the operation itself is asynchronous, I see value in switching to the more explicit method names before adding theasync 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 methodaclose, for consistency with async generators. (This will also makeAsyncExitStack atrio.abc.AsyncResource so long as all the items in it are themselvesAsyncResources. But that's just becauseAsyncResource was designed to be consistent with async generators :-).)

Regarding the bikeshed, I think my preference FWIW is to makeboth sync and async explicitly marked, so it's e.g.enter_sync_context andenter_async_context, and there is noenter_context. This adds a small amount of hassle in the case where you have an existingExitStack 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 anAsyncExitStack and then are confused thatAsyncExitStack.enter_context wants a synchronous context.

@njsmith following this way we should adda 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 writtenstack = contextlib.AsyncExitStack(), youalso have to writeasync with stack: - you can't just writewith 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 definitelynot keen on having the correctness of code likef = 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 thatstack.push(async_cm_or_exit) andstack.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) inAsyncContextManager.__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 addingpush_exit andpush_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 phraseI 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!