Skip to content
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

Implement AF_UNIX sockets on Windows #115

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

slonopotamus
Copy link

See moby/moby#36442


This is a rebase of #98.

@AkihiroSuda
Copy link
Collaborator

CI is failing

@slonopotamus slonopotamus force-pushed the AF_UNIX-on-windows branch 4 times, most recently from 2b1d491 to 0de2531 Compare May 14, 2024 07:04
@slonopotamus slonopotamus marked this pull request as draft May 14, 2024 07:09
@thaJeztah
Copy link
Member

thaJeztah commented May 14, 2024

Gave CI a kick after the last push; failure is in TestUnixSocketWithOpts, but also curious if TestNewUnixSocket needs a change in constraints (currently gated by "must be root", which may need a slightly different check on Windows likely)

=== RUN   TestNewUnixSocket
    unix_socket_test.go:39: requires root
--- SKIP: TestNewUnixSocket (0.00s)
=== RUN   TestUnixSocketWithOpts
    unix_socket_test_windows.go:11: The process cannot access the file because it is being used by another process.

Also look like staticcheck is complaining because on Windows it's a stub, which, well, is the intent here 🤔

Screenshot 2024-05-14 at 12 50 35

I guess a //nolint:staticcheck // Ignore SA4017, which is a stub on Windows, and has no side-effects could work

Alternatively, we could extract the whole block and have a separate Windows and Linux implementation / wrapper for l, err := net.Listen("unix", path) (on Windows skipping the umask)

That whole umask is hairy in either case 😬

@slonopotamus
Copy link
Author

I moved PR to draft until i figure out the test failure.

@slonopotamus
Copy link
Author

That whole umask is hairy in either case

Yes, feels like a race condition waiting to be exploited.

@slonopotamus
Copy link
Author

Okay, I think I'm done here.

Alternatively, we could extract the whole block and have a separate Windows and Linux implementation / wrapper

Yep, I went this route.

@slonopotamus slonopotamus marked this pull request as ready for review May 14, 2024 13:55
Before this change, CI would only trigger on commits to master branch.
When contributor creates a branch that would later become a PR, they do not want to use master branch.
Instead, they want to use feature-branch.
But before this commit, CI would not run in non-master branch at all, so contributor is unable to test their changes before creating a PR to upstream repository.

Signed-off-by: Marat Radchenko <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants