We shouldn't be allowing exceptions to propagate from a destructor. Doing that is always bad in C++. Any code which does it should be treated as a bug and fixed.
It's okay to have exceptions thrown inside a destructor, just as long as you catch() them inside the destructor too and don't allow them to propagate out. On Fri, Jun 6, 2014 at 8:52 AM, chris beck <beck...@gmail.com> wrote: > Dear List, > > A few days ago I was looking at some of the code in the engine on the > master branch, and I realized that we currently are doing > some questionable things with exceptions and destructors that can cause > the program to crash. > > > https://github.com/wesnoth/wesnoth/blob/46299c6a110b09ebfa9f9e3044021ece2568ea4d/src/playmp_controller.cpp#L426 > > At this line, we signal a "quit" from a client event loop state > `end_level_exception (QUIT)`. This is not uncommon in the engine, > we currently use "end_level_exception" and "end_turn_exception" to signal > when a turn has ended or a level had ended, and in mp > we sometimes throw "network::error" when we want to disconnect from the > current game. > > It's debatable whehter using exceptions for flow control is good or bad, > and I guess many people have opinions about it. > However, in this case, it actually leads to crash, because we also have an > object on the stack `turn_info_send` which sends > networking data in it's destructor: > > > https://github.com/wesnoth/wesnoth/blob/afbf5c6e5804ef5a31a40b4bcb59a41f389e5a40/src/playturn.hpp#L91 > > What this means is that if the networking module has an error, like, it > loses connection with the server, instead of handling > that network::error exception normally, and jumping here: > > > https://github.com/wesnoth/wesnoth/blob/afbf5c6e5804ef5a31a40b4bcb59a41f389e5a40/src/playsingle_controller.cpp#L589 > > which would tell the user "a network error has occurred, would you like to > save the game?", the program will *immediately crash > to desktop*. (Because, as I understand, in C++, if an exception is thrown, > the callstack is unwound until a handler for the > exception is reached, or the program is terminated, and in this process > all destructors are called for any objects on the stack. > If any of those destructors throws an exception, the program is > immediately terminated -- you cannot handle multiple exceptions > at once.) > > I'm not sure because there is a lot of code involved in synchroniation and > networking, but I'm concerned that issues like this > from somewhere in the code could be causing instability on 1.12 branch. > For instance we got some bug reports like this: > > https://gna.org/bugs/?21873 > > "We played a couple multiplayer matches, each one ended with him losing > but everytime he lost the game reported him as having disconnected and when > he got back he said that the game had forcibly shut down. This would happen > in the middle of the post-game screen even before I hit end scenario." > > That's pretty much exactly what I would expect if we were getting some > kind of double exception issues. > > --- > > There are two morals that I take away from this: > > 1.) Don't use exceptions for flow control. > 2.) Don't throw exceptions from the destructor. > > As I understand there are some C++ developers out there who will argue on > behalf of both of these things, but if used in concert they will apparently > lead to very poor results. It is sometimes said, it takes two aggressive > drivers to make an accident, but at this point I think we have had an > accident and now should add remarks to "coding standards" on the wiki > disallowing both of these. > > --- > > A bit more on each -- > > (2) After reading about this (I learned in school that you shouldn't do > this but I long ago forgot why and had to read about it...), it sounds like > there are some C++ developers that actually defend this practice of > throwing from a destructor in some cases. In C++98 if you can somehow > guarantee that there is no exception actively being unwound, then throwing > from the destructor won't immediately crash the program. However: > - It will prevent destructors of children being called and can therefore > lead to memory leaks that are very difficult if not impossible to correct > after the throw, > - In C++11 the language is more strict about this -- if a destructor ever > throws an exception, the program is immediately terminated. > So it sounds like it's not something that we want to do. In fact, it might > be a good idea if we tell people that if they do anything nontrivial in a > destructor, they must wrap it in a try { stuff } catch (...) {} block that > will swallow all exceptions. Even writing to a logger in a destructor could > in some cases lead to a crash I think: > > a.) Suppose that at some time in the future, there is a bug our > UTF-conversion facilities. A bad string conversion happens while someone is > writing to a logger object -- the logger chokes and throws some kind of > exception which propogates up the play_controller stack. > > If someone somewhere wrote a destructor X::~X() { logger << "foo bar\n" ; > }, if the logger is already broken it could generate a second exception, > immediately crashing the program rather than reporting the error. > > b.) gfgtdf and I have written a "smart enum" macro which is supposed to > help us when parsing WML files to C++ objects, when there are only a few > valid options for strings. The enums have streaming operations defined so > that they can be converted to strings and back, and these ops have built-in > error reporting -- if an enum can't be parsed or written, and the game is > running in debug mode, it will generate a t_wml_exception, which launches a > transient gui window explaining to the user that they have an error in > their WML. There are several places high up in play_controller where these > exceptions are hanlded and the gui window is actually launched. > > However, suppose someone wrote a destructor that logged some debugging > output based on converting some more enums, or similarly calls lexical cast > in some context. In that case if we encounter a bad enum value and throw an > exception, then when X is destroyed we might encounter it _a second time_, > throw a _second_ exception while writing the debugging, and crash instantly > because of it. > > So I think it should be a coding standards issue that destructors that do > more than, delete pointers, or call a C library "release resource" > function, should be wrapped in a try-catch-all block. If the programmer > doesn't like this because it would interfere with normal exception > handling, then they should consider not putting their code into a > destructor. > > However I'm not a C++ expert by any means, and I normally try to stay away > from destructors anyways, so there are likely more things I don't know > about all of this -- please chime in if you have something to add. > > > (1) I guess that this is somewhat controversial, and there are some C++ > developers who think that using exceptions for flow control is fine esp. if > it leads to less typing. > > However, my viewpoint is that, the reason we were tempted to start doing > (2) to handle networking ops in the engine, is because we were doing (1) > and had no way to guarantee that we are staying in sync unless networking > was happening in destructors. So I think (1) is in some way the root cause > of this. > > I have started a branch here, which attempts to incrementally push all of > the flow control exceptions out of the play_controller object at least, and > down further in the call stack into replay execution and event handling. > The goal of this is to simplify the logic that is happening in play > controller, so that we can properly reason about it and ensure networking > synchronization is happening correctly without using exceptions or > destructors. > > https://github.com/wesnoth/wesnoth/pull/191 > > I view this branch as having been fairly productive, just the review > process has led me to find some other bugs elsewhere in 1.12 branch that > could be corrected, and it also clarifies exactly where the end level and > end turn signals are coming from so that we can better understand exactly > what is going on. > > The experience has led me to think that, we should add a line to the > coding standards page: > http://wiki.wesnoth.org/CodingStandards#Write_exception-safe_code > > "Do not use exceptions for normal flow control. This is generally > unnecessary, leads to less maintainable code, and can interfere with the > proper handling of truly exceptional scenarios." > > The purpose of the coding standards remarks is not to criticize other > programmers, but rather to make sure that new programmers actually know > about these things and that they can cause crashes -- the rules about > exceptions and destructors are fairly unique to C++ anyways and not > everyone knows them intimately... > > Best Regards, > Chris Beck > > _______________________________________________ > Wesnoth-dev mailing list > Wesnoth-dev@gna.org > https://mail.gna.org/listinfo/wesnoth-dev > >
_______________________________________________ Wesnoth-dev mailing list Wesnoth-dev@gna.org https://mail.gna.org/listinfo/wesnoth-dev
