Adds support for UnixStream and UnixListener on Windows #1610

sullivan-sean · 2022-08-16T18:53:11Z

Uses mios existing AFD approach for TCP/UDP to support AF_UNIX sockets on Windows. See #1609 for context

sullivan-sean

Some of the doc tests are currently failing because examples for UnixStream are unix only. Is there an easy way to write cross-platform examples for docs to avoid this?

tests/unix_stream.rs

Cargo.toml

tests/unix_stream.rs

Thomasdezeeuw · 2022-08-17T19:05:36Z

@sullivan-sean also see rust-lang/socket2#249.

sullivan-sean · 2022-08-18T22:41:57Z

I removed the dependency on tempfile and fixed the tests.

I think the current implementation for Windows actually confuses the notion of edge triggered events with oneshot notifications i.e. EPOLLET vs EPOLLONESHOT in epoll and so it was not correctly re-registering for events after successful io.

@sullivan-sean also see rust-lang/socket2#249.

Would you prefer I introduce a dependency on this library for the socket operations? Looks like it would be pretty easy to use this instead of much of the socket code I've added

sullivan-sean · 2022-08-18T22:53:02Z

tests/unix_listener.rs

@@ -187,6 +190,57 @@ where
    handle.join().unwrap();
 }

+#[test]
+fn unix_listener_multiple_accepts() {


Current implementation on Windows fails this test. Subsequent accept calls after the first to the same listener hang because accept is not considered an I/O operation and won't send event when ready.

I've fixed this test by wrapping accept in do_io in net/uds/listener.rs in the same way it is wrapped in net/tcp/listener.rs

Thomasdezeeuw · 2022-08-19T07:50:11Z

I removed the dependency on tempfile and fixed the tests.

👍

I think the current implementation for Windows actually confuses the notion of edge triggered events with oneshot notifications i.e. EPOLLET vs EPOLLONESHOT in epoll and so it was not correctly re-registering for events after successful io.

That's an issue we should fix separately from adding Unix sockets for Windows. Can you open an issue for this?

@sullivan-sean also see rust-lang/socket2#249.

Would you prefer I introduce a dependency on this library for the socket operations? Looks like it would be pretty easy to use this instead of much of the socket code I've added

No. We decided not to add any dependencies and focus on a mostly std based API, i.e. all the net types in there but non-blocking.

Cargo.toml

sullivan-sean · 2022-08-19T10:51:45Z

That's an issue we should fix separately from adding Unix sockets for Windows. Can you open an issue for this?

Issue created here #1612 - I can make a separate PR from sullivan-sean@c725b03 which contains just the fix for that, if this is indeed an issue

sullivan-sean · 2022-08-21T00:01:46Z

I've removed the changes in draining behavior, which requires more discussion in #1611 and fixed the tests instead by adding a would block assertion in compliance with the current drain behavior on windows documented here.

All tests are passing and a naive windows implementation in tokio (just copy pasting the unix implementation https://github.com/sullivan-sean/tokio) seems to pass all of the unix stream + listener tests there as well

sullivan-sean · 2022-08-26T09:11:24Z

I think that was superseded by rust-lang/rfcs#2523.

👍

True, but it's only 1k lines in a single file: https://github.com/tokio-rs/mio/blob/master/src/sys/windows/named_pipe.rs. This pr is larger.

Still not a single file, but after removing unused functions this PR is significantly smaller. There are also a non-trivial number of changes I had to make to pass lint checks that are unrelated to the actual content of this PR, e.g. all of the changes in src/net/tcp/stream.rs, src/sys/unix/pipe.rs, src/sys/unix/uds/mod.rs and a few other files.

I imagine that the std implementation, whenever it arrives, would be a pretty straightforward drop-in given that the internal UnixStream and UnixListener structs in this PR are built to mirror the std::os::unix::net equivalents almost exactly.

Hopefully, that's why I'm even considering this, but it would push back our v1 or we need to add an unstable feature.

If you would prefer to not add this to mio yet it would be really nice to expose a way to patch an event source like this into mio/tokio as an end developer (i.e. given that PollEvented is hidden in tokio it is quite hard to create a compatible source if it is not supported by mio/tokio out of the box).

That's basically #880, but TL;DR that's not really possible.

If it doesn't make sense to add this to mio now, pre-std support for UDS on Windows, what would you advise for users who would like to use mio/tokio with UDS in a cross-platform way? Maintain a personal fork? or maybe turn this into a more up-to-date https://github.com/Azure/mio-uds-windows?

Thomasdezeeuw

Start of the review. I also start the CI, didn't realise it didn't start.

src/sys/windows/mod.rs

src/sys/windows/stdnet/mod.rs

tests/close_on_drop.rs

tests/unix_listener.rs

Thomasdezeeuw · 2022-08-26T15:27:50Z

tests/unix_listener.rs


+#[cfg(windows)]
+use mio::net;


I think we need this because we need blocking version of the UDS types right? If so, let's put that in a separate module with that clearly documented.

oh I interpreted your comment here to mean we don't want to expose the blocking std::os::unix::net Windows equivalent types outside of the crate.

If you'd like me to expose these for use in tests, where would be the best place for them to live?

Thomasdezeeuw · 2022-08-26T15:28:52Z

tests/unix_stream.rs

@@ -77,6 +82,7 @@ fn unix_stream_connect() {
    handle.join().unwrap();
 }

+#[cfg(unix)]


The code has cfg(windows), but here you're making it Unix only?

Currently I was not exposing the internal blocking UDS types for windows, so there is no from_std function on UnixStream in Windows, and this test wouldn't really make sense on windows.

Where does this code have cfg(windows)?

Thomasdezeeuw · 2022-08-26T15:29:27Z

tests/unix_stream.rs

@@ -445,6 +457,8 @@ where

    assert!(stream.take_error().unwrap().is_none());

+    assert_would_block(stream.read(&mut buf));


Why is this needed? Ideally the test are unchanged as the behaviour is already dependent on in existing code.

We need to make a call that would block to reset the interests before trying to write per https://docs.rs/mio/latest/mio/struct.Poll.html#draining-readiness. I believe right now this test does not fully follow this documentation.

src/sys/windows/stdnet/mod.rs

sullivan-sean · 2022-09-08T16:33:31Z

@Thomasdezeeuw any updates on this? Is this still something I should be working on and if so, should I be exposing the Windows blocking types per #1610 (comment)?

Thomasdezeeuw · 2022-09-08T19:22:44Z

@Thomasdezeeuw any updates on this? Is this still something I should be working on and if so, should I be exposing the Windows blocking types per #1610 (comment)?

I just haven't had the time to review this properly. Perhaps I'll have some this weekend.

Thomasdezeeuw

Overall this is looking ok, not good, but ok. There are some things in this pr which aren't going to happen. For example spawning threads, that's a 100% blocker of this pr. But overall I'm willing to accept the new types for Windows, even though I prefer to have them in the standard library first so we can match the API, but I release that will take a while.

Some more questions:

Can remove all code changes not related to UDS. If that means the CI fails we'll fix that in another pr as this one is large enough as it is.
I assume UnixDatagrams are not supported?
What is path forward regarding:
- Getting these types into the standard library?
- Using SocketAddr from the standard library (see Use std::os::unix::net::SocketAddr instead of mio::net::SocketAddr #1527).
I'm too well versed in Windows' inheritance, do we need any no inheritances after calls to accept?
All cfg(...) need a #[cfg_attr(docsrs, doc(cfg(...))], I pointed out some, might have missed some others.
We'll probably want to wrap SocketAddr in a type in mio::net to keep the docs and methods in sync. It already has different methods and trait implementation in this pr.

I didn't have time to review the tests, but ideally very little to nothing changes with them as that means that existing code doesn't have to change to support Windows UDS.

src/sys/windows/stdnet/addr.rs

src/sys/windows/mod.rs

src/net/uds/listener.rs

src/net/uds/mod.rs

src/net/uds/listener.rs

src/sys/windows/stdnet/listener.rs

src/sys/windows/stdnet/stream.rs

sullivan-sean · 2022-09-13T00:39:57Z

Can remove all code changes not related to UDS. If that means the CI fails we'll fix that in another pr as this one is large enough as it is.

👍

I assume UnixDatagrams are not supported?

Correct, the datagram socket type is not supported by Windows implementation of Unix sockets (https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/#unsupportedunavailable)

What is path forward regarding:

Getting these types into the standard library?

Using SocketAddr from the standard library (see Use std::os::unix::net::SocketAddr instead of mio::net::SocketAddr #1527).

Looks like there's some recent movement on rust-lang/rust#56533. I'm not familiar with how/if we could go about adding this to the standard library, and if so what the route would be. But I am happy to push for this with guidance on the approach. I imagine a SocketAddr type for Windows could also be added to std when the other uds types are.

I'm too well versed in Windows' inheritance, do we need any no inheritances after calls to accept?

Seems like sockets returned from accept have the same properties as the listener socket they are called on - so if the listener has WSA_FLAG_NO_HANDLE_INHERIT set this is handled automatically. I will remove the explicit inheritance calls after accept. Then if somebody creates a listener with from_raw_socket, the streams returned from calling accept on that listener will have the same flag as the original socket, which I imagine is desired behavior.

We'll probably want to wrap SocketAddr in a type in mio::net to keep the docs and methods in sync. It already has different methods and trait implementation in this pr.

I'll work on adding this

I didn't have time to review the tests, but ideally very little to nothing changes with them as that means that existing code doesn't have to change to support Windows UDS.

Thanks for the very thorough review of everything else, I know it's a big PR and appreciate your patience! I've minimized test changes as much as possible while accounting for the lack of blocking socket types in std for Windows.

Thomasdezeeuw · 2022-09-14T14:10:06Z

Looks like there's some recent movement on rust-lang/rust#56533. I'm not familiar with how/if we could go about adding this to the standard library, and if so what the route would be. But I am happy to push for this with guidance on the approach. I imagine a SocketAddr type for Windows could also be added to std when the other uds types are.

You can start with a small RFC or maybe ask it on the Rust Zulip chat. Then just implement the types in std::os::windows and later merge them with Unix, maybe under std::net. But the libs team can help with that.

I'm too well versed in Windows' inheritance, do we need any no inheritances after calls to accept?

Seems like sockets returned from accept have the same properties as the listener socket they are called on - so if the listener has WSA_FLAG_NO_HANDLE_INHERIT set this is handled automatically. I will remove the explicit inheritance calls after accept. Then if somebody creates a listener with from_raw_socket, the streams returned from calling accept on that listener will have the same flag as the original socket, which I imagine is desired behavior.

👍 can you add a comment saying as much.

I didn't have time to review the tests, but ideally very little to nothing changes with them as that means that existing code doesn't have to change to support Windows UDS.

Thanks for the very thorough review of everything else, I know it's a big PR and appreciate your patience! I've minimized test changes as much as possible while accounting for the lack of blocking socket types in std for Windows.

👍

Noah-Kennedy · 2022-10-03T03:43:21Z

Where are we with this?

sullivan-sean · 2022-10-03T08:15:50Z

I've been away from my Windows machine and so haven't had a chance to fix the failing docs test. I will add the comment about WSA_FLAG_NO_HANDLE_INHERIT and try to fix the doc test tomorrow.

I posted in the rust-lang Zulip about adding Window UDS to std and the libs-api team seemed at least open to the idea on the condition that the failure mode on older platforms without UDS support is that "call[ing] the relevant functions...give[s] an error" which is the current behavior here for WinSock calls in general.

lumattr · 2022-11-22T16:43:10Z

How is this one doing? Are we waiting for the rust changes of does this just need a re-review?

Thomasdezeeuw · 2022-11-22T17:35:38Z

How is this one doing? Are we waiting for the rust changes of does this just need a re-review?

I think this is pretty close, we need to resolve some small things and need to make sure that the test behave the same on Unix and Windows (which is harder then it sounds).

KolbyML · 2023-04-14T17:05:07Z

Hi @sullivan-sean I was wondering if you are going to finish this? If something came up and your are too busy too I would be happy to take it off your hands :)

sullivan-sean · 2023-08-18T20:55:56Z

Closed in favor of #1667. Thanks @KolbyML for cleaning this up :)

simonsan · 2024-01-21T09:45:09Z

@sullivan-sean @KolbyML Both #1610 and #1667 are closed now. Do you think you can work together on this?

sullivan-sean added 7 commits August 15, 2022 06:17

add first pass implementation of windows uds

7f69b97

modify src/net for windows compatibility

bec570b

fix tests

63e50c3

add docs back in

3591238

cleanup

9015ca2

remove log statements

7265833

clean up selector

3884bb5

sullivan-sean commented Aug 16, 2022

View reviewed changes

tests/unix_stream.rs Outdated Show resolved Hide resolved

sullivan-sean mentioned this pull request Aug 16, 2022

Windows UDS Support tokio-rs/tokio#2201

Open

Thomasdezeeuw reviewed Aug 17, 2022

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

tests/unix_stream.rs Outdated Show resolved Hide resolved

sullivan-sean added 5 commits August 17, 2022 22:08

clean up stream and listener sys logic

5ca8952

fix re-registration

985a145

add test for serial calls to listener.accept

f5ec8ce

fix serial calls to accept

cee5c6b

remove tempfile dependency and fix doc tests

488254d

sullivan-sean requested a review from Thomasdezeeuw August 18, 2022 22:42

sullivan-sean commented Aug 18, 2022

View reviewed changes

sullivan-sean mentioned this pull request Aug 19, 2022

Expose address from SockAddr for unix domains rust-lang/socket2#332

Open

Darksonn reviewed Aug 19, 2022

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

revert change in draining behavior

b6bae73

sullivan-sean marked this pull request as ready for review August 20, 2022 23:56

sullivan-sean requested a review from carllerche as a code owner August 20, 2022 23:56

sullivan-sean added 3 commits August 20, 2022 19:38

re-organize stdnet files to mirror std::os::unix::net

2113b9f

use single syscall vectored approach from rust-lang/socket2

86c4c9a

lint

9f26286

sullivan-sean added 3 commits August 26, 2022 00:57

fmt

b07b4f1

simplify windows mod

a2831ea

clean up tests

73d5fae

Thomasdezeeuw reviewed Aug 26, 2022

View reviewed changes

sullivan-sean added 2 commits August 26, 2022 18:21

fix indentation, imports, address other comments

0baf112

fmt

d9d4bb3

sullivan-sean requested a review from Thomasdezeeuw August 30, 2022 15:00

Thomasdezeeuw requested changes Sep 10, 2022

View reviewed changes

sullivan-sean added 3 commits September 12, 2022 16:11

remove unrelated code changes

82b17e8

fix lint

96e0735

remove explicit SetHandleInformation calls

0e1b6df

sullivan-sean added 2 commits September 13, 2022 14:49

abstract socketaddr behind common API in net

77155cc

fix lint

26a060b

add comment clarifying inheritance during calls to accept

fd8ddc1

jsturtevant mentioned this pull request Jan 12, 2023

Windows support containerd/ttrpc-rust#132

Closed

GJZwiers mentioned this pull request Apr 1, 2023

Deno.listen w/ unix transport on Windows denoland/deno#18236

Open

KolbyML mentioned this pull request May 1, 2023

Adds support for UnixStream and UnixListener on Windows #1667

Closed

sullivan-sean closed this Aug 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for UnixStream and UnixListener on Windows #1610

Adds support for UnixStream and UnixListener on Windows #1610

sullivan-sean commented Aug 16, 2022

sullivan-sean left a comment

Thomasdezeeuw commented Aug 17, 2022

sullivan-sean commented Aug 18, 2022 •

edited

Loading

sullivan-sean Aug 18, 2022 •

edited

Loading

Thomasdezeeuw commented Aug 19, 2022

sullivan-sean commented Aug 19, 2022

sullivan-sean commented Aug 21, 2022

sullivan-sean commented Aug 26, 2022

Thomasdezeeuw left a comment

Thomasdezeeuw Aug 26, 2022

sullivan-sean Aug 26, 2022

Thomasdezeeuw Aug 26, 2022

sullivan-sean Aug 26, 2022 •

edited

Loading

Thomasdezeeuw Aug 26, 2022

sullivan-sean Aug 26, 2022

sullivan-sean commented Sep 8, 2022

Thomasdezeeuw commented Sep 8, 2022

Thomasdezeeuw left a comment

sullivan-sean commented Sep 13, 2022

Thomasdezeeuw commented Sep 14, 2022

Noah-Kennedy commented Oct 3, 2022

sullivan-sean commented Oct 3, 2022

lumattr commented Nov 22, 2022

Thomasdezeeuw commented Nov 22, 2022

KolbyML commented Apr 14, 2023

sullivan-sean commented Aug 18, 2023

simonsan commented Jan 21, 2024

		@@ -445,6 +457,8 @@ where

		assert!(stream.take_error().unwrap().is_none());

		assert_would_block(stream.read(&mut buf));

Adds support for UnixStream and UnixListener on Windows #1610

Adds support for UnixStream and UnixListener on Windows #1610

Conversation

sullivan-sean commented Aug 16, 2022

sullivan-sean left a comment

Choose a reason for hiding this comment

Thomasdezeeuw commented Aug 17, 2022

sullivan-sean commented Aug 18, 2022 • edited Loading

sullivan-sean Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

Thomasdezeeuw commented Aug 19, 2022

sullivan-sean commented Aug 19, 2022

sullivan-sean commented Aug 21, 2022

sullivan-sean commented Aug 26, 2022

Thomasdezeeuw left a comment

Choose a reason for hiding this comment

Thomasdezeeuw Aug 26, 2022

Choose a reason for hiding this comment

sullivan-sean Aug 26, 2022

Choose a reason for hiding this comment

Thomasdezeeuw Aug 26, 2022

Choose a reason for hiding this comment

sullivan-sean Aug 26, 2022 • edited Loading

Choose a reason for hiding this comment

Thomasdezeeuw Aug 26, 2022

Choose a reason for hiding this comment

sullivan-sean Aug 26, 2022

Choose a reason for hiding this comment

sullivan-sean commented Sep 8, 2022

Thomasdezeeuw commented Sep 8, 2022

Thomasdezeeuw left a comment

Choose a reason for hiding this comment

sullivan-sean commented Sep 13, 2022

Thomasdezeeuw commented Sep 14, 2022

Noah-Kennedy commented Oct 3, 2022

sullivan-sean commented Oct 3, 2022

lumattr commented Nov 22, 2022

Thomasdezeeuw commented Nov 22, 2022

KolbyML commented Apr 14, 2023

sullivan-sean commented Aug 18, 2023

simonsan commented Jan 21, 2024

sullivan-sean commented Aug 18, 2022 •

edited

Loading

sullivan-sean Aug 18, 2022 •

edited

Loading

sullivan-sean Aug 26, 2022 •

edited

Loading