Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-29679: Implement @contextlib.asynccontextmanager #360

Merged
merged 18 commits into from May 1, 2017

Conversation

Projects
None yet
7 participants
@JelleZijlstra
Copy link
Contributor

commented Mar 1, 2017

No description provided.

@mention-bot

This comment has been minimized.

Copy link

commented Mar 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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 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.

JelleZijlstra added a commit to JelleZijlstra/cpython that referenced this pull request Mar 1, 2017

@ncoghlan

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2017

@JelleZijlstra The maintainer list is at https://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
Copy link
Contributor

left a comment

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):

This comment has been minimized.

Copy link
@ncoghlan

ncoghlan Mar 1, 2017

Contributor

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.

This comment has been minimized.

Copy link
@JelleZijlstra

JelleZijlstra Mar 1, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@ncoghlan

ncoghlan Mar 1, 2017

Contributor

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.

This comment has been minimized.

Copy link
@ncoghlan

ncoghlan Mar 1, 2017

Contributor

Missing async in the docstring.

@@ -1,5 +1,6 @@
"""Unit tests for contextlib.py, and other context managers."""

import asyncio

This comment has been minimized.

Copy link
@ncoghlan

ncoghlan Mar 1, 2017

Contributor

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.

@JelleZijlstra

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2017

Thanks for reviewing so quickly! I've pushed fixes for the issues you raised.

@ncoghlan ncoghlan self-assigned this Mar 1, 2017

@ncoghlan

This comment has been minimized.

Copy link
Contributor

commented Mar 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 contextlib itself, or introduce a new asyncio.contextlib module.

@ilevkivskyi

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2017

@1st1 is someone who might be interested in this, see for example the discussion of asyncio.run_forever() here python/asyncio#465

@ncoghlan

This comment has been minimized.

Copy link
Contributor

commented Mar 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 1/0) to make sure they're actually being executed by the test harness?

@JelleZijlstra

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2017

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.

@JelleZijlstra

This comment has been minimized.

Copy link
Contributor Author

commented Mar 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. ./python.exe -m test test_asyncio test_contextlib_async failed before this fix and now passes.

@ncoghlan
Copy link
Contributor

left a comment

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

This comment has been minimized.

Copy link
@ncoghlan

ncoghlan Mar 2, 2017

Contributor

Diff coverage shows a missing test case for this line.

except StopAsyncIteration:
return
else:
raise RuntimeError("generator didn't stop")

This comment has been minimized.

Copy link
@ncoghlan

ncoghlan Mar 2, 2017

Contributor

Missing test case here as well.

raise RuntimeError("generator didn't stop")
else:
if value is None:
value = type()

This comment has been minimized.

Copy link
@ncoghlan

ncoghlan Mar 2, 2017

Contributor

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)

This comment has been minimized.

Copy link
@1st1

1st1 Mar 2, 2017

Member

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:

This comment has been minimized.

Copy link
@ncoghlan

ncoghlan Mar 2, 2017

Contributor

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

This comment has been minimized.

Copy link
@ncoghlan

ncoghlan Mar 2, 2017

Contributor

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()

This comment has been minimized.

Copy link
@1st1

1st1 Mar 2, 2017

Member

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):

This comment has been minimized.

Copy link
@1st1

1st1 Mar 2, 2017

Member

Also, I know it's just a test, but please add @functools.wraps(func)

This comment has been minimized.

Copy link
@1st1

1st1 Mar 3, 2017

Member

Please add a wraps decorator.

.. decorator:: asynccontextmanager

Similar to :func:`~contextlib.contextmanager`, but works with
:term:`coroutines <coroutine>`.

This comment has been minimized.

Copy link
@1st1

1st1 Mar 2, 2017

Member

"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

This comment has been minimized.

Copy link
@1st1

1st1 Mar 2, 2017

Member

First line of docstring must be a single sentence. Just make it

"""Shared functionality for the @contextmanager and @asynccontextmanager."""
@JelleZijlstra

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2017

Thanks! Fixed the comment.

@ncoghlan

This comment has been minimized.

Copy link
Contributor

commented Mar 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 :)

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.

This comment has been minimized.

Copy link
@1st1

1st1 Mar 3, 2017

Member

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 <copy>` or :mod:`pickled <pickle>`.
(Contributed by Serhiy Storchaka in :issue:`20804`.)

xmlrpc.server

This comment has been minimized.

Copy link
@1st1

1st1 Mar 3, 2017

Member

I'd leave reordering out of this PR. It should be done in a separate commit.

@1st1

This comment has been minimized.

Copy link
Member

commented Mar 3, 2017

I've left another couple of comments in the review.

JelleZijlstra added some commits Mar 3, 2017

@1st1

1st1 approved these changes Mar 3, 2017

Copy link
Member

left a comment

LGTM. FWIW I'm really glad the tests didn't expose any bugs in asynchronous generators :)

@sashgorokhov

This comment has been minimized.

Copy link

commented Mar 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

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2017

asyncio is no more provisional, and contextlib was never provisional, so that it looks like 3.7 is the earliest option.

@1st1

This comment has been minimized.

Copy link
Member

commented Mar 7, 2017

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?

@ncoghlan

This comment has been minimized.

Copy link
Contributor

commented Mar 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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2017

Not sure what happened to the Appveyor build, the failures don't look related to contextlib.

@JelleZijlstra

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2017

Is there anything blocking this from being merged? There were a few merge conflicts already.

@1st1

This comment has been minimized.

Copy link
Member

commented Apr 30, 2017

I'll merge it when it passes the checks. Thanks!

@JelleZijlstra

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2017

Thanks!

@1st1 1st1 merged commit 2e62469 into python:master May 1, 2017

3 of 4 checks passed

codecov/patch 94.65% of diff hit (target 100%)
Details
bedevere/issue-number Issue number 29679 found.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ilevkivskyi

This comment has been minimized.

Copy link
Contributor

commented May 1, 2017

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

@JelleZijlstra JelleZijlstra deleted the JelleZijlstra:asynccontextmanager branch May 1, 2017

@JelleZijlstra

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2017

Yes, sounds good. I can do that.

zhangyangyu added a commit to zhangyangyu/cpython that referenced this pull request May 8, 2017

JelleZijlstra added a commit to JelleZijlstra/typeshed that referenced this pull request Jun 24, 2017

add typing.AsyncContextManager and contextlib.asynccontextmanager
Implements:
- python/typing#438
- python/cpython#360

python/cpython#1412, which adds
contextlib.AbstractAsyncContextManager, has not yet been merged.

gvanrossum added a commit to python/typeshed that referenced this pull request Jun 27, 2017

add typing.AsyncContextManager and contextlib.asynccontextmanager (#1432
)

Implements:
- python/typing#438
- python/cpython#360

Note that python/cpython#1412, which adds
contextlib.AbstractAsyncContextManager, has not yet been merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.