Author Topic: memory leak  (Read 11870 times)

muumi

  • User
  • *
  • Posts: 9
memory leak
« on: April 12, 2005, 05:54:55 AM »
Hi,

While I was checking out GNE I noticed there's a memory leak in Mutex. MutexData is never deallocated. I see that the mutex itself inside MutexData is deallocated, though. Fixed the leak by making MutexData a smart pointer.

Gillius

  • Administrator
  • User
  • *****
  • Posts: 147
    • http://www.gillius.org/
memory leak
« Reply #1 on: April 12, 2005, 12:00:23 PM »
Good job, you are precisely correct.  I don't see a reason for a smart pointer here, though, since I only allocate in one spot and deallocate in another.  That pointer is never shared.  The only reason why it is even a pointer in the first place is so that I can hide the "implementation" of MutexData which changes between Windows and UNIX.  Perhaps I could change the include structure slightly so that I don't need a pointer in the first place.  Although I think part of the point was so that MutexData was not included in user code so that the user need not include windows.h.  I'll check in on that.
Gillius
Gillius's Programming http://www.gillius.org/

muumi

  • User
  • *
  • Posts: 9
memory leak
« Reply #2 on: April 12, 2005, 07:26:13 PM »
You don't need to make it a reference counted smart pointer nessecarily. Try boost's scoped_ptr for example. The reason I propose using smart pointers is that they are properly deallocated even if errors should occur. There won't probably be many allocations/deallocations for MutexData either so it won't be more expensive to use smart pointers. Removal of the pointer would be best ofcourse  :wink:

Gillius

  • Administrator
  • User
  • *****
  • Posts: 147
    • http://www.gillius.org/
memory leak
« Reply #3 on: April 12, 2005, 09:08:38 PM »
There will be as many as mutexes, but you are right, it's not a time critical section of code.  You are right about scoped_ptr, but at this time, the full boost is not a dependency for GNE.  I am seriously considering making it though, and not distributing the minimal boost, since boost has become more popular and bandwidth has been increasing.  I'd still like to stay as far away as possible from the boost libraries that require complication because its build process is still extreme.  It left me with 300 or 400 megabytes of compiled libraries and took an hour on my AMD 64 machine with 10krpm raptor drive and 1gb of RAM.

Now like I said I have a very strong feeling that I made MutexData that way so I didn't have to force an include of windows.h or Unix headers into the project.
Gillius
Gillius's Programming http://www.gillius.org/

Gillius

  • Administrator
  • User
  • *****
  • Posts: 147
    • http://www.gillius.org/
memory leak
« Reply #4 on: April 12, 2005, 11:46:11 PM »
OK the fix is now in CVS.  How did you find the leak, BTW?  If you were using some sort of automated tool, it would be nice to know if you found other leaks, or espically if you didn't find any.
Gillius
Gillius's Programming http://www.gillius.org/

muumi

  • User
  • *
  • Posts: 9
memory leak
« Reply #5 on: April 13, 2005, 06:56:50 AM »
I'm writing a little test application using Ogre ( http://www.ogre3d.org/ ) and GNE. Ogre has a built-in leak-detector ( made by Paul Nettle, see http://www.fluidstudios.com/ ). First, I had problems getting my application running. It always seemed to crash in some strange place even if I had just an empty main() with gnelib.h included. I noticed that the errors occured because I was linking to Ogre's debug lib and using GNE at the same time. I had to compile GNE using Ogre so that the allocations in GNE would be correctly handled by Ogre's memory manager (I didn't want to disable the manager).  So I made a configuration file that is included in every GNE header and included Ogre headers in it.  After recompile everything worked fine (almost). Leaks were detected and no crashes happened. Unfortunately, Ogre's memory manager doesn't seem to catch boost's smart pointers correctly and they are reported to leak memory. So there is no way for me to actually verify if GNE has leaks. I checked through all the reported 'leaks' and they were all smart pointers so I guess everything should be fine  :)  I'll try to get the memory manager to work with boost as soon as I find the time.

Gillius

  • Administrator
  • User
  • *****
  • Posts: 147
    • http://www.gillius.org/
