Skip to content

gh-123476: Add support for existing TCP_QUICKACK socket setting to Windows #123478

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

Merged
merged 36 commits into from
Sep 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
c030ee4
Add support for existing TCP_QUICKACK socket setting to Windows
nkinnan Aug 29, 2024
8d510a2
📜🤖 Added by blurb_it.
blurb-it[bot] Aug 29, 2024
635e385
bugfix in previous commit
nkinnan Aug 29, 2024
ac49caf
Merge branch 'main' of https://github.com/nkinnan/cpython into main
nkinnan Aug 29, 2024
183a18e
bugfix in previous commit
nkinnan Aug 29, 2024
e96d9db
remove test case since it works on windows but fails in the server en…
nkinnan Aug 29, 2024
8147664
Update Lib/test/test_socket.py
nkinnan Aug 29, 2024
02ffa52
modify news
nkinnan Aug 29, 2024
4f92750
Merge branch 'main' of https://github.com/nkinnan/cpython into main
nkinnan Aug 29, 2024
1ca08b7
remove last bit of unit test straggler code
nkinnan Aug 29, 2024
a0889a5
cleanup
nkinnan Aug 29, 2024
f2e8cca
whitespace
nkinnan Aug 29, 2024
a607fec
Update Modules/socketmodule.c
nkinnan Aug 30, 2024
016a2c6
Update Misc/NEWS.d/next/Windows/2024-08-29-16-13-45.gh-issue-123476.m…
nkinnan Aug 30, 2024
d06cbe7
Merge branch 'main' into main
nkinnan Aug 30, 2024
ce8c1ab
additional documentation in socket.rst -- needs python release versio…
nkinnan Aug 30, 2024
5c887b6
Merge branch 'main' of https://github.com/nkinnan/cpython into main
nkinnan Aug 30, 2024
8185776
unit test that should work even if TCP_QUICKACK is already turned on …
nkinnan Aug 30, 2024
7616b21
lint
nkinnan Aug 30, 2024
4b49a2e
Update socket.rst
nkinnan Aug 30, 2024
94f9087
Update Modules/socketmodule.h
nkinnan Aug 31, 2024
ebd8a9d
code review comments addressed
nkinnan Aug 31, 2024
a478e5e
lint
nkinnan Aug 31, 2024
776d231
revert CR change due to CR comment updated
nkinnan Aug 31, 2024
23c5522
Merge branch 'main' into main
nkinnan Aug 31, 2024
d0527e3
Update Modules/socketmodule.c
nkinnan Sep 3, 2024
9ad2430
Merge branch 'python:main' into main
nkinnan Sep 3, 2024
fe453bc
move test case, remove test support code from asyncio
nkinnan Sep 3, 2024
ec053ad
typo
nkinnan Sep 3, 2024
8c06855
revert change to different test case
nkinnan Sep 3, 2024
d7cb426
whitespace
nkinnan Sep 3, 2024
1bc9b7a
whitespace
nkinnan Sep 3, 2024
9cadf5b
Merge branch 'main' into main
nkinnan Sep 4, 2024
e9a39b6
test blocking path only for now
nkinnan Sep 5, 2024
4b0f5db
Merge branch 'main' of https://github.com/nkinnan/cpython into main
nkinnan Sep 5, 2024
559bbcb
whitespace
nkinnan Sep 5, 2024
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
12 changes: 8 additions & 4 deletions Doc/library/socket.rst
Original file line number Diff line number Diff line change
Expand Up @@ -413,14 +413,14 @@ Constants
``TCP_USER_TIMEOUT``, ``TCP_CONGESTION`` were added.

.. versionchanged:: 3.6.5
On Windows, ``TCP_FASTOPEN``, ``TCP_KEEPCNT`` appear if run-time Windows
supports.
Added support for ``TCP_FASTOPEN``, ``TCP_KEEPCNT`` on Windows platforms
when available.

.. versionchanged:: 3.7
``TCP_NOTSENT_LOWAT`` was added.

On Windows, ``TCP_KEEPIDLE``, ``TCP_KEEPINTVL`` appear if run-time Windows
supports.
Added support for ``TCP_KEEPIDLE``, ``TCP_KEEPINTVL`` on Windows platforms
when available.

.. versionchanged:: 3.10
``IP_RECVTOS`` was added.
Expand Down Expand Up @@ -454,6 +454,10 @@ Constants
Added missing ``IP_RECVERR``, ``IP_RECVTTL``, and ``IP_RECVORIGDSTADDR``
on Linux.

.. versionchanged:: 3.14
Added support for ``TCP_QUICKACK`` on Windows platforms when available.


.. data:: AF_CAN
PF_CAN
SOL_CAN_*
Expand Down
26 changes: 25 additions & 1 deletion Lib/test/test_socket.py
Original file line number Diff line number Diff line change
Expand Up @@ -6826,6 +6826,28 @@ class TestMacOSTCPFlags(unittest.TestCase):
def test_tcp_keepalive(self):
self.assertTrue(socket.TCP_KEEPALIVE)

@unittest.skipUnless(hasattr(socket, 'TCP_QUICKACK'), 'need socket.TCP_QUICKACK')
class TestQuickackFlag(unittest.TestCase):
def check_set_quickack(self, sock):
# quickack already true by default on some OS distributions
opt = sock.getsockopt(socket.IPPROTO_TCP, socket.TCP_QUICKACK)
if opt:
sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_QUICKACK, 0)

