Making errors Sync

[withoutboats]

The failure crate requires that errors are Sync. Currently (but not in all previous versions) error-chain makes errors which are !Sync (because they have a Box<Error + Send> inside them).

It would be beneficial IMO to:

1. Provide a Sync adapter for errors. (This could be optimized to not actually use a Mutex except in the Box case, if you think its warranted).
2. Release a breaking change which reverts back to requiring Sync by default. This is probably at least a little controversial.

[bvinc]

I believe I'm being hit by a problem of my errors being !Sync. I'm implementing Read and Write, so I'm trying to wrap my errors in an io::Error. This should be easy, but it's impossible if my error is !Sync, because io:Error requires it to be Sync.

[tazjin]

This would be beneficial for those that don't want to lose the type-safety of error-chain but have to interface with crates using failure!

[jnicholls]

Agreed, wish there was at least an adapter out of the box. I'm having to translate my errors at the moment and it's a hassle :D

[rustonaut]

I just run into this too it's quite a bumper and very depressing if it's
in a (opt) dependency and you have to have Sync errors because of
another dependency/software architecture/...

[jnicholls]

I fully switched to failure as I find it to be a more robust and cross-cutting approach than error_chain that doesn’t do as much wrapping of dependency error types but instead promotes that you maintain your own errors and contexts, particularly when culminating many libs together into an application and working with all the error types.

On Thu, May 31, 2018 at 8:12 AM Philipp Korber ***@***.***> wrote: I just run into this too it's quite a bumper and very depressing if it's in a (opt) dependency and you have to have Sync errors because of another dependency/software architecture/... — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#240 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABPq0PhtK0QZO4NxOKqK3o4vEbsXVnwks5t394ngaJpZM4QXLNc> .

-- Sent from Gmail Mobile

[elichai]

Any news about this?
This cause a lot of troubles if you use failure::Error but use a library that uses error-chain

[rbtcollins]

Nobody seems to have disagreed with the thrust of this bug, but there are no PR's implementing it either (or at least not tagged appropriately), so its probably in the category of help-wanted.

[rbtcollins]

Oh huh, there it is. Ta. #241 Now I need to figure out how to get that to happen :P.

[rbtcollins]

One mildly ugly workaround if anyone else needs it is an adapter like this. Its not as pretty as straight coercion, but it does allow interop for things like anyhow and failure and so-on (more generically than the failure::SyncFailure that inspired it).

Sync adapter for error-chain

/// Inspired by failure::SyncFailure, but not identical.
///
/// SyncError does not grant full safety: it will panic when errors are used
/// across threads (e.g. by threaded error logging libraries). This could be
/// fixed, but as we don't do that within rustup, it is not needed. If using
/// this code elsewhere, just hunt down and remove the unchecked unwraps.
pub struct SyncError<T: 'static> {
    inner: Arc<Mutex<T>>,
    proxy: Option<CauseProxy<T>>,
}

impl<T: std::error::Error + 'static> SyncError<T> {
    pub fn new(err: T) -> Self {
        let arc = Arc::new(Mutex::new(err));
        let proxy = match arc.lock().unwrap().source() {
            None => None,
            Some(source) => Some(CauseProxy::new(source, Arc::downgrade(&arc), 0)),
        };

        SyncError { inner: arc, proxy }
    }

    pub fn maybe<R>(r: std::result::Result<R, T>) -> std::result::Result<R, Self> {
        match r {
            Ok(v) => Ok(v),
            Err(e) => Err(SyncError::new(e)),
        }
    }

    pub fn unwrap(self) -> T {
        Arc::try_unwrap(self.inner).unwrap().into_inner().unwrap()
    }
}

impl<T: std::error::Error + 'static> std::error::Error for SyncError<T> {
    #[cfg(backtrace)]
    fn backtrace(&self) -> Option<&Backtrace> {}

    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
        match self.proxy {
            None => None,
            Some(ref source) => Some(source),
        }
    }
}

impl<T> Display for SyncError<T>
where
    T: Display,
{
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        self.inner.lock().unwrap().fmt(f)
    }
}

impl<T> Debug for SyncError<T>
where
    T: Debug,
{
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        self.inner.lock().unwrap().fmt(f)
    }
}

struct CauseProxy<T: 'static> {
    inner: Weak<Mutex<T>>,
    next: Option<Box<CauseProxy<T>>>,
    depth: u32,
}

impl<T: std::error::Error> CauseProxy<T> {
    fn new(err: &dyn std::error::Error, weak: Weak<Mutex<T>>, depth: u32) -> Self {
        // Can't allocate an object, or mutate the proxy safely during source(),
        // so we just take the hit at construction, recursively. We can't hold
        // references outside the mutex at all, so instead we remember how many
        // steps to get to this proxy. And if some error chain plays tricks, the
        // user gets both pieces.
        CauseProxy {
            inner: weak.clone(),
            depth: depth,
            next: match err.source() {
                None => None,
                Some(source) => Some(Box::new(CauseProxy::new(source, weak.clone(), depth + 1))),
            },
        }
    }

    fn with_instance<R, F>(&self, f: F) -> R
    where
        F: FnOnce(&(dyn std::error::Error + 'static)) -> R,
    {
        let arc = self.inner.upgrade().unwrap();
        {
            let e = arc.lock().unwrap();
            let mut source = e.source().unwrap();
            for _ in 0..self.depth {
                source = source.source().unwrap();
            }
            f(source)
        }
    }
}

impl<T: std::error::Error + 'static> std::error::Error for CauseProxy<T> {
    #[cfg(backtrace)]
    fn backtrace(&self) -> Option<&Backtrace> {}

    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
        match self.next {
            None => None,
            Some(ref next) => Some(next),
        }
    }
}

impl<T> Display for CauseProxy<T>
where
    T: Display + std::error::Error,
{
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        self.with_instance(|i| std::fmt::Display::fmt(&i, f))
    }
}

impl<T> Debug for CauseProxy<T>
where
    T: Debug + std::error::Error,
{
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        self.with_instance(|i| std::fmt::Debug::fmt(&i, f))
    }
}

[john-sharratt]

I created a pull request that adds 'Sync' using a feature flag if anyone wants to check it out
#300