bpo-27456: Ensure TCP_NODELAY is set on linux #4231

1st1 · 2017-11-02T15:57:00Z

https://bugs.python.org/issue27456

vstinner

Please copy/paste my unit test into your PR: https://github.com/python/cpython/pull/4233/files

You may also copy my NEWS entry. IMHO it's worth it to document this bugfix.

Mariatta · 2017-11-02T21:55:34Z

Misc/NEWS.d/next/Library/2017-11-02-11-57-41.bpo-27456.snzyTC.rst

@@ -0,0 +1 @@
+Ensure TCP_NODELAY is set on Linux


Please end the sentence with a period. :)

vstinner

I didn't check that you replaced all sock with sock.type, but tests pass so it should be fine :-)

LGTM.

vstinner · 2017-11-02T22:50:44Z

Oops, no, Travis CI failed.

asvetlov · 2017-11-03T09:06:49Z

I don't figure out why the failed test shouldn't go though fastpath

vstinner · 2017-11-03T10:35:11Z

Lib/asyncio/base_events.py

@@ -107,7 +107,7 @@ def _ipaddr_info(host, port, family, type, proto):
            host is None:
        return None

-    if type == socket.SOCK_STREAM:
+    if _is_stream_socket(type):


This change is ok. But please fix also "type == socket.SOCK_DGRAM" below, fix test_ipaddr_info(), and please also write a test for non-blocking DGRAM socket.

bedevere-bot · 2017-11-03T10:35:21Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

1st1 · 2017-11-06T17:30:34Z

I'll work on this later today.

vmagamedov · 2017-11-07T10:47:16Z

Lib/asyncio/base_events.py

    # Linux's socket.type is a bitmask that can include extra info
    # about socket, therefore we can't do simple
    # `sock_type == socket.SOCK_STREAM`.
-    return (sock.type & socket.SOCK_STREAM) == socket.SOCK_STREAM
+    return (sock_type & socket.SOCK_STREAM) == socket.SOCK_STREAM


socket.SOCK_STREAM is not a bit, it's a value, but socket.SOCK_NONBLOCK is a bit. That's why it is weird and complicated. Values and socket creation flags are mixed. For example, this check will fail for sock_type == socket.SOCK_SEQPACKET:

>>> (socket.SOCK_SEQPACKET & socket.SOCK_STREAM) == socket.SOCK_STREAM True

All possible values on Linux:

>>> list(socket.SocketKind) [<SocketKind.SOCK_STREAM: 1>, <SocketKind.SOCK_DGRAM: 2>, <SocketKind.SOCK_RAW: 3>, <SocketKind.SOCK_RDM: 4>, <SocketKind.SOCK_SEQPACKET: 5>, <SocketKind.SOCK_NONBLOCK: 2048>, <SocketKind.SOCK_CLOEXEC: 524288>]

pitrou · 2017-11-16T23:17:36Z

Lib/asyncio/selector_events.py

@@ -42,7 +42,7 @@ def _test_selector_event(selector, fd, event):
 if hasattr(socket, 'TCP_NODELAY'):
    def _set_nodelay(sock):
        if (sock.family in {socket.AF_INET, socket.AF_INET6} and
-                sock.type == socket.SOCK_STREAM and
+                base_events._is_stream_socket(sock.type) and


At this point, how about switching to EAFP (Easier to Ask Forgiveness than Permission)?

try: sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1) except OSError: pass

Yeah, that's what I'm planning to do here. There appears to be no reliable and future-proof way of checking socket type on Linux, so EAFP is the best option. The "not so good" option is to do a dance like this:

if sock.type & ~(SOCK_NONBLOCK | SOCK_CLOEXEC) == SOCK_STREAM:

I prefer to filter flags rather than always calling setsockopt() but ignore errors. For best performance, I prefer to avoid useless syscalls.

sock.type & ~(SOCK_NONBLOCK | SOCK_CLOEXEC)

Maybe we should add an attribute to socket.socket which returns the type without SOCK_NONBLOCK and SOCK_CLOEXEC bits.

Another option is to add SOCK_TYPE_MASK, maybe even as socket.SOCK_TYPE_MASK, as Linux does (https://github.com/torvalds/linux/blob/v4.13/include/linux/net.h#L77).

I think that

sock.type & ~(SOCK_NONBLOCK | SOCK_CLOEXEC)

is not as future-proof as

sock.type & SOCK_TYPE_MASK

Shouldn't BSD/Linux devs support backward-compatibility/conventions after introducing SOCK_CLOEXEC and SOCK_NONBLOCK flags? Maybe using SOCK_TYPE_MASK will be safe enough?

I like SOCK_TYPE_MASK idea. At least we can do this on Linux.

I'll do some experiments/more digging and update this PR.

For best performance, I prefer to avoid useless syscalls.

At the cost of maintainability, for a syscall that's called once in a socket's lifetime...

About being "future-proof": SOCK_NONBLOCK and SOCK_CLOEXEC flags are now old and well known. But I agree that it's annoying to have to care of them :-(

>>> def f(sock): ...: try: ...: sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1) ...: except OSError: ...: pass ...: >>> sock = socket.socket(socket.AF_INET) >>> %timeit f(sock) 500 ns ± 18.3 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each) >>> sock = socket.socket(socket.AF_UNIX) >>> %timeit f(sock) 1.66 µs ± 6.82 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

vstinner · 2017-11-17T14:58:12Z

