Adds support for UnixStream and UnixListener on Windows #1667
Conversation
KolbyML
force-pushed
the
continue-the-work-of-UnixStream-for-windows
branch
9 times, most recently
from
c77065a
to
12c4013
Compare
|
Any plan for reviewing/merging this PR? |
Well, this pr has some of the same issues pointed out in previous iterations of this. After that we need some with some more Windows knowledge to review this properly as I don't use Windows myself. |
Would you be able to repost the "issues pointed out in previous iterations of this PR" in this PR. To make tracking the issues what still need to be done easier to track. "previous iterations" had certain changes which you thought took the PR backwards, which is what I tried to resolve in this PR. It is currently unclear exactly what you want. As your statement is quite broad. And the "history" of what you wanted done is confusing because of the reasons I stated before. |
There was a problem hiding this comment.
Well, this pr has some of the same issues pointed out in previous iterations of this.
Would you be able to repost the "issues pointed out in previous iterations of this PR" in this PR. To make tracking the issues what still need to be done easier to track. "previous iterations" had certain changes which you thought took the PR backwards, which is what I tried to resolve in this PR. It is currently unclear exactly what you want. As your statement is quite broad. And the "history" of what you wanted done is confusing because of the reasons I stated before.
I pointed out an obvious one, but see #1610 for my other ~40 comments. I'm not going to repeat myself, I simply don't have the time.
| // blocking windows uds which mimick std implementation used for tests | ||
| cfg_net! { | ||
| pub use crate::sys::windows::stdnet; | ||
| } |
There was a problem hiding this comment.
We're not going to expose this.
There was a problem hiding this comment.
It has previously been stated the tests shouldn't change. To make the tests not change we need to have a blocking windows UDS implementation. Having this requirement only leaves us 2 options.
- 1: Add
#[cfg(test)]to the expose so it can only be used in tests - 2: Implement UDS support in the rust STD
Would option 1 even be allowed? or an Option?
If it isn't an option then if we want windows UDS in tokio there is a requirement for it being implemented in the STD First. Originally it was thought that we could add Windows UDS since MIO already supports Windows Named Pipes without std support.
So is the precedent that std support is required. That is fine, it is nice to just know that is definitly the step forward. Because the only non-std way to fulfill the no-changing-test-requirement would be option 1.
There was a problem hiding this comment.
It has previously been stated the tests shouldn't change.
Yes, we need to ensure that the existing behaviour works on Windows as well.
To make the tests not change we need to have a blocking windows UDS implementation. Having this requirement only leaves us 2 options.
- 1: Add
#[cfg(test)]to the expose so it can only be used in tests- 2: Implement UDS support in the rust STD
Would option 1 even be allowed? or an Option?
The first option doesn't work as we use Rust's integration tests, which doesn't have access to another crate's #[cfg(test)] exposed items. The second option would be ideal, but that's currently not possible as far as I know.
If it isn't an option then if we want windows UDS in tokio there is a requirement for it being implemented in the STD First. Originally it was thought that we could add Windows UDS since MIO already supports Windows Named Pipes without std support.
So is the precedent that std support is required. That is fine, it is nice to just know that is definitly the step forward. Because the only non-std way to fulfill the no-changing-test-requirement would be option 1.
You're overlooking a third option. Have a blocking implementation inside the test code.
modify src/net for windows compatibility fix tests add docs back in cleanup remove log statements clean up selector clean up stream and listener sys logic fix re-registration add test for serial calls to listener.accept fix serial calls to accept remove tempfile dependency and fix doc tests revert change in draining behavior re-organize stdnet files to mirror std::os::unix::net use single syscall vectored approach from rust-lang/socket2 lint improve support across feature matrix fix doc tests use bcrypt instead of rand add -_ to random char set to avoid rejection sampling optimize rng syscall logic fix lint and fmt remove unused functions fmt simplify windows mod clean up tests fix indentation, imports, address other comments fmt remove unrelated code changes fix lint remove explicit SetHandleInformation calls abstract socketaddr behind common API in net fix lint add comment clarifying inheritance during calls to accept
KolbyML
force-pushed
the
continue-the-work-of-UnixStream-for-windows
branch
from
12c4013
to
48db06f
Compare
|
I'm going to close this because it still needs some work and hasn't seen any activity recently. @KolbyML thanks for the effort! |
|
@sullivan-sean @KolbyML Both #1610 and #1667 are closed now. Do you think you can work together on this? |
This PR supersedes PR #1610 and solves issue #1609
Background Info
I have spoken with Thomasdezeeuw and his main complaints with the last PR was
So I was looking into cleaning up the PR and cleaned up some messy edges. I am making this draft pull request just to show in the public where it is at and what I am working on.
Ready for review !!!
The PR is ready review I made it so we didn't have to change the tests. The only test what is different is the try_io doc test which is because the unix one uses pair which isn't supported on Windows. But all the tests in the tests folder matches.
The only addition to the tests would be in
tests/UnixStream.rsThis is because of #1611 which is known about and intentional done this way.
sys/windows/stdnetis a implementation matching the standards libraries implementation of Unix UDS. So if the standard library ever adds support for Unix UDS for windows it should be a drop in replacement.