bpo-32166: Drop Python 3.4 code from asyncio #4612

asvetlov · 2017-11-28T15:05:16Z

types.coroutine, types.CoroutineType, inspect.iscoroutinefunction and collections.abc.Coroutine / collections.abc.Awaitable are always present.

https://bugs.python.org/issue32166

…-from-asyncio

serhiy-storchaka

There is also a code related to Python < 3.5.

serhiy-storchaka · 2017-11-28T15:20:24Z

Lib/asyncio/coroutines.py

-    return gen.send_args != (value,)
-_YIELD_FROM_BUG = has_yield_from_bug()
-del has_yield_from_bug
+_types_coroutine = types.coroutine


Wouldn't the code look clearer if inline these definitions?

serhiy-storchaka · 2017-11-28T15:22:45Z

Lib/asyncio/coroutines.py

-if _CoroutineABC is not None:
-    _COROUTINE_TYPES += (_CoroutineABC,)
-if _types_CoroutineType is not None:
-    # Prioritize native coroutine check to speed-up


Keep this comment.

asvetlov · 2017-11-28T16:15:38Z

There is also a code related to Python < 3.5.

If you are talking about yield from support -- I think we should keep it for sake of backward compatibility.

serhiy-storchaka · 2017-11-28T16:21:16Z

Aren't these changes break backward compatibility with Python < 3.5?

I think it is worth to open an issue on the tracker for discussion.

serhiy-storchaka · 2017-11-28T16:23:27Z

Lib/asyncio/coroutines.py

-    # Prioritize native coroutine check to speed-up
-    # asyncio.iscoroutine.
-    _COROUTINE_TYPES = (_types_CoroutineType,) + _COROUTINE_TYPES
+ # Prioritize native coroutine check to speed-up


The second line of the comment has been lost.

serhiy-storchaka · 2017-11-28T16:24:31Z

Lib/asyncio/coroutines.py

@@ -218,23 +156,20 @@ def coro(*args, **kw):
            if (base_futures.isfuture(res) or inspect.isgenerator(res) or
                isinstance(res, CoroWrapper)):
                res = yield from res
-            elif _AwaitableABC is not None:
+            elif Awaitable is not None:


Is Awaitable imported?

Good catch. Fixed.

Isn't Awaitable is not None always true?

asvetlov · 2017-11-28T16:47:52Z

After PR merging asyncio code cannot be executed on Python 3.4, sure.
But

async def f():
    ...
yield from f()

and

@asyncio.coroutine
def g():
    ...
await g()

is still supported.
By backward compatibility I mean an ability to execute third party libraries written with python 3.4 support (aiohttp 2.x for example) on Python 3.7.

asvetlov · 2017-11-29T09:51:32Z

@serhiy-storchaka I created https://bugs.python.org/issue32166 as you suggested.

asvetlov · 2017-11-29T09:53:58Z

@1st1 should the PR be backported to 3.6?
The change is safe but it is not a bugfix.

1st1 · 2017-11-29T11:59:43Z

No. 3.6 is freezed for any refactorings—this policy ensures we dont break something accidentally. Let's treat 3.6 as a legacy code :)

serhiy-storchaka · 2017-11-29T12:04:54Z

Lib/asyncio/coroutines.py

 from . import constants
 from . import events
 from . import base_futures
 from .log import logger


-# Opcode of "yield from" instruction
-_YIELD_FROM = opcode.opmap['YIELD_FROM']


Is opcode still used?

No. It was used for making a workaround for bug already fixed in Python 3.4

serhiy-storchaka · 2017-11-29T12:11:34Z

Lib/asyncio/coroutines.py

@@ -218,23 +156,20 @@ def coro(*args, **kw):
            if (base_futures.isfuture(res) or inspect.isgenerator(res) or
                isinstance(res, CoroWrapper)):
                res = yield from res
-            elif _AwaitableABC is not None:
+            elif Awaitable is not None:


Isn't Awaitable is not None always true?

serhiy-storchaka · 2017-11-29T12:28:29Z

Lib/asyncio/coroutines.py

