Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 12 commits into from Nov 29, 2017
Merged

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

merged 12 commits into from Nov 29, 2017

Conversation

asvetlov
Copy link
Contributor

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

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

https://bugs.python.org/issue32166

@asvetlov asvetlov requested a review from 1st1 as a code owner November 28, 2017 15:05
serhiy-storchaka
serhiy-storchaka reviewed Nov 28, 2017
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

There is also a code related to Python < 3.5.

Lib/asyncio/coroutines.py Outdated
return gen.send_args != (value,)
_YIELD_FROM_BUG = has_yield_from_bug()
del has_yield_from_bug
_types_coroutine = types.coroutine
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

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
Copy link
Member

Choose a reason for hiding this comment

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

Keep this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense

@asvetlov
Copy link
Contributor Author

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
Copy link
Member

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
serhiy-storchaka reviewed Nov 28, 2017
View reviewed changes
Lib/asyncio/coroutines.py Outdated
# Prioritize native coroutine check to speed-up
# asyncio.iscoroutine.
_COROUTINE_TYPES = (_types_CoroutineType,) + _COROUTINE_TYPES
# Prioritize native coroutine check to speed-up
Copy link
Member

Choose a reason for hiding this comment

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

The second line of the comment has been lost.

Lib/asyncio/coroutines.py Outdated
@@ -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:
Copy link
Member

Choose a reason for hiding this comment

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

Is Awaitable imported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Fixed.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't Awaitable is not None always true?

@asvetlov
Copy link
Contributor Author

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 asvetlov changed the title Drop Python 3.4 code from asyncio bpo- 32166: Drop Python 3.4 code from asyncio Nov 29, 2017
@asvetlov 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
@asvetlov
Copy link
Contributor Author

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

@asvetlov
Copy link
Contributor Author

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

@1st1
Copy link
Member

1st1 commented Nov 29, 2017

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
serhiy-storchaka reviewed Nov 29, 2017
View reviewed changes
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']
Copy link
Member

Choose a reason for hiding this comment

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

Is opcode still used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Lib/asyncio/coroutines.py Outdated
@@ -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:
Copy link
Member

Choose a reason for hiding this comment

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

Isn't Awaitable is not None always true?

Lib/asyncio/coroutines.py Outdated
_COROUTINE_TYPES = (_types_CoroutineType,) + _COROUTINE_TYPES
# Prioritize native coroutine check to speed-up
# asyncio.iscoroutine.
_COROUTINE_TYPES = (types.CoroutineType, Coroutine,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

@asvetlov
Copy link
Contributor Author

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

serhiy-storchaka
serhiy-storchaka approved these changes Nov 29, 2017
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

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

@asvetlov
Copy link
Contributor Author

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

asvetlov
asvetlov commented Nov 29, 2017
View reviewed changes
Lib/asyncio/unix_events.py Outdated
@@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Moved the question from #4633
@vstinner please review

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks.
The call is removed.

@vstinner
Copy link
Member

@1st1 should the PR be backported to 3.6?

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

vstinner
vstinner approved these changes Nov 29, 2017
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

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

@asvetlov asvetlov merged commit cc83920 into python:master Nov 29, 2017
@asvetlov asvetlov deleted the drop-py34-support-from-asyncio branch November 29, 2017 16:23
@asvetlov
Copy link
Contributor Author

Thanks everybody for review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner approved these changes

@1st1 1st1 1st1 approved these changes

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@asvetlov @serhiy-storchaka @1st1 @vstinner @the-knights-who-say-ni @bedevere-bot