calloop icon indicating copy to clipboard operation
calloop copied to clipboard

Stabilization

Open notgull opened this issue 1 year ago • 10 comments

I'd like to stabilize calloop's public API to reduce breaking changes. This way when we release v1.0, we shouldn't have to release v2.0. Since we have no more public dependencies as of v0.14.0 this is now feasible.

Potential semver hazards, just from glancing over docs.rs:

  • [ ] It should be explicitly stated somewhere that calloop is not compatible with the web. We also don't support WASI at the moment.
  • [ ] calloop::Readiness, calloop::Mode, calloop::PostAction and calloop::Interest could be non_exhaustive. User code doesn't need to match on these.
  • [ ] calloop::Readiness's error field could be removed, since it's not emitted by polling.
  • [ ] calloop::futures::ExecutorDestroyed should be made non-instantiable by users.
  • [ ] We can probably deprecate/remove calloop::generic::FdWrapper, since it can be replaced by BorrowedFd::borrow_raw.
  • [ ] Make the pub fields of Generic not exposed, or only exposed via methods.
  • [ ] Make calloop::io::{Readable, Writable} return Result<()> instead of ().
  • [ ] Make calloop::ping::{Ping, PingSource} a newtype wrapper instead of a type alias.
  • [ ] Make calloop::signal::Singals's method take impl IntoIterator<Item = impl Borrow<Signal>> instead of &[Signal] to increase compatibility.
  • [ ] Do we want to keep calloop::timer::Timer::current_deadline?
  • ~~For EventSource::Error, what do we gain from it being Sync? Especially since the rest of calloop is usually thread-unsafe.~~
  • [ ] Do we want to expose RefCell::ref in Dispatcher? It might be better to use closures here.
  • [ ] EventLoop::try_new should be renamed to EventLoop::new. (#217)

Other possible improvements:

  • Enable all features on docs.rs
  • Web compatibility? Probably not feasible.
  • Add a way to get a LoopHandle reference without cloning the loop Rc.

notgull avatar Jun 20 '24 04:06 notgull

Poke @ids1024, I know this has been a problem upstream.

notgull avatar Jun 20 '24 04:06 notgull

Playing with calloop in conjunction with color-eyre, some of the errors being non Send sure to Rc(RefCell) was a little annoying, and makes me wonder if some of these places should instead use Arc(RwLock) instead. I haven't cut an issue about this yet (as I'm just getting familiar with calloop), but bring it up because that sounds like this sort of change would be a breaking change if things did want to implement thread safe errors (iirc, this problem affects registering a timer source).

joshka avatar Jun 20 '24 12:06 joshka

The goal of calloop is to be single threaded; multi-threaded code is explicitly not a goal. This should be written down somewhere.

However all errors should be Send and Sync. I guess anyhow compatibility is a good reason for that. Is there a particular error that isn't?

notgull avatar Jun 21 '24 02:06 notgull

I'll create another issue for this instead of derailing this issue.

joshka avatar Jun 21 '24 02:06 joshka

Something that struck me as a little bit odd when getting up to speed with calloop was that the channel method returns a (Sender, Channel) tuple, whereas the various mpsc channels it's based upon name these (Sender, Receiver). I wonder if the naming on that might be worth migrating to something more in line with the existing code. Even the docs seem to suggest this when they call this "The receiving end of the channel".

Just an observation on things which could be worth considering.

joshka avatar Jul 08 '24 14:07 joshka

the channel method returns a (Sender, Channel) tuple, whereas the various mpsc channels it's based upon name these (Sender, Receiver).

I think it's because the "Sender" in calloop's case is more a handle to the "Channel", rather than it being a relatively symmetric relationship like it is in libstd.

I can understand that this is confusing, however. I'll think of a way to fix this while also emphasizing the importance of the underlying event source.

notgull avatar Jul 11 '24 04:07 notgull

Would RecieverSource be a reasonable name? This name would give a hint to the reader that it should be inserted into the event loop, that it will act generally like any other source, while keeping it obviously related to being the reciever side of the channel. The problem with calling this Channel is that a channel refers to the entire Sender-Receiver pair, not just one side of the pair, and this is really only the receiving side of things.

joshka avatar Jul 13 '24 02:07 joshka

I've found the name a bit odd, and RecieverSource does seem a bit more descriptive.

ids1024 avatar Jul 13 '24 03:07 ids1024

Yes, I think it would be nice to change the name in this way. In fact, it would probably be more intuitive if everything that implemented EventSource ended with the suffix -Source as well.

notgull avatar Jul 15 '24 02:07 notgull

Perhaps the other side of a source might be a -Handle (or perhaps -Signal)? Thinking a little more about ReceiverSource, even that name might be a little (linguistically) confusing as the two words kind of mean opposite things (i.e. a receiver accepts something and a source produces something). So I'd maybe come back to just calling this Receiver (and trusting that the uesrs of this library understand that it semantically does more than the receiver that it wraps.

joshka avatar Jul 15 '24 04:07 joshka