Use failure

[richard-uk1]

I had an issue with alsa recently where I had updated my computer but was using an old kernel, so trying to load a .so failed, and cpal panicked. I've done an example of how using failure might work with cpal here. If I was using this PR's version of cpal, I would have got info on the specific IO error, and moreover could have chosen to continue without sound (without using catch_unwind).

I hope you'll consider this PR, or another similar one.

[richard-uk1]

friendly (and last) ping.

[torkleyy]

Looks good to me

[mitchmindtree]

I personally find this convention of grouping all possible errors that might occur within a library into a single Error type for all possible results quite counter productive. I used to advocate for this myself until I came across tomaka's error handling within the glium crate and realised how much nicer it is to have speciific errors for each part of the API, especially when you actually care about how to handle the errors (and not just creating nice ergonomic ways of propagating them up through APIs).

The reason for this is that single monolithic error types make it difficult to determine exactly what errors might occur when calling a specific function. E.g. I originally attempted to represent all errors that might occur within the coreaudio crate with a single error type, but as a result it has been really difficult to determine what errors might actually occur and to translate them to the relevant errors in CPAL in the coreaudio backend. (It's hard to remedy in this case though when even Apple won't document exactly what errors might occur).

By having unique errors per method and function call as necessary it is clearer to see what errors might actually occur just by looking at the API. It is nice to be able to match on an error enum and specifically handle each possible error exhaustively, knowing that these are all the things that might go wrong when making a specific call. When errors are all grouped into a single type, we lose this benefit of rust in that we end up having to ignore a large set of errors that can never happen when calling certain methods, most often with a wildcard. Then, if new error kinds are added in updates that might actually happen, the compiler can't help us with its exhaustiveness check as we have a wildcard ignoring or propagating the new error along with the rest of the error kinds that can't actually occur in this call.

All this said, I totally agree that CPAL's error handling needs a lot of attention! The benefits you mention in your OP certainly seem appealing - I just wanted to voice my concern about the the monolithic error approach. It obviously has a lot of precedence with std::io::Error and other library errors, but to be honest I find these APIs frustrating for all reasons mentioned above and do not think they are a good example to follow. Although more verbose, I think I'd prefer if we kept errors broken up into more unique, specific error types that better describe the possible things that might go wrong per-method/function.

[torkleyy]

@mitchmindtree I agree that having specific error types is often helpful, especially when you need to handle the error. Thus, the return type of API functions should be as specific as possible. However, for the user it might be that they don't or don't directly handle it, but just cancel the function and log it at a later point. In that situation a combined error type is convenient and actually improves the type safety as the alternative wouls be boxing it up into a trait object.

TL;DR: use specific return types in the library, expose the combined type just for the user. That's worked very good in Specs for me.

[richard-uk1]

My main motivation for this PR was to avoid panics that I was getting at present. However, I agree that it can be useful to know exactly what errors you may get from a particular function.

I think there are 2 approaches we could take

1. Accept this PR as a stop-gap and look to a future PR to improve errors.
2. Discuss how errors should be on this thread.
3. Do nothing

OK, that's 3, but you get the idea.

It obviously has a lot of precedence with std::io::Error

I think this is the way it is so that io::Error can be cross-platform. Most of the standard library uses specialised error types restricted to only errors that may actually occur.

[mathstuf]

Note that failure_derive is reexported from failure and not needed as a direct dependency.

[mathstuf]

Just remove this line and use the macro from failure.

[mathstuf]

Could these reformatting changes be removed from this PR and made in a separate one? It makes reviewing the changes here much harder.

[mathstuf]

Style: IMO, imported top-level functions are best used scoped. Here, I think failure::err_msg would be better.

[mathstuf]

Hmm. It seems to me that near every check_errors is followed by map_err. Maybe it can just return Result<(), AlsaError> (AlsaError being a new type)?

[richard-uk1]

I've been looking at this again.

I think that failure is probably the wrong approach. I've come to realize that often io::Error is the correct error abstraction at the lowest level when interacting with C dependencies using errno-style error reporting.

I've been looking over the alsa backend and I think there's scope to improve the error handling a lot - removing some of the panics/unwraps/expects. You can write the check_errors function like so:

#[inline]
fn check_errors(errno: libc::c_int) -> io::Result<()> {
    if errno < 0 {
        Err(err_from_code(errno))
    } else {
        Ok(())
    }
}

/// Convert error code to io::Error (from alsa source)
// This is on the slow path, so maybe mark to avoid inlining.
fn err_from_code(errnum: libc::c_int) -> io::Error {
    use std::ffi::CStr;

    let errnum = errnum.abs();
    match io::Error::from_raw_os_error(errnum).kind() {
        io::ErrorKind::Other => {
            // If we don't recognise the error code, use strerror from alsa
            let msg = unsafe {
                CStr::from_ptr(alsa::snd_strerror(errnum))
                    .to_str()
                    .unwrap_or("Invalid utf-8 in error message")
            };
            io::Error::new(io::ErrorKind::Other, msg.to_owned())
        },
        kind => kind.into(),
    }
}

and structure the error nicely as an io::Error for user consumption.

Basically, I don't want to do any work on this unless it's likely to be accepted, so what do people think of the idea?

[mathstuf]

I think std::error::Error being fixed makes this a lot less urgent.

[richard-uk1]

I agree. I'd still like to do some work on better error reporting, converting unwrap and expect into io::Errors.

In alsa error codes basically correspond to io::Error, except for a few errors that don't happen very often. So we can use io::Error to report errors, and wrap any extra info as the inner error.

Regarding extra info, there are some debug utilities in alsa that give you info on what your current config is etc. We could perhaps have a debug_verbose feature that included all this data in the error.

All this would be incredibly useful to me - I still cannot use cpal because the set_hw_params fails and cpal then crashes. aplay works so there must be something wrong with the way cpal sets up the pcm. Having this debug data would help me find the problem.

What do you think about this? Would you accept a PR along these lines?

[mathstuf]

I'm just another contributor, so I'm not the one to answer the last question.

However, I don't think it should be a feature flag. If the Debug impl wants to grab the extra info on-demand, that might be better if it's expensive (rather than grabbing it up front and then throwing it away). If it's cheap to query/store, just making it part of the error returned would be more useful.

[richard-uk1]

Your debug idea is better because there is some allocation involved. I might make up a PR just for demonstration processes. I'm not sure how this crate is maintained now, or who the maintainers are.