bpo-29679: Implement @contextlib.asynccontextmanager by JelleZijlstra · Pull Request #360 · python/cpython (original) (raw)
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 our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation55 Commits18 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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.
JelleZijlstra added a commit to JelleZijlstra/cpython that referenced this pull request
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.
| @@ -54,8 +54,9 @@ def inner(*args, **kwds): |
|---|
| return inner |
| class _GeneratorContextManager(ContextDecorator, AbstractContextManager): |
| """Helper for @contextmanager decorator.""" |
| class _GeneratorContextManagerBase(ContextDecorator): |
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 to asynccontextmanager 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.
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 uses with instead of async with, not because of anything to do with __call__.)
We could add a separate AsyncContextDecorator implementation, but I don't currently have a good use for that.
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 :)
| @@ -160,6 +203,40 @@ def helper(*args, **kwds): |
|---|
| return helper |
| def asynccontextmanager(func): |
| """@contextmanager decorator. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing async in the docstring.
| @@ -1,5 +1,6 @@ |
|---|
| """Unit tests for contextlib.py, and other context managers.""" |
| import asyncio |
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 separate Lib/test_contextlib_async.py file.
Thanks for reviewing so quickly! I've pushed fixes for the issues you raised.
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 contextlib itself, or introduce a new asyncio.contextlib module.
@1st1 is someone who might be interested in this, see for example the discussion of asyncio.run_forever() here python/asyncio#465
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 1/0) to make sure they're actually being executed by the test harness?
That's strange. When I change the code locally to make one of the tests fail, they definitely do fail. (I ran ./python.exe -m test -uall test_contextlib_async.)
The detailed Travis results show that test_contextlib_async failed in coverage mode (together with test_traceback and test_xml_etree), but doesn't give details. Maybe coverage implements async-related bytecode incompletely or something; I'll run the command on a local clone to figure out what is going on.
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. ./python.exe -m test test_asyncio test_contextlib_async failed before this fix and now passes.
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 from contextlib 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: |
|---|
| return await self.gen.__anext__() |
| except StopAsyncIteration: |
| raise RuntimeError("generator didn't yield") from None |
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.
| except StopAsyncIteration: |
|---|
| return |
| else: |
| raise RuntimeError("generator didn't stop") |
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.
| raise RuntimeError("generator didn't stop") |
|---|
| else: |
| if value is None: |
| value = type() |
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 the async 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)
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.
| except RuntimeError as exc: |
|---|
| if exc is value: |
| return False |
| if exc.__cause__ is value: |
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 that value 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 (throw StopAsyncIteration 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: |
| if sys.exc_info()[1] is not value: |
| raise |
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 neither StopAsyncIteration nor RuntimeError
| def _async_test(func): |
|---|
| """Decorator to turn an async function into a test case.""" |
| def wrapper(*args, **kwargs): |
| loop = asyncio.new_event_loop() |
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.""" |
| def wrapper(*args, **kwargs): |
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)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a wraps decorator.
| .. decorator:: asynccontextmanager |
|---|
| Similar to :func:`~contextlib.contextmanager`, but works with |
| :term:`coroutines `. |
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".
| class _GeneratorContextManager(ContextDecorator, AbstractContextManager): |
|---|
| """Helper for @contextmanager decorator.""" |
| class _GeneratorContextManagerBase: |
| """Shared functionality for the @contextmanager and @asynccontextmanager |
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."""
Thanks! Fixed the comment.
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 :)
| 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. |
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.
| unittest.mock |
|---|
| ------------- |
| The :const:`~unittest.mock.sentinel` attributes now preserve their identity |
| when they are :mod:`copied ` or :mod:`pickled `. |
| (Contributed by Serhiy Storchaka in :issue:`20804`.) |
| xmlrpc.server |
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.
I've left another couple of comments in the review.
1st1 approved these changes Mar 3, 2017
Member
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 :)
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?
asyncio is no more provisional, and contextlib was never provisional, so that it looks like 3.7 is the earliest option.
asyncio is no more provisional, and contextlib was never provisional, so that it looks like 3.7 is the earliest option.
Correct.
@ncoghlan do you want me to merge this PR?
@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
Not sure what happened to the Appveyor build, the failures don't look related to contextlib.
Is there anything blocking this from being merged? There were a few merge conflicts already.
I'll merge it when it passes the checks. Thanks!
@JelleZijlstra
I have another feature proposal: there is a class contextlib.AbstractContextManager and its typing generic counterpart. I would propose to add a very similar contextlib.AbstractAsyncContextManager (with __aenter__ and __aexit__). This will complete the picture and will be also useful in the context of PEP 544: Protocols. Are you interested in implementing this?
Yes, sounds good. I can do that.
JelleZijlstra added a commit to JelleZijlstra/typeshed that referenced this pull request
gvanrossum pushed a commit to python/typeshed that referenced this pull request
jaraco pushed a commit that referenced this pull request
#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