Memory leaks?

[Eric Rosenquist <er...@rosenquist.com>]

I'm using log4cxx in a C++ application I'm writing and it's great -
gives us a large measure of compatibility between our C++ and Java code.
I needed threading support in my own app and ideally wanted something
that worked on Linux and Windows.  Linux is the target platform, but
it's nice to be able to debug things on Windows with Visual Studio in
the early going.

Rather than cook up my own thread classes I ended up using the Runnable
and Thread classes from log4cxx. What I've noticed with both valgrind
and the VC++ memory leak detection is that every thread I create causes
a small leak.  The object getting lost is due to the MDC object
associated with each thread.  Thread::run() calls
MDC::setContext(parentMDCMap) which causes a new Map object to be
associated with the thread. The per-thread map never seems to get
deleted anywhere when the thread terminates.

I've added a call to MDC::clear() at the end of Thread::run() - that
seems to cure the leak nicely and so far I haven't encountered any ill
effects.

One other minor thing that valgrind was complaining about is the
StringTokenizer destructor. The member variable this->str is allocated
using an array version of "new" in the constructor, but the delete call
in the destructor did not use the array version of "delete".

The patches are trivial if anyone wants to try them out:

Index: src/stringtokenizer.cpp
===================================================================
RCS file: /home/cvspublic/logging-log4cxx/src/stringtokenizer.cpp,v
retrieving revision 1.6
diff -r1.6 stringtokenizer.cpp
42c42
< 	delete this->str;
---
> 	delete [] this->str;
Index: src/thread.cpp
===================================================================
RCS file: /home/cvspublic/logging-log4cxx/src/thread.cpp,v
retrieving revision 1.14
diff -r1.14 thread.cpp
115a116
> 	MDC::clear();


Eric

Re: ThreadSpecificData improvement.

[Christophe de Vienne <cd...@alphacent.com>]

Curt Arnold wrote:

>> Another solution is to typedef the main types we use in threading, 
>> depending on PTHREAD (not HAVE_PTHREAD), and WIN32. This will require 
>> the library user to add -DPTHREAD on it's compiler command line, 
>> which is not a problem I think (a lot of other library do).
>> For example in portability.h, having:
>
> The mechanism used for critical sections is an implementation detail 
> that is ideally invisible to the client application.  I think it is 
> much more preferable to hide that implementation detail than to make 
> the build for the client application guess the value used to build the 
> library.

Ok then let's go for pimpl. I'll keep you informed.

Cheers,

Christophe

Re: ThreadSpecificData improvement.

[Curt Arnold <ca...@houston.rr.com>]

>> 2. The CriticalSection instance in log4cxx::helpers::ObjectImpl 
>> causes config.h to be expanded for the abstract classes in public 
>> API.  Isolating implementation details from the client API would 
>> require removing the CriticalSection from the abstract base class and 
>> reintroducing it in the concrete implementation classes. perhaps via 
>> templating and/or macros.
>
> Another solution is to typedef the main types we use in threading, 
> depending on PTHREAD (not HAVE_PTHREAD), and WIN32. This will require 
> the library user to add -DPTHREAD on it's compiler command line, which 
> is not a problem I think (a lot of other library do).
> For example in portability.h, having:
>

The mechanism used for critical sections is an implementation detail 
that is ideally invisible to the client application.  I think it is 
much more preferable to hide that implementation detail than to make 
the build for the client application guess the value used to build the 
library.

ThreadSpecificData improvement.

[Christophe de VIENNE <cd...@alphacent.com>]

Curt Arnold a écrit :

