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

Adding catch_panic anti-pattern #109

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions SUMMARY.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
- [Visitor](./patterns/visitor.md)

- [Anti-patterns](./anti_patterns/index.md)
- [catch_unwind for exceptions](./anti_patterns/catch_panic.md)
- [`#[deny(warnings)]`](./anti_patterns/deny-warnings.md)
- [Deref Polymorphism](./anti_patterns/deref.md)

Expand Down
79 changes: 79 additions & 0 deletions anti_patterns/catch_panic.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
# catch_unwind for exceptions

## Description

This anti-pattern arises when the method for catching ([panic::catch_unwind](https://doc.rust-lang.org/std/panic/fn.catch_unwind.html)) an
unexpected problem (implementation bug) is used to handle an expected problem,
such as a missing file.

## Example

```rust
use std::io::prelude::*;
use std::fs::File;
use std::panic;

fn main() {
// panic::catch_unwind catches any panic that occurs within the
// function passed to it
let result = panic::catch_unwind(|| {
let mut file_result = File::open("foo.txt");
file_result.unwrap(); // causes a panic if the file is not found
});

// the program continues running despite the panic
println!("potential error: {:?}", result);
assert!(result.is_err());
}
```

## Motivation

In rust, there are two ways an operation can fail: An expected problem, like a
file not being found, or a HTTP request timing out. Or, an unexpected problem,
such as an index being out of bounds, or an assert!() failing. These are
unexpected because they are bugs that should not happen. It would not make sense
to handle an error for QuickSort returning an incorrectly unsorted array, as
this should not happen.

## Advantages

There are scenarios where using `panic::catch_unwind` is the correct choice, e.g. a
simonsan marked this conversation as resolved.
Show resolved Hide resolved
web server implementation wants to save an unwinding thread in order to send a
valid response if the route for that request (as in: logic outside of the web server
implementor's control) is producing a panic.

## Disadvantages

`panic::catch_unwind` may not catch all panics in Rust. A panic in Rust is not always
implemented via unwinding, but can be implemented by aborting the process as well.
`panic::catch_unwind` only catches unwinding panics, not those that abort the process.

Also note that unwinding into Rust code with a foreign exception
(e.g. a an exception thrown from C++ code) is undefined behavior.

TODO: since Result::unwrap() converts the error to a string, it's harder to distinguish
between different kinds of errors than if we had matched the result directly.

## Discussion

TODO:
?-operator to propagate errors
explain why unwinding is bad
other disadvantages of panic::catch_unwind

- "The example could be improved by adding a function and which panics and catching the panic
in the caller, then matching the Result. Describing the example you could show how by returning
a Result, the Result-ness of the function is described in the signature."

Expected errors should not result in stack unwinding. Instead, expected errors
should be handled through the Result and Option types. [The Rust Book's chapter
on Error Handling](https://doc.rust-lang.org/book/error-handling.html) elaborates further on this.

## See also

- [The Rust Book: Error Handling](https://doc.rust-lang.org/book/error-handling.html)
- [Rust 1.9 announcement, which contains a description of this antipattern](http://blog.rust-lang.org/2016/05/26/Rust-1.9.html)
- [Result documentation](http://doc.rust-lang.org/std/result/enum.Result.html)
- [panic::catch_unwind documentation](https://doc.rust-lang.org/std/panic/fn.catch_unwind.html)