Error cannot be sent/shared between threads safely

In case of fallible conversions, the original value is often returned as part of the error, e.g. the implementation of TryFrom<Vec<T, A>> for [T; N] has an error type which is Vec<T, A>. Another example is FromUtf8Error, which allows getting "back the byte vector that was used in the conversion attempt". See FromUtf8Error::into_bytes.

This helps to not lose the original value.

I did something similar in sandkiste::errors::DatumConversionError:

pub struct DatumConversionError<D> {
    pub original: D,
    pub details: &'static str,
}

However, this results in DatumConversionError<D> to be able to be !Send and !Sync if the type D is !Send or !Sync respectively.

Consequently, I cannot convert this to an anyhow::Error, because that requires the error to be Send + Sync.

To demonstrate the problem, I made a small Playground example:

use std::fmt;
use std::marker::PhantomData;

#[derive(Debug)]
pub struct Datum {
    // this is `!Send` and `!Sync`:
    _phantom: PhantomData<*mut ()>,
}

#[derive(Debug)]
pub struct DatumConversionError {
    // we return the original `Datum`:
    pub original: Datum,
    pub details: &'static str,
}

impl fmt::Display for DatumConversionError {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        f.write_str(self.details)
    }
}

impl std::error::Error for DatumConversionError {}

impl TryFrom<Datum> for String {
    type Error = DatumConversionError;
    fn try_from(value: Datum) -> Result<String, DatumConversionError> {
        Err(DatumConversionError {
            original: value,
            details: "not implemented",
        })
    }
}

#[tokio::main]
async fn main() -> anyhow::Result<()> {
    let datum = Datum {
        _phantom: PhantomData,
    };
    let _s: String = datum.try_into()?;
    Ok(())
}

(Playground)

Errors:

   Compiling playground v0.0.1 (/playground)
error[E0277]: `*mut ()` cannot be sent between threads safely
  --> src/main.rs:40:38
   |
40 |     let _s: String = datum.try_into()?;
   |                                      ^ `*mut ()` cannot be sent between threads safely
   |
   = help: within `DatumConversionError`, the trait `Send` is not implemented for `*mut ()`
   = help: the following other types implement trait `FromResidual<R>`:
             <Result<T, F> as FromResidual<Result<Infallible, E>>>
             <Result<T, F> as FromResidual<Yeet<E>>>
   = note: required because it appears within the type `PhantomData<*mut ()>`
note: required because it appears within the type `Datum`
  --> src/main.rs:5:12
   |
