Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Adding catch_panic anti-pattern #22

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ language.

### Anti-patterns

* TODO thread + catch_panic for exceptions
* [panic::catch_unwind for exceptions](anti_patterns/catch_panic.md)
* TODO Clone to satisfy the borrow checker
* [Deref polymorphism](anti_patterns/deref.md)
* TODO Matching all fields of a struct (back compat)
Expand Down
54 changes: 54 additions & 0 deletions anti_patterns/catch_panic.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
# panic::catch_unwind for exceptions

## Description

This antipattern arises when the method for catching (`panic::catch_unwind`) 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());
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

}
```


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

There are scenarios where using panic::catch_unwind is the correct choice, eg, a
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.

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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add why stack unwinding is bad? And perhaps the other disadvantages of using catch_unwind? Also, where is it appropriate to use catch_unwind

Copy link
Collaborator

Choose a reason for hiding this comment

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

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


## 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)