Skip to content

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

Merged
asvetlov merged 2 commits into
python:masterfrom
asvetlov:tcp-nodelay-windows
Dec 3, 2018
Merged

bpo-35380: Enable TCP_NODELAY for proactor event loop#10867
asvetlov merged 2 commits into
python:masterfrom
asvetlov:tcp-nodelay-windows

Conversation

@asvetlov

@asvetlov asvetlov commented Dec 3, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

https://bugs.python.org/issue35380

@asvetlov asvetlov requested a review from 1st1 as a code owner December 3, 2018 12:43
1st1
1st1 approved these changes Dec 3, 2018
View reviewed changes

@1st1 1st1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm a bit cautious (i.e. I'm +0) with backporting this to 3.7. Are you sure it's 100% safe?

Comment thread Lib/asyncio/base_events.py
futures._get_loop(fut).stop()


if hasattr(socket, 'TCP_NODELAY'):

@1st1 1st1 Dec 3, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@asvetlov asvetlov Dec 3, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

@1st1 1st1 Dec 3, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@asvetlov asvetlov Dec 3, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

@asvetlov

asvetlov commented Dec 3, 2018

Copy link
Copy Markdown
Contributor Author

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

1st1 commented Dec 3, 2018

Copy link
Copy Markdown
Member

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
@miss-islington

miss-islington commented Dec 3, 2018

Copy link
Copy Markdown
Contributor

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

@bedevere-bot

bedevere-bot commented Dec 3, 2018

Copy link
Copy Markdown

@asvetlov: Please replace # with GH- in the commit message next time. Thanks!

@asvetlov asvetlov deleted the tcp-nodelay-windows branch December 3, 2018 19:08
@bedevere-bot

bedevere-bot commented Dec 3, 2018

Copy link
Copy Markdown

GH-10872 is a backport of this pull request to the 3.7 branch.

miss-islington pushed 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

miss-islington commented Dec 3, 2018

Copy link
Copy Markdown
Contributor

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

bedevere-bot commented Dec 3, 2018

Copy link
Copy Markdown

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) (
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)…
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 1st1 approved these changes

Assignees

@asvetlov asvetlov

Labels

topic-asyncio

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@asvetlov @1st1 @miss-islington @bedevere-bot @the-knights-who-say-ni