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

Use own error type for API #57

Closed
detly opened this issue Jul 12, 2021 · 5 comments · Fixed by #66
Closed

Use own error type for API #57

detly opened this issue Jul 12, 2021 · 5 comments · Fixed by #66

Comments

@detly
Copy link
Contributor

detly commented Jul 12, 2021

One of the little-but-common annoyances I run into with Calloop is that eg. process_events() returns a std::io::Result, but std::io::Error is just about the only Rust error type in existence that doesn't have a single-argument constructor. So I always have to tack on .map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e)). This is compounded by the fact that you can use any type you like for EventSource's associated Ret, which is usually going to be a Result<T, E> where E: std::error::Error rather than std::io::Error (precisely because std::io::Error is a pain to use in your own API and arguably is not intended to be).

Should Calloop have its own error (and corresponding result) for process_events() (and possibly other trait methods on EventSource? It would be possible to have From implementations to ensure that ? still works. Then (a) using ? on std::io::Results from fd operations would still be fine, and (b) so would using ? on core::result::Result or anyhow::Result or errors from thiserror etc.

I would be happy to make such a change, but it is potentially a breaking one so I wanted to see if it's something you'd be open to.

@elinorbgr
Copy link
Member

Yes, definitely. Notably, process_events() should return a result that indicates, when an error occurred, if it came from the polling machinery itself or from one of the event sources, in which case it could be a Box<dyn Error>. Probably worth thinking to some mechanism that allows to easily detect which source errored, to act upon that if possible/necessary.

@detly
Copy link
Contributor Author

detly commented Jul 12, 2021

Excellent, we can discuss the general approach here and then I'll code it up.

I tend to avoid Box<dyn Error> - basically it doesn't do a whole lot that anyhow or thiserror don't achieve between them. Both of those libs will happily work with (ie. wrap, allow unwrapping, combine) errors that implement std::error::Error, so we should be fine there.

My first suggestion would be to have a calloop::CalloopError that

  • implements std::error::Error
  • has From impls from std::io::Error into itself
    • possibly has a few distinct types that correspond to obvious std::io::ErrorKinds without going overboard
  • has From impls from std::error::Error into itself
  • has enough additional information that the event source can at least be identified (access it directly might have ownership issues, because the loop owns the source, but eg. the handle's registration token or the source's token could be recorded)

Off the top of my head, we would also need a From impl for std::result::Result so that ? works (because it can't automatically do Result<T, E> into Result<S, F> just because T: Into<S> and E: Into<F>).

I think we get a lot of that for free with thiserror, so if you're not averse to adding that as a dependency, I'll use it. If not, I won't, but there might be extra boilerplate.

@elinorbgr
Copy link
Member

I'm fine with using thiserror, but without Box<dyn Error> we cannot allow the event sources to have their own custom error types. I thought that that was what you wanted?

@detly
Copy link
Contributor Author

detly commented Jul 12, 2021

Ah I see what you're saying. The significant part for me is not so much "event sources having custom error types" as "having error types that can easily be chained with ? or maintain a traceback through the callbacks." I'm actually not sure how much a boxed error would have to proliferate, so maybe I should code something up and see what's possible?

@elinorbgr
Copy link
Member

Ah I see. I don't know how much of a necessity having custom error types for event sources is. But I guess we can still have a fancy calloop error type based on thiserror, which a Custom(Box<dyn Error>) variant for cases where the event source really needs it.

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

Successfully merging a pull request may close this issue.

2 participants