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

Add a Sync bounds to errors #110

Merged
merged 2 commits into from
Jan 9, 2017
Merged

Conversation

sfackler
Copy link
Contributor

@sfackler sfackler commented Jan 4, 2017

Box<Error + Sync + Send> is far more common than Box<Error + Send> - common conveniences like conversion from strings are not
implemented for Box<Error + Send>, Box<Error + Send> cannot be
stored in io::Errors, etc.

This is a breaking change, but non-Sync errors should be rare so the
fallout shouldn't be too bad.

r? @brson I've confirmed that rustup builds without modification with this change.

`Box<Error + Sync + Send>` is far more common than `Box<Error +
Send>` - common conveniences like conversion from strings are not
implemented for `Box<Error + Send>`, `Box<Error + Send>` cannot be
stored in `io::Error`s, etc.

This is a breaking change, but non-`Sync` errors should be rare so the
fallout shouldn't be too bad.
@Yamakaky
Copy link
Contributor

Yamakaky commented Jan 4, 2017

Do you have an example of code that requires this change?

@sfackler
Copy link
Contributor Author

sfackler commented Jan 4, 2017

Returning an error_chain generated error as a Box<Error + Sync + Send>.

@Yamakaky
Copy link
Contributor

Yamakaky commented Jan 8, 2017

Most errors are Sync I think, since they are read-only?

@sfackler
Copy link
Contributor Author

sfackler commented Jan 8, 2017

Yep!

@Yamakaky
Copy link
Contributor

Yamakaky commented Jan 8, 2017

Please update the changelog.

@sfackler
Copy link
Contributor Author

sfackler commented Jan 8, 2017

Done

@Yamakaky
Copy link
Contributor

Yamakaky commented Jan 8, 2017

common conveniences like conversion from strings are not implemented for Box<Error + Send>

What do you mean?

@sfackler
Copy link
Contributor Author

sfackler commented Jan 8, 2017

This works: let e: Box<Error> = "uh oh".into(), as does let e: Box<Error + Sync + Send> = "uh oh.into(), but not let e = Box<Error + Send> = "uh oh".into().

@Yamakaky
Copy link
Contributor

Yamakaky commented Jan 9, 2017

Hum, strange, do you know why?

@sfackler
Copy link
Contributor Author

sfackler commented Jan 9, 2017

We never bothered implementing it since Box<Error + Send> is exceedingly rare.

@Yamakaky Yamakaky merged commit 6e5ac7a into rust-lang-deprecated:master Jan 9, 2017
@Yamakaky
Copy link
Contributor

Yamakaky commented Jan 9, 2017

Ok, let see if it breaks something important in another crate ;)

@diwic
Copy link

diwic commented Jan 23, 2017

JFTR, my main D-Bus error struct won't implement Sync, because it is a wrapper around an FFI struct, and as such I cannot provide any guarantees that the "C" side of the struct is (or will remain) Sync.

@Yamakaky
Copy link
Contributor

That's what I feared...

@durka
Copy link
Contributor

durka commented Feb 7, 2017

What is the suggested workaround for errors that cannot be made Sync? Can I just not use error-chain anymore?

@Yamakaky
Copy link
Contributor

Yamakaky commented Feb 8, 2017

I'll just remove it in 0.9.

Yamakaky added a commit that referenced this pull request Feb 8, 2017
@sfackler
Copy link
Contributor Author

sfackler commented Feb 8, 2017

Things break without the bound just as they do with it. Can we make the error bounds configurable in the error_chain! {} macro?

@Yamakaky
Copy link
Contributor

Yamakaky commented Feb 8, 2017

Yes, see #121.

@jsgf
Copy link

jsgf commented Feb 10, 2017

I ran into this because std::io::Error requires a nested error to be Sync. It makes it hard to incorporate error-chain errors into interfaces which return std::io::Result.

@Yamakaky
Copy link
Contributor

See #134

@rnewman
Copy link

rnewman commented Apr 18, 2017

Reverting this means that error_chain can't easily be used with combine. In Mentat we've had to roll back to 0.8.1 until this is fixed: mozilla/mentat#418

withoutboats pushed a commit to withoutboats/error-chain that referenced this pull request Nov 15, 2017
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.
ccope pushed a commit to ccope/error-chain that referenced this pull request Feb 21, 2020
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.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants