erickt / rust-zmq

Rust zeromq bindings.
Apache License 2.0
887 stars 189 forks source link

zmq_sys::zmq_pollitem_t is not represented correctly on Windows - causes Access Violation with zmq_poll #290

Closed oblique closed 4 years ago

oblique commented 4 years ago

zmq_pollitem_t currently is defined as:

#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct zmq_pollitem_t {
    pub socket: *mut ::std::os::raw::c_void,
    pub fd: ::std::os::raw::c_int,
    pub events: ::std::os::raw::c_short,
    pub revents: ::std::os::raw::c_short,
}

This is not correct on Windows since fd should be RawSocket (i.e. u64).

So the possible solutions are:

  1. Always regenerate with bindgen. Is there any reason you don't do this?
  2. Add zmq_sys::RawPollItem.
  3. Manually modify ffi.rs (I don't like this).

Which one do you prefer so I can implement it?

rotty commented 4 years ago

zmq_sys::zmq_pollitem_t is an accidentially exposed item in the zmq-sys crate, which is not used in the zmq crate; there's zmq::PollItem, which shouldn't have the problem you describe. Are you using zmq_sys directly? That is not its intended purpose, it's existence is an implementation detail. I'll remove the bogus zmq_pollitem_t definition in the next release of zmq-sys.

rotty commented 4 years ago

And regarding not auto-generating the bindings, since the libzmq ABI is supposed to be stable, manually updating the generated bindings when covering added ABI seems like a reasonable approach to me.

oblique commented 4 years ago

I'm experimenting with async/await and I want to keep poll items in just one Vec. So, I'm using zmq-sys in the case of polling because:

  1. zmq::PollItem needs a lifetime. However I can workaround it with transmute and 'static.
  2. zmq::PollItem does not have getters for socket and fd. So I can not know which item is ready without keeping a second Vec.
  3. I don't want to collect zmq::PollItem in a new Vec for each zmq::poll to solve both of the above.

I can implement getters for socket and fd, if that's ok.

rotty commented 4 years ago

Adding a getter for PollItem::socket() is not easily done, due to zmq::Socket not being isomorphic to a libzmq socket pointer, as it includes extra fields. A getter fd(&self) -> Option<RawFd> would make sense, though.

In general, it should be the goal to let you write performant code using just the zmq API, without having to use unsafe; maybe you come up with an idea about an API extension during your experiments; either way I'd appreciate if you opened an issue stating the shortcoming of the API, and including your example code using unsafe (once it's ready for public consumption), altough I seem to recall that this issue was discussed already.

oblique commented 4 years ago

What about PollItem::socket_ptr()?

rotty commented 4 years ago

That cannot be used without requiring direct use of zmq-sys, and hence is not a desirable API extension. Have you benchmarked your code to verify that the performance gains over simply (re-)using a Vec<PollItem<'a>> actually makes the use unsafe worthwhile? If you expect an upper bound on the number of sockets, which is met most of the time, you could also use the smallvec crate to avoid heap allocation.

rotty commented 4 years ago

Also, I wonder why you even need this kind of API; when I played with Tokio bindings for zmq, I got to PoC state without using zmq::poll at all, by integrating the ZMQ sockets directly into the Tokio event loop, using the file descriptor returned by Socket::get_fd; see https://github.com/rotty/zmq-tokio for the code, but keep in mind it's a PoC that I have not touched for a long time now.

rotty commented 4 years ago

I'm closing this issue now, as (a) this is not a bug, as the zmq-sys API is considered a private implementation detail, and zmq_socket_t should not be used, and (b) the API additions proposed do not add value, IMO. This also holds for the fd getter, now that I've thought about it a bit more:

You already know the FD value when creating the PollItem, and having just an fd getter when you'd also need access to the socket to make use of PollItem in a general way, other than sticking it into zmq::poll.

Hence, I maintain that Polltem should remain just a handle, without access to the underlying socket or FD. If you want mainloop integration or polling without any allocation, you should not use zmq::poll, but do the underlying OS calls in another way, e.g. by integrating with the mio crate.