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!
