Skip to content

Make all error types Send + Sync#625

Open
atro7318 wants to merge 1 commit into
smartive:mainfrom
surus-inc:send-sync-errors
Open

Make all error types Send + Sync#625
atro7318 wants to merge 1 commit into
smartive:mainfrom
surus-inc:send-sync-errors

Conversation

@atro7318

Copy link
Copy Markdown

I am trying to use this library for calling the Zitadel API in a multi-threaded environment, thus I need all error types to be Send + Sync. Making errors Send + Sync is also recommended as a Rust API best practice. See the second through fourth paragraphs from this section:

Additionally, error types should implement the Send and Sync traits. An error that is not Send cannot be returned by a thread run with thread::spawn. An error that is not Sync cannot be passed across threads using an Arc. These are common requirements for basic error handling in a multithreaded application.

Send and Sync are also important for being able to package a custom error into an IO error using std::io::Error::new, which requires a trait bound of Error + Send + Sync.

One place to be vigilant about this guideline is in functions that return Error trait objects, for example reqwest::Error::get_ref. Typically Error + Send + Sync + 'static will be the most useful for callers. The addition of 'static allows the trait object to be used with Error::downcast_ref.

I first tried to use the currently used custom_error dependency, but it seems to only support Box<dyn Error>, not Box<dyn Error + Send + Sync + 'static>. I instead switched all of the error type definitions in this repo to be annotated with thiserror derive and attribute macros. I then changed all usages of Box<dyn Error> to Box<dyn Error + Send + Sync + 'static> and got rid of the custom_error dependency. I believe this solution minimizes boilerplate just as well as custom_error, allows for writing more code in plain Rust (rather than function-like macros), and allows for this repo's dependency on a much more widely adopted and maintained crate (thiserror).

If you agree, merging and releasing this change would be very useful for my work. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant