Skip to content
This repository was archived by the owner on Nov 23, 2017. It is now read-only.

Set TCP_NODELAY on TCP transports by default #373

Merged
merged 1 commit into from
Sep 12, 2016
Merged

Set TCP_NODELAY on TCP transports by default #373

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 16 additions & 0 deletions asyncio/selector_events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,17 @@ def _test_selector_event(selector, fd, event):
return bool(key.events & event)


if hasattr(socket, 'TCP_NODELAY'):
Copy link

Choose a reason for hiding this comment

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

Is there case when this option is not available ? If yes, please add comment to source code. If no, just remove that check.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, some old linux kernels don't implement this. And some other OSes might not too.

How would a comment add any kind of clarity here?

Copy link

Choose a reason for hiding this comment

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

  1. Seems, you mean TCP_CORK, which was intorduced near Linux 2.4. AFAIK, TCP_NODELAY was implemented much earlier
  2. Which OSes really still not implements that?

Don't misunderstand me. I'm asking that in order to prevent code bloating and helping the code to be simple to read and maintain.

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems, you mean TCP_CORK, which was intorduced near Linux 2.4. AFAIK, TCP_NODELAY was implemented much earlier

Maybe you're right. Anyways, from socketmodule.c:

#ifdef  TCP_NODELAY
    PyModule_AddIntMacro(m, TCP_NODELAY);
#endif

CPython doesn't make an assumption that it's always available, so shouldn't we.

def _set_nodelay(sock):
if (sock.family in {socket.AF_INET, socket.AF_INET6} and
sock.type == socket.SOCK_STREAM and
sock.proto == socket.IPPROTO_TCP):
sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1)
else:
def _set_nodelay(sock):
pass


class BaseSelectorEventLoop(base_events.BaseEventLoop):
"""Selector event loop.

Expand Down Expand Up @@ -640,6 +651,11 @@ def __init__(self, loop, sock, protocol, waiter=None,
self._eof = False
self._paused = False

# Disable the Nagle algorithm -- small writes will be
# sent without waiting for the TCP ACK. This generally
Copy link

@socketpair socketpair Feb 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

No, TCP_NODELAY is not connected with ACK. Actually, when nodelay is NOT set, kernel will delay sending small TCP packet, waiting for possible additional data to form one big packet. Since we buffer data in our own way, we already concatenate sequence of small writes to big one. So, kernel never sees sequence of small writes and therefore it is not needed to wait for data to concatenate in kernel.

Copy link
Member

Choose a reason for hiding this comment

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

@socketpair see https://en.wikipedia.org/wiki/Nagle%27s_algorithm
nagle waits ACK or enough data in send buffer. That's why delayed ACK + nagle cause trouble.

Copy link

Choose a reason for hiding this comment

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

image

You have said only part of algorightm (that is connected with ACK). Much more important thing is what I tried to describe (that is connected with writes < MSS).

Copy link

Choose a reason for hiding this comment

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

P.S. I'm not right too. It's best not to describe Nagle's algorithm in the comment.

# decreases the latency (in some cases significantly.)
_set_nodelay(self._sock)
Copy link

Choose a reason for hiding this comment

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

This will work only for TCP sockets, and not for UNIX stream sockets...

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. We need to add functional unittests for TCP/UNIX transports to catch these sort of errors...

Copy link

Choose a reason for hiding this comment

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

Also, accepted sockets should be set as NODELAY too. Please check that this is true.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright. I'll prepare a new patch some time later. Thanks for reviewing this iteration!

Copy link

Choose a reason for hiding this comment

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

See also:
aio-libs/aiohttp#664

Copy link

@socketpair socketpair Jul 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Good catch. We need to add functional unittests for TCP/UNIX transports to catch these sort of errors...

Tests probably will show nothing, since error is swallowed. Or, tests should test that TCP_NODELAY has been actually set.


self._loop.call_soon(self._protocol.connection_made, self)
# only start reading when connection_made() has been called
self._loop.call_soon(self._loop.add_reader,
Expand Down