opt = sock.getsockopt(socket.IPPROTO_TCP, socket.TCP_QUICKACK)
self.assertFalse(opt)

sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_QUICKACK, 1)

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

def test_set_quickack(self):
sock = socket.socket(family=socket.AF_INET, type=socket.SOCK_STREAM,
proto=socket.IPPROTO_TCP)
with sock:
self.check_set_quickack(sock)


@unittest.skipUnless(sys.platform.startswith("win"), "requires Windows")
class TestMSWindowsTCPFlags(unittest.TestCase):
Expand All @@ -6839,7 +6861,9 @@ class TestMSWindowsTCPFlags(unittest.TestCase):
'TCP_KEEPCNT',
# available starting with Windows 10 1709
'TCP_KEEPIDLE',
'TCP_KEEPINTVL'
'TCP_KEEPINTVL',
# available starting with Windows 7 / Server 2008 R2
'TCP_QUICKACK',
}

def test_new_tcp_flags(self):
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add support for ``socket.TCP_QUICKACK`` on Windows platforms.
22 changes: 22 additions & 0 deletions Modules/socketmodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -3166,6 +3166,17 @@ sock_setsockopt(PySocketSockObject *s, PyObject *args)
/* setsockopt(level, opt, flag) */
if (PyArg_ParseTuple(args, "iii:setsockopt",
&level, &optname, &flag)) {
#ifdef MS_WINDOWS
if (optname == SIO_TCP_SET_ACK_FREQUENCY) {
int dummy;
res = WSAIoctl(s->sock_fd, SIO_TCP_SET_ACK_FREQUENCY, &flag,
Copy link
Member

@picnixz picnixz Aug 30, 2024

Choose a reason for hiding this comment

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

Should I understand that this constant cannot be used by setsockopt and that you need this special path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, on windows platform, QUICKACK can only be set through WSAIoctl() and is not supported through setsockopt()

Copy link
Contributor Author

@nkinnan nkinnan Aug 30, 2024

Choose a reason for hiding this comment

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

Another quick note to help with review:

On Windows, the only values accepted for SIO_TCP_SET_ACK_FREQUENCY is 0 and 1, with 1 meaning "ack every 1 packets" aka TCP_QUICKACK is on, and 0 meaning "use default value" aka TCP_QUICKACK is off. So there's no need to translate the value set by the user. 1 is on, 0 is off, just like TCP_NODELAY and other socket flags. A convenient coincidence.

Copy link
Member

Choose a reason for hiding this comment

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

I see. I un-resolved this conversation so that future reviewers can see it (otherwise GH hides it).

Copy link
Contributor Author

@nkinnan nkinnan Sep 1, 2024

Choose a reason for hiding this comment

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

I'll throw another note in here for future reviewers then: there is no equivalent SIO_TCP_GET_ACK_FREQUENCY, so the value must be saved in the socket object. On Windows SIO_TCP_SET_ACK_FREQUENCY is always 0 for a new socket, and the socket->quickack variable is always initialized to 0, so the response value from socket.getsockopt for TCP_QUICKACK will always be correct on Windows even if not previously set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

another explanation of behavior for future reviewers, please see this comment: #123478 (comment)

sizeof(flag), NULL, 0, &dummy, NULL, NULL);
if (res >= 0) {
s->quickack = flag;
}
goto done;
}
#endif
res = setsockopt(s->sock_fd, level, optname,
(char*)&flag, sizeof flag);
goto done;
Expand Down Expand Up @@ -3251,6 +3262,11 @@ sock_getsockopt(PySocketSockObject *s, PyObject *args)
return s->errorhandler();
return PyLong_FromUnsignedLong(vflag);
}
#endif
#ifdef MS_WINDOWS
if (optname == SIO_TCP_SET_ACK_FREQUENCY) {
return PyLong_FromLong(s->quickack);
}
#endif
flagsize = sizeof flag;
res = getsockopt(s->sock_fd, level, optname,
Expand Down Expand Up @@ -5316,6 +5332,9 @@ sock_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
((PySocketSockObject *)new)->sock_fd = INVALID_SOCKET;
((PySocketSockObject *)new)->sock_timeout = _PyTime_FromSeconds(-1);
((PySocketSockObject *)new)->errorhandler = &set_error;
#ifdef MS_WINDOWS
((PySocketSockObject *)new)->quickack = 0;
#endif
}
return new;
}
Expand Down Expand Up @@ -8616,6 +8635,9 @@ socket_exec(PyObject *m)
#ifdef TCP_CONNECTION_INFO
ADD_INT_MACRO(m, TCP_CONNECTION_INFO);
#endif
#ifdef SIO_TCP_SET_ACK_FREQUENCY
#define TCP_QUICKACK SIO_TCP_SET_ACK_FREQUENCY
#endif
#ifdef TCP_QUICKACK
ADD_INT_MACRO(m, TCP_QUICKACK);
#endif
Expand Down
3 changes: 3 additions & 0 deletions Modules/socketmodule.h
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,9 @@ typedef struct {
PyTime_t sock_timeout; /* Operation timeout in seconds;
0.0 means non-blocking */
struct _socket_state *state;
#ifdef MS_WINDOWS
int quickack;
#endif
} PySocketSockObject;

/* --- C API ----------------------------------------------------*/
Expand Down
Loading