Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a Sync bounds to errors #110

Merged
merged 2 commits into from Jan 9, 2017

Conversation

Projects
None yet
6 participants
@sfackler
Copy link
Member

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.

Add a Sync bounds to errors
`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

This comment has been minimized.

Copy link
Collaborator

Yamakaky commented Jan 4, 2017

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

@sfackler

This comment has been minimized.

Copy link
Member Author

sfackler commented Jan 4, 2017

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

@Yamakaky

This comment has been minimized.

Copy link
Collaborator

Yamakaky commented Jan 8, 2017

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

@sfackler

This comment has been minimized.

Copy link
Member Author

sfackler commented Jan 8, 2017

Yep!

@Yamakaky

This comment has been minimized.

Copy link
Collaborator

Yamakaky commented Jan 8, 2017

Please update the changelog.

@sfackler

This comment has been minimized.

Copy link
Member Author

sfackler commented Jan 8, 2017

Done

@Yamakaky

This comment has been minimized.

Copy link
Collaborator

Yamakaky commented Jan 8, 2017

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

What do you mean?

@sfackler

This comment has been minimized.

Copy link
Member 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

This comment has been minimized.

Copy link
Collaborator

Yamakaky commented Jan 9, 2017

Hum, strange, do you know why?

@sfackler

This comment has been minimized.

Copy link
Member 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-nursery:master Jan 9, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Yamakaky

This comment has been minimized.

Copy link
Collaborator

Yamakaky commented Jan 9, 2017

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

@dfaust dfaust referenced this pull request Jan 21, 2017

Closed

Error should implement Sync #71

@diwic

This comment has been minimized.

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

This comment has been minimized.

Copy link
Collaborator

Yamakaky commented Jan 23, 2017

That's what I feared...

@durka

This comment has been minimized.

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

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
Member 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

This comment has been minimized.

Copy link
Collaborator

Yamakaky commented Feb 8, 2017

Yes, see #121.

@Arnavion Arnavion referenced this pull request Feb 8, 2017

Open

Make Sync configurable #12

@jsgf

This comment has been minimized.

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

This comment has been minimized.

Copy link
Collaborator

Yamakaky commented Feb 21, 2017

See #134

@rnewman

This comment has been minimized.

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

@Cldfire Cldfire referenced this pull request Jul 5, 2017

Open

Comply with the Rust API guidelines as much as possible #6

14 of 58 tasks complete

withoutboats added a commit to withoutboats/error-chain that referenced this pull request Nov 15, 2017

Require that errors are Send/Sync/'static.
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-nursery#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 join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.