Add a safe binding to WSAPoll()

[psychon]

Hi,

I am trying to support Windows, but Windows being Windows, it is difficult. I want to use forbid(unsafe_code), but, well, it is difficult. For this reason, I hope I can hide my unsafe code in another crate. This is where I stumbled upon winapi-util.

Per https://github.com/BurntSushi/winapi-util/blob/master/README.md#notes, I am not sure if you would want something like this in here or not. Feel free to reject this.

Since Rust was complaining about WSAPoll, I went with wsa_poll for the function name. Perhaps poll would be better...?

I do not have access to a Windows development environment and so I used CI for developing this. That's also a pain to do.

I'd like to use this in psychon/x11rb#408.

Thanks :-)

[psychon]

Hm... I just found #2 (comment)

Do you want me to remove the use of winapi types in the public API?

[BurntSushi]

Thanks! In principle, I'm not opposed to adding something like what you've submitted, but as you've guessed, I'd really like to avoid making winapi a public dependency. But perhaps such a thing isn't worth it. winapi is after all a fundamental building block and very rarely publishes breaking change releases.

Also, off topic feedback: you might consider resisting the use of async I/O for x11. My guess is that it is unnecessary. (I don't say this loosely. I built the canonical x11 pure Go bindings and wrote my own window manager using them.)

[BurntSushi]

Hmmm... I see why making winapi a public dependency is convenient here. Wrapping WSAPOLLFD would be pretty annoying. I'm not quite sure what to do, as I don't really grok these Windows APIs. Do you see any way of buttoning this up in a more convenient fashion?

[psychon]

Your call. Is the following better or worse? (I haven't looked at your other comments yet. That will have to wait until tomorrow. But thanks a lot for this quick feedback.)

diff --git a/src/socket.rs b/src/socket.rs
index 78d216a..a43b7f6 100644
--- a/src/socket.rs
+++ b/src/socket.rs
@@ -1,21 +1,55 @@
 use std::convert::TryInto;
 use std::io;
+use std::os::raw::c_short;
+use std::os::windows::io::RawSocket;
 
 use winapi::shared::minwindef::INT;
-use winapi::um::{winnt::SHORT, winsock2};
+use winapi::um::winsock2::{self, WSAPOLLFD};
 
-pub const POLLRDNORM: SHORT = winsock2::POLLRDNORM;
-pub const POLLRDBAND: SHORT = winsock2::POLLRDBAND;
-pub const POLLIN: SHORT = winsock2::POLLIN;
-pub const POLLPRI: SHORT = winsock2::POLLPRI;
-pub const POLLWRNORM: SHORT = winsock2::POLLWRNORM;
-pub const POLLOUT: SHORT = winsock2::POLLOUT;
-pub const POLLWRBAND: SHORT = winsock2::POLLWRBAND;
-pub const POLLERR: SHORT = winsock2::POLLERR;
-pub const POLLHUP: SHORT = winsock2::POLLHUP;
-pub const POLLNVAL: SHORT = winsock2::POLLNVAL;
+pub const POLLRDNORM: c_short = winsock2::POLLRDNORM;
+pub const POLLRDBAND: c_short = winsock2::POLLRDBAND;
+pub const POLLIN: c_short = winsock2::POLLIN;
+pub const POLLPRI: c_short = winsock2::POLLPRI;
+pub const POLLWRNORM: c_short = winsock2::POLLWRNORM;
+pub const POLLOUT: c_short = winsock2::POLLOUT;
+pub const POLLWRBAND: c_short = winsock2::POLLWRBAND;
+pub const POLLERR: c_short = winsock2::POLLERR;
+pub const POLLHUP: c_short = winsock2::POLLHUP;
+pub const POLLNVAL: c_short = winsock2::POLLNVAL;
 
-pub use winsock2::WSAPOLLFD;
+/// Information necessary for the [`wsa_poll`] function.
+#[derive(Debug, Clone)]
+pub struct WSAPollFD {
+    fd: RawSocket,
+    events: c_short,
+    revents: c_short,
+}
+
+impl WSAPollFD {
+    /// Create a new `WSAPollFD` apecifying the events of interest for a given socket.
+    pub fn new(fd: RawSocket, events: c_short) -> Self {
+        Self { fd, events, revents: 0 }
+    }
+
+    /// Returns the events that occurred in the last call to [`wsa_poll`].
+    pub fn revents(&self) -> c_short {
+        self.revents
+    }
+
+    fn to_winapi(&self) -> WSAPOLLFD {
+        WSAPOLLFD {
+            fd: self.fd.try_into().unwrap(),
+            events: self.events,
+            revents: self.revents,
+        }
+    }
+
+    fn from_winapi(&mut self, poll: &WSAPOLLFD) {
+        self.fd = poll.fd.try_into().unwrap();
+        self.events = poll.events;
+        self.revents = poll.revents;
+    }
+}
 
 /// `wsa_poll` waits for one of a set of file descriptors to become ready to perform I/O.
 ///
@@ -23,15 +57,18 @@ pub use winsock2::WSAPOLLFD;
 ///
 /// [`WSAPoll`]: https://docs.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-wsapoll
 pub fn wsa_poll(
-    fd_array: &mut [WSAPOLLFD],
+    fd_array: &mut [WSAPollFD],
     timeout: INT,
 ) -> io::Result<usize> {
-    unsafe {
-        let length = fd_array.len().try_into().unwrap();
-        let rc = winsock2::WSAPoll(fd_array.as_mut_ptr(), length, timeout);
-        if rc < 0 {
-            return Err(io::Error::last_os_error());
-        };
-        Ok(rc.try_into().unwrap())
-    }
+    let mut array2 =
+        fd_array.iter().map(WSAPollFD::to_winapi).collect::<Vec<_>>();
+    let length = array2.len().try_into().unwrap();
+    let rc =
+        unsafe { winsock2::WSAPoll(array2.as_mut_ptr(), length, timeout) };
+    if rc < 0 {
+        return Err(io::Error::last_os_error());
+    };
+    // Copy the results to the output array
+    fd_array.iter_mut().zip(array2.iter()).for_each(|(a, b)| a.from_winapi(b));
+    Ok(rc.try_into().unwrap())
 }
diff --git a/tests/poll.rs b/tests/poll.rs
index bf67360..8882080 100644
--- a/tests/poll.rs
+++ b/tests/poll.rs
@@ -2,9 +2,9 @@
 mod windows {
     use std::io::{Result, Write};
     use std::net::{TcpListener, TcpStream};
+    use std::os::raw::c_short;
     use std::os::windows::io::{AsRawSocket, RawSocket};
 
-    use winapi::um::winnt::SHORT;
     use winapi_util::socket::*;
 
     /// Get a pair of connected TcpStreams
@@ -16,11 +16,15 @@ mod windows {
         Ok((stream1, stream2))
     }
 
-    fn poll(socket: RawSocket, events: SHORT, revents: SHORT) -> Result<()> {
-        let mut sockets = [WSAPOLLFD { fd: socket as _, events, revents: 0 }];
+    fn poll(
+        socket: RawSocket,
+        events: c_short,
+        revents: c_short,
+    ) -> Result<()> {
+        let mut sockets = [WSAPollFD::new(socket, events)];
         let count = wsa_poll(&mut sockets, -1)?;
         assert_eq!(count, 1);
-        assert_eq!(sockets[0].revents, revents);
+        assert_eq!(sockets[0].revents(), revents);
 
         Ok(())
     }

[psychon]

Also, off topic feedback: you might consider resisting the use of async I/O for x11. My guess is that it is unnecessary.

The need for this came from psychon/x11rb#409. This tries to implement xclock. Basically: This uses poll() with a timeout of one second to wait for new data on the X11 socket. On a timeout, the clock is redrawn. When something new is available... well, I need to non-blockingly read from the socket in case only a part of some big X11 packet is available.

The idea was to have the socket non-blocking all the time, but now that you made me write this: I can just make the socket nonblocking only while needed and have it blocking the rest of the time. This solution is a lot simpler. Thanks a lot for this hint!

[psychon]

Updated to address your comments. Only "should winapi appear in the public API or not" is left.

(Also: Since you got rid of my need for this (and even did so just in passing), I could also live with this PR just sitting around unmerged or being closed.)

[BurntSushi]

All righty. If this is now unnecessary, then I'd prefer to close it. I'd rather not add something unless there is an active need for it by someone, otherwise it's too easy to mess things up without an active driving use case.

[BurntSushi]

On a timeout, the clock is redrawn. When something new is available... well, I need to non-blockingly read from the socket in case only a part of some big X11 packet is available.

Can you use threads and channels for this instead? i.e., One thread reading from the socket in a blocking fashion and sending results on a channel, and another thread reading from that channel.

[psychon]

Once upon a time I made x11rb internally spawn a thread as you outline. Feedback was towards using set_nonblocking() instead: psychon/x11rb#199
The resulting alternative code without dedicated background threads grew into six mutexes and one condition variable.

I agree that this would be so much easier with a dedicated thread, but I also "feel" like it is excessive to start a thread just for this. Goroutines makes this indeed easier (but then makes shutdown more complicated, apparently: BurntSushi/xgb#39)

[BurntSushi]

Interesting.

As for XGB, I didn't really design its core with shutdown in mind, so I'm not sure it actually complicates it. But sure, it's a consideration.

[psychon]

For future reference / people who find this via Google: I moved the code from this PR into its own git repo instead: https://github.com/psychon/winapi-wsapoll