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

Stabilization #192

Open
12 tasks
notgull opened this issue Jun 20, 2024 · 10 comments
Open
12 tasks

Stabilization #192

notgull opened this issue Jun 20, 2024 · 10 comments

Comments

@notgull
Copy link
Member

notgull commented Jun 20, 2024

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. (Rename EventLoop::try_new to 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
Copy link
Member Author

notgull commented Jun 20, 2024

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

@joshka
Copy link
Contributor

joshka commented Jun 20, 2024

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).

@notgull
Copy link
Member Author

notgull commented Jun 21, 2024

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?

@joshka
Copy link
Contributor

joshka commented Jun 21, 2024

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

@joshka
Copy link
Contributor

joshka commented Jul 8, 2024

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.

@notgull
Copy link
Member Author

notgull commented Jul 11, 2024

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.

@joshka
Copy link
Contributor

joshka commented Jul 13, 2024

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.

@ids1024
Copy link
Member

ids1024 commented Jul 13, 2024

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

@notgull
Copy link
Member Author

notgull commented Jul 15, 2024

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.

@joshka
Copy link
Contributor

joshka commented Jul 15, 2024

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.

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

3 participants