[roderickvd]

This is my last design goal for v0.18: a unified error type inspired by std::io::Error.

This new cpal::Error replaces HostUnavailable, DevicesError,SupportedStreamConfigsError, and all other per-operation error types in favor of a single Error/ErrorKind with an optional descriptive message.

While there, I took the opportunity to reduce the number of poisoned-lock panics, and making the timestamps on PipeWire infallible.

[tachibanayui]

While we working on revamping Errors, should we consider capture backtrace and/or retriving source (inner error)? It makes debugging easier when we have the original error from host backends instead of a generic Other error

[roderickvd]

In this implementation I tried to address that use case by capturing e.to_string() from every backend error into the message field, which covers the "what was the original error?" question without capturing its source.

If in addition to that we'd add source(), then that'd have some downsides: it would require storing Box<dyn Error> or Arc<dyn Error + Send + Sync>, breaking the Clone + PartialEq + Eq + Hash derives. The platform-specific inner types also aren't part of cpal's public API, so downcasting to them would create cross-platform coupling.

I feel that with the message field, those downsides aren't worth it.

[tachibanayui]

I see, in that case I think we should make sure that host implementations don't forget to attach "context" when propogating error up, which I think we could do with some From<T> implementations or an .context() method similar to anyhow. Also this maybe insignificant but maybe the message field could use Cow<'static, str> to save a few allocs on static error messages (I don't know other hosts very well but I see wasapi could use a few)

[roderickvd]

I see, in that case I think we should make sure that host implementations don't forget to attach "context" when propogating error up, which I think we could do with some From<T> implementations or an .context() method similar to anyhow.

I think we have that, right? There are From<T> implementations, and there is a with_message to add that context.

Also this maybe insignificant but maybe the message field could use Cow<'static, str> to save a few allocs on static error messages (I don't know other hosts very well but I see wasapi could use a few)

Good idea, I'll look into it.

[tachibanayui]

I meant something like this:

// I type this directly in github so treat this as more like pseudo-code
impl Error {
	pub fn context(self, msg: &str) -> Self {
	    self.message = format!("{}\nCaused by: {}", msg, self);
	    self
	}
}

It is similar to host::wasapi::windows_err_to_cpal_err_message but generalized to support adding context.

[roderickvd]

I'm not sure the "Caused by:" format or similar is the right fit for cpal. As a general-purpose library, formatting error messages should be the downstream app's choice. If the concern is that backends might lose context when propagating errors, I think they already handle that today via with_message.

[tachibanayui]

If the concern is that backends might lose context when propagating errors, I think they already handle that today via with_message.

Well the function I proposed is just a helper for propogating errors so when we propogate errors we dont have to do:

pub(crate) fn build_output_stream_raw_inner() {
    // -- snip --
    let max_frames_in_buffer = audio_client.GetBufferSize().map_err(|e| {
	    windows_err_to_cpal_err_message::<BuildStreamError>(
	        e,
	        "failed to obtain buffer size: ",
	    )
    })?;

    // We can shorten it into
    let max_frames_in_buffer = audio_client.GetBufferSize().context("failed to obtain buffer size")?;
}

Although to have the above syntax work we need to redefine the helper function to an extension trait

pub trait ErrorExt{...}
impl<T, E: Display> ErrorExt for Result<T, E> {
    fn context(self, msg: &str) -> Result<T, Error> {...}
}

If a specific host want to process the inner error, they have option to format the string manually and call with_message.
I saw host::wasapi::windows_err_to_cpal_err_message get used quite frequently so it might be worth it to have the function available crate-wide

[roderickvd]

Ah, as convenience method. Yeah, we can do that.

[roderickvd]

I'm going to merge this so it gets some exposure. Please open an issue for any further points.

[tachibanayui]

Is there a particular reason it need to be 'static?

[roderickvd]

We didn't need more now but we can make it impl Display.

[tachibanayui]

I think we could remove the lifetime here and it would still compile. We don't need the static lifetime for the extension to work and it restrict the user to only string literals

Suggested change

	fn context(self, msg: &'static str) -> Result<T, Error>;
	fn context(self, msg: &str) -> Result<T, Error>;

we can make it impl Display.

This could work too

[roderickvd]

I'll change it to impl Display on master. Thanks for thinking along.