API suggestion: clone sender vs. take sender vs. send

[detly]

There are a few inconsistencies in Calloop's higher-level event sources, and even though they are extremely minor, I thought I'd make the suggestion since I've coded up an alternative for the event sources I've made for ZeroMQ and USB.

Take for example:

• Ping has make_ping() to construct, which returns a (sender, source) pair. Using it requires calling ping() on the sender.
• Channel has channel() (not make_channel()!) which returns a (sender, source) pair. Using it requires calling send() on the sender.
• Timer has Timer::new(). Using it requires getting a handle from the Timer itself.

Both Ping and Channel have handles that close on drop. Timer does not.

This all quickly becomes apparent if you have a composite event source that uses multiple kinds of these, and kind of unwieldy at times. For example, if your composite source has both a ping and a channel for internal reasons, you need four fields to use them.

Here is an API we stabilised on that kind of gives the best of both worlds:

/// This event source also allows you to use different event sources to publish
/// messages over the same writeable ZeroMQ socket (usually PUB or PUSH).
/// Messages should be sent over the Calloop MPSC channel sending end. This end
/// can be cloned and used by multiple senders. Common use cases are:
///
/// - One sender: just use `send()`. Useful for any scoket kind except `PULL` or
///   `SUB`.
/// - Multiple senders: clone the sending end of the channel with
///   `clone_sender()`. Useful for `PUSH`, `PUB`, `DEALER`, but probably not at
///   all useful for `REQ`/`REP`.
/// - No senders: take the sending end of the channel with `take_sender()` and
///   drop it. Useful for `SUB` and `PULL`.

pub struct ZeroMQSource<T> {
    /// Sending end of channel.
    mpsc_sender: Option<calloop::channel::Sender<T>>,
    // ...
}

impl<T> ZeroMQSource<T> {
    // Send a message via the ZeroMQ socket. If the sending end has been
    // taken then this will return an error (as well as for any other error
    // condition on a still-existing channel sending end).
    pub fn send(&self, msg: T) -> Result<(), std::sync::mpsc::SendError<T>> {
        if let Some(sender) = &self.mpsc_sender {
            sender.send(msg)
        } else {
            Err(std::sync::mpsc::SendError(msg))
        }
    }

    // Clone the channel sending end from this ZeroMQ source. Returns [`None`]
    // if it's already been removed via [`take()`], otherwise the sender can be
    // safely cloned more and sent between threads.
    pub fn clone_sender(&self) -> Option<calloop::channel::Sender<T>> {
        self.mpsc_sender.clone()
    }

    // Remove the channel sending end from this ZeroMQ source and return it. If
    // you do not want to use it, you can drop it and the receiving end will be
    // disposed of too. This is useful for ZeroMQ sockets that are only for
    // incoming messages.
    pub fn take_sender(&mut self) -> Option<calloop::channel::Sender<T>> {
        self.mpsc_sender.take()
    }

Disadvantages:

• more complicated
• new API
• extra checks in methods

Advantages:

• if the source are used internally, no extra senders need to be kept in fields, you can just call self.pinger.ping()
• if you want to close a channel you can just call self.channel.take_sender() instead of needing to keep (a) the sender and (b) an option wrapper
• reflects API of std type Option (send/clone_sender/take_sender) == (map/clone/take)
• constructor is more familar source::Source::new() -> Result<Source> instead of source::make_source() -> Result<(sender, source)>
• can be made backwards compatible easily eg. make_source() just becomes let source = Source::new(); (source.take_sender(), source)

Let me know what you think, and if you're interested I'll code something up for the existing types that have sending handles.

[elinorbgr]

That sounds like a solid proposal, thanks for putting all that on writing. I hadn't given a lot of thought about this, and what you suggest makes a lot of sense.

I suppose with that we should ensure that sources with a sending end like that should take care of automatically removing themselves from the event loop when then realize their sending end has been destroyed and all pending events have been processed?

In any case I'm quite interested with this!

[detly]

I suppose with that we should ensure that sources with a sending end like that should take care of automatically removing themselves from the event loop when then realize their sending end has been destroyed and all pending events have been processed?

It's funny you should say that, because I was about to open another issue/suggestion about that. It might help to keep it separate, otherwise I think it might become hard to follow.

[detly]

I wrote an essay over in #78 for you.

[i509VCB]

I guess I should ask, how would the x11 backend in smithay be brought more in line with this. Currently X11Backend allows you to get X11Handles, effectively acting like the timer sources.

https://github.com/Smithay/smithay/blob/master/src/backend/x11/mod.rs

[detly]

@i509VCB So firstly, what I'm suggesting is merely a convention for the library, not a trait or anything like that. So in the simplest sense, dependent libraries don't have to change if they don't want to (eg. they don't want to break backwards compatibility, it doesn't suit their architecture).

Having said that, if they wanted to follow the convention, it sounds like the use-case you mention would be to just use a clone_handle() style method.

[i509VCB]

what I'm suggesting is merely a convention for the library

Ah I see.

[elinorbgr]

So I've started experimenting a little with that, and I have two things coming to mind I'm not really sure about:

• the direct send() method kind of mixes "real" errors and error caused by the fact that the sender was previously taken, which is kind of weird. Would it make sense to make the method panic if the sender was taken?
• For sources like Channel, there are actually two different senders (Sender and SyncSender) which work with the same receiver. Such an API would force the source type to be duplicated as well.

[detly]

mixes "real" errors and error caused by the fact that the sender was previously taken

Yeah, possibly. I had it that way because, frankly, I tend to panic!() on send errors anyway or ignore them (it just happens that in my applications' architecture, either they're as critical as method calls, or the failure means something's gone away that I don't have to worry about any more). Since a panic generally means "you, the programmer, made a mistake", your suggestion seems perfectly valid to do. The other alternative is to have a second level of error, and... I can't really think of a use case that makes it worth the extra complexity?

I had missed the detail of the Sender/SyncSender, let me think about that.

[elinorbgr]

With https://github.com/Smithay/calloop/pull/89 this point is becoming weaker given the Timer event source is no longer relevant for that issue, and Channel and Ping are already very close in API and would be pretty trivial to unify. We'll need to adjust the book to that though, given the timer event source was used to introduce the pattern. But presenting it as a pattern might be misleading now... :thinking: