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

Broken Result type, missing Send/Sync in v0.7.1 #35

Closed
mixalturek opened this issue Jan 16, 2019 · 3 comments
Closed

Broken Result type, missing Send/Sync in v0.7.1 #35

mixalturek opened this issue Jan 16, 2019 · 3 comments

Comments

@mixalturek
Copy link
Contributor

Dipstick's Result seems broken, Error part should be extended by Send & Sync. Please also consider to return concrete error types instead of generic trait.

/// Just put any error in a box.
pub type Result<T> = result::Result<T, Box<Error>>;
let statsd = Statsd::send_to((config.statsd_host.as_ref(), config.statsd_port))?;
error[E0277]: the size for values of type `dyn std::error::Error` cannot be known at compilation time
  --> src/metrics.rs:46:22
   |
46 |         let statsd = Statsd::send_to((config.statsd_host.as_ref(), config.statsd_port))?;
   |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
   |
   = help: the trait `std::marker::Sized` is not implemented for `dyn std::error::Error`
   = note: to learn more, visit <https://doc.rust-lang.org/book/second-edition/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
   = note: required because of the requirements on the impl of `std::error::Error` for `std::boxed::Box<dyn std::error::Error>`
   = note: required because of the requirements on the impl of `failure::Fail` for `std::boxed::Box<dyn std::error::Error>`
   = note: required because of the requirements on the impl of `std::convert::From<std::boxed::Box<dyn std::error::Error>>` for `failure::error::Error`
   = note: required by `std::convert::From::from`

error[E0277]: `dyn std::error::Error` cannot be sent between threads safely
  --> src/metrics.rs:46:22
   |
46 |         let statsd = Statsd::send_to((config.statsd_host.as_ref(), config.statsd_port))?;
   |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `dyn std::error::Error` cannot be sent between threads safely
   |
   = help: the trait `std::marker::Send` is not implemented for `dyn std::error::Error`
   = note: required because of the requirements on the impl of `std::marker::Send` for `std::ptr::Unique<dyn std::error::Error>`
   = note: required because it appears within the type `std::boxed::Box<dyn std::error::Error>`
   = note: required because of the requirements on the impl of `failure::Fail` for `std::boxed::Box<dyn std::error::Error>`
   = note: required because of the requirements on the impl of `std::convert::From<std::boxed::Box<dyn std::error::Error>>` for `failure::error::Error`
   = note: required by `std::convert::From::from`

error[E0277]: `dyn std::error::Error` cannot be shared between threads safely
  --> src/metrics.rs:46:22
   |
46 |         let statsd = Statsd::send_to((config.statsd_host.as_ref(), config.statsd_port))?;
   |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `dyn std::error::Error` cannot be shared between threads safely
   |
   = help: the trait `std::marker::Sync` is not implemented for `dyn std::error::Error`
   = note: required because of the requirements on the impl of `std::marker::Sync` for `std::ptr::Unique<dyn std::error::Error>`
   = note: required because it appears within the type `std::boxed::Box<dyn std::error::Error>`
   = note: required because of the requirements on the impl of `failure::Fail` for `std::boxed::Box<dyn std::error::Error>`
   = note: required because of the requirements on the impl of `std::convert::From<std::boxed::Box<dyn std::error::Error>>` for `failure::error::Error`
   = note: required by `std::convert::From::from`

error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0277`.
error: Could not compile `urlite`.
@fralalonde
Copy link
Owner

Since Dipstick itself doesnt return errors and only propagates the errors from other layers, just boxing them up seemed to be the simplest way, no failure crate / dependency required. Could just adding Send + Sync to the Boxed trait satisfy your requirements? What would be the gain in returning a concrete Dipstick Error struct? All it would do (for now) is wrap other errors...

@vorner
Copy link
Contributor

vorner commented Jan 19, 2019

Send + Sync is a good start (things that are not are really hard to work with and mostly incompatible with any crate that helps with error handling). In the case of just propagating, it might be a good solution.

The advantage of an enum wrapper around concrete types are:

  • You can see in documentation what erros may happen.
  • All the Send, Sync and possibly other auto-traits are handled by the compiler.
  • It's a bit easier to handle some of the errors in a different way than the rest, by matching. You can still downcast the boxed ones, but that's a bit more work.

@fralalonde
Copy link
Owner

I've made a PR to add Send + Sync to boxed errors.

For the record, I also I rewrote an Error enum, but pulled it back. Maintaining an Enum along with the required From impls is not something I want to commit to as it feels the whole Rust error management is still in shift anyway and a better way might yet come up. Also, it's not a showstopper, especially as the errors currently being reported are probably not rich enough to mandate handling them in a match. I understand your points though and will keep them in mind when it's time to revisit the error mechanism.

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

No branches or pull requests

3 participants