exception handling anti-patterns in documentation
Hi Everyone, I wanted to share an observation. A number of Boost libraries, in documentation, when it comes to demonstrating that a given function throws an exception, has a code sample like the following: try {
lib::function(); } catch(lib::exception const&) { // handle failure }
This applies for instance to: * Boost.Lexical_cast: https://www.boost.org/doc/libs/1_79_0/doc/html/boost_lexical_cast/examples.h... * Boost.Optional: https://www.boost.org/doc/libs/1_79_0/libs/optional/doc/html/boost_optional/... * Boost.URL, waiting in the review queue: https://master.url.cpp.al/url/parsing/url.html What bothers me is that this is a very dangerous antipattern related to handling exceptions. If there is any benefit to be had from exceptions, it is that they separate the "positive" control flow form the "negative" (failure handling) one. Whereas in the examples like the one above it is quite the opposite: we are trying to catch the exception as soon as it is thrown. This is usually the cause of bugs and resource leaks. Such examples also lead many young programmers into thinking that exceptions are difficult or illogical. But they are not illogical: it is antipatterns like the one above that, which are working against the design of the exception handling mechanism, that confuse the programmers and enforce bad habits. One could say that the goal of the documentation is to demonstrate, in an efficient way, the interface of the library: not to teach how to program. But I would disagree. I have always considered Boost libraries to be more than that. I personally learned a lot about how to program, organize my code and organize my thinking about abstractions from reading the documentation of Boost libraries. Boost is a place to promote better programming practices. And these "catch immediately after throw and swallow" examples just do a disservice to the community. I would like to know if there are more people in this list that feel the same way. Maybe we could come up with a systematic way of demonstrating a throwing library interface, or guidelines for documentation authors. Regards, &rzej;
On Sat, May 28, 2022 at 3:39 PM Andrzej Krzemienski via Boost <boost@lists.boost.org> wrote:
* Boost.URL, waiting in the review queue: https://master.url.cpp.al/url/parsing/url.html
Boost.URL is designed for the model where a thrown exception terminates the program. Such as when the author knows that the inputs are valid. People writing Real World Applications (TM) are not going to be throwing on untrusted inputs, they are going to be using the APIs which use error codes.
What bothers me is that ...Such examples...lead...young programmers into thinking that exceptions are difficult or illogical.
The goal of the examples that show exceptions in the documentation is merely to demonstrate that the exception is thrown. It is expected that the reader is in possession of sufficient ability so as to be able to make a leap of induction and apply this knowledge to their particular use case (where the exception handler may be in another function).
One could say that the goal of the documentation is to demonstrate, in an efficient way, the interface of the library: not to teach how to program.
Yes that is precisely the mindset when I write documentation. I assume that the user of the library already understands everything in the C++ language, everything in the standard library, and a reasonably average grasp of algorithms and general terminology. And I have a very good reason for doing so. It is because the user who lacks these things can always go someplace else to learn it (or teach themselves). External resources which specialize in teaching C++ are going to do a better job than I could. It is already hard enough to develop a Boost-quality library with Boost-quality documentation. Trying to also teach novices the techniques that are applicable to any code in general only creates additional work. Or to put it another way, having N authors of N libraries all trying to solve the same problem is not efficient.
I would like to know if there are more people in this list that feel the same way. Maybe we could come up with a systematic way of demonstrating a throwing library interface, or guidelines for documentation authors.
I think there is room for a project (or projects) aimed towards providing code and exposition for the purpose of learning C++, aimed towards individuals who do not "know Asio, know HTTP, know concurrent programming, and know error handling" (these the requirements for using Beast). This could be a Boost-branded offering available on its own from our the new website, with its own pages and discussion area. The C++ Alliance is open to explore implementing such a project if there are volunteers. I imagine that if started, such a resource would only continue to grow in size, utility, and participation over time (as it likely could never be correctly described as "done"). Thanks
On Sat, May 28, 2022 at 3:39 PM Andrzej Krzemienski via Boost < boost@lists.boost.org> wrote:
Hi Everyone, I wanted to share an observation. A number of Boost libraries, in documentation, when it comes to demonstrating that a given function throws an exception, has a code sample like the following:
try {
lib::function(); } catch(lib::exception const&) { // handle failure }
This applies for instance to: * Boost.Lexical_cast:
https://www.boost.org/doc/libs/1_79_0/doc/html/boost_lexical_cast/examples.h...
* Boost.Optional:
https://www.boost.org/doc/libs/1_79_0/libs/optional/doc/html/boost_optional/...
* Boost.URL, waiting in the review queue: https://master.url.cpp.al/url/parsing/url.html
What bothers me is that this is a very dangerous antipattern related to handling exceptions.
I agree, it is not the job of the documentation of a function to show how to handle errors, only to specify the behavior of the function. Ideally this should be done formally, using this pattern: 1) Requires: lists the requirements that must be met in order for the function call to be valid. The user is not allowed to call the function otherwise. 2) Effects: specifies how the state of the program is altered by the function call. 3) Postconditions: the conditions that are guaranteed to be in place upon return from the function (if it doesn't throw). 4) Throws: lists exceptions that may be thrown if the postconditions could not be established. 5) Exception safety: specifies the state if an exception was thrown. For an example, see the documentation of the pointer constructor of shared_ptr: https://www.boost.org/doc/libs/1_79_0/libs/smart_ptr/doc/html/smart_ptr.html... . That said, it can be useful to demonstrate how to handle errors, but this wouldn't be attached to a particular function -- rather, it would show a pattern of correct error handling using the library.
Hi Everyone, I wanted to share an observation. A number of Boost libraries, in documentation, when it comes to demonstrating that a given function throws an exception, has a code sample like the following:
try {
lib::function(); } catch(lib::exception const&) { // handle failure }
I don't think there's anything wrong with that code. Exceptions should be handled at the point where it makes sense for the program. Sometimes
On 29.05.22 00:39, Andrzej Krzemienski via Boost wrote: that's far up the call tree, but just as often it's very close to where the exception is thrown. The value in exceptions is not just that they can be handled non-locally, but that they provide a uniform way of handling both local and non-local failures. -- Rainer Deyke (rainerd@eldwood.com)
On 5/28/22 3:39 PM, Andrzej Krzemienski via Boost wrote:
Hi Everyone, I wanted to share an observation. A number of Boost libraries, in documentation, when it comes to demonstrating that a given function throws an exception, has a code sample like the following:
try {
lib::function(); } catch(lib::exception const&) { // handle failure }
Could you explain what's wrong with the above code? Robert Ramey
Regards, &rzej;
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
On 30/05/2022 02:10, Robert Ramey wrote:
On 5/28/22 3:39 PM, Andrzej Krzemienski wrote:
try {
lib::function(); } catch(lib::exception const&) { // handle failure }
Could you explain what's wrong with the above code?
I believe the point was that if exceptions are always caught immediately then it smacks of the "exceptions as control flow" anti-pattern, and a non-throwing form of the library function ought to be available/used instead. Essentially, the exception is being used as an alternative return value rather than as an actual exception. It's from the school of thought that exceptions should be "truly exceptional" -- i.e. any condition that might theoretically raise an exception can be explicitly checked in advance by a non-throwing method, such that a well-written program operating in a reasonable environment (e.g. not running out of heap or being actively attacked) should never throw nor catch any exceptions at all -- if it happens, it's a programmer error and should probably crash the app entirely. I don't entirely agree with that sentiment (particularly the last part), but I do agree that you should be able to turn on first-chance exceptions in your debugger and *not* have it breakpoint every second.
On Sun, May 29, 2022 at 3:49 PM Gavin Lambert via Boost < boost@lists.boost.org> wrote:
On 30/05/2022 02:10, Robert Ramey wrote:
On 5/28/22 3:39 PM, Andrzej Krzemienski wrote:
try {
lib::function(); } catch(lib::exception const&) { // handle failure }
Could you explain what's wrong with the above code?
I believe the point was that if exceptions are always caught immediately then it smacks of the "exceptions as control flow" anti-pattern, and a non-throwing form of the library function ought to be available/used instead. Essentially, the exception is being used as an alternative return value rather than as an actual exception.
It's from the school of thought that exceptions should be "truly exceptional" -- i.e. any condition that might theoretically raise an exception can be explicitly checked in advance by a non-throwing method
I'll let him speak for himself, but I'd be surprised if Andrzej's point comes from that school of thought, he is not a Rust programmer. :) The issue with such examples is that they are useless at best: an experienced programmer knows what to do, while a novice might think that he is always supposed to immediately catch exceptions, as may be the case in other languages (e.g. Java) which lack deterministic termination. The correct use of exception handling in C++ is to let exceptions propagate unmolested by default; there is no explicit checking for errors after calling a function that may throw. You catch only where the error can be handled and the program can restore normal operations. And while it is true that unhandled exceptions terminate the program, that is always a logic error, programs should handle all exceptions that may be thrown. Terminating the program when an exception is thrown is valid only under -fno-exceptions (in Boost you'd do that by providing a suitable definition for boost::throw_exception).
On 30/05/2022 11:59, Emil Dotchevski wrote:
The correct use of exception handling in C++ is to let exceptions propagate unmolested by default; there is no explicit checking for errors after calling a function that may throw. You catch only where the error can be handled and the program can restore normal operations.
The problem with this definition is that it is too vague (although that is somewhat of necessity due to being general advice). Someone can, by the letter (but not the spirit) of that rule, argue that their program can resume normal operations immediately outside of that function call (to e.g. handle a "file not found" or a "input string was not a number" type of error that really ought to be checked a different way).
And while it is true that unhandled exceptions terminate the program, that is always a logic error, programs should handle all exceptions that may be thrown. Terminating the program when an exception is thrown is valid only under -fno-exceptions (in Boost you'd do that by providing a suitable definition for boost::throw_exception). >
The argument for the "truly exceptional" case is that since exceptions should never normally happen, there's no reasonable way to resume normal operations (e.g. data structures might be in an inconsistent state if an exception was thrown mid-mutation). If everything uses the strong exception guarantee all the way down, or you can otherwise guarantee that you've destroyed all data structures that might have been mutated by the exception-generating codepath, then it may be safe to continue. But this is a more difficult bar to meet than "exceptions should never happen, just die" -- and it's easy to get it wrong and continue running with inconsistent data that acts as a time-bomb for later misbehaviour or incorrect calculations.
Gavin Lambert wrote:
On 30/05/2022 11:59, Emil Dotchevski wrote:
The correct use of exception handling in C++ is to let exceptions propagate unmolested by default; there is no explicit checking for errors after calling a function that may throw. You catch only where the error can be handled and the program can restore normal operations.
The problem with this definition is that it is too vague (although that is somewhat of necessity due to being general advice).
Someone can, by the letter (but not the spirit) of that rule, argue that their program can resume normal operations immediately outside of that function call (to e.g. handle a "file not found" or a "input string was not a number" type of error that really ought to be checked a different way).
Someone can, but it would be hard if lib::function also supplies a nonthrowing overload. It makes absolutely no sense to prefer the throwing overload if the exception is handled immediately, as in the example given.
The argument for the "truly exceptional" case is that since exceptions should never normally happen,
This sort of programs use exceptions to detect logic errors, e.g. vector::at even if the index is known to be in range. That's kind of legitimate, if not particularly appealing, but I suspect the documentation examples we're discussing here don't fall into this category. We're probably talking more about json::value::at, where the value comes from external input, and exceptions happening is a totally normal occurrence.
On Sun, May 29, 2022 at 5:56 PM Peter Dimov via Boost <boost@lists.boost.org> wrote:
Gavin Lambert wrote:
On 30/05/2022 11:59, Emil Dotchevski wrote:
The correct use of exception handling in C++ is to let exceptions propagate unmolested by default; there is no explicit checking for errors after calling a function that may throw. You catch only where the error can be handled and the program can restore normal
The problem with this definition is that it is too vague (although that
is
somewhat of necessity due to being general advice).
Someone can, by the letter (but not the spirit) of that rule, argue
program can resume normal operations immediately outside of that function call (to e.g. handle a "file not found" or a "input string was not a number" type of error that really ought to be checked a different way).
Someone can, but it would be hard if lib::function also supplies a nonthrowing overload. It makes absolutely no sense to prefer the throwing overload if
exception is handled immediately, as in the example given.
The argument for the "truly exceptional" case is that since exceptions should never normally happen,
This sort of programs use exceptions to detect logic errors, e.g. vector::at even if the index is known to be in range. That's kind of legitimate, if not
operations. that their the particularly
appealing
It is legitimate only in the sense that it is valid to throw an exception when the preconditions for calling a function aren't met (just as it would be valid to do other things, like assert or outright abort). It's a bug, we shouldn't be trying to recover a buggy program by catching exceptions.
pon., 30 maj 2022 o 01:59 Emil Dotchevski via Boost <boost@lists.boost.org> napisał(a):
On Sun, May 29, 2022 at 3:49 PM Gavin Lambert via Boost < boost@lists.boost.org> wrote:
On 30/05/2022 02:10, Robert Ramey wrote:
On 5/28/22 3:39 PM, Andrzej Krzemienski wrote:
try {
lib::function(); } catch(lib::exception const&) { // handle failure }
Could you explain what's wrong with the above code?
The code itself need not necessarily be wrong. My concern is that it appears in the context of the documentation of a Boost library. It is my experience from 15 years ago -- maybe the situation is different now -- that there is no place or course where you can learn good C++: why value semantics are important, why exceptions and exception safety is important, why resource handling is important. You can program in C++ for 5 years, be considered an "expert" in your workplace, and not know the foundations of C++. In that case, you learn from what you got: libraries you use, and reading the docs. This is how I learned good C++: by reading Boost docs, and Bost discussion groups. So, a programmer with 5 years experience in C++, who thinks they know C++, and believe that "if library throws an exception you must immediately catch it, or else the program will crash" (I was literally taught that in my C++ classes), sees an example like this, then the bat habit is immediately reinforced: they will think, "ok, so Boost does this this way too, so it must be the right way". Also, I simplified it too much. The examples that I was referring to have this pattern: try { lib::type p = lib::function(); } catch(lib::exception const&) { // handle failure } Now, it is almost impossible that someone -- experienced or not -- would be writing a code like this. If you create an object `p`, it is because you want to use it: read its value, or alter its state. And you cannot use the object that immediately goes out of scope. So the real code would be try { lib::type p = lib::function(); use(p); } catch(lib::exception const&) { // handle failure } Or: lib::type p = {}; try { lib::type p = lib::function(); } catch(lib::exception const&) { // handle failure } use(p); Which is usually exception unsafe, goes against the design of exception handling, and is often the source of bugs. I have seen this pattern too often in the programs that I am in charge of: literally with the comment like this "// handle failure", and without no handling actually, only a "TODO" comment.
I believe the point was that if exceptions are always caught immediately then it smacks of the "exceptions as control flow" anti-pattern, and a non-throwing form of the library function ought to be available/used instead. Essentially, the exception is being used as an alternative return value rather than as an actual exception.
It's from the school of thought that exceptions should be "truly exceptional" -- i.e. any condition that might theoretically raise an exception can be explicitly checked in advance by a non-throwing method
I'll let him speak for himself, but I'd be surprised if Andrzej's point comes from that school of thought, he is not a Rust programmer. :)
The issue with such examples is that they are useless at best: an experienced programmer knows what to do, while a novice might think that he is always supposed to immediately catch exceptions, as may be the case in other languages (e.g. Java) which lack deterministic termination.
The correct use of exception handling in C++ is to let exceptions propagate unmolested by default; there is no explicit checking for errors after calling a function that may throw. You catch only where the error can be handled and the program can restore normal operations. And while it is true that unhandled exceptions terminate the program, that is always a logic error, programs should handle all exceptions that may be thrown. Terminating the program when an exception is thrown is valid only under -fno-exceptions (in Boost you'd do that by providing a suitable definition for boost::throw_exception).
To a great extent I agree with Emil's view. although I would probably use different words: you only catch (and not rethrow) an exception potentially thrown by instruction X when you are sure the subsequent instructions do not rely on the successful execution of X. Reards, &rzej;
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
On 5/28/22 3:39 PM, Andrzej Krzemienski via Boost wrote:
try {
lib::function(); } catch(lib::exception const&) { // handle failure }
Pretty good and reasonable answers. These seem to boil down to, any kind of failure in lib::function (or below) we would handle at the point of failure hence if we need to throw an exception we've skipped over a failure case. (paraphrasing - still not sure I get it). I'm envisioning some that I think happens to me on a regular base. Inside of lib::function (or lower) I request a large memory allocation which I expect to almost never fail. I can trap this failure. If I return failure at each call up the chain, I end up doing a lot of "if" at each level making the code slower and messier. And this slowness messiness happens even on cases where there is no failure. I don't have exceptions in this scenario. What I actually do in this case is throw a custom exception at the point of failure which gives me information of where I was and what I was doing when I requested this "too large" memory allocation. This seems to be a very reasonable scenario to me. Robert Ramey
On 30/05/2022 13:50, Robert Ramey wrote:
What I actually do in this case is throw a custom exception at the point of failure which gives me information of where I was and what I was doing when I requested this "too large" memory allocation. This seems to be a very reasonable scenario to me.
Technically, the "right" thing to do in this case would be to allow the original bad_alloc to bubble up, perhaps using Boost.Exception, Boost.Leaf, or std::throw_with_nested to decorate it with additional context via catch-and-rethrow. That could potentially be a good use for exceptions (when handled far from the point of actual failure). Although it can still be argued that trying to make a too-large memory allocation is usually the result of a failure of input sanitisation (e.g. not checking data from input files), which is still a kind of coding bug.
On Sun, May 29, 2022 at 7:25 PM Gavin Lambert via Boost < boost@lists.boost.org> wrote:
Although it can still be argued that trying to make a too-large memory allocation is usually the result of a failure of input sanitisation (e.g. not checking data from input files), which is still a kind of coding bug.
This was the case some time ago but no longer -- in today's virtual environments std::bad_alloc is quite possible and it ought to be handled properly.
On Sun, May 29, 2022 at 7:30 PM Emil Dotchevski via Boost <boost@lists.boost.org> wrote:
std::bad_alloc is quite possible and it ought to be handled properly.
In almost 100% of cases, std::bad_alloc is thrown because the process has default memory restrictions via ulimit. Also I made up that number. Thanks
On Sun, May 29, 2022 at 7:37 PM Vinnie Falco via Boost < boost@lists.boost.org> wrote:
On Sun, May 29, 2022 at 7:30 PM Emil Dotchevski via Boost <boost@lists.boost.org> wrote:
std::bad_alloc is quite possible and it ought to be handled properly.
In almost 100% of cases, std::bad_alloc is thrown because the process has default memory restrictions via ulimit.
Also I made up that number.
At any rate it is no longer safe to assume that std::bad_alloc is the result of a failure to check for valid input; in a VM environment the system limits are often restricted.
On 5/30/22 04:37, Vinnie Falco via Boost wrote:
In almost 100% of cases, std::bad_alloc is thrown because the process has default memory restrictions via ulimit.
It may be restricted by other means: https://www.boost.org/libs/pool/doc/html/index.html
On Sat, May 28, 2022 at 3:39 PM Andrzej Krzemienski via Boost <boost@lists.boost.org> wrote:
Boost is a place to promote better programming practices. And these "catch immediately after throw and swallow" examples just do a disservice to the community.
Are you proposing something broad, that Boost documentation and sample code should adopt as a secondary goal, to teach best C++ practices in general? Or is the scope of your post limited to just exception handling? Thanks
On 5/29/22 7:36 PM, Vinnie Falco via Boost wrote:
On Sat, May 28, 2022 at 3:39 PM Andrzej Krzemienski via Boost <boost@lists.boost.org> wrote:
Boost is a place to promote better programming practices. And these "catch immediately after throw and swallow" examples just do a disservice to the community.
Are you proposing something broad, that Boost documentation and sample code should adopt as a secondary goal, to teach best C++ practices in general?
Or is the scope of your post limited to just exception handling?
Thanks
I think it's reasonable to expect that boost documentation demonstrate best practices and not antipatterns. Of course there might not be as much agreement on what "best practices and not antipatterns" as people think there are. In fact, I run into this all the time. Robert Ramey
participants (8)
-
Andrzej Krzemienski -
Bjorn Reese -
Emil Dotchevski -
Gavin Lambert -
Peter Dimov -
Rainer Deyke -
Robert Ramey -
Vinnie Falco