-    _COROUTINE_TYPES = (_types_CoroutineType,) + _COROUTINE_TYPES
+# Prioritize native coroutine check to speed-up
+# asyncio.iscoroutine.
+_COROUTINE_TYPES = (types.CoroutineType, Coroutine,


In the current code the natural type types.GeneratorType is tested before the abstract type Coroutine.

When async/await is used types.CoroutineType and collections.abc.Coroutine are more frequent than old styled generators.
That's why I've changed the order.

But checking an instance of types.GeneratorType is much faster than running the Python code in collections.abc.Coroutine.__subclasshook__.

asvetlov · 2017-11-29T12:45:09Z

Redundant check for Awaitable is not None dropped. Thanks for pointing out.

serhiy-storchaka

LGTM except that I'm not sure about the order in _COROUTINE_TYPES.

asvetlov · 2017-11-29T13:00:34Z

Well, if check for generator is much faster than ABC -- let's put it first.

asvetlov · 2017-11-29T13:07:12Z

Lib/asyncio/unix_events.py

@@ -680,7 +646,7 @@ def _start(self, args, shell, stdin, stdout, stderr, bufsize, **kwargs):
            # needed by close_fds=False on Python 3.3 and older
            # (Python 3.4 implements the PEP 446, socketpair returns
            # non-inheritable sockets)
-            _set_inheritable(stdin_w.fileno(), False)
+            os.set_inheritable(stdin_w.fileno(), False)


Does not the above comment make this unneeded? @vstinner, what are your thoughts?

Moved the question from #4633
@vstinner please review

os.set_inheritable(stdin_w.fileno(), False) is useless on Python 3.4 and newer. Remove the call with its comment.

If you don't trust me, please trust the tests ;-)

cpython/Lib/test/test_socket.py

Lines 5145 to 5146 in ef83806

self.assertEqual(s1.get_inheritable(), False)

self.assertEqual(s2.get_inheritable(), False)

Thanks.
The call is removed.

vstinner · 2017-11-29T14:19:40Z

@1st1 should the PR be backported to 3.6?

NO! Such refactoring change must not be backport to prevent any risk of regression.

vstinner

LGTM. It's nice to see such large cleanup changes, to simplify asyncio code base! It seems like Python core became better ;-)

asvetlov · 2017-11-29T16:24:07Z

Thanks everybody for review!

Drop Python 3.4 code from asyncio

2303c6c

asvetlov requested a review from 1st1 as a code owner November 28, 2017 15:05

bedevere-bot added the awaiting merge label Nov 28, 2017

the-knights-who-say-ni added the CLA signed label Nov 28, 2017

asvetlov added skip issue skip news labels Nov 28, 2017

Merge remote-tracking branch 'upstream/master' into drop-py34-support…

dd03455

…-from-asyncio

1st1 approved these changes Nov 28, 2017

View reviewed changes

serhiy-storchaka reviewed Nov 28, 2017

View reviewed changes

Fix notes

ae275fd

serhiy-storchaka reviewed Nov 28, 2017

View reviewed changes

asvetlov added 2 commits November 28, 2017 18:41

Add missing imports

107d25c

Restore comment

7ba1e2c

serhiy-storchaka removed the skip issue label Nov 29, 2017

asvetlov changed the title ~~Drop Python 3.4 code from asyncio~~ bpo- 32166: Drop Python 3.4 code from asyncio Nov 29, 2017

Resort imports

99724da

asvetlov changed the title ~~bpo- 32166: Drop Python 3.4 code from asyncio~~ bpo-32166: Drop Python 3.4 code from asyncio Nov 29, 2017

Drop Python 3.4-3.5 specific code

6c8785e

serhiy-storchaka reviewed Nov 29, 2017

View reviewed changes

Drop redunant check

2be3913

serhiy-storchaka approved these changes Nov 29, 2017

View reviewed changes

asvetlov added 2 commits November 29, 2017 14:51

Fix tests

36a3354

Merge branch 'bpo-32169' into drop-py34-support-from-asyncio

b202e14

Restore _COROUTINE_TYPES order

7819890

asvetlov commented Nov 29, 2017

View reviewed changes

asvetlov requested a review from vstinner November 29, 2017 13:07

Remove useless code

4809b82

serhiy-storchaka approved these changes Nov 29, 2017

View reviewed changes

vstinner approved these changes Nov 29, 2017

View reviewed changes

asvetlov merged commit cc83920 into python:master Nov 29, 2017

bedevere-bot removed the awaiting merge label Nov 29, 2017

asvetlov deleted the drop-py34-support-from-asyncio branch November 29, 2017 16:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-32166: Drop Python 3.4 code from asyncio #4612

bpo-32166: Drop Python 3.4 code from asyncio #4612

asvetlov commented Nov 28, 2017 •

edited by bedevere-bot

Loading

serhiy-storchaka left a comment

serhiy-storchaka Nov 28, 2017

asvetlov Nov 28, 2017

serhiy-storchaka Nov 28, 2017

asvetlov Nov 28, 2017

asvetlov commented Nov 28, 2017

serhiy-storchaka commented Nov 28, 2017

serhiy-storchaka Nov 28, 2017

serhiy-storchaka Nov 28, 2017

asvetlov Nov 28, 2017

serhiy-storchaka Nov 29, 2017

asvetlov commented Nov 28, 2017

asvetlov commented Nov 29, 2017

asvetlov commented Nov 29, 2017

1st1 commented Nov 29, 2017

serhiy-storchaka Nov 29, 2017

asvetlov Nov 29, 2017

serhiy-storchaka Nov 29, 2017

serhiy-storchaka Nov 29, 2017

asvetlov Nov 29, 2017

serhiy-storchaka Nov 29, 2017

asvetlov commented Nov 29, 2017

serhiy-storchaka left a comment

asvetlov commented Nov 29, 2017

asvetlov Nov 29, 2017

vstinner Nov 29, 2017

vstinner Nov 29, 2017

asvetlov Nov 29, 2017

vstinner commented Nov 29, 2017

vstinner left a comment

asvetlov commented Nov 29, 2017

	self.assertEqual(s1.get_inheritable(), False)
	self.assertEqual(s2.get_inheritable(), False)

bpo-32166: Drop Python 3.4 code from asyncio #4612

bpo-32166: Drop Python 3.4 code from asyncio #4612

Conversation

asvetlov commented Nov 28, 2017 • edited by bedevere-bot Loading

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asvetlov commented Nov 28, 2017

serhiy-storchaka commented Nov 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asvetlov commented Nov 28, 2017

asvetlov commented Nov 29, 2017

asvetlov commented Nov 29, 2017

1st1 commented Nov 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asvetlov commented Nov 29, 2017

serhiy-storchaka left a comment

Choose a reason for hiding this comment

asvetlov commented Nov 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner commented Nov 29, 2017

vstinner left a comment

Choose a reason for hiding this comment

asvetlov commented Nov 29, 2017

asvetlov commented Nov 28, 2017 •

edited by bedevere-bot

Loading