memory leak
« Reply #6 on: April 13, 2005, 08:43:21 AM »
Is the issue with smart pointers something known with the OGRE memory manager?  When I converted GNE to use smart pointers, I noticed that a nieve application would almost certainly cause circular references between objects (which would cause the objects to never get collected).  I attempted to try to eliminate this possiblity by constructing a reference graph as the class level (so that if A can possibly refer to an object of type B, I forbid B to even contain a member that might refer to an object of type A).  However, it it is quite possible that my efforts were not correct or complete and circular references do exist (although I feel pretty certain the code is correct in this respect).  If you feel that this could possibly be why you see "memory leaks" with the boost pointers, let me know.
Gillius
Gillius's Programming http://www.gillius.org/

muumi

  • User
  • *
  • Posts: 9
memory leak
« Reply #7 on: April 13, 2005, 12:01:51 PM »
Oops  :oops:  A few days ago I read a thread somewhere about boost and Ogre's memory manager compatibility problems. I assumed that the information there was correct and never even bothered to check myself.  After reading your post I decided to write a little test just in case and noticed that it actually reports boost's smart pointers correctly!

Code: [Select]

#include <iostream>
#include <boost/shared_ptr.hpp>
#include <Ogre.h>

class TestClass
{
public:
TestClass() { std::cout << "TestClass:TestClass()" << std::endl; }
~TestClass() { std::cout << "TestClass:~TestClass()" << std::endl; }
};

int main( int argc, char* argv[] )
{
boost::shared_ptr<TestClass> pPointer1( new TestClass() );
boost::shared_ptr<TestClass> pPointer2( new TestClass() );

pPointer2 = pPointer1;

return 0;
}


