bpo-32166: Drop Python 3.4 code from asyncio by asvetlov · Pull Request #4612 · python/cpython

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

-    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

-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

-    # 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

                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



-# 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

                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

-    _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

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



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

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

Uh oh!

Conversation

asvetlov commented Nov 28, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asvetlov commented Nov 28, 2017

Uh oh!

serhiy-storchaka commented Nov 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asvetlov commented Nov 28, 2017

Uh oh!

asvetlov commented Nov 29, 2017

Uh oh!

asvetlov commented Nov 29, 2017

Uh oh!

1st1 commented Nov 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asvetlov commented Nov 29, 2017

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

asvetlov commented Nov 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Nov 29, 2017

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

asvetlov commented Nov 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

asvetlov commented Nov 28, 2017 •

edited by bedevere-bot

Loading