[TRIVIAL] Improve reporting of missing node exceptions #3351
Conversation
| : std::runtime_error ("SHAMapMissingNode") | ||
| , mType (t) | ||
| , mNodeHash (nodeHash) | ||
| SHAMapMissingNode (SHAMapType t, SHAMapHash const& hash) |
There was a problem hiding this comment.
What do you think of adding the stack trace to the message when in debug mode? (here and the other constructors)
There was a problem hiding this comment.
I thought about doing this in LogThrow actually. My primary concern is that capturing a call stack is expensive to do everytime we call Throw even in debug mode. Perhaps a special config and/or runtime option?
@HowardHinnant: is it reasonable to use std::set_terminate to attempt to print a call stack from the termination handler? The actual call-stack displayed will depend on how the underlying system handles stack unwinding, so collecting the stack from there may not be helpful.
There was a problem hiding this comment.
On Mac and Linux, the existing terminate handler creates a very good stack dump. I think we used to have a custom terminate handlers and got rid of them in order to improve our stack dumps.
There was a problem hiding this comment.
The question is where do we get the usable stack dumps from? I know on Mac you can get them in the log, but what about Linux?
There was a problem hiding this comment.
As not-a-Linux-user, I don't know. I thought we had that knowledge within the rippled team though. We used to get them mailed to us whenever a production server went down (hasn't happened in awhile).
Perhaps I'm misunderstanding this thread...
There was a problem hiding this comment.
That was caused by a script we were running on production servers ran by Ripple that would detect the crash and automatically ran gdb on the resulting core file. The question is what happens if there is no core file? On the Mac we do get a very pretty call stack in the logs. We'll have to research what happens on Linux.
| @@ -830,7 +830,7 @@ RCLConsensus::timerEntry(NetClock::time_point const& now) | |||
| catch (SHAMapMissingNode const& mn) | |||
| { | |||
| // This should never happen | |||
| JLOG(j_.error()) << "Missing node during consensus process " << mn; | |||
| JLOG(j_.error()) << "During consensus process: " << mn.what(); | |||
There was a problem hiding this comment.
Don't need the .what() (here and below) if you're still defining the stream operator (it still used in other places in the code).
There was a problem hiding this comment.
I'm thinking of just removing the << and printing what() instead. Not sure if that's better.
There was a problem hiding this comment.
I look at the stream operator<< as the C++ version of __str__ in Python or Show in Haskell, generally considered good practice, so I like it.
There was a problem hiding this comment.
Hmmm. That seems reasonable.
Codecov Report
@@ Coverage Diff @@
## develop #3351 +/- ##
===========================================
+ Coverage 70.46% 70.48% +0.01%
===========================================
Files 684 683 -1
Lines 55001 54995 -6
===========================================
+ Hits 38759 38762 +3
+ Misses 16242 16233 -9
Continue to review full report at Codecov.
|
carlhua
added
the
Passed
No description provided.