Implemented custom errors types for Calloop

[detly]

Closes #57. Sorry this took so long, this year has been a bit garbage.

My plan was to have something that lets you use ? seamlessly in process_events() and other places, but I couldn't quite achieve that due to Rust's coherence rules. When specialisation becomes stable, that will help. Nonetheless I've added an extension trait makes things less fiddly for most of the uses cases I personally have encountered, which is hopefully a good start.

On that note, I expect that this will change a bit in the near future as you and Calloop's users determine what works for them and what doesn't. I'm happy to be tagged in any issue reports or questions about it.

A super quick tour:

• Pretty much all trait methods and internal functions should use calloop::Error.
• The only exception to this is the InsertError, which needs to "return" the event source that couldn't be inserted. This is a bit of a weird case, but is very similar to MPSC channel sending errors - they have to return the item as well. This means you can't assume it will be Sync + Send, which breaks a few assumptions in libraries like thiserror. Fortunately it's not a huge deal, and I don't think it happens much.
• Overwhelmingly I expect users to use calloop::Error::CallbackError. Maybe the IO error too.
• Over time maybe we'll see more specific use cases that will benefit from a new variant, or conversion function, or whatever.

Final note: I haven't run cargo fmt because I don't know what your policy is on that, and I didn't want to pollute the PR with formatting changes. Let me know if you'd rather I did.

[codecov]

Codecov Report

Merging #66 (ec6c5e8) into master (5ae36c6) will decrease coverage by 0.48%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           master      #66      +/-   ##
==========================================
- Coverage   81.84%   81.36%   -0.49%     
==========================================
  Files          14       15       +1     
  Lines        1526     1524       -2     
==========================================
- Hits         1249     1240       -9     
- Misses        277      284       +7     

Impacted Files	Coverage Δ
doc/src/zmqsource.rs	0.00% <0.00%> (ø)
src/lib.rs	100.00% <ø> (ø)
src/error.rs	13.33% <13.33%> (ø)
src/sources/generic.rs	91.66% <40.00%> (-5.29%)	⬇️
src/sources/signals.rs	78.94% <64.70%> (-1.06%)	⬇️
src/sys/mod.rs	91.66% <71.42%> (+8.33%)	⬆️
src/io.rs	67.95% <76.47%> (-0.53%)	⬇️
src/sources/channel.rs	71.42% <80.00%> (-1.08%)	⬇️
src/sources/futures.rs	84.84% <87.50%> (+4.25%)	⬆️
src/sys/kqueue.rs	98.23% <87.50%> (ø)
... and 7 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ae36c6...ec6c5e8. Read the comment docs.

[elinorbgr]

Thanks! That looks nice at first glance, I'll take some time for a more in-depth review later.

However glancing over your code this raises a question. It is a pre-existing issue, but your PR is related: currently, whenever an event source returns an error, the event loop dispatch is short-circuited and that error is early returned. I don't think that is a robust way to handle such errors, a single source erroring should not bring down the whole event loop.

I'm thinking we should instead distinguish errors generated by calloop itself from errors generated by an event source, and errors generated by an event source should be accumulated (in a Vec for example) during the dispatch while still processing all sources (possibly disabling the faulty source in the process), and returned to the user, with an indication of which source generated the error. So that the user can gracefully handle a faulty source.

With such an architecture, I'm thinking there would not actually be any need to unify source-generated errors and calloop errors in a single error enum. 🤔

[detly]

I think it greatly depends on how you're using Calloop. I tend to use it with only one or two top level sources that are ever actually added with insert_source(). All other event sources are added via register() etc. on the event sources themselves. In this case, the "short-circuiting" is entirely a matter for the code I write in process_events(), and it's up to me to decide to use ?/early returns, or log errors and continue, or collect them and handle them elsewhere as you suggest. The loop logic only really gets invoked at the very top level.

In my code, once inside a process_events() call, there aren't a huge number of non-event-source errors that can happen (that aren't usually critical, like asserts or OOM).

A big part of this was also getting away from using std::io::Error as the de facto error type, because it's a bit different from other error types, and opening the door to implementing error behaviour that emerges from use. With a Calloop-specific error type we can add behaviour that works for the specifics of the library, which as you say, may include collecting multiple errors or interfacing with crates that do this.

