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

Windows support #14

Closed
jonhoo opened this issue Apr 6, 2020 · 12 comments
Closed

Windows support #14

jonhoo opened this issue Apr 6, 2020 · 12 comments

Comments

@jonhoo
Copy link
Contributor

jonhoo commented Apr 6, 2020

I know this is probably low priority, but it'd be nice to have, and I figured having a tracking issue might be useful.

The implementation I think would go like this:

  • Switch to std::os::raw::c_int where relevant
  • On cfg(windows), use std::os::windows::AsRawHandle instead of std::os::unix::AsRawFd
  • Use winapi::um::commapi::SetCommTimeouts to set a timeout for the user's handle for an operation. It is documented here.

My guess is that you could do away with the current Poll-based implementation, and instead just implement the read/write operations as "set timeout on fd/handle, call read/write, unset timeout". If you specifically want it to be poll-based, mio would probably be the way to do that (#3), though that'd be a larger rewrite I think.

@jcreekmore
Copy link
Owner

I don't really have any experience writing software on Windows and don't really have a use-case for this on Windows (because I haven't actively used a Windows system in a decade or so). That said, I am open to PRs that add support for Windows to this crate. I am not specifically tied to the Poll based implementation; it was just the quickest way to get things working on macOS (for my basic testing) and Linux (which was really my use-case).

@jonhoo
Copy link
Contributor Author

jonhoo commented Apr 7, 2020

Yeah, I don't actually need it either, it's just that this crate is the only thing that makes my crate not support Windows at the moment, and I'm sure others are in the same boat. I wouldn't normally care, it's just that since this is a relatively low-level crate, its lack of support for Windows is likely to affect many higher-level crates. In theory, with the approach I gave above, the Linux and Windows solutions should be practically identical, just with different syscalls to set/reset the timeout.

@jcreekmore
Copy link
Owner

If you have any way of testing this, I wired up something in #15 that at least builds and doesn't crash when I run through my basic (very basic) tests. I don't really have a good test environment for Windows...

@jonhoo
Copy link
Contributor Author

jonhoo commented Apr 8, 2020

I've actually been fairly happy with Azure Pipelines, and that also gives Windows testing. Since I maintain the template for it, I'd be happy to help you get set up! I think the first-time setup notes should still be accurate, but if you run into any issues, let me know and I'll see what I can do!

@jonhoo
Copy link
Contributor Author

jonhoo commented Apr 8, 2020

Ohh, I see, you meant in terms of actually writing tests!
Best I can think of is to block on reading from STDIN with a timeout, and see that the tests actually finish?

@jcreekmore
Copy link
Owner

So, blocking on reading from STDIN is a problem and is part of the reason I didn't support Windows earlier. The issue is, the SetCommTimeouts call really only applies to sockets (not general IO handles) as best as I can tell. So, if I read from STDIN, it blocks forever because the timeout will never be fulfilled. It works fine on unix because a file descriptor is a file descriptor. So, I need to pull that test back out.

@jonhoo
Copy link
Contributor Author

jonhoo commented Apr 8, 2020

Oh, then, how about you start a TcpListener, connect to it from some other thread, have the other thread enter a std::sync::Barrier (so it won't drop the TcpStream). Then do a blocking read from the "server-side" TcpStream with a timeout?

The "doesn't work for stdio on windows" caveat seems pretty important to at least document. Especially because I think for some that'll be a big use-case for this. I wonder why...

@jonhoo
Copy link
Contributor Author

jonhoo commented Apr 8, 2020

There's some discussion over at https://stackoverflow.com/questions/19955617/win32-read-from-stdin-with-timeout for how to make it work on STDIN, but having to special-case that sounds pretty painful..

@jcreekmore
Copy link
Owner

The more I am digging into this, the messier it gets. So, on unix-like systems, I can use select or poll (and am currently using poll) and that works on any file descriptor (i.e., a normal file, a socket, a pipe, etc.). The socket bits are the least interesting because I can use with TcpStream::set_read_timeout and TcpStream::set_write_timeout to manipulate the timeouts and do not actually need my crate at all. If all you are doing is reading from a normal file, you will not get a timeout event at all because the file is always "ready" (you might get back 0 bytes, but you should not block longer than the amount of time it takes to read the data normally, not a big issue). So, the whole purpose of this crate was to support a timeout read or write on a pipe (I wrote it so that I could read data from a subprocess that might not ever actually respond to me and I could time out the process appropriately), although there is no reason you couldn't use it to do any of the other operations (including reading from stdin).

So, on Windows, there are a minimum of three different APIs that I can find. WaitForSingleObject looks to be what has to be used for "the console" (meaning stdin, I suppose). SetCommTimeouts is used for "communications resources" meaning:

A communications resource is a physical or logical device that provides a single bidirectional, asynchronous data stream. Serial ports, parallel ports, fax machines, and modems are examples of communications resources.

Then, there is the socket API in winsock2 that is used for setting the timeouts on a SOCKET. Unfortunately, a TcpStream does not implement AsRawHandle but instead implements AsRawSocket and there does not appear to be a way to convert between them. But, socket timeouts are best handled by TcpStream anyway.

I am not entirely sure the mechanism that Windows uses (really, std::process::ChildStdout) for dealing with interprocess communications. That is going to take more research.

@jonhoo
Copy link
Contributor Author

jonhoo commented Apr 9, 2020

Ugh, yeah, it does look like a mess. I think there's a decent argument here for leaving the crate as UNIX-only, and then maybe someone writing a specialized crate just for timing out stdin on Windows. That is probably what people are using this crate for anyway (since, as you say, TcpStream is already dealt with).

@jcreekmore
Copy link
Owner

Yeah, I think that is going to be what I do. I just don't have the experience with the Windows APIs to even know what the right thing is in this cases.

@jonhoo
Copy link
Contributor Author

jonhoo commented Apr 9, 2020

I agree with you! Thanks for looking into it :)

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

No branches or pull requests

2 participants