This code
stack = AsyncExitStack()
await stack.enter_async_context(open_connection())
(no async context, no error handling …) arguably is broken, yet doesn’t raise an exception or anything.
IMHO this should warn, or even raise a RuntimeError.
Why should it?
This is something to be used by people knowing how to use it - that said, it is perfectly legal to be used “bare”: either the `.aclose` or `_aexit_` methods can be explicitly called later.
While it may be hard to think of a use case for that in scripted/programmed code, it is a useful pattern in the REPL, for example: and just forbidding that would let one no work around should they want to type interactive expressions inside several contexts (as opposed to having to type all expressions within a `with` block and have they all run at once)
Not to mention that while mandating an active async context by “enabling” the “enter_context” just after `_aenter_` is called would be a simple change it would nonetheless be a backwards incompatible change at this point (the same for the sync “ExitStack”), the other scenario: detecting if errors are being handled, i.e. if it is called inside a `try` block, would require pretty hardcore introspection involving both Frame and possibly even runtime bytecode introspection - the gains would be minimal.
This sounds like something that static checkers (linters, etc…) could do - but I can’t see how changing the runtime to error on a perfectly valid statement would be of help.
That would still be a preserved. You would just call __aenter__first.
I don’t see how that usage would be stopped by just requiring “entering” it first.
Because of this a warning at first seems very reasonable (as was suggested) but with that I don’t see why it should be an issue.
It would be an appropriate lint rule sure but it’s not unusual to store the exit stacks on objects where the linter won’t have the context to detect if it’s opened or not (and it’s also those cases that most likely leads to the harder to debug cases)
One point to it being an actual used case is this example from the stdlib documentation.
stack = ExitStack()
try:
x = stack.enter_context(cm)
except Exception:
# handle __enter__ exception
else:
with stack:
# Handle normal case
It too can be solved with calling enter manually and then pop_all()in the exception block.
Edit: or probably better by something like
try:
x = cm.__enter__()
except Exception:
# handle __enter__ exception
else:
with ExitStack() as stack:
stack.push(cm)
# Handle normal case
This didn’t get any more traction. If no one weighs in I’ll probably open an issue and/or PR for it (if @smurfix doesn’t do it first…)
If open_connection() is asyncio.open_connection(), using python -m asyncio, I get:
>>> import contextlib
>>> s = contextlib.AsyncExitStack()
>>> await s.enter_async_context(asyncio.open_connection())
TypeError: 'builtins.coroutine' object does not support the asynchronous context manager protocol
So I do see an error. As such, unless you explain what open_connection() is meant to do and what it should return, I don’t think there is anything to change.
That’s just a mistake in that it’s an async function returning an AsyncContextManager so you have to await it (and supply correct arguments). The issue here applies to any ContextManager, async or not.
The risk here would be that since (Async)ExitStack.__enter__() itself hasn’t been called yet it’s not in a with block and as such there is likely no exit that will actually call the added context’s __(a)exit__(...)
import asyncio
import contextlib
class Resource:
async def __aenter__(self):
print("Allocated resource")
async def __aexit__(self, *exc_info):
print("De-allocated resource")
async def main():
s = contextlib.AsyncExitStack()
await s.enter_async_context(Resource()) # Will print "Allocated resource"
# Resource is never deallocated
asyncio.run(main())
In reality where this has been seen it’s usually where an ExitStack is part of an object which is itself a resource for example.
Ah, the problem is entering AsyncExitStack itself. I thought the problem was what was entered. Then no, we don’t want to change this because it’s the user responsibility to close the context manager. It’s not uncommon to pass around stacks if you need to compose with others.
I’m not insisting very heavily on this and am personally prepared to concede it on simple churn/benefit analysis (especially with needing to warn on a documented pattern).
I’m not sure I follow the objection of responsibility on a categorical level though. If something is a bug substantially more often than not and the fix in the cases where it’s intended is trivial and arguably clarifying some small rate of false positives of warnings could be acceptable?
I’m also not sure how this precludes passing stacks around or burdens that pattern. In most cases of that I’ve seen the stacks are still entered/exited and simply uses pop_stack to transfer/compose what’s needed.
Unless there is some other positive swing I won’t file any issue or PR.
For interest I tried searching through the standard library for the pattern and found 1 use in the unittest module and 11 more in test-cases. All would be trivially fixed and arguably clarified by an explicit __enter__
My point exactly: it’s a context manager. You use a context manager by calling __enter__ and __exit__, and you do that, in idiomatic python, by way of [async] with …:, which incidentally passes the responsibility for actually calling __exit__ to the Python interpreter, which reduces boilerplate clutter as well as line count and bug surface.
You can still do that. You just need one single chunk of code that’s actually responsible for opening and closing the context, instead of spreading that responsibility around. I’d contend that the latter is sufficiently uncommon; if your code really needs that pattern (a) your spaghetti structure wants to be refactored, (b) there’s nothing that prevents you from calling the dunder methods explicitly.
Also, when you use async contexts and taskgroups (or the library you’re using does) the whole idea of opening an async exit stack here and closing it there is no longer viable: you must not break taskgroup nesting.
In the example I was given, namely:
import asyncio
import contextlib
class Resource:
async def __aenter__(self):
print("Allocated resource")
async def __aexit__(self, *exc_info):
print("De-allocated resource")
async def main():
s = contextlib.AsyncExitStack()
await s.enter_async_context(Resource()) # Will print "Allocated resource"
# Resource is never deallocated
asyncio.run(main())
AFAIU, the problem is that the resource is never deallocated. Ok, but that’s the caller responsibility. They decided not to use a context manager so they are on their own for managing their resources. They must themselves call await s.aclose() when necessary. Or do async with s: [...].
It is also documented in ExitContext already:
Each instance maintains a stack of registered callbacks that are called in reverse order when the instance is closed (either explicitly or implicitly at the end of a with statement). Note that callbacks are not invoked implicitly when the context stack instance is garbage collected.
So, once the caller decide not to use it as a real context manager, they MUST call explicitly close()/aclose().
The call for this came from a real bug with a hairy debug situation where that wasn’t apparent to a user.
The argument for warning is that it would nudge them towards using a with block (or Enter it into another ContextStack) and I have yet to see a single example where the existing pattern could be used as is with the only needed difference being an explicit call to __(a)enter__ when constructed (even in the doc sample above that would work for simply silencing a warning as it __enter__ would still be idempotent)
Currently that is the case most often triggering this but that required nesting invariant really applies to more ContextManagers . The bugs are often just more subtle and harder to trigger other than on shutdown where errors are often tolerated and discarded. (I just don’t want this to be seen as an argument introduced only for structured concurrency, it’s more fundamental than that. For example contextlib.chdir, decimal.localcontext
I’m sorry but this is not enough for raising a warning where legitimate code would be annoyed.
and I have yet to see a single example where the existing pattern could be used as is
I’m not sure if my answer will be the answer to this as I’m not sure I understood correctly what you meant by “existing pattern”, but there are some examples of entering the stack as is without using it as a context:
This is clearly something that frameworks may rely on, and even custom projects.
So? People also relied on being able to do
try:
....
finally:
....
return
and we decided to warn about that pattern anyway.
PEP-765 (which AFAIR remains controversial) was proposed because there were hardly any projects that were using it correctly. This is not the same for ExitStack and this is not the same level of issues.
The API on ExitStacks is well-documented, and still documented as being low-level. Adding a warning would defeat the purpose of using its methods outside a with context.
I will step down from this discussion because you won’t be able to convince me that it is useful nor desired and I don’t have more time to convince you that emitting warnings or raising errors for legitimate patterns will affect downstream users. Leaking resources are usually reported at interpreter finalization so it’s also not impossible to debug.
It may very well be the conclusion since the cost for false warnings from the stdlib is quite high. I’m just not convinced the prevalence of uses and the purported hassle of fixing them aren’t overestimated.
How common is the existing pattern?
Single use in non-test code (unittest.mock) 10 more in tests as the examples shown above. All fixable by adding single __enter__() call inline on construct or on following line.
Third party code I don’t have a simple way to check yet but I would imagine a large majority of the addCleanup pattern in tests and actually quite few otherwise as people seem to actually use the ExitStacks a lot less than they should.
How annoying disruptive are the warnings?
I would say lower than expected from frequency since they in many cases would only be in tests.
How hard is the intended fix?
Trivial (add a call to __enter__) and could probably be explicitly documented in the warning
There may indeed be some case where an AsyncExitStack is constructed in a sync context but where it can’t await __aenter__ but then it can’t await enter_async_context() either and the enter can be delayed until it can.
How common is the error it catches and what is the impact if not?
Not that common but I would say it definitely can lead to nasty debugging when it does.
They still can at the cost of detecting a warning and suppressing it with a very simple modification. Not a zero cost but very far from not being able to rely on.
I’m fully aware this will affect some legitimate downstream users and have never argued otherwise. I’m not trying to convince you it won’t, just to leave some room for consideration despite that.
My own position is probably a cautious +0.5 but I fully expect and respect the position of treating warnings for legitimate code being near unacceptable as well.