-
Notifications
You must be signed in to change notification settings - Fork 400
Adding catch_panic anti-pattern #109
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
Draft
simonsan
wants to merge
12
commits into
main
Choose a base branch
from
catch_panic_anti_pattern
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
82eef12
Adding catch_panic anti-pattern
liamzdenek ee68daf
[Fix] Review feedback
simonsan f810323
Added todos
simonsan cdda188
Add catch_unwind.md to SUMMARY.md
simonsan 028e8af
Update anti_patterns/catch_panic.md
simonsan 4a85198
Update anti_patterns/catch_panic.md
simonsan 9e2651a
markdownlint
simonsan 9ea460f
replace +
simonsan 4388f63
Merge branch 'master' into catch_panic_anti_pattern
simonsan 2f97923
Fixing lint
simonsan 422d9cb
Adding suggestion
simonsan 9aa2d47
Adding example for exception safety
simonsan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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 | ||
| 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) | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.