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

nkinnan · 2024-08-29T16:07:56Z

On Windows, sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_QUICKACK, 1) throws AttributeError: module 'socket' has no attribute 'TCP_QUICKACK'

TCP_QUICKACK already exists and has support on the Linux side, this change just adds support when running on Windows as well.

Issue: TCP_QUICKACK is available on Windows, but is only implemented by the python runtime for Linux #123476

On Windows, sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_QUICKACK, 1) throws AttributeError: module 'socket' has no attribute 'TCP_QUICKACK' TCP_QUICKACK already exists and has support on the Linux side, this change just adds support when running on Windows as well.

ghost · 2024-08-29T16:07:59Z

All commit authors signed the Contributor License Agreement.

bedevere-app · 2024-08-29T16:08:03Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

…vironment

Lib/test/test_socket.py

Misc/NEWS.d/next/Windows/2024-08-29-16-13-45.gh-issue-123476.m2DFS4.rst

Co-authored-by: Kirill Podoprigora <[email protected]>

nkinnan · 2024-08-29T17:39:56Z

Update: disregard this comment, I found a good way to add the unit test.

I had to remove the (new) unit tests for QUICKACK:

They passed locally on windows
They failed in the github CI workflow
I believe this is because on the github servers, QUICKACK is set to true by default as a system-wide setting, I have seen this on other Linux based servers. Therefore, when the test asserts that QUICKACK is false (before setting true and asserting that as well) it is already true causing test failure.
This PR is windows specific, the change was not even being exercised in the CI workflow.
The test cases for QUICKACK Linux support didn't exist in the first place when I added Windows support, so I'm not removing anything that was already there.
Behavior has been manually verified.

For all of those reasons I believe it is reasonable to omit the unit tests from the PR.

picnixz

This PR is windows specific, the change was not even being exercised in the CI workflow.

Actually, we have build bots for that so we can test it. So tests are required. You can make them conditioned on some information.

I'm surprised that you need a special casing for TCP_QUICKACK by the way and an additional structure field. Can't you just use get/setsockopt as with other constants?

Misc/NEWS.d/next/Windows/2024-08-29-16-13-45.gh-issue-123476.m2DFS4.rst

picnixz · 2024-08-30T14:28:26Z

Modules/socketmodule.c

