cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation36 Commits35 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

@@ -71,15 +71,13 @@ def __await__(self):
yield from self.acquire()

Choose a reason for hiding this comment

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

with await lock prevents converting .acquire() to async function. This is sad.

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

(use two leading underscores, I want 0 chance of someone using acquire_ctx method)

Choose a reason for hiding this comment

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

Done.

asyncio.sleep() uses bare yield for zero timeout.
I can convert it to loop.call_soon(...) if we agree to use this approach.

I can convert it to loop.call_soon(...) if we agree to use this approach.

sleep(0) is frequently used to skip a beat of the event loop... I'd like it to be as fast as possible. Can you check if this works:

@types.coroutine def __sleep0(): yield

async def sleep(...): if delay == 0: await __sleep0() return result ...

Another question: should stubs from AbstractEventLoop be converted to async functions?
Now they are just regular funcs not decorated by @coroutine, e.g. def connect_read_pipe(self, protocol_factory, pipe): ...
I believe changing the signature to async def connect_read_pipe(self, protocol_factory, pipe): ... is backward compatible (tests should be fixed but semantics is kept the same) but more obvious.

Another question: should stubs from AbstractEventLoop be converted to async functions?

Don't see any problem with converting them.

def sleep(delay, result=None, *, loop=None):
@types.coroutine
def _sleep0():
yield

Choose a reason for hiding this comment

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

Need a comment here explaining why we have this hack.

Choose a reason for hiding this comment

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

Done

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'

Choose a reason for hiding this comment

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

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'

@@ -56,6 +56,7 @@ def test_pipe(self):
res = self.loop.run_until_complete(self._test_pipe())
self.assertEqual(res, 'done')

@asyncio.coroutine

Choose a reason for hiding this comment

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

Shouldn't this be converted to 'async def' too?

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

I think it's OK to upgrade tests too.

Everything is converted.

Should Doc/whatsnew/3.7.rst be updated?

Should Doc/whatsnew/3.7.rst be updated?

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

Choose a reason for hiding this comment

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

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.
aiohttp works perfectly fine.

1st1 approved these changes Dec 8, 2017

Member

@1st1 1st1 left a comment

Choose a reason for hiding this comment

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

OK, let's merge it! Thanks, Andrew.

Thanks for careful review

BTW, can you write a test that with (yield from lock) is still working?