Skip to content

Error Handling Overhaul - #286

Merged
mitchmindtree merged 17 commits into
RustAudio:masterfrom
mitchmindtree:error_handling
Jun 21, 2019
Merged

Error Handling Overhaul#286
mitchmindtree merged 17 commits into
RustAudio:masterfrom
mitchmindtree:error_handling

Conversation

@mitchmindtree

@mitchmindtree mitchmindtree commented Jun 20, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

The aim of this PR is to overhaul the way that errors are handled throughout the crate as discussed within #268.

I will continue to update the following list and commit to this PR as I make progress. Feel free to comment at any stage!

TODO:

  • Integrate the failure crate to make error implementation and propagation more ergonomic.
  • Rename existing error types to more closely match the methods from which they may be returned. Carry this on going forward as a general convention.
  • Review every function of the synchronous API and their implementations within each of the different backends. Remove panic!s and improve error handling where necessary, which may include adding new error types or new variants to existing error types.

Follow-up PR:

  • Change the user callback to emit an Event enum as described here.

@mitchmindtree
Remove std Error implementations in favour of using failure
1275db8 
This will make adding new errors in the following commits towards better
error handling a lot easier.
@mitchmindtree
Rename FormatsEnumerationError to SupportedFormatsError
0f27c1e 
This more tightly associates the error with the device method on which
this might occur.
@mitchmindtree
Rename CreationError to BuildStreamError
cf84ab9 
For clarity and to tie the name more closesly to the methods within from
which it may be returned.
@mitchmindtree
Add BackendSpecificError. Add new DevicesError.
42fc702 
See the documentation for both new errors for details.

The new `DevicesError` has been added to allow for returning errors when
enumerating devices. This has allowed to remove multiple potential
`panic!`s in each of the alsa, coreaudio and wasapi backends.
@mitchmindtree mitchmindtree mentioned this pull request Jun 20, 2019
Closed
@mitchmindtree
Change SupportedFormatsError::Unknown variant to BackendSpecific
78a7cb9 
This allows for also passing through a description of the
unknown/platform-specific error.
@mitchmindtree
Add new DeviceNameError type
105086a 
The coreaudio and wasapi backends may both potentially fail to produce
the name associated with a device. This changes the API to allow for
returning the errors in these cases.
@mitchmindtree
Update examples for DeviceNameError
fbb97f5
@mitchmindtree
Improve handling of the DefaultFormatError
f0e4e31
@mitchmindtree
Improve handling of BuildStreamError throughout crate.
ba8d354
@mitchmindtree
Add PlayStreamError and PauseStreamError.
eae0e18 
This allows for properly handling potential failure on macOS. We should
also consider propagating the mutex/channel poison errors through these
new types, especially considering the potential removal of the event
loop in favour of switching over to high-priority audio threads on
windows and linux.
@mitchmindtree
Fix compile errors on macOS
a733bdb
@mitchmindtree mitchmindtree changed the title [WIP] Error Handling Overhaul Error Handling Overhaul Jun 21, 2019
@mitchmindtree

mitchmindtree commented Jun 21, 2019

Copy link
Copy Markdown
Member Author

OK this should be just about ready to go, but I still want to test the examples on macOS and check that windows is compiling and running correctly (appveyor seems to be erroring out early on something unrelated).

cc @ishitatsuyuki would you mind reviewing this if you get a chance in the meantime? It would be great to get your thoughts.

@ishitatsuyuki

ishitatsuyuki commented Jun 21, 2019

Copy link
Copy Markdown
Collaborator

Here are my thoughts after roughly looking through:

  • The AppVeyor failure is likely due to failure pulling in a new dependency that requires C compiler. We should grab MinGW GCC somehow, or test with MSVC instead.
  • Do you think it's better to make use of Termination (? in main())? Personally I prefer it bacause it's shorter to write that unwrap, and also displays the error more cleanly.

Regarding error types on play/pause methods: should we make it synchronous as described in the last line of #278 (comment)? Running the operations synchronously will allow us to return error (if it exists) on all platforms, making it more consistent.

@mitchmindtree

mitchmindtree commented Jun 21, 2019

Copy link
Copy Markdown
Member Author

The AppVeyor failure is likely due to failure pulling in a new dependency that requires C compiler. We should grab MinGW GCC somehow, or test with MSVC instead.

