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 UnwindSafe impl for AsDynError #155

Merged
merged 1 commit into from Oct 9, 2021

Conversation

cosmicexplorer
Copy link
Contributor

Hello!! I love this crate :))

Problem

thiserror::Error is not able to convert a Box<dyn Error + Send + Sync + UnwindSafe + 'static> into a dyn Error with the #[source] annotation, which caused it to be unsuitable for the purposes of the Signal client: see signalapp/libsignal#296 (comment). Signal uses UnwindSafe (I just learned about this trait) to convert errors into a format suitable for its ffi/jni/node.js bindings.

Solution

@jrose-signal pointed me to #41, which enabled wrapping a Box<dyn Error + Send + 'static> with #[source], and I was able to figure out how to extend it for the purposes of boxed UnwindSafe errors as well.

Result

Using this change allowed me to remove the boilerplate error manipulation code in the signal client, and this commit (cosmicexplorer/libsignal-client@9d82971) passed tests locally.

Please let me know if there are test cases I should add -- it looks like the test_boxed_source() method is already intended to apply to all the types of boxed errors thiserror supports, and I tried to create a test wrapping e.g. ParseIntError which is UnwindSafe using std::panic::catch_unwind, but was having difficulty and was thinking it might be better to leave the tests simple as they are. Let me know if I should investigate more on that.

Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@dtolnay dtolnay merged commit 5c62f5e into dtolnay:master Oct 9, 2021
@dtolnay
Copy link
Owner

dtolnay commented Oct 9, 2021

Published in 1.0.30.

@cosmicexplorer
Copy link
Contributor Author

Amazing!! Thanks so much!!

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

Successfully merging this pull request may close these issues.

None yet

2 participants