bpo-29679: Implement @contextlib.asynccontextmanager by JelleZijlstra · Pull Request #360 · python/cpython · GitHub

No description provided.

Needs docs and tests

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

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.

See python#360 (comment)

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

General concept and approach looks good to me, just some specific feedback on particular aspects of the implementation and test layout.

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.

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.

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

Missing async in the docstring.

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.

…fix docstring

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.

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.

Diff coverage shows a missing test case for this line.

Missing test case here as well.

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)

We could also write a small C helper to do that, but your idea is much better.

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.

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

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)

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

Please add a wraps decorator.

"works with coroutine" isn't really descriptive. I'd rephrase to something akin to "but creates asynchronous context managers".

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

I'd add that the decorator expects to be applied to asynchronous generator functions.

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.

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!

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.

Implements: - python/typing#438 - python/cpython#360 python/cpython#1412, which adds contextlib.AbstractAsyncContextManager, has not yet been merged.

) Implements: - python/typing#438 - python/cpython#360 Note that python/cpython#1412, which adds contextlib.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

JelleZijlstra added 4 commits February 28, 2017 15:38

implement @asynccontextmanager

904e8a8

Needs docs and tests

add it to __all__

b3d59f1

add tests (duplicating the @contextmanager ones)

a1d5b3f

add docs

c5b8b43

the-knights-who-say-ni added the CLA signed label Mar 1, 2017

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

Fix syntax error in .mention-bot

77b25b3

See python#360 (comment)

JelleZijlstra mentioned this pull request Mar 1, 2017

Fix syntax error in .mention-bot #362

Closed

ncoghlan requested changes Mar 1, 2017

View reviewed changes

JelleZijlstra added 2 commits February 28, 2017 20:42

back out ContextDecorator for asynccontextmanager (it doesn't work); …

ca77cd2

…fix docstring

move asynccontextmanager tests into their own file

689f4a5

ncoghlan self-assigned this Mar 1, 2017

ncoghlan approved these changes Mar 1, 2017

View reviewed changes

fix when tests are run after test_asyncio

299d968

ncoghlan requested changes Mar 2, 2017

View reviewed changes

JelleZijlstra added 3 commits March 2, 2017 08:01

add a few more tests

e974d48

combine duplicate tests

5808a4c

additional test for RuntimeError wrapping

9caa243

1st1 reviewed Mar 2, 2017

View reviewed changes

old-style classes, not strings

6d0dddb

ncoghlan mentioned this pull request Mar 3, 2017

Require code coverage not drop in a PR python/core-workflow#38

Closed

add to whatsnew (and alphabetize modules)

ad65b4d

1st1 reviewed Mar 3, 2017

View reviewed changes

JelleZijlstra added 2 commits March 3, 2017 00:10

add wraps decorator, add to docstring

737fd0f

unalphabetize whatsnew

3fc20a7

1st1 approved these changes Mar 3, 2017

View reviewed changes

Merge branch 'master' into asynccontextmanager

06697a8

Merge branch 'master' into asynccontextmanager

bb8de0d

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

JelleZijlstra deleted the asynccontextmanager branch May 1, 2017 13:35

JelleZijlstra mentioned this pull request Jun 24, 2017

add typing.AsyncContextManager and contextlib.asynccontextmanager python/typeshed#1432

Merged

+                      try:
+                          return await self.gen.__anext__()
+                      except StopAsyncIteration:
+                          raise RuntimeError("generator didn't yield") from None

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

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

+                          except RuntimeError as exc:
+                              if exc is value:
+                                  return False
+                              if exc.__cause__ is value:

+              def _async_test(func):
+                  """Decorator to turn an async function into a test case."""
+                  def wrapper(*args, **kwargs):
+                      loop = asyncio.new_event_loop()

-              class _GeneratorContextManager(ContextDecorator, AbstractContextManager):
-                  """Helper for @contextmanager decorator."""
+              class _GeneratorContextManagerBase(ContextDecorator):

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

		import asyncio

+                              raise
+                          except:
+                              if sys.exc_info()[1] is not value:
+                                  raise

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

+                 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.

               when they are :mod:`copied <copy>` or :mod:`pickled <pickle>`.
               (Contributed by Serhiy Storchaka in :issue:`20804`.)
-              xmlrpc.server



		def asynccontextmanager(func):
		"""@contextmanager decorator.

bpo-29679: Implement @contextlib.asynccontextmanager by JelleZijlstra · Pull Request #360 · python/cpython · GitHub

Repository navigation

bpo-29679: Implement @contextlib.asynccontextmanager - #360

bpo-29679: Implement @contextlib.asynccontextmanager#3601st1 merged 18 commits intopython:masterpython/cpython:masterfrom JelleZijlstra:asynccontextmanagerJelleZijlstra/cpython:asynccontextmanagerCopy head branch name to clipboard

Conversation

JelleZijlstra commented Mar 1, 2017

Uh oh!

mention-bot commented Mar 1, 2017

Uh oh!

JelleZijlstra commented Mar 1, 2017

Uh oh!

ncoghlan commented Mar 1, 2017

Uh oh!

ncoghlan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JelleZijlstra commented Mar 1, 2017

Uh oh!

ncoghlan commented Mar 1, 2017

Uh oh!

ilevkivskyi commented Mar 1, 2017

Uh oh!

ncoghlan commented Mar 2, 2017

Uh oh!

JelleZijlstra commented Mar 2, 2017

Uh oh!

JelleZijlstra commented Mar 2, 2017

Uh oh!

ncoghlan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncoghlan Mar 2, 2017 • edited by 1st1 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JelleZijlstra commented Mar 3, 2017

Uh oh!

ncoghlan commented Mar 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1st1 commented Mar 3, 2017

Uh oh!

1st1 left a comment

Choose a reason for hiding this comment

Uh oh!

sashgorokhov commented Mar 7, 2017

Uh oh!

bpo-29679: Implement @contextlib.asynccontextmanager#360
1st1 merged 18 commits into
python:masterfrom
JelleZijlstra:asynccontextmanager

ncoghlan Mar 2, 2017 •
edited by 1st1
Loading