bpo-32193: Convert asyncio to async/await usage#4753
bpo-32193: Convert asyncio to async/await usage#4753asvetlov merged 35 commits intopython:masterfrom
Conversation
| @@ -71,15 +71,13 @@ def __await__(self): | |||
| yield from self.acquire() | |||
There was a problem hiding this comment.
with await lock prevents converting .acquire() to async function. This is sad.
There was a problem hiding this comment.
acquire can be converted to async def, just do this:
async def acquire(self):
...
def __await__(self):
return self.acquire().__await__()I'd also raise a DeprecationWarning in Lock.__await__. The new idiomatic way is to write async with lock. We can open a separate issue for this.
There was a problem hiding this comment.
The problem is that __await__(self) should return not awaitable for self.acquire() but an awaitable object with __enter__ / __exit__ for usage in statements like:
lock = asyncio.Lock()
with await lock as _lock:
assert _lock is None
assert lock.locked()
There was a problem hiding this comment.
Alright, you can do this then:
async def __acquire_ctx(self):
await self.acquire()
return _ContextManager(self)
def __await__(self):
return self.__acquire_ctx().__await__()There was a problem hiding this comment.
(use two leading underscores, I want 0 chance of someone using acquire_ctx method)
|
|
@types.coroutine
def __sleep0():
yield
async def sleep(...):
if delay == 0:
await __sleep0()
return result
... |
|
Another question: should stubs from |
Don't see any problem with converting them. |
|
|
| def sleep(delay, result=None, *, loop=None): | ||
| @types.coroutine | ||
| def _sleep0(): | ||
| yield |
There was a problem hiding this comment.
Need a comment here explaining why we have this hack.
|
Stubs are converted |
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @1st1: please review the changes made to this pull request. |
|
|
||
| @asyncio.coroutine | ||
| def coro(): | ||
| return 'ok' |
There was a problem hiding this comment.
Please test both types of old-style coroutines in this test: the ones that have 'yield from' in them and the ones that don't:
@coroutine
def coro1():
return 'ok'
@coroutine
def coro2():
yield from asyncio.sleep(0)
return 'ok'| res = self.loop.run_until_complete(self._test_pipe()) | ||
| self.assertEqual(res, 'done') | ||
|
|
||
| @asyncio.coroutine |
There was a problem hiding this comment.
Shouldn't this be converted to 'async def' too?
There was a problem hiding this comment.
I was trying to minimize tests change for sake of easy review.
One line for adding missing @asyncio.coroutine or several yield from -> await replacements.
There was a problem hiding this comment.
I think it's OK to upgrade tests too.
|
Everything is converted. Should |
Up to you. Elvis will take care of it anyways. |
|
Also, can you apply your patch and run aiohttp tests with the updated asyncio codebase? |
| # the hack is needed to make a quick pass | ||
| # of task step iteration when delay == 0 | ||
| # Task._step has an optimization for bare yield | ||
| yield |
There was a problem hiding this comment.
Please rewrite to:
"""Skip one event loop run cycle.
This is a private helper for 'asyncio.sleep()', used
when the 'delay' is set to 0. It uses a bare 'yield'
expression (which Task._step knows how to handle)
instead of creating a Future object.
"""|
Comments are addressed. |
1st1
left a comment
There was a problem hiding this comment.
OK, let's merge it! Thanks, Andrew.
|
Thanks for careful review |
|
BTW, can you write a test that |
|
Let's do it as part of https://bugs.python.org/issue32253 |
|
@asvetlov could you please strip commit messages like "Code cleanup" and "Fix indentation" next time when you merge a PR? See the step 3 at https://devguide.python.org/gitbootcamp/#accepting-and-merging-a-pull-request for details. |
|
@berkerpeksag will do. |
The PR is not finished yet but subprocesses, streams and queues are converted.
https://bugs.python.org/issue32193