Memory leak?

145 views
Skip to first unread message

Adam

unread,
Mar 4, 2009, 6:50:11 AM3/4/09
to Protocol Buffers
Hi,

I'm using protocol buffers in my project (C++), and while I was
hunting for memory leaks, I discovered that in the generated code,
static members are being allocated, but there's no way to free them.
It's not a major issue, because the leak only occur when you exit the
application (and the static members are not being free as they
should), but I would like to know if you are aware of this issue, and
is there a workaround or any other solution?

Thanks,

Adam Oren

Ivan Kharin

unread,
Mar 4, 2009, 7:08:10 AM3/4/09
to Protocol Buffers
> I'm using protocol buffers in my project (C++), and while I was
> hunting for memory leaks, I discovered that in the generated code,
> static members are being allocated, but there's no way to free them.

yes.

> It's not a major issue, because the leak only occur when you exit the
> application (and the static members are not being free as they
> should), but I would like to know if you are aware of this issue, and
> is there a workaround or any other solution?

it's major issue if PB used in shared library with load/call/unload usage pattern :(


--
Ivan Kharin

lahi...@gmail.com

unread,
Mar 4, 2009, 8:39:52 AM3/4/09
to Protocol Buffers
Perhaps you could use protobuf-c which uses global variables for its
metadata and hence has no leaks upon unload (as long as you free all
your messages of course).

- dave

Kenton Varda

unread,
Mar 4, 2009, 3:06:52 PM3/4/09
to Adam, Protocol Buffers

marc

unread,
Mar 6, 2009, 8:50:44 AM3/6/09
to Protocol Buffers
Isn't this a common scenario in C++ for which stdlib provides a simple
solution, std::auto_ptr? std::auto_ptr is a lightweight class designed
to "RAIIify" pointers. What am I missing?

On Mar 4, 3:06 pm, Kenton Varda <ken...@google.com> wrote:
> http://code.google.com/p/protobuf/issues/detail?id=54
>

Kenton Varda

unread,
Mar 6, 2009, 12:52:07 PM3/6/09
to marc, Protocol Buffers
On Fri, Mar 6, 2009 at 5:50 AM, marc <vail...@cis.jhu.edu> wrote:
Isn't this a common scenario in C++ for which stdlib provides a simple
solution, std::auto_ptr? std::auto_ptr is a lightweight class designed
to "RAIIify" pointers.  What am I missing?

Deleting the objects on shutdown is easy.  The problem is that doing so while another thread is still using them may crash the program.  If your program is careful to shut down all background threads before exiting, then it's fine, but many people write code which isn't so clean, and some of these people insist that libraries should not delete their objects at exit for exactly this reason.

Marc Vaillant

unread,
Mar 6, 2009, 2:13:23 PM3/6/09
to Kenton Varda, marc, Protocol Buffers

I'm referring to having a static auto_ptr variable. I'm not familiar
with how this can manifest the danger you describe. Can you point it
out in the following example?


#include <pthread.h>
#include <memory>
#include <iostream>


class B
{
public:
~B()
{
std::cout<<"destructor called"<<std::endl;
}

int alive()
{
return 1;
}

};

class A
{
public:
static std::auto_ptr<B> b;
};

std::auto_ptr<B> A::b(new B);

void *threadFunc(void* p)
{
while(1)
{
std::cout<<A::b->alive()<<" ";
}
}

int main()
{
pthread_t t;
int rc;
int p;
rc = pthread_create(&t, NULL, threadFunc, (void *)p);
sleep(1);

return 1;
}

Kenton Varda

unread,
Mar 6, 2009, 2:44:58 PM3/6/09
to Marc Vaillant, Protocol Buffers
The only reason you aren't seeing a problem with that code is because it exits very quickly after ~B() is called, before the other thread gets a chance to be scheduled again.  In a large program, shutdown might not be so fast.  Try putting a sleep(1) at the end of B::~B() -- you'll see that the destructor is called with the background thread still running.

Marc Vaillant

unread,
Mar 6, 2009, 9:41:20 PM3/6/09
to Kenton Varda, Marc Vaillant, Protocol Buffers
Ok, thanks. There are other problems though. If A::b was a B* instead
of a std::auto_ptr<B>, then the data that A::b points to will be ok
because it won't be deleted. However, A::b would get deleted so access
to the data via A::b may be undefined.

Also I found that static vars local to the thread function get deleted
before the thread gets killed.

Static is dangerous in multithreaded scenarios for obvious reasons.
These issues just add more complications.

Marc

Kenton Varda

unread,
Mar 6, 2009, 9:52:05 PM3/6/09
to Marc Vaillant, Protocol Buffers
On Fri, Mar 6, 2009 at 6:41 PM, Marc Vaillant <ma...@jhu.edu> wrote:
Ok, thanks.  There are other problems though.  If A::b was a B* instead
of a std::auto_ptr<B>, then the data that A::b points to will be ok
because it won't be deleted.  However, A::b would get deleted so access
to the data via A::b may be undefined.

If your global variables (and static members / static locals) are only POD types then there's no problem.  The memory in which they reside will not be unmapped or zeroed out until the process actually ends.
 
Also I found that static vars local to the thread function get deleted
before the thread gets killed.

Yep, same problem.
 
Static is dangerous in multithreaded scenarios for obvious reasons.
These issues just add more complications.

Static constants are not dangerous.  All of the data structures involved here are built once and then become read-only for the life of the process.  Static variables which are modified at runtime -- aka singletons -- are very bad and should never be used.

Marc Vaillant

unread,
Mar 9, 2009, 9:12:58 AM3/9/09
to Kenton Varda, Marc Vaillant, Protocol Buffers

Except that you still have the shared lib load/unload problem :)

Marc

Kenton Varda

unread,
Mar 9, 2009, 2:48:36 PM3/9/09
to Marc Vaillant, Protocol Buffers
On Mon, Mar 9, 2009 at 6:12 AM, Marc Vaillant <ma...@jhu.edu> wrote:
Except that you still have the shared lib load/unload problem :)

Yeah, well, I agree with you.  I wish people wouldn't write programs that attempt to exit while background threads are still running, but a lot of people at Google (where we don't use dynamic library loading much) seem to have other ideas.  :/

I'm going to have to make this some sort of option.
Reply all
Reply to author
Forward
0 new messages