fralalonde/dipstick

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

mixalturek opened this issue · 3 comments

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`.

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...

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.

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.