Skip to content

/ cpython

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

Already on GitHub? Sign in to your account

bpo-35380: Enable TCP_NODELAY for proactor event loop #10867

Merged
merged 2 commits into from Dec 3, 2018
Merged

bpo-35380: Enable TCP_NODELAY for proactor event loop #10867

merged 2 commits into from Dec 3, 2018

Conversation

@asvetlov
Copy link
Contributor

asvetlov commented Dec 3, 2018 •
edited

Restore a consitensy with selector based event loops.
TCP_NODELAY is enabled starting from Python 3.6

https://bugs.python.org/issue35380
@asvetlov
Enable TCP_NODELAY for proactor event loop
Loading status checks…
a0bdbb4
@asvetlov asvetlov requested a review from 1st1 as a code owner Dec 3, 2018
@asvetlov
Tests cleanup
Loading status checks…
a810b5a
@1st1
1st1 approved these changes Dec 3, 2018
View changes
Copy link
Member

1st1 left a comment

I'm a bit cautious (i.e. I'm +0) with backporting this to 3.7. Are you sure it's 100% safe?
Lib/asyncio/base_events.py
@@ -168,6 +168,17 @@ def _run_until_complete_cb(fut):
futures._get_loop(fut).stop()


if hasattr(socket, 'TCP_NODELAY'):

This comment has been minimized.

Sign in to view
Copy link
@1st1

1st1 Dec 3, 2018

Member

I'd add a new asyncio.sockutils module for this kind of stuff.

This comment has been minimized.

Sign in to view
Copy link
@asvetlov

asvetlov Dec 3, 2018

Author Contributor

sockutils.py sounds great but in the case I doubt if the PR can be backported.

This comment has been minimized.

Sign in to view
Copy link
@1st1

1st1 Dec 3, 2018

Member

Indeed, I suggest to do that in a follow up PR specifically for the master branch.

This comment has been minimized.

Sign in to view
Copy link
@asvetlov

asvetlov Dec 3, 2018

Author Contributor

Created https://bugs.python.org/issue35392 for it.
@asvetlov

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

asvetlov commented Dec 3, 2018

Yes, the change is safe: Windows supports TCP_NODELAY very well for any version, at least I used it on Win 2000 and XP.

Without backport third parties like aiohttp should check for event loop type and enable the mode for proactor only, which looks very odd.
@1st1

This comment has been minimized.

Sign in to view
Copy link
Member

1st1 commented Dec 3, 2018

Without backport third parties like aiohttp should check for event loop type and enable the mode for proactor only, which looks very odd.

SGTM
@asvetlov asvetlov merged commit 3bc0eba into python:master Dec 3, 2018
5 checks passed
5 checks passed
Azure Pipelines PR #20181203.24 succeeded
Details
bedevere/issue-number Issue number 35380 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@miss-islington

This comment has been minimized.

Sign in to view
Copy link

miss-islington commented Dec 3, 2018

Thanks @asvetlov for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Dec 3, 2018

@asvetlov: Please replace # with GH- in the commit message next time. Thanks!
@asvetlov asvetlov deleted the asvetlov:tcp-nodelay-windows branch Dec 3, 2018
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Dec 3, 2018

GH-10872 is a backport of this pull request to the 3.7 branch.
miss-islington added a commit to miss-islington/cpython that referenced this pull request Dec 3, 2018
@asvetlov @miss-islington
bpo-35380: Enable TCP_NODELAY for proactor event loop (pythonGH-10867)
a6f24bd 
(cherry picked from commit 3bc0eba)

Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com>
@miss-islington

This comment has been minimized.

Sign in to view
Copy link

miss-islington commented Dec 3, 2018

Sorry, @asvetlov, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 3bc0ebab17bf5a2c29d2214743c82034f82e6573 3.6
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Dec 3, 2018

GH-10874 is a backport of this pull request to the 3.6 branch.
asvetlov added a commit to asvetlov/cpython that referenced this pull request Dec 3, 2018
@asvetlov
[3.6] bpo-35380: Enable TCP_NODELAY for proactor event loop (pythonGH…
6b9d758 
…-10867).

(cherry picked from commit 3bc0eba)

Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com>
asvetlov added a commit that referenced this pull request Dec 3, 2018
@miss-islington @asvetlov
[3.7] bpo-35380: Enable TCP_NODELAY for proactor event loop (GH-10867) (
Loading status checks…
fe91e9b 
GH-10872)

* bpo-35380: Enable TCP_NODELAY for proactor event loop (GH-10867)
(cherry picked from commit 3bc0eba)

Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com>
asvetlov added a commit that referenced this pull request Dec 5, 2018
@asvetlov
[3.6] bpo-35380: Enable TCP_NODELAY for proactor event loop (GH-10867)…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
bfb8818 
…. (GH-10874)

* [3.6] bpo-35380: Enable TCP_NODELAY for proactor event loop (GH-10867).
(cherry picked from commit 3bc0eba)

Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@1st1 1st1

Assignees

@asvetlov asvetlov

Labels
CLA signed expert-asyncio
Projects
None yet
Milestone
No milestone
5 participants
@asvetlov @1st1 @miss-islington @bedevere-bot @the-knights-who-say-ni
You can’t perform that action at this time.