Skip to content

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

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

Merged
merged 2 commits into from
Dec 16, 2017
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2e208b5
bpo-27456: Ensure TCP_NODELAY is set on linux
1st1 Nov 2, 2017
3c0f674
Remove nonsense test
1st1 Dec 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 22 additions & 21 deletions Lib/asyncio/base_events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,18 +82,24 @@ def _set_reuseport(sock):
'SO_REUSEPORT defined but not implemented.')


def _is_stream_socket(sock):
# 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
def _is_stream_socket(sock_type):
if hasattr(socket, 'SOCK_NONBLOCK'):
# Linux's socket.type is a bitmask that can include extra info
# about socket (like SOCK_NONBLOCK bit), therefore we can't do simple
# `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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

else:
return sock_type == socket.SOCK_STREAM


def _is_dgram_socket(sock):
# Linux's socket.type is a bitmask that can include extra info
# about socket, therefore we can't do simple
# `sock_type == socket.SOCK_DGRAM`.
return (sock.type & socket.SOCK_DGRAM) == socket.SOCK_DGRAM
def _is_dgram_socket(sock_type):
if hasattr(socket, 'SOCK_NONBLOCK'):
# See the comment in `_is_stream_socket`.
return (sock_type & 0xF) == socket.SOCK_DGRAM
else:
return sock_type == socket.SOCK_DGRAM


def _ipaddr_info(host, port, family, type, proto):
Expand All @@ -106,14 +112,9 @@ def _ipaddr_info(host, port, family, type, proto):
host is None:
return None

if type == socket.SOCK_STREAM:
# Linux only:
# getaddrinfo() can raise when socket.type is a bit mask.
# So if socket.type is a bit mask of SOCK_STREAM, and say
# SOCK_NONBLOCK, we simply return None, which will trigger
# a call to getaddrinfo() letting it process this request.
if _is_stream_socket(type):
proto = socket.IPPROTO_TCP
elif type == socket.SOCK_DGRAM:
elif _is_dgram_socket(type):
proto = socket.IPPROTO_UDP
else:
return None
Expand Down Expand Up @@ -777,7 +778,7 @@ async def create_connection(self, protocol_factory, host=None, port=None,
if sock is None:
raise ValueError(
'host and port was not specified and no sock specified')
if not _is_stream_socket(sock):
if not _is_stream_socket(sock.type):
# We allow AF_INET, AF_INET6, AF_UNIX as long as they
# are SOCK_STREAM.
# We support passing AF_UNIX sockets even though we have
Expand Down Expand Up @@ -827,7 +828,7 @@ async def create_datagram_endpoint(self, protocol_factory,
allow_broadcast=None, sock=None):
"""Create datagram connection."""
if sock is not None:
if not _is_dgram_socket(sock):
if not _is_dgram_socket(sock.type):
raise ValueError(
f'A UDP Socket was expected, got {sock!r}')
if (local_addr or remote_addr or
Expand Down Expand Up @@ -1043,7 +1044,7 @@ async def create_server(self, protocol_factory, host=None, port=None,
else:
if sock is None:
raise ValueError('Neither host/port nor sock were specified')
if not _is_stream_socket(sock):
if not _is_stream_socket(sock.type):
raise ValueError(f'A Stream Socket was expected, got {sock!r}')
sockets = [sock]

Expand All @@ -1066,7 +1067,7 @@ async def connect_accepted_socket(self, protocol_factory, sock,
This method is a coroutine. When completed, the coroutine
returns a (transport, protocol) pair.
"""
if not _is_stream_socket(sock):
if not _is_stream_socket(sock.type):
raise ValueError(f'A Stream Socket was expected, got {sock!r}')

transport, protocol = await self._create_connection_transport(
Expand Down
2 changes: 1 addition & 1 deletion Lib/asyncio/selector_events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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:

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

sock.proto == socket.IPPROTO_TCP):
sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1)
else:
Expand Down
4 changes: 2 additions & 2 deletions Lib/asyncio/unix_events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ async def create_unix_connection(self, protocol_factory, path=None, *,
if sock is None:
raise ValueError('no path and sock were specified')
if (sock.family != socket.AF_UNIX or
not base_events._is_stream_socket(sock)):
not base_events._is_stream_socket(sock.type)):
raise ValueError(
f'A UNIX Domain Stream Socket was expected, got {sock!r}')
sock.setblocking(False)
Expand Down Expand Up @@ -276,7 +276,7 @@ async def create_unix_server(self, protocol_factory, path=None, *,
'path was not specified, and no sock specified')

if (sock.family != socket.AF_UNIX or
not base_events._is_stream_socket(sock)):
not base_events._is_stream_socket(sock.type)):
raise ValueError(
f'A UNIX Domain Stream Socket was expected, got {sock!r}')

Expand Down
7 changes: 0 additions & 7 deletions Lib/test/test_asyncio/test_base_events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,13 +107,6 @@ def test_ipaddr_info(self):
self.assertIsNone(
base_events._ipaddr_info('::3%lo0', 1, INET6, STREAM, TCP))

if hasattr(socket, 'SOCK_NONBLOCK'):
self.assertEqual(
None,
base_events._ipaddr_info(
'1.2.3.4', 1, INET, STREAM | socket.SOCK_NONBLOCK, TCP))


def test_port_parameter_types(self):
# Test obscure kinds of arguments for "port".
INET = socket.AF_INET
Expand Down
27 changes: 27 additions & 0 deletions Lib/test/test_asyncio/test_selector_events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from asyncio.selector_events import _SelectorTransport
from asyncio.selector_events import _SelectorSocketTransport
from asyncio.selector_events import _SelectorDatagramTransport
from asyncio.selector_events import _set_nodelay
from test.test_asyncio import utils as test_utils


Expand Down Expand Up @@ -1463,5 +1464,31 @@ def test_fatal_error_connected(self, m_exc):
'Fatal error on transport\nprotocol:.*\ntransport:.*'),
exc_info=(ConnectionRefusedError, MOCK_ANY, MOCK_ANY))


class TestSelectorUtils(test_utils.TestCase):
def check_set_nodelay(self, sock):
opt = sock.getsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY)
self.assertFalse(opt)

_set_nodelay(sock)

opt = sock.getsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY)
self.assertTrue(opt)

@unittest.skipUnless(hasattr(socket, 'TCP_NODELAY'),
'need socket.TCP_NODELAY')
def test_set_nodelay(self):
sock = socket.socket(family=socket.AF_INET, type=socket.SOCK_STREAM,
proto=socket.IPPROTO_TCP)
with sock:
self.check_set_nodelay(sock)

sock = socket.socket(family=socket.AF_INET, type=socket.SOCK_STREAM,
proto=socket.IPPROTO_TCP)
with sock:
sock.setblocking(False)
self.check_set_nodelay(sock)


if __name__ == '__main__':
unittest.main()
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2017-11-02-11-57-41.bpo-27456.snzyTC.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Ensure TCP_NODELAY is set on Linux. Tests by Victor Stinner.