Why isn't Whatever Send + Sync by default? #446

zardini123 · 2024-03-25T14:47:12Z

Hi @shepmaster! Thank you for the wonderful crate!

Problem

I have been switching my project to utilize snafu. Previously, I used anyhow::Error as a generic error type when I didn't understand how to implement error types. Now I am rewriting my project with proper error types, and in the meantime switching out anyhow::Error with snafu::Whatever.

When switching, I ran into a problem. I have a future being spawned via Tokio, where the future has a output type of Result<(), snafu::Whatever>. As this is concurrency, the rust compiler complains that that Whatever does not have Send + Sync markers for its source type. The compile error is pretty indirect about this issue. I have the error included at the bottom of this post. To figure out where this issue was coming from required a bunch of research, as shown below.

Research

Searching "snafu thread saftey" on Google returns the project http-whatever, where they essentially implement a new Whatever but with Send + Sync attached to the StdError:

https://github.com/bassmanitram/http-whatever/blob/af4fb2d672011e76a5746c27f31a2fdf65979326/src/lib.rs#L84-L93

Snafu's Whatever:

snafu/src/lib.rs

Lines 1557 to 1569 in 073cc38

    
           #[derive(Debug, Snafu)] 
        
           #[snafu(crate_root(crate))] 
        
           #[snafu(whatever)] 
        
           #[snafu(display("{message}"))] 
        
           #[snafu(provide(opt, ref, chain, dyn std::error::Error => source.as_deref()))] 
        
           #[cfg(any(feature = "std", test))] 
        
           pub struct Whatever { 
        
               #[snafu(source(from(Box<dyn std::error::Error>, Some)))] 
        
               #[snafu(provide(false))] 
        
               source: Option<Box<dyn std::error::Error>>, 
        
               message: String, 
        
               backtrace: Backtrace, 
        
           }

I found a snafu issue from January 2022 that asks for implementation details for making a custom snafu error type be async compatible. From there in May 2022 a PR was merged that showed an error type with async/multi-threading as part of the tests. This is part of the tests, but not part of the examples, so finding this example required a bunch of searching as its not part of the doc examples.

In addition, anyhow::Error has Sync + Send markers (source in anyhow code, source in anyhow docs).

Question

Why isn't Whatever Send + Sync by default?

I could implement a new Whatever as done by http-whatever and as shown in the snafu test. But, if we consider snafu::Whatever to be a catch-all error type until the user can make more specific types, shouldn't we expect as a user that it can be used in all places including async and multi-threading? Is there a detail I am missing that makes Whatever not able to be Send + Sync compatible by default?

Thank you!

Whatever Send + Sync error report

error[E0277]: `(dyn StdError + 'static)` cannot be sent between threads safely
    --> src\file\file.rs:435:88
     |
435  |   ...                   task = Some(Handle::current().spawn(async move {
     |  ____________________________________________________________________-----_^
     | |                                                                    |
     | |                                                                    required by a bound introduced by this call
...    |
452  | | ...                       return Ok::<(), snafu::Whatever>(());
453  | | ...                   }));
     | |_______________________^ `(dyn StdError + 'static)` cannot be sent between threads safely
     |
     = help: the trait `std::marker::Send` is not implemented for `(dyn StdError + 'static)`
     = note: required for `Unique<(dyn StdError + 'static)>` to implement `std::marker::Send`
note: required because it appears within the type `Box<(dyn StdError + 'static)>`
    --> C:\Users\zardini123\.rustup\toolchains\1.76.0-x86_64-pc-windows-msvc\lib/rustlib/src/rust\library\alloc\src\boxed.rs:195:12
     |
195  | pub struct Box<
     |            ^^^
note: required because it appears within the type `std::option::Option<Box<(dyn StdError + 'static)>>`
    --> C:\Users\zardini123\.rustup\toolchains\1.76.0-x86_64-pc-windows-msvc\lib/rustlib/src/rust\library\core\src\option.rs:570:10
     |