5  | pub struct Datum {
   |            ^^^^^
note: required because it appears within the type `DatumConversionError`
  --> src/main.rs:11:12
   |
11 | pub struct DatumConversionError {
   |            ^^^^^^^^^^^^^^^^^^^^
   = note: required for `anyhow::Error` to implement `From<DatumConversionError>`
   = note: required for `Result<(), anyhow::Error>` to implement `FromResidual<Result<Infallible, DatumConversionError>>`

error[E0277]: `*mut ()` cannot be shared between threads safely
  --> src/main.rs:40:38
   |
40 |     let _s: String = datum.try_into()?;
   |                                      ^ `*mut ()` cannot be shared between threads safely
   |
   = help: within `DatumConversionError`, the trait `Sync` is not implemented for `*mut ()`
   = help: the following other types implement trait `FromResidual<R>`:
             <Result<T, F> as FromResidual<Result<Infallible, E>>>
             <Result<T, F> as FromResidual<Yeet<E>>>
   = note: required because it appears within the type `PhantomData<*mut ()>`
note: required because it appears within the type `Datum`
  --> src/main.rs:5:12
   |
5  | pub struct Datum {
   |            ^^^^^
note: required because it appears within the type `DatumConversionError`
  --> src/main.rs:11:12
   |
11 | pub struct DatumConversionError {
   |            ^^^^^^^^^^^^^^^^^^^^
   = note: required for `anyhow::Error` to implement `From<DatumConversionError>`
   = note: required for `Result<(), anyhow::Error>` to implement `FromResidual<Result<Infallible, DatumConversionError>>`

For more information about this error, try `rustc --explain E0277`.
error: could not compile `playground` due to 2 previous errors

I wonder what's the best way to solve this issue? I feel like it's idiomatic to return the original value as part of the error. But I also feel like it's idiomatic to ensure that errors are Send + Sync. :exploding_head:

I guess I could require the caller to convert the error using .map_err(SomeOtherError::from) before invoking the ? operator. But this kinda ruins the anyhow fluff of being able to simply not care about error handling that much.

I don't have an elegant-to-use solution for you, but the way I like to think about this problem is that the cases of returning the original input aren't really general purpose error values of the sort that should be put into anyhow::Error; they're doing something else, that is slightly incompatible, with the data flowing through the error path. (Note that in the TryFrom<Vec<T, A>> for [T; N] case, the error type does not even implement Error, though that isn't required for this situation.)

A general principle of error propagation is that the error information has to be expressed in a form that will be intelligible to the caller, not solely the internal details of the callee; and this occurs recursively across many layers of abstraction. Stripping out the raw value being operated on happens to be necessary in this case, but it is also often good practice, because that value should be replaced with a higher level explanation or formatting. (Where did that value come from? What is the right Displayish formatting for the datum, given knowledge of its contents? Is it even appropriate to display it?)

So, treat this problem not as something needing a workaround, but as a reminder to improve error reporting.

As a library author, you can help out by making an error explanation type that doesn't include the data. Maybe that details: &'static str could be an enum or struct, and offer a conversion to that type that drops the data?

4 Likes

I found out that if I don't implement std::error::Error, I can provide my own conversion function into anyhow::Error:

+#[derive(Debug)]
+pub struct DatumConversionError2 {
+    pub details: &'static str,
+}
+
+impl fmt::Display for DatumConversionError2 {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.write_str(self.details)
+    }
+}
+
-impl std::error::Error for DatumConversionError {}
+impl std::error::Error for DatumConversionError2 {}
+
+impl From<DatumConversionError> for DatumConversionError2 {
+    fn from(value: DatumConversionError) -> DatumConversionError2 {
+        DatumConversionError2 {
+            details: value.details,
+        }
+    }
+}
+
+impl From<DatumConversionError> for anyhow::Error {
+    fn from(value: DatumConversionError) -> anyhow::Error {
+        DatumConversionError2::from(value).into()
+    }
+}

(Playground)

But this comes with some implications:

  • I need my crate (sandkiste in this case) to be aware of anyhow. I guess I could use a feature flag to enable anyhow support, but it would create an optional dependency, require following updates, etc., etc. It's not a nice solution.
  • The error type returned by try_from will no longer implement std::error::Error. This is also not really nice.

Okay, I see that stripping out that raw value is necessary. But I'm not yet sure where to do it? On every call? Should/could I provide a convenience method for it? An extension trait to Result? Or just provide a second error type (there exists already one: MachineError, which even has a corresponding conversion). Or should I consider making two methods, like .try_into() and .try_into_recoverable(); or .try_into() and .try_into_nonrecoverable()? I don't really know.

See link to MachineError above. Not sure what exactly you mean with "offer a conversion to that type that drops the data"? You mean a conversion from ErrorWithOriginalValue to ErrorWithoutThatValue?


P.S.: Note that std::string::FromUtf8Error does implement the std::error::Error trait.

I'm not sure how MachineError relates, but you started with

pub struct DatumConversionError<D> {
    pub original: D,
    pub details: &'static str,
}

and what I'm proposing is something like this:

pub struct DatumConversionFailure<D> {
    pub original: D,
    pub error: DatumConversionError,
}

// this could also be an `impl From<...`
impl<D> DatumConversionFailure<D> {
    fn into_error(self) -> DatumConversionError {
        self.error
    }
}

pub enum DatumConversionError {
    // info you would have put in the str...
}
impl Error for DatumConversionError {}

It's necessarily up to the caller to use .into_error() here. That's why I said I don't have an elegant solution.

2 Likes

I tried this. Unfortunately, Rust demands I use type annotations:

    //let _s: String = datum.try_into().map_err(DatumConversionError::from)?;
    let _s: String = datum.try_into().map_err(|x: DatumConversionFailure| x.into_error())?;
}

(Playground)

Is there any way to make this more concise?


I guess I could do something like this:

pub trait ResultExt {
    type OkType;
    type BareErr;
    fn drop_orig(self) -> Result<Self::OkType, Self::BareErr>;
}

impl<T> ResultExt for Result<T, DatumConversionFailure> {
    type OkType = T;
    type BareErr = DatumConversionError;
    fn drop_orig(self) -> Result<T, Self::BareErr> {
        self.map_err(Self::BareErr::from)
    }
}

And then:

    let _s: String = datum.try_into().drop_orig()?;

(Playground)

But it feels not good for some reason. I don't know why.

Too much indirection I suppose, between TryFrom to TryInto into a From into a ?.

This works:

    let _s = String::try_from(datum).map_err(|x| x.into_error())?;
1 Like

Oh thanks.

Hmmm… :thinking: still a bit verbose though in cases the type is already declared. It would be nice if I didn't have to duplicate the type in those cases.

I do have this convenience method though: MaybeString::try_into_string. I guess with this one, it would work too.

I further wonder if perhaps I should change the return type of this convenience method to give me the "throwable" error, and keep try_into() as returning the "recoverable" error/failure. Or vice versa. I do believe it's idiomatic to make try_into() return errors that might not even implement error, so perhaps try_into() should return a Result<_, DatumConversionFailure> and try_into_string() could return a Result<_, DatumConversionError>, which is Send + Sync. But it all still feels pretty fragile and/or confusing to a user of the API. It's not straightforward.

UFCS and drop the redundant closure:

let _s: String = datum.try_into().map_err(DatumConversionFailure::into_error)?;

Point-free style is not only nicer, it's lighter on inference, too. (In fact there's no inference in the point-free case here, and there was inference needed – and failed – in the redundant closure case.)

Oh I see, it's similar to what I wrote here:

But maybe your version is slightly better because it doesn't need to know about the type being converted into.

I'm still not fully happy with either solution. But maybe there is no better solution.

Thanks again for all your input. I decided to make an DatumConversionFailure type, which does not implement Error but contains an Error, just like what @kpreid suggested:

Whereas the "DatumConversionError" is named TypeMismatch in my case now.

As users of the API often won't need the original, several convenience methods (like sandkiste::types::MaybeString::try_into_string) automatically convert the DatumConversionFailure into a TypeMismatch error.

While this solution works for me, I feel like it is still a problem in the generic case, and I feel like Rust is suboptimal in that matter. I miss a trait like ToError which could convert some "failure" into an error that can be used for reporting rather than recovery).

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.