+#ifdef MS_WINDOWS
+        if (optname == SIO_TCP_SET_ACK_FREQUENCY) {
+            int dummy;
+            res = WSAIoctl(s->sock_fd, SIO_TCP_SET_ACK_FREQUENCY, &flag,


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

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

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.

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

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.

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

Modules/socketmodule.c

if statement brackets Co-authored-by: Bénédikt Tran <[email protected]>

nkinnan · 2024-08-30T18:49:40Z

This PR is windows specific, the change was not even being exercised in the CI workflow.

Actually, we have build bots for that so we can test it. So tests are required. You can make them conditioned on some information.

edit: realized a better way, TC added,

I'll look for examples of how to do that in other tests so that I can re-add them. If you have any quick pointers that might help as I am unfamiliar with this codebase. I would note that QUICKACK isn't being tested at all right now, not even the existing support for Linux, but test cases are a good thing so I'll make the effort ;)

I'm surprised that you need a special casing for TCP_QUICKACK by the way and an additional structure field. Can't you just use get/setsockopt as with other constants?

Unfortunately not for this platform, please see the code review comment replies.

…2DFS4.rst Improve phrasing Co-authored-by: Bénédikt Tran <[email protected]>

…n filled in

Lib/test/test_asyncio/test_base_events.py

Lib/asyncio/base_events.py

Modules/socketmodule.c

zooba

Change overall looks fine to me (apart from the apparent dead code), but I can only assume that this is the right way to enable TCP_QUICKACK. If someone more knowledgeable can confirm there are no unintended tricks or incompatibilities (that is, it does on Windows precisely what code written for POSIX will expect it to do), then it seems fine.

Lib/asyncio/base_events.py

nkinnan · 2024-09-03T18:05:03Z

Change overall looks fine to me (apart from the apparent dead code),

edit: Realized what this meant after, will move that func.

but I can only assume that this is the right way to enable TCP_QUICKACK. If someone more knowledgeable can confirm there are no unintended tricks or incompatibilities (that is, it does on Windows precisely what code written for POSIX will expect it to do), then it seems fine.

Discussion between myself and picnixz on specific behavior: https://github.com/python/cpython/pull/123478/files#r1738786292

SIO_TCP_SET_ACK_FREQUENCY is not well documented on Windows but its behavior is confirmed, here is some discussion from folks at MS about adding this same support to the .NET runtime.

dotnet/runtime#798

triage: workaround exist through platform specific. Since there is no consistent cross-platform it seems like bad fit for general Socket API. We can revisit this in the future.

That "workaround exist" refers to this: dotnet/runtime#798 (comment)

Note that the value 12 passed for linux is the linux header defined TCP_QUICKACK value which was already implemented in cpython. Note that on Windows, IOControl is the .NET equivalent to WSAIoctl (what this PR calls).

And when they say "no consistent cross platform support" that might mean the lack of Mac support (fair enough) or the fact that the flag isn't persistent on Linux. Lack of Mac support: dotnet/runtime#798 (comment)

As to the lack of persistency on Linux, this flag was already exposed in cpython on Linux, my PR is only to add support for Windows which doesn't have that flaw. So that quirk of behavior on Linux is already there and "accounted for". It literally does the same thing, Linux just decides "maybe you changed your mind" and turs it off after every receive whereas windows respects your intent. Part of the reason they didn't add it to .NET is they didn't want to have to keep turning it back on when running on Linux if the user selected QUICKACK behavior. I suspect this "resetting flag" issue is also why many linux distros are forcing QUICKACK permanently on across the whole system nowadays so the flag doesn't even need to be set (over and over again) and because the alternative is unnecessary with modern networking systems and detrimental to latency sensitive applications.

zooba · 2024-09-03T19:36:17Z

Thanks, that .NET thread is convincing. Once you've moved out the asyncio changes, I'll merge.

Modules/socketmodule.c

formatting style oops Co-authored-by: Steve Dower <[email protected]>

vstinner · 2024-09-03T20:21:30Z

Maybe it makes sense to set TCP_QUICKACK flag by default on asyncio sockets, similar to what's done with TCP_NODELAY. But I would prefer to have a separated PR for that and have an asyncio expert to say if it makes sense or not.

nkinnan · 2024-09-03T21:52:28Z

Maybe it makes sense to set TCP_QUICKACK flag by default on asyncio sockets, similar to what's done with TCP_NODELAY. But I would prefer to have a separated PR for that and have an asyncio expert to say if it makes sense or not.

Agree on leaving it to an expert (my gut says yes) but if that ends up happening it may be worth looking into
net.inet.tcp.ack_strategy and net.inet.tcp.delayed_ack to add support for this flag on MacOS for consistency. I don't have access to an Apple platform to try it out though, and I believe those settings are system-wide on MacOS (I didn't investigate too deeply). Just a heads up.

nkinnan · 2024-09-03T21:59:25Z

Thanks, that .NET thread is convincing. Once you've moved out the asyncio changes, I'll merge.

Thank you. I believe I've moved them to the right place but am unfamiliar with cpython's test setup so please let me know if any further tweaks are required.

I want to quickly point out that I wasn't sure what version number to put into Doc/library/socket.rst so that may need changing before merge.

bedevere-bot · 2024-09-03T22:32:59Z

🤖 New build scheduled with the buildbot fleet by @zooba for commit 1bc9b7a 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

nkinnan · 2024-09-04T04:56:59Z

There was an unrelated failure in the CI workflow, but pulling main seems to have fixed it.

vstinner

LGTM. I just have a remark/question on the test.

Lib/test/test_socket.py

vstinner · 2024-09-05T20:59:59Z

Merged, thanks for your contribution.

nkinnan requested review from 1st1, asvetlov, gvanrossum, kumaraditya303 and willingc as code owners August 29, 2024 16:07

bedevere-app bot mentioned this pull request Aug 29, 2024

TCP_QUICKACK is available on Windows, but is only implemented by the python runtime for Linux #123476

Closed

bedevere-app bot added the awaiting review label Aug 29, 2024

blurb-it bot and others added 5 commits August 29, 2024 16:13

📜🤖 Added by blurb_it.

8d510a2

bugfix in previous commit

635e385

Merge branch 'main' of https://github.com/nkinnan/cpython into main

ac49caf

bugfix in previous commit

183a18e

remove test case since it works on windows but fails in the server en…

e96d9db

…vironment

Eclips4 reviewed Aug 29, 2024

View reviewed changes

Lib/test/test_socket.py Outdated Show resolved Hide resolved

Misc/NEWS.d/next/Windows/2024-08-29-16-13-45.gh-issue-123476.m2DFS4.rst Outdated Show resolved Hide resolved

Update Lib/test/test_socket.py

8147664

Co-authored-by: Kirill Podoprigora <[email protected]>

nkinnan added 5 commits August 29, 2024 10:43

modify news

02ffa52

Merge branch 'main' of https://github.com/nkinnan/cpython into main

4f92750

remove last bit of unit test straggler code

1ca08b7

cleanup

a0889a5

whitespace

f2e8cca

nkinnan mentioned this pull request Aug 29, 2024

Try to set TCP_QUICKACK where support for that option is available esphome/aioesphomeapi#932

Merged

picnixz reviewed Aug 30, 2024

View reviewed changes

Update Modules/socketmodule.c

a607fec

if statement brackets Co-authored-by: Bénédikt Tran <[email protected]>

nkinnan and others added 3 commits August 30, 2024 11:58

Update Misc/NEWS.d/next/Windows/2024-08-29-16-13-45.gh-issue-123476.m…

016a2c6

…2DFS4.rst Improve phrasing Co-authored-by: Bénédikt Tran <[email protected]>

Merge branch 'main' into main

d06cbe7

additional documentation in socket.rst -- needs python release versio…

ce8c1ab

…n filled in

vstinner reviewed Sep 3, 2024

View reviewed changes

Lib/test/test_asyncio/test_base_events.py Outdated Show resolved Hide resolved

Lib/test/test_asyncio/test_base_events.py Outdated Show resolved Hide resolved

Lib/asyncio/base_events.py Outdated Show resolved Hide resolved

Modules/socketmodule.c Show resolved Hide resolved

zooba reviewed Sep 3, 2024

View reviewed changes

Lib/asyncio/base_events.py Outdated Show resolved Hide resolved

zooba reviewed Sep 3, 2024

View reviewed changes

Modules/socketmodule.c Outdated Show resolved Hide resolved

Update Modules/socketmodule.c

d0527e3

formatting style oops Co-authored-by: Steve Dower <[email protected]>

nkinnan and others added 6 commits September 3, 2024 14:19

Merge branch 'python:main' into main

9ad2430

move test case, remove test support code from asyncio

fe453bc

typo

ec053ad

revert change to different test case

8c06855

whitespace

d7cb426

whitespace

1bc9b7a

zooba added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 3, 2024

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 3, 2024

bdraco mentioned this pull request Sep 3, 2024

subscribe_state excessive buffering esphome/aioesphomeapi#815

Closed

Merge branch 'main' into main

9cadf5b

vstinner approved these changes Sep 4, 2024

View reviewed changes

Lib/test/test_socket.py Outdated Show resolved Hide resolved

bedevere-app bot added awaiting merge and removed awaiting review labels Sep 4, 2024

nkinnan added 3 commits September 5, 2024 13:24

test blocking path only for now

e9a39b6

Merge branch 'main' of https://github.com/nkinnan/cpython into main

4b0f5db

whitespace

559bbcb

vstinner merged commit b5aa271 into python:main Sep 5, 2024
37 checks passed

bedevere-app bot removed the awaiting merge label Sep 5, 2024

Uh oh!

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

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

Uh oh!

Conversation

nkinnan commented Aug 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Aug 29, 2024 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-app bot commented Aug 29, 2024

Uh oh!

Uh oh!

Uh oh!

nkinnan commented Aug 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

picnixz Aug 30, 2024 • 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!

nkinnan Aug 30, 2024

Choose a reason for hiding this comment

Uh oh!

nkinnan Aug 30, 2024 • 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!

picnixz Aug 31, 2024

Choose a reason for hiding this comment

Uh oh!

nkinnan Sep 1, 2024 • 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!

nkinnan Sep 3, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nkinnan commented Aug 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zooba left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nkinnan commented Sep 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zooba commented Sep 3, 2024

Uh oh!

Uh oh!

vstinner commented Sep 3, 2024

Uh oh!

nkinnan commented Sep 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nkinnan commented Sep 3, 2024

Uh oh!

bedevere-bot commented Sep 3, 2024

Uh oh!

nkinnan commented Sep 4, 2024

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vstinner commented Sep 5, 2024

Uh oh!

nkinnan commented Aug 29, 2024 •

edited

Loading

ghost commented Aug 29, 2024 •

edited by ghost

Loading

nkinnan commented Aug 29, 2024 •

edited

Loading

picnixz Aug 30, 2024 •

edited

Loading

nkinnan Aug 30, 2024 •

edited

Loading

nkinnan Sep 1, 2024 •

edited

Loading

nkinnan commented Aug 30, 2024 •

edited

Loading

nkinnan commented Sep 3, 2024 •

edited

Loading

nkinnan commented Sep 3, 2024 •

edited

Loading