Ahh I see, I'm quite unfamiliar with appveyor, do you happen to know off the top of your head what needs to be added to our appveyor.yml to get this working again?

Do you think it's better to make use of Termination (? in main())? Personally I prefer it bacause it's shorter to write that unwrap, and also displays the error more cleanly.

This sounds good to me! Do you still get the line number of the source of the error? I guess this error overhaul should make it clear where the error is coming from anyway. I'll give this a go!

Regarding error types on play/pause methods: should we make it synchronous as described in the last line of #278 (comment)? Running the operations synchronously will allow us to return error (if it exists) on all platforms, making it more consistent.

Yeah I agree we should. I kind of had this in mind while making the new PlayStreamError and PauseStreamError types, I figured that while they're basically just wrappers around BackendSpecificError for now, we could expand upon them as we work through #278. Do you mind if this work waits until #278?

In the meantime I'm going to spin up a VM to test windows locally, and see if I can borrow a friend's MBP to test the coreaudio backend.

@ishitatsuyuki

ishitatsuyuki commented Jun 21, 2019

Copy link
Copy Markdown
Collaborator

do you happen to know off the top of your head what needs to be added to our appveyor.yml to get this working again?

Sorry no, I only looked at the error message and I have no experience with AppVeyor.

Do you mind if this work waits until #278?

Yes, that was just a by-the-way proposal. Let's revisit after we redesign things around EventLoop.

@mitchmindtree

mitchmindtree commented Jun 21, 2019

Copy link
Copy Markdown
Member Author

OK I've tested locally on both wasapi and coreaudio and can confirm everything compiles and that the examples all run as well as they did beforehand. I'm going to do some investigating into this appveyor issue now.

@ishitatsuyuki

ishitatsuyuki commented Jun 21, 2019

Copy link
Copy Markdown
Collaborator

Tip: you can check code by installing the x86_64-pc-windows-gnu target on Linux and then use either Cargo or RLS.

@mitchmindtree
Try using a slightly more modern yml to address appveyor build bug
61c3520 
This uses the script from here with all comments stripped:

https://github.com/starkat99/appveyor-rust/

The beta targets have also been removed to keep build times low.
@mitchmindtree
Update lib docs to allow for testing on devices with no audio
2667547
@mitchmindtree
Remove 32-bit appveyor targets. Only build nightly on MSVC.
d1a4f45 
As appveyor only does one job at a time, this should help to keep
appveyor builds under 5 minutes... Otherwise we're looking at 15+ min
build times.
@mitchmindtree
Remove gnu target to avoid need to install gcc for cc crate
e2ec0ea
@mitchmindtree
mitchmindtree merged commit 9cc5df8 into RustAudio:master Jun 21, 2019
This was referenced Jun 21, 2019
Closed
Closed
Closed
@mitchmindtree mitchmindtree mentioned this pull request Jun 21, 2019
Closed
@mitchmindtree
mitchmindtree deleted the error_handling branch June 21, 2019 16:33
mitchmindtree added a commit to mitchmindtree/cpal that referenced this pull request Jul 5, 2019
@mitchmindtree
Update CPAL to version 0.10.0.
391b32f 
This is quite a significant update for CPAL, including a number of
breaking changes. Here is a list of the breaking changes along with
links to where you can find more information:

- A `Host` API has been introduced in RustAudio#289 along with a follow-up
  refactor in RustAudio#295. Please see the examples for a demonstration of how
  to update your code. The necessary changes should hopefully be
  minimal. If this has caused you any major difficulty please let us
  know in an issue!
- An ASIO host has been introduced in RustAudio#292. This adds support for
  Steinberg's ASIO audio driver API on Windows. Please see the ASIO
  section of the README for more information on how to setup CPAL with
  ASIO support for your project.
- The user callback API has been overhauled to emit `StreamEvent`s
  rather than buffers in order to support handling stream errors. RustAudio#288.
- Error handling in general was overhauled in RustAudio#286. This meant taking
  advantage of the `failure` crate and adding support for
  backend-specific errors with custom messages. Many unnecessary
  `panic!`s have been removed, but a few remain that would indicate bugs
  in CPAL.

In general, checking out the updated examples will be the easiest way to
get a quick overview on how you can update your own code for these
changes.

The CHANGELOG.md has been updated to include these changes.
@mitchmindtree mitchmindtree mentioned this pull request Jul 5, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mitchmindtree @ishitatsuyuki