[elinorbgr]

Ok, so, took some time to look closer, and I'm not sure about the whole "error conversion extension traits".

Wouldn't it be much simpler to just change EventSource::process_events() to return a Result<PostAction, Box<dyn Error + Send + Sync>> ?

[detly]

• Yes, simpler for us writing the library Maybe for us writing the library. OTOH, integrating the potentially different errors from process_events() into the loop logic could become harder.
• Yes, for anyone wanting to propagate an arbitrary error back up from a callback. The custom error automatically implements From<Box<dyn Error + Sync + Send>> so it will work with ? if they have the boxed error already; if not they'll have to convert it with map_err(Box::new), or map_err(Into::into), which is what the conversion trait does. See below.
• No, for anyone who wants to consume the error and take appropriate action, because they'll only have the trait object. They won't be able to tell (without a substantial amount of boilerplate) whether it was an IO error, something from their own library, etc.

If you were to do that, I'd suggest using anyhow (or maybe snafu). Anyhow has all the benefits of a boxed error, but also has some useful extra features and conversions, particularly the ability to statically downcast back to the original error. However the general consensus I've seen has been that anyhow is for binary crates, or libraries that consume a lot of different other APIs, whereas single-purpose libraries should have their own error type. The other wart here is that std::io::Error (which you use a lot) doesn't play nice with any of these. You would need a conversion trait somewhere anyway I think, or users would have to write their own (which is what I've done in every project I've used Calloop in).

The conversion traits aren't necessary, but they're a common strategy I've seen used elsewhere - they're a workaround for the fact that you can't implement From<E: std::error::Error> for MyError because it conflicts with the From<T> for T blanket implementation in core. So no matter what you do, if you want to return a concrete error type from your trait methods and allow using ?, you need to use map_err() first. If and when specialisation stabilises, it won't be needed any more.

There is a third approach too: add an associated type alongside Event, Ret and Metadata for the error type. The downside to that is it's much harder to integrate all that into Calloop's own loop logic.

[detly]

To be more specific, my call structure might look a lot like:

• main()

  • insert_source(TopLevelSource::new()).expect("Could not add event source")

• TopLevelSource::process_events()

  • SecondLevelSource::process_events()
  • OtherSecondLevelSource::process_events()
    • ThirdLevelSource::process_events()

A lot of the time, the error handling for my specific use of event sources won't take the form of returned errors from process_events(), but conveying the error through the type of Event. If, for example, I'm making a request to hardware, I might post a hardware request enum over a channel, make the request in the lower level event source, and then post the result back via the callback passed to process_events(). This enables me to keep context attached to the request/response, to process things in a queue, etc.

An error returned from process_events() is more about critical event-processing errors eg. process_events() on one of Calloop's own sources failed, or a channel was closed unexpectedly, or something like that. If all those process_events() calls returned a trait object or anyhow's error, all it would do is change the boilerplate from using map_callback_err() in each callee, to using error.downcast() or whatever in each caller, to figure out whether it was an IO error, a Calloop-specific error, or whatever.

Anyway, that's just my usage, but it's what motivated this approach so you might find it informative.

[elinorbgr]

Ah yes, if you want to retrieve the error in a nested process_events() the boxed error is not very nice. However, this is mostly what your PR already does with CallbackError(#[from] Box<dyn std::error::Error + Sync + Send>), so the difference seems pretty small to me.

In that case, what I'd suggest instead is to add a new associated type Error to the EventSource trait (with the appropriate bounds for being converted to a boxed error, and make process_events() return Result<PostAction, Self::Error>, allowing each event source to potentially have its own error type.

With that, a process_events calling into a nested process_events will be able to process the error adequately, and if the toplevel process_events returns an error, the event loop can just box it and forward it as the return value of dispatch. How does that sound?

[detly]

I like that approach too, but will it cause problems when integrating event sources into the loop itself (ie. in loop_logic.rs)?

[elinorbgr]

I don't think so, the conversion to Box<dyn Error> and wrapping into the crate-global error type can be done in the EventDispatcher internal impl I think:

calloop/src/sources/mod.rs

Lines 148 to 160 in 7198d71

	fn process_events(
	&self,
	readiness: Readiness,
	token: Token,
	data: &mut Data,
	) -> std::io::Result<PostAction> {
	let mut disp = self.borrow_mut();
	let DispatcherInner {
	ref mut source,
	ref mut callback,
	} = *disp;
	source.process_events(readiness, token, |event, meta| callback(event, meta, data))
	}

[detly]

Okay I'll code it up and see if I hit any snags.

[detly]

The associated type approach works very well! I kept the custom errors, but reduced their scope to internal errors that might need to bubble up to the public API. For actual user implemented event sources, they don't really need to be used unless you're doing extremely bespoke things in the (un|re|)register functions.

I still need to update the examples and book, but I'll leave that until you've had a look at the latest approach.

[elinorbgr]

I quite like how this is getting at, nice 👍

On thing though, I was mostly expecting that each event source could have its own error type, to be able to express errors in a way that is insightful for the user, rather than just reuse the crate error type. Though this is mostly boring stuff, that I can take care of once your PR is merged if you don't feel like making all those detailed error types.

[detly]

They could, I only did that for the crate's own event sources because it... sort of... seemed more ergonomic. That's not a strong opinion though. If they didn't use a common error type, how would you break it down? Maybe:

• Generic - std::io::IoError (these are kind of a pain to work with, but isolating it to the one source makes it easier)
• Timers - probably just its own extremely simple wrapper type? What goes wrong with a timer?
• Ping - maybe std::io::IoError again?
• Channels - similar to ping. Note that the only tricky error to deal with is in send(), which is the caller's problem. process_events() is pretty simple.
• Executor - probably a futures-related error?

Let me know, I don't mind coding it up, or at least a first pass at it.

[detly]

Take a look at this, I think it's closer to what you meant. Once I started writing it, it made a lot of sense.

[elinorbgr]

Ah yes, sorry for not answering your previous message, I have a lot on my plate these days.

Indeed, I really like the shape it takes, in particular each event source having its own opaque error type leaves room open for future changes (like #9 or #15).

[elinorbgr]

Ok, I quite like the shape of this PR as it is now. 👍

Only missing things for are now a changelog entry and fixing of the FreeBSD CI.

Also, do you want to rebase your PR to make a clean commit history, or shall I squash the when merging it?

[detly]

Ah yes, sorry for not answering your previous message, I have a lot on my plate these days.

Hah, absolutely no problem. It took me almost a year to actually write it, so I understand.

Only missing things for are now a changelog entry and fixing of the FreeBSD CI.

Ah I overlooked FreeBSD, I'll fix that. Also the book and examples to some extent.

Also, do you want to rebase your PR to make a clean commit history, or shall I squash the when merging it?

No, leave it to me to clean up, I've already done that a few times behind the scenes anyway.

[detly]

Updated changelog, fixed BSD and squashed the commits into code, book and changelog. Feel free to start reviewing, but wait before merging - I'm going to test these changes on some real projects and see how they fare.

[detly]

Recent change: I realised as I was making my changes that I needed to add a couple of paras in the new book chapter on error handling. See if you agree with the following:

As a rule, if you implement EventSource you should try to split your errors into two different categories:

• Errors that make sense as a kind of event. These should be a part of the Event associated type eg. as an enum or Result.
• Errors that mean your event source simply cannot process more events. These should form the Error associated type.

For an example, take Calloop's channel type, calloop::channel::Channel. When the sending end is dropped, no more messages can be received after that point. But this is not returned as an error when calling process_events(), because you still want to (and can!) receive messages sent before that point that might still be in the queue. Hence the events received by the callback for this source can be Msg(e) or Closed.

However, if the internal ping source produces an error, there is no way for the sending end of the channel to notify the receiver. It is impossible to process more events on this event source, and the caller needs to decide how to recover from this situation. Hence this is returned as a ChannelError from process_events().

Another example might be an event source that represents a running subprocess. If the subprocess exits with a non-zero status code, or the executable can't be found, those don't mean that events can no longer be processed. They can be provided to the caller through the callback. But if the lower level sources being used to run (eg. an asynchronous executor or subprocess file descriptor) fail to work as expected, process_events() should return an error.

[detly]

Hold up, I just realised a way I can simplify a few things. My last comment still applies, but wait a bit before reviewing the code.

[elinorbgr]

The paragraph you proposed seems like a good explanation to me! 👍

[detly]

Okay, I think this is a lot closer to what you'd hoped for. I realised that if the associated type is constrained by Into<Box<dyn Error>>, all the common flexible error types can be used to (Box<dyn Error>, anyhow, snafu, etc.). I also simplified the crate-specific error because I had applied the different variants a bit inconsistently, and I figured I wasn't actually adding much info in doing that. This all meant I could delete a tonne of stuff that I previously added to the book, which is nice.

[detly]

There is one thing this breaks that might not be obvious: std::io::Error is sync + send, but calloop::Error is not. If users previously relied on applying ? to an API function result to convert it into something that is sync + send, they'll get an error:

fn main() -> anyhow::Result<()> {
	let _ = calloop::EventLoop::<()>::try_new()?;
	Ok(())
}

previously compiled, but now gives:

error[E0277]: `(dyn std::error::Error + 'static)` cannot be sent between threads safely
   --> src/main.rs:5:45
    |
5   |     let _ = calloop::EventLoop::<()>::try_new()?;
    |                                                ^ `(dyn std::error::Error + 'static)` cannot be sent between threads safely
    |
    = help: the trait `Send` is not implemented for `(dyn std::error::Error + 'static)`
    = note: required because of the requirements on the impl of `Send` for `Unique<(dyn std::error::Error + 'static)>`
    = note: required because it appears within the type `Box<(dyn std::error::Error + 'static)>`
    = note: required because it appears within the type `calloop::Error`
    = note: required because of the requirements on the impl of `From<calloop::Error>` for `anyhow::Error`

This is tricky though. Making Calloop's error sync + send means that errors going into it need to be the same, which adds complexity to user code in other places. On the other hand, these are top-level functions and it's not hard to just convert the errors into strings or change the error type for main. Let me know what you think.

[detly]

Hmm, this has some other implications too. Let me do some more work on that aspect of things.

[detly]

Okay, I reached a compromise. Our new crate::Error is still Sync + Send. This makes it work much better with any async code, and is more general for other libraries to consume.

The EventSource::Error associated type isn't Sync + Send. This makes it work with more error types a user might want to use. BUT it does mean that consuming it in libraries that want Sync + Send requires "flattening" it into a string. That's what I do in src/sources/mod.rs:167. This removes any ability to downcast it to a concrete type later.

I went with flattening them into strings, since it's unlikely that errors which propagate all the way out of the event loop will need to be downcast anyway - probably a user will need to either restart the event loop or the entire program. It's more likely to be useful to let users use a wider variety of errors in that associated type so they can compose them easily.

[elinorbgr]

How restrictive would it be in practice to require that EventSource::Error is Send+Sync? Tbh I don't think I've ever encountered an error type that wasn't Send+Sync or couldn't easily be made to be.

[detly]

How restrictive would it be in practice to require that EventSource::Error is Send+Sync? Tbh I don't think I've ever encountered an error type that wasn't Send+Sync or couldn't easily be made to be.

You have two in Calloop in fact 😉 — both the InsertError and SendError can't be, because they "give back" the inserted source/sent message that failed. But those are both exceptions to the general rule, and can be made Sync + Send by destructuring to remove the offending value and pulling out the original source error.

So to answer your question honestly: in general, not very restrictive. While I was playing around with the different approaches, the only general purpose error library I found that didn't have these baked in was Snafu, and only then its general purpose Whatever type, and even then you can make your own equivalent that's Sync + Send with the proc macros in that crate.

If we went the other way, the only major annoyance would be if eg. a user used a library that bubbled up a non Sync + Send error type and had to do some conversions at the boundary (probably in process_events()), and that would consist of doing what I did in dispatch_events() ie. flattening it into a string and converting it to Box<dyn Error + Sync + Send>.

[elinorbgr]

You have two in Calloop in fact wink — both the InsertError and SendError can't be, because they "give back" the inserted source/sent message that failed. But those are both exceptions to the general rule, and can be made Sync + Send by destructuring to remove the offending value and pulling out the original source error.

Right, fair point. 😅

However, I'd argue that this kind of error (which give back a value) is not really errors that are subject to being forwarding from an EventSource right to the output of dispatch_events()... So I'm leaning towards just requiring EventSource::Error to be Send+Sync, and event source writers would convert them to string if necessary. It seems to me that it's the solution that gives the most control to event source writers, rather than unconditionally converting every source error to string.

[detly]

I'd argue that this kind of error (which give back a value) is not really errors that are subject to being forwarding from an EventSource right to the output of dispatch_events()

Exactly, typically they'd stay within the scope where the original value being "handed back" was created in the first place.

I'll put those bounds back on and tidy up some things.

[detly]

I've updated some projects to use this and I'm quite happy with how it's turned out. Go ahead and review it and let me know if you have any questions or feedback.

[elinorbgr]

Okay, so there is one point about specifically the error type for Generic, but other than that I really like this PR!

[elinorbgr]

Regarding the error type for Generic, given this source is really meant as a building block for other event sources, I wonder if it would be more convenient to instead make it possible to specify the error type, and so have a Generic<F, E>, with a default to E = std::io::Error for convenience.

That would make something like this:

struct Generic<F: AsRawFd, E=std::io::Error> {
    _error_type: std::marker::PhantomData<E>,
   /* the rest of the struct */
}

impl<F, E> EventSource for Generic<F, E> where F: AsRawFd, E: Error + Send + Sync + 'static {
    type Error = E;
    type Ret = Result<Postaction, E>;

    /* the rest of the impl */
}

This way, users that are fine with std::io::Error can just use Generic<F> with the default value for E, and users that want to customize it for their internal use still can. How does that sound?

[detly]

You know, I was just hacking around this with both the inotify and gpio crates, I think this would work quite well.

[detly]

Ah wait, this might require "generic associated types" which are still being implemented. I'll look into it.

[detly]

Nope, I was wrong, it works fine. Let me check with some non-std crates.

[detly]

Annoyingly the default type parameter doesn't actually help a lot of the time, but it's not onerous

So

Generic::new(420, Interest::READ, Mode::Level)

might need to become

Generic::<_>::new(420, Interest::READ, Mode::Level)

see https://stackoverflow.com/questions/57824584/why-rust-dont-use-default-generic-parameter-type

[elinorbgr]

We can use the same trick the stdlib uses to handle the Alloc parameter with collections, something like this:

struct Generic<F: AsRawFd, E=std::io::Error> {
    _error_type: std::marker::PhantomData<E>,
    fd: F
   /* the rest of the struct */
}

impl<F: AsRawFd> Generic<F, std::io::Error> {
    fn new(fd: F) -> Generic<F, std::io::Error> {
        Generic {
            fd,
            _error_type: std::marker::PhantomData,
        }
    }
    
    fn new_with_error<E>(fd: F) -> Generic<F, E> {
        Generic {
            fd,
            _error_type: std::marker::PhantomData,
        }
    }
}

[detly]

Nice, that worked. One more thing, should the trait bound be Into<Box<dyn Error + etc>> to match that on EventSource?

[elinorbgr]

Sounds good yes. 👍

[detly]

Extremely happy with this change, it leads to some good gains in error handling for generic-based event sources eg. I can use anyhow to propagate informative errors up to a point where I can log them, or use my own structured errors to manage things. I deleted about 200 lines of extension trait and error mapping code in my own project.

[elinorbgr]

What kind of future revision are you thinking about?

[detly]

I think, but I'm not 100% sure, that when GATs are stable you should be able to get rid of the phantom data marker and just have the <E> in the impl<F: AsRawFd, E: Into<Box<dyn Error etc>>> EventSource for Generic<F> instead.

[elinorbgr]

I don't think that will work, using GAT here would require changing the EventSource trait itself, and I'm not sure it'd be appropriate. I think that is going to stay as is, so I don't think this comment is necessary.

[detly]

Okay I re-read the RFC and blog post and you're right, I'll remove this.

[elinorbgr]

Alright! Sorry for the many rounds of review, I think this is now in a pretty good shape. There's just this comment about GAT that I don't think we should keep, but other than that, all is 👍

[detly]

Sorry for the many rounds of review

No, it's good! I think it ended up in really good shape.