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
