Better error handling interface

The codebase is currently filled extensively with expect() and unwrap(). The thing is that device disconnect does happen, and we don't want to crash the entire application/game when your Bluetooth connection get lost.

A brief idea is to add some error event/callback to event loop. Probably need some more research on how other libraries do this.

@ishitatsuyuki

Right now I see two main areas of improvement for error handling that need to be addressed.

1. Synchronous API functions

Currently it is possible to return errors properly in functions like default_output_device() and device.default_output_format(), however in some cases there aren't enough descriptive variants on the associated error enum to provide enough information. As a result, this leads to

panic!ing instead as there is no matching error variant
lumping in errors that are only vaguely related to the actual variant

This leads me to believe that we need a more robust error type for handling these cases. Here are some ideas for improvements that come to mind:

Adding more variants to the error enum. Enum variants are especially useful on errors when actually handling them. E.g. if an attempted format was incompatible, we can match on the associated variant and instead try another format. I think it would be worth reviewing the kinds of errors that can occur in each of the different functions across each of the different backends to see what kind of common variants we can add. I think it might be worth adding variants even if they are only specific to one or two backends. We could add doc comments describing which platforms these errors can occur on. CPAL users can then also be aware of the full set of potential errors that might occur across each of the different backends. This would surely be more useful than only offering the ability to handle the subset of errors that can occur on every different backend.
Allowing to provide backend-specific error causes. While the actual errors that occur could be described in the error enum associated with each function, the cause for the error might be different depending on the platform. Perhaps it's worth adding a cause: String field or something along these lines that allow for adding the word-for-word cause provided by the platform's specific error message?

2. Errors that occur while running the callback

These are errors that occur during the "looping" process where the EventLoop waits on file descriptors or some other fence until the host is ready to receive or deliver more audio and the user callback is called. Often-times this is on a separate thread to which the EventLoop is run (macOS comes to mind).

@ishitatsuyuki yes it seems like some sort of callback approach might be the only way to handle errors directly where they occur. I think I like your idea of providing a separate callback specifically for errors. I guess the other approach would be to change the user audio callback argument to something like Result<Buffer, SomeError>? Either way, both of these options would allow the user to do any cleanup necessary of state in the audio callback and potentially send the error over a channel to the GUI/CLI thread to provide feedback to users 👍

Allowing to provide backend-specific error causes.

I guess this is what crates like failure helps. Let's adopt it.

Errors that occur while running the callback

I'm not actually sure on this. The Android AAudio API is a bit weird: the error callbacks happens on a dedicated thread (which is neither caller thread or playback processing thread), and for some reason you cannot directly perform stream operations like stop or pause within the callback. (ref)

We could deliver those events to the same callback (discriminated by an enum), but that could be somewhat unexpected behavior because on OSes where data callback occurs on a high priority thread, you rarely have access to ways to wake it up. Which means that events are only delivered (polled) when data is demanded.

I guess this is what crates like failure helps. Let's adopt it.

Yes I've been using failure in some other projects more recently and it certainly simplifies this process.

cannot directly perform stream operations like stop or pause within the callback

Hmm maybe the safest option then is to go with your original idea of having a dedicated error callback, especially if there's a chance stream-running related errors might be signalled on totally different threads anyway? We can then leave it up to users to work out their own synchronisation on how to handle those errors (e.g. using a dedicated channel or Mutex or whatever suits them). If we do have information about whether or not errors can be synchronised on certain platforms, perhaps we can just document this instead?

@ishitatsuyuki

@ishitatsuyuki have you made a start on this? If not, would you like to? If not, let me know and I'll make a start next time I get a chance.

I don't plan to work on code this week. I would leave a note when I start, so feel free to go ahead.

Hmm maybe the safest option then is to go with your original idea of having a dedicated error callback, especially if there's a chance stream-running related errors might be signaled on totally different threads anyway?

Personally I think error callback forms a pretty hard-to-use API.

I think the only error we should expose on all platforms is device disconnect, and for that purpose we should not pass the burden of synchronization to users.

I think the only error we should expose on all platforms is device disconnect, and for that purpose we should not pass the burden of synchronization to users.

I agree that it would be nice to remove this burden from users if possible. On the other hand, my understanding of the goal for CPAL is to be a close-to-zero-cost-as-possible abstraction over audio I/O in a cross-platform manner. I think if the callback is the only safe approach that is actually zero-cost (e.g. doesn't require us making synchronisation decisions for the user) and provides consistent behaviour across platforms, I think it's a totally reasonable approach. At least having a zero-cost approach allows for higher-level, more ergonomic approach to be built on top of CPAL. All that said, if you have another approach in mind I'd be happy to hear it!

Thinking on this more, I guess we could make this a little easier using the Result<Buffer, StreamError> approach I mentioned earlier, then on platforms where error reporting occurs on a separate thread, we can just use a Mutex around the user callback? There should never be any contention over the lock during successful playback, and in the case of an error such as a device disconnect, the platform's error callback can acquire the lock as I guess the error implies that the stream has stopped anyway? This way, the user doesn't have to try and synchronise their audio state across both a separate error handling callback and the data stream callback.

Come to think of it, I think we already have no choice but to use a Mutex around the user callback anyway, as the user callback already needs to be shared between different device streams, which means contention is already possible if two devices attempt to deliver/request data at the same time. This is of course a pretty glaring issue as locking on the audio thread can cause big issues like missing deadlines, thread de-scheduling, etc. In my mind this makes the case for #278 even stronger.

Perhaps rather than Result, some other custom enum would be a better option and allow us to also notify the user of successful cases. Perhaps something like this?

pub enum Event<'a> {
    Data(Buffer<'a>),
    Play,
    Pause,
    Close(CloseCause),
}

pub enum CloseCause {
    User,
    Error(StreamError),
}

I’m also thinking of using Result or whatever enum, but without using Mutex. I think the only platform that is “weird” is Android, and on other platforms you can either signal the thread (idea similar to eventfd) or simply the error callback runs on the same thread as data. (No idea about ASIO because it’s proprietary - please correct me. So yeah, when needed a channel will be used to deliver error.

Regarding locking the callback unconditionally - that definitely sounds like what we want to avoid. Should any of the backends being unable to synchronize before calling callback (without us implementing it), we would just get rid of the functionality of running multiple threads with one callback.

I'm leaning toward having a dedicated error callback again.

The primary reason, is that Windows turned out to implement this in a way similar to Android - IMMNotificationClient is called in an internal thread. I don't think we can assume that data and notification callbacks doesn't race, which means we would resort to Mutex to be safe.

Apart from that, code handling data and device disconnect tends to have nothing in common, so it makes sense to have the closure separately. We can make the user to set both at once though, to prevent the user from forgetting about errors completely.

Navigation Menu

Repository navigation

Better error handling interface #268

1. Synchronous API functions

2. Errors that occur while running the callback

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Better error handling interface #268

Description

Activity

mitchmindtree commented on Jun 5, 2019

1. Synchronous API functions

2. Errors that occur while running the callback

ishitatsuyuki commented on Jun 6, 2019

mitchmindtree commented on Jun 6, 2019

mitchmindtree commented on Jun 6, 2019

ishitatsuyuki commented on Jun 7, 2019

mitchmindtree commented on Jun 7, 2019

mitchmindtree commented on Jun 7, 2019

mitchmindtree commented on Jun 7, 2019

ishitatsuyuki commented on Jun 7, 2019

ishitatsuyuki commented on Jul 6, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions