Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.3k
bpo-29679: Implement @contextlib.asynccontextmanager#360
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
mention-bot commentedMar 1, 2017
@JelleZijlstra, thanks for your PR! By analyzing the history of the files in this pull request, we identified@gvanrossum,@brettcannon and@Yhg1s to be potential reviewers. |
JelleZijlstra commentedMar 1, 2017
Also adding@ncoghlan who I believe maintains contextlib (although I can't find module maintainers listed anywhere in the devguide or the repo). Not sure what prompted the error in mention-bot's first message. |
ncoghlan commentedMar 1, 2017
@JelleZijlstra The maintainer list is athttps://docs.python.org/devguide/experts.html#experts (you can also just start typing the module name into the nosy list field on bugs.python.org and it will autopopulate the dropdown with the relevant usernames) |
ncoghlan left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
General concept and approach looks good to me, just some specific feedback on particular aspects of the implementation and test layout.
Lib/contextlib.py Outdated
| class_GeneratorContextManager(ContextDecorator,AbstractContextManager): | ||
| """Helper for @contextmanager decorator.""" | ||
| class_GeneratorContextManagerBase(ContextDecorator): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ContextDecorator is designed to work with__call__ rather than__await__, so having it also applied toasynccontextmanager doesn't look right to me.
So I'd suggest moving the ContextDecorator inheritance and the_recreate_cm method down into_GeneratorContextManager and only keeping the__init__ method common between the two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You're right, my implementation didn't actually work as a decorator. (Although it's because ContextDecorator useswith instead ofasync with, not because of anything to do with__call__.)
We could add a separateAsyncContextDecorator implementation, but I don't currently have a good use for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I actually consider making _GeneratorContextManager inherit from ContextDecorator a design error on my part (that's why _recreate_cm is still a private API:https://bugs.python.org/issue11647#msg161113 )
So simply not supporting an equivalent for async context managers is entirely fine by me :)
Lib/contextlib.py Outdated
| defasynccontextmanager(func): | ||
| """@contextmanager decorator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Missingasync in the docstring.
Lib/test/test_contextlib.py Outdated
| @@ -1,5 +1,6 @@ | |||
| """Unit tests for contextlib.py, and other context managers.""" | |||
| importasyncio | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd prefer for other implementations to be able to readily test the rest of contextlib without requiring a working asyncio implementation, so it would be good to split these new tests out to a separateLib/test_contextlib_async.py file.
JelleZijlstra commentedMar 1, 2017
Thanks for reviewing so quickly! I've pushed fixes for the issues you raised. |
ncoghlan commentedMar 1, 2017
The PR itself looks good to me now, but I'm going to place it on hold for the moment pending further consideration of whether we should add this to |
ilevkivskyi commentedMar 1, 2017
@1st1 is someone who might be interested in this, see for example the discussion of |
ncoghlan commentedMar 2, 2017
OK, based on the python-dev discussion, I'm happy with the idea of proceeding with this basic API design:http://bugs.python.org/issue29679#msg288781 However, those code coverage results are suspicious, as they suggest that either the async-based tests aren't actually running properly, or else the coverage report isn't capturing their execution correctly. Could you take another look at those tests and inject some deliberate failures (I like |
JelleZijlstra commentedMar 2, 2017
That's strange. When I change the code locally to make one of the tests fail, they definitely do fail. (I ran The detailed Travis results show that |
JelleZijlstra commentedMar 2, 2017
Found the issue and pushed a fix. The problem was that test_asyncio closes the default event loop, so if test_asyncio and test_contextlib_async are run consecutively in the same process, the contextlib_async tests that rely on get_event_loop() returning a usable loop fail. I fixed the problem by instead creating a new event loop. |
ncoghlan left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Noted several lines from the diff coverage report that the new test cases should really be hitting. The other uncovered lines all step fromcontextlib being imported being the coverage data starts being collected, so there isn't a lot to be done about that.
Feel free to ask for more info if it isn't clear how to craft a test case that covers the commented on lines.
| try: | ||
| returnawaitself.gen.__anext__() | ||
| exceptStopAsyncIteration: | ||
| raiseRuntimeError("generator didn't yield")fromNone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Diff coverage shows a missing test case for this line.
| exceptStopAsyncIteration: | ||
| return | ||
| else: | ||
| raiseRuntimeError("generator didn't stop") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Missing test case here as well.
Lib/contextlib.py Outdated
| raiseRuntimeError("generator didn't stop") | ||
| else: | ||
| ifvalueisNone: | ||
| value=type() |
ncoghlanMar 2, 2017 • edited by 1st1
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by 1st1
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You won't be able to hit this line via theasync with syntax, but it can be exercised by awaiting__aexit__ directly with a non-normalised exception (i.e. only the exception type, with both the exception value and the traceback as None)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We could also write a small C helper to do that, but your idea is much better.
Lib/contextlib.py Outdated
| exceptRuntimeErrorasexc: | ||
| ifexcisvalue: | ||
| returnFalse | ||
| ifexc.__cause__isvalue: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Huh, this (and its counterpart in_GeneratorContextManager) is buggy, since it isn't checking thatvalue was the appropriate kind of exception to start with:http://bugs.python.org/issue29692
Don't fix the synchronous version in this PR, but do add a test case for the async version that hits this line (throwStopAsyncIteration from inside an async with statement and then catch it again outside), and also one that chains some other exception type with RuntimeError and make sure that still gets chained correctly.
| raise | ||
| except: | ||
| ifsys.exc_info()[1]isnotvalue: | ||
| raise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hitting this line means adding a test case that replaces the thrown in exception with an entirely unrelated one that is neitherStopAsyncIteration norRuntimeError
Lib/test/test_contextlib_async.py Outdated
| def_async_test(func): | ||
| """Decorator to turn an async function into a test case.""" | ||
| defwrapper(*args,**kwargs): | ||
| loop=asyncio.new_event_loop() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You need to close the loop (1), and make sure it's the default loop (2).
Please rewrite to:
loop=asyncio.new_event_loop()asyncio.set_event_loop(loop)try:loop.run_until_complete(coro)finally:loop.close()asyncio.set_event_loop(None)
| def_async_test(func): | ||
| """Decorator to turn an async function into a test case.""" | ||
| defwrapper(*args,**kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Also, I know it's just a test, but please add@functools.wraps(func)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Please add awraps decorator.
Doc/library/contextlib.rst Outdated
| ..decorator::asynccontextmanager | ||
| Similar to:func:`~contextlib.contextmanager`, but works with | ||
| :term:`coroutines <coroutine>`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
"works with coroutine" isn't really descriptive. I'd rephrase to something akin to "but creates asynchronous context managers".
Lib/contextlib.py Outdated
| class_GeneratorContextManager(ContextDecorator,AbstractContextManager): | ||
| """Helper for @contextmanager decorator.""" | ||
| class_GeneratorContextManagerBase: | ||
| """Shared functionality for the @contextmanager and @asynccontextmanager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
First line of docstring must be a single sentence. Just make it
"""Shared functionality for the @contextmanager and @asynccontextmanager."""JelleZijlstra commentedMar 3, 2017
Thanks! Fixed the comment. |
ncoghlan commentedMar 3, 2017
If you'd like, you can add a note to Docs/whatsnew/3.7.rst about the addition, otherwise I'll add it when I update Misc/NEWS prior to merging :) |
Doc/library/contextlib.rst Outdated
| This function is a:term:`decorator` that can be used to define a factory | ||
| function for:keyword:`async with` statement asynchronous context managers, | ||
| without needing to create a class or separate:meth:`__aenter__` and | ||
| :meth:`__aexit__` methods. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd add that the decorator expects to be applied to asynchronous generator functions.
| when they are:mod:`copied <copy>` or:mod:`pickled <pickle>`. | ||
| (Contributed by Serhiy Storchaka in:issue:`20804`.) | ||
| xmlrpc.server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd leave reordering out of this PR. It should be done in a separate commit.
1st1 commentedMar 3, 2017
I've left another couple of comments in the review. |
1st1 left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM. FWIW I'm really glad the tests didn't expose any bugs in asynchronous generators :)
sashgorokhov commentedMar 7, 2017
Created the same package that implements asynccontextmanager decorator on these weekends, and wanted to push it into python. And here it is! Any thoughts in which python release this will be included? |
ilevkivskyi commentedMar 7, 2017
|
1st1 commentedMar 7, 2017
Correct. @ncoghlan do you want me to merge this PR? |
ncoghlan commentedMar 8, 2017
@1st1 Go ahead, thanks :) For folks interested in 3.5 and 3.6 support, I opened an issue against contextlib2 to discuss the options for handling that:jazzband/contextlib2#12 |
JelleZijlstra commentedMar 8, 2017
Not sure what happened to the Appveyor build, the failures don't look related to contextlib. |
JelleZijlstra commentedApr 30, 2017
Is there anything blocking this from being merged? There were a few merge conflicts already. |
1st1 commentedApr 30, 2017
I'll merge it when it passes the checks. Thanks! |
JelleZijlstra commentedApr 30, 2017
Thanks! |
ilevkivskyi commentedMay 1, 2017
@JelleZijlstra |
JelleZijlstra commentedMay 1, 2017
Yes, sounds good. I can do that. |
Implements:-python/typing#438-python/cpython#360python/cpython#1412, which addscontextlib.AbstractAsyncContextManager, has not yet been merged.
)Implements:-python/typing#438-python/cpython#360Note thatpython/cpython#1412, which addscontextlib.AbstractAsyncContextManager, has not yet been merged.
#360)* Remove 'Automerge-Triggered-By' text when removing the automerge label* Add more tests* Add test case for the scenario that the trailer text does not exist
No description provided.