Curt Arnold wrote:
>
> On Jan 16, 2009, at 9:34 AM, Rhosyn wrote:
>
>> Dear all,
>>
>>
>> The crashes
>> ===========
>>
>> We would like to share with you some patches to log4cxx  we have found
>> useful in our environment.
>>
>> Our product  (a Linux platform server product, compiled with g++ 4.3.x)
>> started suffering from crashes during (what should have been) graceful
>> process exit ("exit(0);")- after we replaced our old logging framework
>> with a log4cxx backend.
>>
>> Commonly we would see segmentation faults in:
>>
>> * log4cxx::helpers::ObjectPtrBase()
>> * log4cxx::LogManager::getLoggerLS()
>>
>>
>>
>> The cause
>> =========
>>
>> We eventually traced this issue to the pervasive pattern of (mis)usage
>> of singletons in the log4cxx code.
>>
>> The log4cxx library uses the "Meyers" singleton pattern, first
>> popularised in Scott Meyers "Effective C++" (Item 47 in the 2nd edition
>> of that book):
>>
>> Thing & getThingSingleton()
>> {
>>    static Thing t;
>>    return t;
>> }
>>
>>
>> For many years, the above pattern was considered "best practice" for
>> using Singletons in C++ - and was generally safe for most popular
>> compiler implementations and most applications.
>>
>> Unfortunately, this recommendation is not actually guaranteed to be
>> thread-safe for construction or destruction - something which is alluded
>> to on Scott Meyers' own "Errata List for Effective C++, Second Edition"
>> as described here: http://www.aristeia.com/BookErrata/ec++2e-errata.html
>>
>>
>> The nub of the problem is that when a process calls "exit(0);" or
>> similar, one thread will start running, in order, any user-registered
>> "atexit" functions.
>>
>> Along with these, the compiler will execute the (conceptually similar)
>> compiler-registered functions which invoke the destructors of any static
>> file or function scope objects (also in order - the reverse order to
>> static object construction).
>>
>> Unfortunately, other threads may still be in the process of running -and
>> logging, perhaps using the static objects - during or after the
>> execution of their destructors - thus opening the door to a bunch of
>> potential SEGFAULTs.
>>
>>
>> Solutions
>> =========
>>
>> Andrei Alexandrescu goes into a lot of detail about this C++ design
>> problem in chapter 6 of "Modern C++ design" and proposes two elegant
>> solutions -  a "Phoenix Singleton" or  SingletonHolder class.
>>
>> The patches attached are somewhat less elegant than either of
>> Alexandrescus suggestions (we were pressed for time and needed a quick
>> fix in order to ship on time).
>>
>> However, the patches supplied are simple, pragmatic - and did appear to
>> hold up during our testing (testing which was readily producing the
>> crashes described earlier, before we patched).
>>
>> In summary, the patches change Singleton functions to work thus:
>>
>> Thing & getThingSingleton()
>> {
>>    static Thing * t = new t;
>>    return *t;
>> }
>>
>> Of course, the downside of this flavour of fix is that:
>>
>> * the static objects - now allocated on the heap with new() - never get
>> their destructors run.  AFAIK, no other resources (other than memory)
>> appear to be leaked (due to this patch).  Fortunately, as we are running
>> on a modern OS, we are able to rely on the OS to reclaim the process
>> memory on process exit - thus nullifying this particular issue for our
>> product (but not neccessarily so in other environments).
>>
>> * The patch doesn't address the startup/initialisation race  (for that
>> we'd need a multiple-locked singleton initialization pattern everywhere
>> log4cxx creates a singleton) - we're less worried about that at this
>> stage as we have yet to notice any issues.
>>
>> apr
>> ===
>>
>> Finally we couldn't work out how it could ever be safe to deiinitialise
>> APR if there was even the slightest chance that any extant log4cxx
>> objects existed (accessible to any thread).  We therefore removed the
>> apr_terminate() call in APRInitializer::~APRInitializer()
>>
>>
>> Future
>> ======
>>
>> I would like to respecfully suggest that there is a discussion in the
>> log4cxx community about  the best way of reworking the use of singletons
>> in the log4cxx library  (multithreaded-safe construction and
>> destruction)  - and that we look to moving towards a different pattern
>> of usage.
>>
>> I suspect Alexandrescu's "singleton holder" idea might form a part of a
>> possible solution - but it's not the only game in town.
>>
>>
>>
>
> Thanks for the analysis.
>
> There is a bit of tension here since those who are running under
> BoundsChecker, Valgrind, Purify et al will then complain about leaks.  
> Probably the best approach is to try to isolate the singleton pattern
> into a preprocessor macro and then allow the user to select what
> singleton pattern they'd like to use.
>
> Please file this as a bug report at http://issues.apache.org/jira
>


Thanks; JIRA issue LOGCXX-322 raised, as requested.


I totally see your point re: BoundsChecker, Valgrind etc. and also re:
isolating the behaviour.

A more sophisticated patch than my first cut is definitely the order of
the day!