While the idea of a new socket.socket attribute getting type without flags and SOCK_TYPE_MASK idea are nice, I suggest to keep this PR simple as the initially proposed fix, and open a new issue on bugs.python.org to propose to enhance the socket module.

asvetlov · 2017-12-13T15:35:23Z

ping

1st1 · 2017-12-14T04:26:02Z

I have made the requested changes; please review again

bedevere-bot · 2017-12-14T04:26:04Z

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

1st1 · 2017-12-14T04:26:29Z

@vstinner I think this should now be ready to be merged.

vstinner · 2017-12-14T06:36:24Z

Lib/asyncio/base_events.py

+        # `sock_type == socket.SOCK_STREAM`, see
+        # https://github.com/torvalds/linux/blob/v4.13/include/linux/net.h#L77
+        # for more details.
+        return (sock_type & 0xF) == socket.SOCK_STREAM


Would it be possible to add a private constant to the socket module, rather rhan using a private constant? If available, even use the Linux constant.

This constant can be ~0 if SOCK_NONBLOCK is missing to prevent the if.

Would it be possible to add a private constant to the socket module, rather rhan using a private constant?

Maybe, but I'd like to do that in a separate PR to the socket module first. I'd like to merge this PR as is.

IOW -- in my opinion this PR should be merged as is and backported to 3.6. I'll work on a new PR to expose SOCK_TYPE_MASK in socket and we'll update asyncio to use that.

I'm ok with merging, backporting to 3.6 and polishing 3.7 at the end.

bedevere-bot · 2017-12-14T06:38:53Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

A more generic fix is in progress: https://bugs.python.org/issue32331

1st1 · 2017-12-16T00:32:12Z

A more generic fix is in progress: https://bugs.python.org/issue32331

Yes, I'll merge this in now, and when that issue is resolved, I'll update the code for 3.7.

miss-islington · 2017-12-16T00:32:27Z

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

miss-islington · 2017-12-16T00:32:31Z

Sorry, @1st1, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker e796b2fe26f220107ac50667de6cc86c82b465e3 3.6

(cherry picked from commit e796b2f)

1st1 · 2017-12-16T00:57:52Z

Backport PR: #4898

(cherry picked from commit e796b2f)

bedevere-bot · 2018-03-18T20:16:25Z

GH-4898 is a backport of this pull request to the 3.6 branch.

1st1 added the needs backport to 3.6 label Nov 2, 2017

1st1 self-assigned this Nov 2, 2017

1st1 requested review from vstinner and asvetlov November 2, 2017 15:57

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

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

1st1 mentioned this pull request Nov 2, 2017

bpo-27456: asyncio: Fix _set_nodelay() for non-blocking socket #4233

Closed

vstinner reviewed Nov 2, 2017

View reviewed changes

Mariatta reviewed Nov 2, 2017

View reviewed changes

vstinner approved these changes Nov 2, 2017

View reviewed changes

vstinner requested changes Nov 3, 2017

View reviewed changes

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

bedevere-bot added the awaiting changes label Nov 3, 2017

vmagamedov reviewed Nov 7, 2017

View reviewed changes

pitrou reviewed Nov 16, 2017

View reviewed changes

bpo-27456: Ensure TCP_NODELAY is set on linux

2e208b5

1st1 force-pushed the fixaio branch from 2380a91 to 2e208b5 Compare December 14, 2017 04:25

bedevere-bot added awaiting change review and removed awaiting changes labels Dec 14, 2017

asvetlov approved these changes Dec 14, 2017

View reviewed changes

bedevere-bot removed the awaiting change review label Dec 14, 2017

bedevere-bot added the awaiting merge label Dec 14, 2017

vstinner previously requested changes Dec 14, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Dec 14, 2017

Remove nonsense test

3c0f674

1st1 merged commit e796b2f into python:master Dec 16, 2017

bedevere-bot removed the awaiting changes label Dec 16, 2017

1st1 added a commit to 1st1/cpython that referenced this pull request Dec 16, 2017

bpo-27456: Ensure TCP_NODELAY is set on linux (python#4231)

6547094

(cherry picked from commit e796b2f)

1st1 added a commit that referenced this pull request Dec 16, 2017

bpo-27456: Ensure TCP_NODELAY is set on linux (#4231) (#4898)

572636d

(cherry picked from commit e796b2f)

bedevere-bot removed the needs backport to 3.6 label Mar 18, 2018

tjhowse mentioned this pull request Feb 2, 2024

create_datagram_endpoint() no longer compatible with CAN bus raw sockets #114887

Closed

Uh oh!

bpo-27456: Ensure TCP_NODELAY is set on linux #4231

bpo-27456: Ensure TCP_NODELAY is set on linux #4231

Uh oh!

Conversation

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

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Mariatta Nov 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Nov 2, 2017

Uh oh!

asvetlov commented Nov 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Nov 3, 2017

Uh oh!

1st1 commented Nov 6, 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!

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 17, 2017

Uh oh!

asvetlov commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

1st1 commented Dec 14, 2017

Uh oh!

bedevere-bot commented Dec 14, 2017

Uh oh!

1st1 commented Dec 14, 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!

bedevere-bot commented Dec 14, 2017

Uh oh!

1st1 commented Dec 16, 2017

Uh oh!

miss-islington commented Dec 16, 2017

Uh oh!

miss-islington commented Dec 16, 2017

1st1 commented Nov 2, 2017 •

edited by bedevere-bot

Loading

Mariatta Nov 2, 2017 •

edited

Loading

asvetlov commented Dec 13, 2017 •

edited

Loading