Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
1st1 merged 18 commits intopython:masterfromJelleZijlstra:asynccontextmanager
May 1, 2017

Conversation

JelleZijlstra
Copy link
Member

No description provided.

@mention-bot
Copy link

@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
Copy link
MemberAuthor

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 requestMar 1, 2017
@ncoghlan
Copy link
Contributor

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

Copy link
Contributor

@ncoghlanncoghlan left a 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.

@@ -54,8 +54,9 @@ def inner(*args, **kwds):
return inner


class _GeneratorContextManager(ContextDecorator, AbstractContextManager):
"""Helper for @contextmanager decorator."""
class _GeneratorContextManagerBase(ContextDecorator):
Copy link
Contributor

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.

Copy link
MemberAuthor

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.

Copy link
Contributor

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Missingasync in the docstring.

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

import asyncio
Copy link
Contributor

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
Copy link
MemberAuthor

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

@ncoghlanncoghlan self-assigned thisMar 1, 2017
@ncoghlan
Copy link
Contributor

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 tocontextlib itself, or introduce a newasyncio.contextlib module.

@ilevkivskyi
Copy link
Member

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

@ncoghlan
Copy link
Contributor

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

@JelleZijlstra
Copy link
MemberAuthor

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 thattest_contextlib_async failed in coverage mode (together withtest_traceback andtest_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
Copy link
MemberAuthor

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.

Copy link
Contributor

@ncoghlanncoghlan left a 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:
return await self.gen.__anext__()
except StopAsyncIteration:
raise RuntimeError("generator didn't yield") from None
Copy link
Contributor

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")
Copy link
Contributor

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()
Copy link
Contributor

@ncoghlanncoghlanMar 2, 2017
edited by 1st1
Loading

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)

Copy link
Member

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:
Copy link
Contributor

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:
if sys.exc_info()[1] is not value:
raise
Copy link
Contributor

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

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

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):
Copy link
Member

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Please add awraps decorator.

.. decorator:: asynccontextmanager

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

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
Copy link
Member

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
Copy link
MemberAuthor

Thanks! Fixed the comment.

@ncoghlan
Copy link
Contributor

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.
Copy link
Member

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

xmlrpc.server
Copy link
Member

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
Copy link
Member

1st1 commentedMar 3, 2017

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

Copy link
Member

@1st11st1 left a 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
Copy link

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
Copy link
Member

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

@1st1
Copy link
Member

1st1 commentedMar 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
Copy link
Contributor

@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
Copy link
MemberAuthor

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

@JelleZijlstra
Copy link
MemberAuthor

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

@1st1
Copy link
Member

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

@JelleZijlstra
Copy link
MemberAuthor

Thanks!

@1st11st1 merged commit2e62469 intopython:masterMay 1, 2017
@ilevkivskyi
Copy link
Member

@JelleZijlstra
I have another feature proposal: there is a classcontextlib.AbstractContextManager and itstyping generic counterpart. I would propose to add a very similarcontextlib.AbstractAsyncContextManager (with__aenter__ and__aexit__). This will complete the picture and will be also useful in the context ofPEP 544: Protocols. Are you interested in implementing this?

@JelleZijlstraJelleZijlstra deleted the asynccontextmanager branchMay 1, 2017 13:35
@JelleZijlstra
Copy link
MemberAuthor

Yes, sounds good. I can do that.

JelleZijlstra added a commit to JelleZijlstra/typeshed that referenced this pull requestJun 24, 2017
Implements:-python/typing#438-python/cpython#360python/cpython#1412, which addscontextlib.AbstractAsyncContextManager, has not yet been merged.
gvanrossum pushed a commit to python/typeshed that referenced this pull requestJun 27, 2017
)Implements:-python/typing#438-python/cpython#360Note thatpython/cpython#1412, which addscontextlib.AbstractAsyncContextManager, has not yet been merged.
jaraco pushed a commit that referenced this pull requestDec 2, 2022
#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
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@ncoghlanncoghlanncoghlan approved these changes

@1st11st11st1 approved these changes

Assignees

@ncoghlanncoghlan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

7 participants
@JelleZijlstra@mention-bot@ncoghlan@ilevkivskyi@1st1@sashgorokhov@the-knights-who-say-ni

[8]ページ先頭

©2009-2025 Movatter.jp