570  | pub enum Option<T> {
     |          ^^^^^^
note: required because it appears within the type `Whatever`
    --> C:\Users\zardini123\.cargo\registry\src\index.crates.io-6f17d22bba15001f\snafu-0.8.2\src\lib.rs:1563:12
     |
1563 | pub struct Whatever {
     |            ^^^^^^^^
note: required because it appears within the type `Result<(), Whatever>`
    --> C:\Users\zardini123\.rustup\toolchains\1.76.0-x86_64-pc-windows-msvc\lib/rustlib/src/rust\library\core\src\result.rs:502:10
     |
502  | pub enum Result<T, E> {
     |          ^^^^^^
note: required by a bound in `Handle::spawn`
    --> C:\Users\zardini123\.cargo\registry\src\index.crates.io-6f17d22bba15001f\tokio-1.36.0\src\runtime\handle.rs:189:20
     |
186  |     pub fn spawn<F>(&self, future: F) -> JoinHandle<F::Output>
     |            ----- required by a bound in this associated function
...
189  |         F::Output: Send + 'static,
     |                    ^^^^ required by this bound in `Handle::spawn`

The text was updated successfully, but these errors were encountered:

zardini123 · 2024-04-02T21:12:35Z

I managed to get snafu's Whatever to be sendable between threads by adding + Send + Sync next to all instances of std::error::Error in Whatever definition. In addition, a test in snafu's test suite converts a Whatever to a std::error::Error, so adding the markers there are required too. The changes to make this work are available in my PR: #448

Snafu compiles and tests run on my machine with this. From my naive testing in one of my personal projects, it successfully allows for Whatever's to be sent between threads.

I am curious to hear your thoughts. Thanks!

Stargateur · 2024-04-02T22:47:47Z

I'm not an expert in send and sync trait I often try to avoid them :p but I think sync is not needed here it's maybe too much requirement, I don't see a case where you would need to share a reference to a Whatever in multiple thread.

I think the error here is solved with just send that is a perfectly reasonable constraint for a Whatever error.

Enet4 · 2024-04-03T07:54:03Z

It is unclear whether the lack of Send + Sync was just an oversight, or whether it was deliberately made in order to admit error types which might not be Send and Sync. On the side of caution, it would be nice to understand whether someone out there is using Whatever to encapsulate causes which cannot be sent or shared between threads.

In any case, making the change now is a breaking change, so I will mark it as such in the pull request.

zardini123 · 2024-04-07T22:02:42Z

send that is a perfectly reasonable constraint for a Whatever error.

Ah, that is true. The error I experienced was only in relation to Send, not Sync. You bring up a great point @Stargateur, Send certainly sounds more reasonable as a constraint than Send + Sync.

On the side of caution, it would be nice to understand whether someone out there is using Whatever to encapsulate causes which cannot be sent or shared between threads.

Great point @Enet4. Would making the error type only Sync make it any less of a breaking change?

In any case, making the change now is a breaking change, so I will mark it as such in the pull request.

Would there be a better method to allow users to enable Send or Send + Sync for Whatever if they find they need it? Maybe a feature flag of sorts? Something like this could allow for backwards compatibility and full choice of error types while not forcing all users out of using whatever_context in a threaded context.

jreppnow · 2024-05-13T08:35:32Z

Just chiming in to say that there are use cases where Sync is also necessary. In our case, we needed a cloneable dyn Error type (using contents as API return values and passing separately to logging system), but could not make all of our error types Clone. So we are passing an Arc<dyn Error + Send + Sync> around, instead of a Box<dyn Error + Clone + Send>.

zardini123 linked a pull request Apr 2, 2024 that will close this issue

Make Whatever thread-safe by default (add markers Send + Sync) #448

Open

nightkr mentioned this issue May 8, 2024

0.3.0 regression: Whatever isn't Send+Sync stackabletech/tokio-zookeeper#43

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Why isn't Whatever Send + Sync by default? #446

Why isn't Whatever Send + Sync by default? #446

zardini123 commented Mar 25, 2024 •

edited

zardini123 commented Apr 2, 2024 •

edited

Stargateur commented Apr 2, 2024 •

edited

Enet4 commented Apr 3, 2024

zardini123 commented Apr 7, 2024

jreppnow commented May 13, 2024

Why isn't Whatever Send + Sync by default? #446

Why isn't Whatever Send + Sync by default? #446

Comments

zardini123 commented Mar 25, 2024 • edited

Problem

Research

Question

Whatever Send + Sync error report

zardini123 commented Apr 2, 2024 • edited

Stargateur commented Apr 2, 2024 • edited

Enet4 commented Apr 3, 2024

zardini123 commented Apr 7, 2024

jreppnow commented May 13, 2024

zardini123 commented Mar 25, 2024 •

edited

zardini123 commented Apr 2, 2024 •

edited

Stargateur commented Apr 2, 2024 •

edited