>
> On Aug 13, 2004, at 9:44 AM, Christophe de VIENNE wrote:
>
>> Any idea/suggestion welcome,
>>
>>
>
> Not directly on point, but I had a couple of random, unconsidered 
> thoughts related to threads that I'd like to throw out if anyone is 
> going to be digging into to this.
>
> 1. Apache Portable Runtime (http://apr.apache.org) provides a C 
> threading API.  Thought it probably isn't desirable to require APR 
> especially just for threading, it might be good to support it as a 
> threading option to support rarer platforms.

The threading API of APR looks quite simple (and close to pthread), it 
shouldn't be a problem to support it the day it's needed.

> 2. The CriticalSection instance in log4cxx::helpers::ObjectImpl causes 
> config.h to be expanded for the abstract classes in public API.  
> Isolating implementation details from the client API would require 
> removing the CriticalSection from the abstract base class and 
> reintroducing it in the concrete implementation classes. perhaps via 
> templating and/or macros.

Another solution is to typedef the main types we use in threading, 
depending on PTHREAD (not HAVE_PTHREAD), and WIN32. This will require 
the library user to add -DPTHREAD on it's compiler command line, which 
is not a problem I think (a lot of other library do).
For example in portability.h, having:

#ifdef PTHREAD
typedef pthread_mutex_t log4cxx_mutex_t;
typedef phtread_key_t log4cxx_key_t;
typedef pthread_t log4cxx_thread_t;
#else
#ifdef WIN32
typedef CRITICAL_SECTION log4cxx_mutex_t;
typedef void * log4cxx_key_t;
typedef void * log4cxx_thread_t;
#endif
#warning "No threading support"
#endif



OR, we keep the HAVE_xx macros, but build them from PTHREAD and WIN32 :

#if defined(PTHREAD)
# define LOG4CXX_HAVE_PTHREAD
#elif defined(WIN32)
# define LOG4CXX_HAVE_MSTHREAD
#endif



About TheadSpecificData, I'm thinking of improving the API this way :

class ThreadSpecificData
{
    public:
        ThreadSpecificData();
        ThreadSpecificData(void (*cleanup)(void*));  // this is a new 
constructor.

        void * GetData() const;
        void SetData(void * data);

   // ....
};


/** This ptr interface is taken from boost, I don't know if we can take 
it like this. On the other hand, starting from scratch with the same 
goal in mind, we'd end with the exact same result... */
template < class T >
class thread_specific_ptr
{
    public:
         tsd_ptr(): m_impl( &tsd_ptr::cleanup ) {}

         T* get() const {
              return m_imp->getData();
         }

         T* operator->() const { return get(); }
         T& operator*() const { return *get(); }
        
         T* release() {
             T* tmp = get();
             m_impl.setData(0);
             return tmp;
         }

         operator bool () { return get() == 0; }
 
         operator T& () { return *get(); }

          void reset(T* p=0) {
             T* tmp = m_impl.getData();
             if(tmp)
                 delete tmp;
            impl_.setData(p);
         }


    private:
       static void cleanup(void* p) { delete static_cast<T*>(p); }

        ThreadSpecificData m_impl;
};





Christophe

Re: Memory leaks?

[Curt Arnold <ca...@apache.org>]

On Aug 13, 2004, at 9:44 AM, Christophe de VIENNE wrote:
> Any idea/suggestion welcome,
>
>

Not directly on point, but I had a couple of random, unconsidered 
thoughts related to threads that I'd like to throw out if anyone is 
going to be digging into to this.

1. Apache Portable Runtime (http://apr.apache.org) provides a C 
threading API.  Thought it probably isn't desirable to require APR 
especially just for threading, it might be good to support it as a 
threading option to support rarer platforms.

2. The CriticalSection instance in log4cxx::helpers::ObjectImpl causes 
config.h to be expanded for the abstract classes in public API.  
Isolating implementation details from the client API would require 
removing the CriticalSection from the abstract base class and 
reintroducing it in the concrete implementation classes. perhaps via 
templating and/or macros.

Re: Memory leaks?

[Christophe de VIENNE <cd...@alphacent.com>]

Eric Rosenquist wrote:

>The per-thread map never seems to get
>deleted anywhere when the thread terminates.
>  
>

I had a closer look to threadspecificdata.cpp. It seems that no mecanism 
is provided to allow an automatic deletion of objects on thread 
termination. Adding it would be a more clean and reliable solution to 
this memory leak.
This can easily be done, at least with pthreads, with a templated class 
looking like :

template<class T>
class TSD
{
    public:
       TSD(): key(0) {
            pthread_key_create(&key, &destr_function);
       }

       // ...

    private:
        static void destr_function(void * data) {
             delete static_cast<T*>(data);
        }
        pthread_key_t key;
};


If it's possible to do it with msthreads (I guess so) I'd like to 
propose a more complete TSD handling solution.
If we do it, we can profit of the occasion to use the pimpl idiom on 
ThreadSpecificData, which would solve the HAVE_PTHREAD macro problem, at 
least for this file.

Any idea/suggestion welcome,


Christophe