Skip to content

gh-77589: Add unix domain socket for Windows #137420

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

Jump to bottom
Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

gh-77589: Add unix domain socket for Windows #137420

wants to merge 14 commits into from
+95 −7

Conversation

aisk
Copy link
Member

@aisk aisk commented Aug 5, 2025 •
edited by bedevere-app bot
Loading

Didn't add asyncio support in this PR to avoid too many code changes and keep the review process simpler.

@bedevere-app bedevere-app bot mentioned this pull request Aug 5, 2025
Open
aisk
aisk commented Aug 5, 2025
View reviewed changes
@aisk aisk changed the title gh-77589: Windows unix socket gh-77589: Add unix domain socket for Windows Aug 5, 2025
@aisk aisk requested a review from gpshead as a code owner August 5, 2025 16:12
serhiy-storchaka
serhiy-storchaka reviewed Aug 5, 2025
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Instead of skipping some tests, would not be worth to test a Windows specific behavior?

Some tests can currently be skipped on Windows, but they may work after adding support of AF_UNIX. Please check all currently skipped tests for socket and multiprocessing.

@serhiy-storchaka serhiy-storchaka requested a review from zooba August 5, 2025 18:54
@serhiy-storchaka
Copy link
Member

It seems that the previous attempt did have more changes in the socket and socketserver modules. Are they no longer relevant?

@aisk
Copy link
Member Author

aisk commented Aug 6, 2025

It seems that the previous attempt did have more changes in the socket and socketserver modules. Are they no longer relevant?

The previous attempt modified the socketpair function in the socket module to use AF_UNIX by default on Windows, so it would introduce more changes. I'd like to keep the current behavior to reduce code changes in one PR and compatibility break risk, and we can change it in the future.

The change in socketserver in the previous attempt is mainly because in asyncio's test case, it tries to create an HTTP server over unix domain sockets. On Windows, currently AF_UNIX doesn't support reuse_address, so there needs to be this modification. I changed the test to disable reuse_address on Windows with AF_UNIX. But I think the previous attempt is the right way because users can encounter this issue too. Will update this later today. Thank you for the review!

@aisk aisk requested a review from vsajip as a code owner August 7, 2025 15:54
@aisk aisk marked this pull request as draft August 9, 2025 15:21
@aisk aisk marked this pull request as ready for review August 11, 2025 15:40
@aisk
Copy link
Member Author

aisk commented Aug 11, 2025 •
edited
Loading

Instead of skipping some tests, would not be worth to test a Windows specific behavior?

Some tests can currently be skipped on Windows, but they may work after adding support of AF_UNIX. Please check all currently skipped tests for socket and multiprocessing.

Hi, I updated the test to test Windows-specific behaviors, and checked current tests which can be enabled after we added AF_UNIX, but not finding one. Most tests are skipped by some stuff like 'skipUnless(hasattr(socket, "AF_UNIX))'.

There is a noticeable remark for the reviewer: The PC\pyconfig.h is maintained by hand, so we don't have a configure step to detect if current build environment have AF_UNIX support (have afunix.h). So I introduced __has_include which haven't seen in current repository. Please let me know if there are better way to do this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments

@barneygale barneygale Awaiting requested review from barneygale barneygale is a code owner

@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner

@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner

@kumaraditya303 kumaraditya303 Awaiting requested review from kumaraditya303 kumaraditya303 is a code owner

@willingc willingc Awaiting requested review from willingc willingc is a code owner

@gpshead gpshead Awaiting requested review from gpshead gpshead is a code owner

@zooba zooba Awaiting requested review from zooba

@vsajip vsajip Awaiting requested review from vsajip vsajip is a code owner

Assignees
No one assigned
Labels
awaiting review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aisk @serhiy-storchaka