Fix server crashing on Lua errors #3305
Conversation
Previously, the server called FATAL_ERROR when a Lua error occured. This caused a (mostly useless) core dump. The server now simply throws an exception, which is caught and printed before exiting with a non-zero return value. This also fixes a number of instances where errors were logged multiple times.
ShadowNinja
added
Bug
|
👍 |
| const ModSpec &mod = *i; | ||
| for (std::vector<ModSpec>::iterator it = m_mods.begin(); | ||
| it != m_mods.end(); ++it) { | ||
| const ModSpec &mod = *it; |
There was a problem hiding this comment.
This has nothing to do with what the PR does. What's bad about i? Is it only reserved for integers?
There was a problem hiding this comment.
Iterators aren't something spooky, only because their incrementation operation is slightly more complex.
There was a problem hiding this comment.
I prefer it because i is traditionaly an abbreviation for integer. This doesn't really matter much though, I wouldn't mind removing it.
There was a problem hiding this comment.
Well, I'm ok if you want to keep it in.
There was a problem hiding this comment.
i is traditionally an abbreviation for integer? Way back in 1994 we were using 'i' because we believed it was an abbreviation for iterator :/ :)
There was a problem hiding this comment.
I like it or iter for iterators because it's immediately clear that the variable is an iterator. I approve of this change too.
Really, I like anything that makes the code clearer.
|
Otherwise, it looks good, but we have to take care that nothing gets deinitialized when |
|
So just to recap what went on in this PR:
Overall, 59 lines were added and 81 removed, while increasing organization. Sounds like a success as long as it doesn't introduce any new bugs. |
|
Yep, that's basically all other than adding a try/catch to handle those exceptions. |
|
Okay then, 👍 |
Previously, the server called
FATAL_ERRORwhen a Lua error occured.This caused a (mostly useless) core dump, and, in servers running managed in gdb, a similarly useless and confusing backtrace.
The server now simply throws an exception, which is caught and printed before exiting with a non-zero return value.
This also fixes a number of instances where errors were logged multiple times (as many as three times).
Fixes #3213 (which was also reported by VanessaE).
Implementation note: I changed loadMod and loadScript from returning a bool and taking a error argument to just throwing an exception on error. This made things easier since the error message was often just turned into an exception and thrown.