Avoid crashes at shutdown due to printf() in global destructors. #1909
Conversation
| // allocate mutexDebugLog on the heap the first time this routine | ||
| // is called to avoid crashes during shutdown. | ||
| static boost::mutex* mutexDebugLog = NULL; | ||
| if (mutexDebugLog == NULL) mutexDebugLog = new boost::mutex(); |
There was a problem hiding this comment.
What if OutputDebugStringF is being called twice simultaneously? This doesn't solve anything, unless you have a guaranteed call of this function while still in single-thread modus. That's almost certainly the case, but if it is, why not make it obvious, and have an InitLogging() function, called in init?
I'm nitpicking. ACK.
There was a problem hiding this comment.
@sipa I also proposed that in #1832. I'm for explicitly constructing and destructing the global objects, such as logging and db, enforced by assertions. This makes thiings more predictable and maintainable than armoring against all possible non-determinism (which is pretty much impossible, you will forget something).
Then again, it's good to have a fix in for 0.7.1. ACK.
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/cac6b389d101999d98c3137b17812cce062f924d for binaries and test log. |
|
Yeah, it just seems terribly odd and possibly error-prone to allocate a lock... in a racy fashion. Just make the lock global and put it early in main, to make sure it is instantiated/initialized before anything else in the program. As it stands now, either this your change or without, the first-use occurs very late in the program, and that seems like a root cause (or at least contributing factor). |
|
@jgarzik making it global and putting it early in main won't fix the problem; the order of global destructors is undefined in C++. As long as there is a printf/OutputDebugStringF before we start creating threads (and there is, early in AppInit2()) there is no race. Reworking logging should be done... someday... For now, I think this little change is the right thing to do. |
|
gcc has long followed the now defined C++0x order described here: http://cpp0x.centaur.ath.cx/basic.start.term.html "If the completion of the constructor or dynamic initialization of an object with static storage duration is sequenced before that of another, the completion of the destructor of the second is sequenced before the initiation of the destructor of the first. If an object is initialized statically, the object is destroyed in the same order as if the object was dynamically initialized" |
|
I already acked this for 0.7.1 (assuming we want this out of the door soon), but it does need more thinking. If you make the pointer global (i.s.o static) and explicitly initialize it in an InitLogging() (called directly at the beginning of AppInit2) function you avoid the destructor trouble, plus you don't make the safety of initialization dependent on who-calls-first. Then add an assert() to printf that the lock is allocated, just in case. Another plus is that you can use the same lock in After all, printf does have some prerequisite requirements: it needs the arguments to be parsed and DataDir to be set correctly, which is only guaranteed when entering AppInit2. What if someone accidentally adds a printf before this is done? (ie, in GUI initialization, after all a printf looks reaaallly harmless):
An explicit InitLogging() call would solve this, and trap all accidental calls to printf before everything is in order. Talking of GetDataDir, it has the same issue. Though we jump the shark as it will never enter the lock when the path is cached, which you expect by the time it reaches destructors :-) But I can see another subtle initialization race, I think. |
|
Ok. I pinky-swear promise I'll rewrite this The Right Way for 0.8. |
|
Maybe The Right Way means using http://www.boost.org/doc/libs/1_32_0/doc/html/call_once.html ? |
1947519 [Cleanup] Remove unneeded "fix" for segfault with getinfo on startup (random-zebra) 84acfa1 [RPC] Throw immediately if RPC is used when the server is in warmup (random-zebra) Pull request description: **Problem**: Most RPC commands crash the wallet during the initialization phase. **Solution**: An attempt to fix this specifically for `getinfo` was made in bitcoin#543, checking the existence of `chainActive.Tip()`. A better solution, instead of checking all places that try to access the block index, (or the chainstate, or the second layer maps, etc...), is to throw a JSON-RPC exception with any call, if the server is still in warm-up phase (returning the exact init message), directly from `CRPCTable::execute`. Closes bitcoin#1908 ACKs for top commit: furszy: ACK 1947519 Fuzzbawls: utACK 1947519 Tree-SHA512: 4f37e512cb411473135c6d1fbb5013bddbcfca62b8a1ed6777481e90f60374480e3617ed1329a10d3cb1e614a2f17a827da2dce1da793b0db3c6b13222cdec19
Fixes #1832