No leaks are reported and two test classes are created and destroyed. So the leaks must be real then :( I ran exhello to check that it wasn't something that I was doing wrong that made the leaks. No leaks are reported when exhello acts as a server. When it act as a client on the other hand several leaks are reported:

Code: [Select]

 ----------------------------------------------------------------------------------------------------------------------------------
|                                          Memory leak report for:  04/13/2005 18:54:33                                            |
 ----------------------------------------------------------------------------------------------------------------------------------


18 memory leaks found:

Alloc.   Addr       Size       Addr       Size                        BreakOn BreakOn              
Number Reported   Reported    Actual     Actual     Unused    Method  Dealloc Realloc Allocated by
------ ---------- ---------- ---------- ---------- ---------- -------- ------- ------- ---------------------------------------------------
000030 0x00C8A798 0x00000004 0x00C8A788 0x00000024 0x00000000 new         N       N    exhello.cpp(63) OurClient::create
000031 0x00C8A7E8 0x00000018 0x00C8A7D8 0x00000038 0x00000000 new         N       N    shared_count.hpp(340) boost::detail::shared_count::shared_cou
000032 0x00C8A850 0x00000088 0x00C8A840 0x000000A8 0x00000000 new         N       N    clientconnection.cpp(52) GNE::ClientConnection::create
000033 0x00C8A928 0x00000038 0x00C8A918 0x00000058 0x00000000 new         N       N    mutex.cpp(43) GNE::Mutex::Mutex
000034 0x00C8A9B0 0x00000038 0x00C8A9A0 0x00000058 0x00000000 new         N       N    mutex.cpp(43) GNE::Mutex::Mutex
000035 0x00C8AA38 0x00000008 0x00C8AA28 0x00000028 0x00000000 new         N       N    thread.cpp(123) GNE::Thread::Thread
000036 0x00C8BAC0 0x00000018 0x00C8BAB0 0x00000038 0x00000000 new         N       N    shared_count.hpp(340) boost::detail::shared_count::shared_cou
000037 0x00C8BB28 0x000000B0 0x00C8BB18 0x000000D0 0x00000000 new         N       N    eventthread.cpp(42) GNE::EventThread::create
000038 0x00C8BC28 0x00000038 0x00C8BC18 0x00000058 0x00000000 new         N       N    mutex.cpp(43) GNE::Mutex::Mutex
000039 0x00C8BCB0 0x00000008 0x00C8BCA0 0x00000028 0x00000000 new         N       N    thread.cpp(123) GNE::Thread::Thread
000040 0x00C8BD08 0x00000018 0x00C8BCF8 0x00000038 0x00000000 new         N       N    conditionvariablewin32.inc(48) GNE::ConditionVariable::ConditionVariab
000041 0x00C8BD70 0x00000008 0x00C8BD60 0x00000028 0x00000000 new         N       N    conditionvariablewin32.inc(51) GNE::ConditionVariable::ConditionVariab
000042 0x00C8BDC8 0x00000038 0x00C8BDB8 0x00000058 0x00000000 new         N       N    mutex.cpp(43) GNE::Mutex::Mutex
000043 0x00C8BE50 0x00000018 0x00C8BE40 0x00000038 0x00000000 new         N       N    conditionvariablewin32.inc(48) GNE::ConditionVariable::ConditionVariab
000044 0x00C8BEB8 0x00000008 0x00C8BEA8 0x00000028 0x00000000 new         N       N    conditionvariablewin32.inc(51) GNE::ConditionVariable::ConditionVariab
000045 0x00C8BF10 0x00000038 0x00C8BF00 0x00000058 0x00000000 new         N       N    mutex.cpp(43) GNE::Mutex::Mutex
000046 0x00C8BF98 0x00000038 0x00C8BF88 0x00000058 0x00000000 new         N       N    mutex.cpp(43) GNE::Mutex::Mutex
000047 0x00C8C020 0x00000018 0x00C8C010 0x00000038 0x00000000 new         N       N    shared_count.hpp(340) boost::detail::shared_count::shared_cou


I debugged the code to make sure that the destructors weren't called. All traces seem to lead to ClientConnection. At the end of exhello execution ClientConnection's use count was still 2 and weak count was 3. I'll try to find the cause of this sometime tomorrow.

Gillius

  • Administrator
  • User
  • *****
  • Posts: 147
    • http://www.gillius.org/
memory leak
« Reply #8 on: April 13, 2005, 12:18:50 PM »
You can enable at the highest level of the GNE debugging prints that will show object creation and destruction, if that helps you out.  I believe they are DLEVEL5.
Gillius
Gillius's Programming http://www.gillius.org/

muumi

  • User
  • *
  • Posts: 9
memory leak
« Reply #9 on: April 13, 2005, 06:08:30 PM »
Okay. Now all the leaks should be fixed  :)  The problem was that I wasn't actually connecting to a server so as the connection failed ClientConnection was never properly disconnected. I added a disconnect() to ClientConnection.cpp line around line 190 when the failed connections are handled. Not sure if this is an appropriate place but you probably know a better place if it isn't fine to disconnect there. Maybe even only portions of disconnect need to be added because the client has never actually established a connection with the server. As soon as I have time I'll run leak-checks on all the examples provided with GNE since they probably use different components of it.

Gillius

  • Administrator
  • User
  • *****
  • Posts: 147
    • http://www.gillius.org/
memory leak
« Reply #10 on: April 13, 2005, 09:21:50 PM »
If by not connect you mean there was an error that caused it to terminate early and not clean up, what do you think about my post that I made in a new thread about exceptional programming with GNE?  I was thinking of changing the API to that -- if you combine exceptions with RAII you can get a good cleanup even if a connect or other initial operation failed.
Gillius
Gillius's Programming http://www.gillius.org/

muumi

  • User
  • *
  • Posts: 9
memory leak
« Reply #11 on: April 17, 2005, 04:40:01 PM »
I was connecting to localhost but had no server running when I connected so I got an connection refused error. This didn't (and shouldn't either) terminate the application but caused a memory leak because disconnect() was never called for ClientConnection.

Gillius

  • Administrator
  • User
  • *****
  • Posts: 147
    • http://www.gillius.org/
memory leak
« Reply #12 on: April 17, 2005, 10:46:40 PM »
I'll take a look into this.  If the connect failed, there probably shouldn't need to be any additional cleanup tasks by the user.
Gillius
Gillius's Programming http://www.gillius.org/