Skip to content
This repository has been archived by the owner on Aug 16, 2021. It is now read-only.

Require that errors are Send/Sync/'static. #241

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

withoutboats
Copy link

Currently, they are not Sync because they contain a non-Sync trait
object. This is a breaking change.

The decision to make errors Send but not Sync was made in #110. We
believe that decision was a mistake, because it perpetuates a !Sync
restriction on all users even if their errors are, in fact, Sync.
Instead, users who need errors that are !Sync should use
synchronization when transforming their errors into error-chain
errors.

cc @aturon

Currently, they are not Sync because they contain a non-Sync trait
object. This is a breaking change.

The decision to make errors Send but not Sync was made in rust-lang-deprecated#110. We
believe that decision was a mistake, because it perpetuates a !Sync
restriction on all users even if their errors are, in fact, Sync.
Instead, users who need errors that are !Sync should use
synchronization when transforming their errors into error-chain
errors.
@bvinc
Copy link

bvinc commented Dec 6, 2017

I support this PR. Here are some arguments I have for supporting it.

  • io::Error is designed to wrap other errors. This is required if anyone wants to implement Read or Write and have meaningful errors. io::Error has decided that all errors that it wraps must be Sync. This means that error-chain is fighting against the standard library.
  • error-chain is recursively making everyone's errors !Sync when they don't need to be.
  • If someone wants to make their errors Sync, but one of the causal errors is !Sync because it uses error-chain, they can't implement the std::error::Error trait's cause method. The cause method returns a reference, which it can't do if the causal error is behind a Mutex.
  • If anyone rightfully has !Sync data inside of their error, it would seem pretty easy to add synchronization without affecting anything else.
  • If this isn't fixed, I would think that any library that uses error-chain should rightfully get an issue reported that their error isn't Sync when it should be. I would think it would hurt adoption of error-chain in the long run.

@aturon
Copy link

aturon commented Dec 6, 2017

cc @rust-lang-nursery/libs I'd like to merge this PR, any objections?

@KodrAus
Copy link
Contributor

KodrAus commented Dec 6, 2017

I'm in favour of this too 👍

@alexcrichton
Copy link
Contributor

I have no ticky box to check off :(

Sounds great!

@luser
Copy link

luser commented Feb 9, 2018

If everyone is in favor, can this be merged? We'll still have to get crates that use error-chain updated to the new version, but it will definitely make interop between error-chain and failure crates much simpler.

@aturon
Copy link

aturon commented Feb 9, 2018

@withoutboats, can you merge and publish?

@withoutboats
Copy link
Author

Will do this week!

@aidanhs
Copy link

aidanhs commented Aug 21, 2018

@withoutboats I guess you got a bit delayed? 😄

@L-as
Copy link

L-as commented Nov 3, 2018

I'd really love to see this merged.

@Terkwood
Copy link

Terkwood commented Nov 3, 2018

Ooo

@pietroalbini
Copy link

Ping @withoutboats :)

@Kixunil
Copy link

Kixunil commented Nov 26, 2020

Did you fall into a black hole? I need this. :(

@AndyGauge
Copy link
Contributor

I don't believe we have the support required to add features to this library. We are currently only maintaining compatibility with rustc and not adding features.

@AndyGauge AndyGauge added Requires Engineers and removed 0.13 Breaking Change labels Nov 30, 2020
@Kixunil
Copy link

Kixunil commented Nov 30, 2020

Does that mean error-chain is deprecated or you don't have the resources? Frankly, this change is so important to me that I'm willing to resolve merge conflict if someone is willing to review it and hit "Merge" button.

@AndyGauge
Copy link
Contributor

I'm most concerned with the breaking change adversely affecting users of the library. I guess I may be taking too measured an approach.

@Kixunil
Copy link

Kixunil commented Nov 30, 2020

Ah, good point. Though the consensus was that this is wanted, so it may be worth it? It'd definitely need to be semver-breaking, so that people update at their own pace.

@luser
Copy link

luser commented Dec 1, 2020

I don't think I'm alone in my assessment that error-chain, while a useful exploration of the design space, is out of date with regards to the evolution of Rust error handling practices. If you're still actively using this crate you should consider replacing it with one of several newer crates that are actively maintained. This blog post from last year has a survey of several options, including some words on error-chain itself.

@AndyGauge
Copy link
Contributor

error-chain was a pioneer in this space, and it was the work done here that created the rust primitives (that story is probably longer, more nuanced, and only influenced by error-chain). Now, it left to show why rust needed an error-handling library. Now enums and simple macros can provide the combination of error types; a library is less important.

@Kixunil
Copy link

Kixunil commented Dec 1, 2020

Right, the change would have to be breaking anyway, so might as well change the library.

@john-sharratt
Copy link

Made a new pull request that adds 'Sync' via a feature toggle for backwards compatibility
#300

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.