8000 bpo-29679: Implement @contextlib.asynccontextmanager by JelleZijlstra · Pull Request #360 · python/cpython · GitHub
[go: up one dir, main page]

Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 18 commits into from
May 1, 2017
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
implement @asynccontextmanager
Needs docs and tests
  • Loading branch information
JelleZijlstra committed Feb 28, 2017
commit 904e8a8bb699b6eff1f086a2e60f36303d614968
81 changes: 79 additions & 2 deletions Lib/contextlib.py
10000
Original file line number Diff line number Diff line change
Expand Up @@ -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 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.

Copy link
Member Author

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.

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

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

implementations."""

def __init__(self, func, args, kwds):
self.gen = func(*args, **kwds)
Expand All @@ -77,6 +78,10 @@ def _recreate_cm(self):
# called
return self.__class__(self.func, self.args, self.kwds)


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

def __enter__(self):
try:
return next(self.gen)
Expand Down Expand Up @@ -126,6 +131,44 @@ def __exit__(self, type, value, traceback):
raise


class _AsyncGeneratorContextManager(_GeneratorContextManagerBase):
"""Helper for @asynccontextmanager."""

async def __aenter__(self):
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.


async def __aexit__(self, type, value, traceback):
Copy link
Member

Choose a reason for hiding this comment

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

type -> err_type or ex_type or typ. Don't mask globals.

if type is None:
try:
await self.gen.__anext__()
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.

else:
if value is None:
value = type()
Copy link
Contributor
@ncoghlan ncoghlan Mar 2, 2017

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)

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.

# See _GeneratorContextManager.__exit__ for comments on subtleties
# in this implementation
try:
await self.gen.athrow(type, value, traceback)
raise RuntimeError("generator didn't stop after throw()")
except StopAsyncIteration as exc:
return exc is not value
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 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.

return False
raise
except:
if sys.exc_info()[1] is not value:
Copy link
Member

Choose a reason for hiding this comment

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

Why we use sys.exc_info() here instead of

except BaseException as exc:
    if exc is not value:
        raise

Copy link
Member Author

Choose a reason for hiding this comment

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

I did this in order to not make unnecessary changes relative to the @contextmanager implementation, although I agree your code is better. I'm happy to change this if @ncoghlan is OK with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It uses sys.exc_info() because the corresponding _GeneratorContextManager code predates the acceptance and implementation of PEP 352, and is also common between 2.7 and 3.x (and hence needs to handle exceptions that don't inherit from BaseException)

I'm fine with switching this to using the BaseException form, but add a comment to the synchronous version saying it's written that way on purpose to keep the code consistent with the 2.7 branch and the contextlib2 backport.

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 neither StopAsyncIteration nor RuntimeError



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

Expand Down Expand Up @@ -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.

Missing async in the docstring.


Typical usage:

@asynccontextmanager
async def some_async_generator(<arguments>):
<setup>
try:
yield <value>
finally:
<cleanup>

This makes this:

async with some_async_generator(<arguments>) as <variable>:
<body>

equivalent to this:

<setup>
try:
<variable> = <value>
<body>
finally:
<cleanup>

Copy link
Member

Choose a reason for hiding this comment

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

Trailing empty newline.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean I should remove or add a newline? The formatting here is the same as in @contextmanager.

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 remove both empty newlines then. They aren't needed.

"""
@wraps(func)
def helper(*args, **kwds):
return _AsyncGeneratorContextManager(func, args, kwds)
return helper


class closing(AbstractContextManager):
"""Context to automatically close something at the end of a block.

Expand Down
0