From 82eef1284975385881f584438f9f37e8951bd541 Mon Sep 17 00:00:00 2001 From: Liam Zdenek Date: Tue, 31 May 2016 15:09:50 +0000 Subject: [PATCH 01/11] Adding catch_panic anti-pattern --- anti_patterns/catch_panic.md | 54 ++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 anti_patterns/catch_panic.md diff --git a/anti_patterns/catch_panic.md b/anti_patterns/catch_panic.md new file mode 100644 index 00000000..4d08370c --- /dev/null +++ b/anti_patterns/catch_panic.md @@ -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()); +} +``` + + +## 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. + +## 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) From ee68dafaea2dc8f29a3ef82e8f48d3e4a3ef938f Mon Sep 17 00:00:00 2001 From: Simon Date: Fri, 1 Jan 2021 04:57:10 +0100 Subject: [PATCH 02/11] [Fix] Review feedback --- anti_patterns/catch_panic.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/anti_patterns/catch_panic.md b/anti_patterns/catch_panic.md index 4d08370c..b0ea2182 100644 --- a/anti_patterns/catch_panic.md +++ b/anti_patterns/catch_panic.md @@ -1,8 +1,8 @@ -# panic::catch_unwind for exceptions +# catch_unwind for exceptions ## Description -This antipattern arises when the method for catching (`panic::catch_unwind`) an +This anti-pattern 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. From f810323d6202778bf4539953a5e06e75e4178168 Mon Sep 17 00:00:00 2001 From: Simon Date: Fri, 1 Jan 2021 05:30:05 +0100 Subject: [PATCH 03/11] Added todos --- anti_patterns/catch_panic.md | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/anti_patterns/catch_panic.md b/anti_patterns/catch_panic.md index b0ea2182..d03343d1 100644 --- a/anti_patterns/catch_panic.md +++ b/anti_patterns/catch_panic.md @@ -37,11 +37,35 @@ 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 +## 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. @@ -49,6 +73,9 @@ on Error Handling](https://doc.rust-lang.org/book/error-handling.html) elaborate ## 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) From cdda188b830b7c7c16fe002a01cb183a38babe62 Mon Sep 17 00:00:00 2001 From: Simon Date: Fri, 1 Jan 2021 13:53:18 +0100 Subject: [PATCH 04/11] Add catch_unwind.md to SUMMARY.md --- SUMMARY.md | 1 + 1 file changed, 1 insertion(+) diff --git a/SUMMARY.md b/SUMMARY.md index 99f93e6a..3cd45193 100644 --- a/SUMMARY.md +++ b/SUMMARY.md @@ -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) From 028e8af7048d1e5c9610962be4ddc46301a83376 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sat, 2 Jan 2021 01:49:08 +0100 Subject: [PATCH 05/11] Update anti_patterns/catch_panic.md Co-authored-by: Marco Ieni <11428655+MarcoIeni@users.noreply.github.com> --- anti_patterns/catch_panic.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/anti_patterns/catch_panic.md b/anti_patterns/catch_panic.md index d03343d1..723d7f9f 100644 --- a/anti_patterns/catch_panic.md +++ b/anti_patterns/catch_panic.md @@ -2,7 +2,7 @@ ## Description -This anti-pattern arises when the method for catching (`panic::catch_unwind`) an +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. From 4a851985eda09280a7fd4412e544bdb7495a90ba Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Mon, 4 Jan 2021 06:37:53 +0100 Subject: [PATCH 06/11] Update anti_patterns/catch_panic.md --- anti_patterns/catch_panic.md | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/anti_patterns/catch_panic.md b/anti_patterns/catch_panic.md index 723d7f9f..fd66295e 100644 --- a/anti_patterns/catch_panic.md +++ b/anti_patterns/catch_panic.md @@ -72,10 +72,7 @@ on Error Handling](https://doc.rust-lang.org/book/error-handling.html) elaborate ## 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) +- [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) From 9e2651a82c2197d353dca7c80ad191c1acb2e3f5 Mon Sep 17 00:00:00 2001 From: Simon Date: Wed, 6 Jan 2021 10:54:04 +0100 Subject: [PATCH 07/11] markdownlint --- anti_patterns/catch_panic.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/anti_patterns/catch_panic.md b/anti_patterns/catch_panic.md index fd66295e..782a1b0f 100644 --- a/anti_patterns/catch_panic.md +++ b/anti_patterns/catch_panic.md @@ -27,7 +27,6 @@ fn main() { } ``` - ## Motivation In rust, there are two ways an operation can fail: An expected problem, like a @@ -45,6 +44,7 @@ valid response if the route for that request (as in: logic outside of the web se 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. @@ -62,6 +62,7 @@ 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." From 9ea460f904ded053b78a9c526afeb9d724611ac7 Mon Sep 17 00:00:00 2001 From: Simon Date: Wed, 6 Jan 2021 15:39:45 +0100 Subject: [PATCH 08/11] replace + --- anti_patterns/catch_panic.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/anti_patterns/catch_panic.md b/anti_patterns/catch_panic.md index 782a1b0f..cd382875 100644 --- a/anti_patterns/catch_panic.md +++ b/anti_patterns/catch_panic.md @@ -63,7 +63,7 @@ TODO: 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 +- "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." From 2f97923a73163583851bf31696238c97cc01085a Mon Sep 17 00:00:00 2001 From: Simon Date: Mon, 29 Mar 2021 20:59:49 +0200 Subject: [PATCH 09/11] Fixing lint --- anti_patterns/catch_panic.md | 37 +++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/anti_patterns/catch_panic.md b/anti_patterns/catch_panic.md index cd382875..5586f3dc 100644 --- a/anti_patterns/catch_panic.md +++ b/anti_patterns/catch_panic.md @@ -2,8 +2,8 @@ ## 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, +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 @@ -38,23 +38,24 @@ 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. +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. +​`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. +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 @@ -63,17 +64,19 @@ TODO: 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." +- "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. +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) +- [Rust 1.9 announcement, which contains a description of this anti-pattern](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) From 422d9cb070769c1b5cd046fdc198ec8649583a20 Mon Sep 17 00:00:00 2001 From: Simon Date: Mon, 29 Mar 2021 21:01:57 +0200 Subject: [PATCH 10/11] Adding suggestion --- anti_patterns/catch_panic.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/anti_patterns/catch_panic.md b/anti_patterns/catch_panic.md index 5586f3dc..adb4957d 100644 --- a/anti_patterns/catch_panic.md +++ b/anti_patterns/catch_panic.md @@ -38,10 +38,11 @@ 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. +When code is split between multiple threads, or wants to create an `isolation boundary`, +`panic::catch_unwind` is the correct choice. For example, 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 From 9aa2d475e935affab86dff00e7364eb307e297fe Mon Sep 17 00:00:00 2001 From: Simon Date: Mon, 29 Mar 2021 21:03:12 +0200 Subject: [PATCH 11/11] Adding example for exception safety --- anti_patterns/catch_panic.md | 1 + 1 file changed, 1 insertion(+) diff --git a/anti_patterns/catch_panic.md b/anti_patterns/catch_panic.md index adb4957d..f0db44f9 100644 --- a/anti_patterns/catch_panic.md +++ b/anti_patterns/catch_panic.md @@ -81,3 +81,4 @@ further on this. - [Rust 1.9 announcement, which contains a description of this anti-pattern](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) +- [Example for exception safety](https://doc.rust-lang.org/nomicon/exception-safety.html#vecpush_all)