[PATCH] Fix mostly harmless but still annoying memory leaks

[olafvdspek]

[PATCH] Fix mostly harmless but still annoying memory leaks

[GoogleCodeExporter]

Philosophically, I'm opposed to changing the program because of deficiencies in 
tools
people use on it.  With google-perftools, it's possible to set the "strictness" 
of
the memory checking, and at the default strictness level, code accessible from 
global
pointers is considered live, even at program-exit.  I believe that's the most 
useful
mode; it would be nice if MSVC could support that too.

That said, if we can change this in ways that don't contort the code, I'm fine 
with
that.  Your solution, unfortunately, isn't safe: what if the main thread exits,
causing GlobalsCleanup to run, but other threads are still running, and they 
try to
access template_string_set or arena?  By far the cleanest way to handle this 
memory
is to let the O/S clean it up on program exit, as we do now.

The only other solution I can see is to make template_string_set and arena not 
be
pointers.  But then we get into another problem, with crashes caused by code run
before main: function-call assignments to global variables, and global 
constructors.
 Again, our current solution is the cleanest way to solve it.

For the Template class, we actually defined a method ClearCache() for this same 
case.
 However, that's safe to call always, since it's just a cache, so I didn't feel bad
adding it to the API.  However, a similar function call here would actually 
cause
crashes if it were called too soon (for instance, at the end of main, where it 
will
be called by the main thread while other threads are possibly still running).

I'll leave this bug open for now, but unless someone can come up with a clean 
and
safe way to implement this, I'll probably close it WontFix.

Original comment by [email protected] on 11 Aug 2009 at 7:18

• Added labels: Type-Defect, Priority-Low

[GoogleCodeExporter]

I probably won't change your mind but a "deficiency" such as this is so 
extremely
common that you can't even really call it like this, almost all memory checking 
tools
would report this as a leak. Besides, they have a point: it would be a leak if 
the
library could be initialized/shut down multiple times, it's only the fact that 
this
is not supported currently that prevents it from being a real leak.

Finally, IME letting the other threads run after the main thread main() exits 
is a
sure and certain recipe for disaster. At the very best, these threads will be 
killed
instead of being nicely joined. But usually much worse problems will result.

As for accessing these objects from threads created before main() starts, your
current code doesn't support it anyhow, of course, as the mutex may not even be
initialized yet when TemplateString::AddToGlobalIdToNameMap() is called 
resulting in
an instant crash. If you really want to support this you must replace all global
variables with statics wrapped in functions which will ensure that they are
initialized before they are used (and would probably be enough to fix the leak 
too).
But IMNSHO supporting this use case is futile anyhow, threads should not be 
created
before main() starts and must be terminated before it exits.

Original comment by [email protected] on 11 Aug 2009 at 7:27

[GoogleCodeExporter]

} almost all memory checking tools would report this as a leak.

I don't know if that is true or not, but I would argue they shouldn't.  In any 
case,
the only memory checking tool I have intimate knowledge of, does not.

} it would be a leak if the library could be initialized/shut down multiple 
times

A fair point.  I guess my attitude is more unix-centric and yours more
windows-centric, where it's plausible to unload and reload dll's all the time.

} Finally, IME letting the other threads run after the main thread main() exits 
is
} a sure and certain recipe for disaster.

I don't think that's true.  It seems very plausible to me that you have worker
threads writing to disk, say, and it takes them a while to finish committing 
some
data to disk after the rest of the program has exited.  I don't see that being a
disaster.

} As for accessing these objects from threads created before main() starts, your
} current code doesn't support it anyhow, of course

You're right we don't support threads created before main() starts, but that's 
not
the problem situation I was describing.  I was describing normal global 
constructors
and the like.  And we designed the code carefully to support that operation, 
which
indeed we do correctly.

} as the mutex may not even be
} initialized yet when TemplateString::AddToGlobalIdToNameMap() is called 
resulting
} in an instant crash.

One could imagine writing code where that is true, but ctemplate is not such 
code.

} If you really want to support this you must replace all global
} variables with statics wrapped in functions

Or you can make all global variables pointers, which is what we do.

} I probably won't change your mind

Well, I do have to say your arguments would be more convincing if they showed 
more
familiarity with what the current code does.

Original comment by [email protected] on 11 Aug 2009 at 7:58

[GoogleCodeExporter]

Again, I hardly see you changing your mind in a pinch but I can only say that I
remain convinced that letting the system kill the threads is a horrible idea 
and your
example is a perfect illustration of why it is so: if you let the writer 
threads live
beyond main() they are going to be killed before they can finish writing and 
your
data will be lost. The only way to ensure that this does not happen (and also 
handle
possible errors which might happen during writing, notify the user about them 
and do
just about anything else reasonable) is to make main() wait for them. I really 
don't
see how can you justify doing anything like this.


As for being familiar with the current code, I was replying to

> The only other solution I can see is to make template_string_set and arena 
not be
pointers.

and not speaking about the current code in that part, sorry if this was 
unclear. To
repeat, this would work for the threads running after main() (as you yourself 
wrote)
and the fact that it doesn't work for threads created before main() shouldn't 
matter
if you don't support it anyhow. So, to bring this discussion to a more
precise/constructive topic: what exactly is wrong with using objects instead of 
pointers?

Original comment by [email protected] on 11 Aug 2009 at 9:15

[GoogleCodeExporter]

} I really don't see how can you justify doing anything like this.

I'm not saying I do this, only that it's done.  (Typically, it's not in threads 
the
user controls, but ones created by a library.)  I feel ctemplate needs to 
handle that
case robustly.

But you're right, using objects is inherently unsafe, even if we can deal with 
the
construction issues, because they might be destroyed while other threads are 
still
trying to access them.  So that suggestion was a blind alley.  The only other
solution I can think of is to make them objects and manually construct them (via
placement-new) so the destructor is never called.  But that will probably cause 
MSVC
to think it's a memory leak just as much as the pointer does.

Original comment by [email protected] on 11 Aug 2009 at 9:33

[GoogleCodeExporter]

Using placement new won't result in any memory leak reports for the memory of 
these
objects themselves but still will report as leaked any memory they allocate
themselves (and at least unordered_set does allocate some).

But I still struggle to understand what are you trying to achieve exactly. As 
the
mutex used to protect these pointers is an object itself and as it must be 
acquired
before accessing them what do you gain by making just these objects into 
pointers? If
they're used after static cleanup is done, the mutex is already invalid and 
trying to
access it would result in a crash anyhow.

So again what is the goal here? I.e. do you have any real(istic) examples of 
the code
which works now and which would be broken by replacing TemplateStringSet and
UnsafeArena pointers with objects?

Original comment by [email protected] on 11 Aug 2009 at 9:46

[GoogleCodeExporter]

} As the
} mutex used to protect these pointers is an object itself and as it must be 
acquired
} before accessing them what do you gain by making just these objects into 
pointers?

The Mutex class is carefully designed to work before main is called.  (There 
may be a
few other classes we do this with too.)  It's difficult and annoying to get it 
right
-- not even possible always, and even the Mutex implementation makes a few
assumptions that are fine in practice for mutexes but maybe not for every class 
(for
instance, we assume that you don't lock a mutex in one .cc file and unlock it in
another).  The general pattern is it's safe to use a Mutex object, but 
everything
else has to be a pointer.

} If they're used after static cleanup is done, the mutex is already invalid

Hmm, that's a good point.  That's a bug. :-(  I'll have to think about the best 
fix.

} So again what is the goal here? I.e. do you have any real(istic) examples of 
the
} code which works now and which would be broken by replacing TemplateStringSet 
and
} UnsafeArena pointers with objects?

Sure thing.  Consider file a.cc:
   static const StaticTemplateString kMyVar = STS_INIT(kMyVar, "MY_VALUE");

This calls the StaticTemplateStringInitializer constructor before main() starts,
which calls AddToGlobalIdToNameMap().  If we're unlucky, a.cc's global 
constructors
will run before template_string.cc's global constructors run.  This means that
TemplateStringSet and UnsafeArena will be garbage (all 0's, in practice), and 
trying
to do any operation on them will crash.

The way the code is now, the TemplateStringSet and UnsafeArena have a 
well-defined
value: NULL.  We can detect that in AddToGlobalIdToNameMap, and handle it 
correctly.

Original comment by [email protected] on 11 Aug 2009 at 10:36

[GoogleCodeExporter]

Err, ok, this has nothing to do with threads and can be easily solved while 
still
using objects and not pointers and the solution is the well-known "wrap them in 
a
function" idiom that I mentioned above. I.e. instead of accessing the object foo
directly you should use GetFoo() which will be defined like this:

namespace {

Foo& GetFoo() {
  static Foo s_foo;
  return s_foo;
}

}

This ensures that s_foo is initialized before it is used. This is simple, 
totally
safe if you don't speak about crazy usage scenarios with threads running 
before/after
main() and there is no need to use pointers at all...

Original comment by [email protected] on 11 Aug 2009 at 11:12

[GoogleCodeExporter]

Yes, I know it has nothing to do with threads.  But you asked.

} you should use GetFoo() which will be defined like this

This is unsafe on windows, and older versions of gcc, where the function-local 
static
is not guaranteed to be initialized in a threadsafe manner.

This is a hard problem.  I'm aware of everything you've suggested so far, and 
they
all have one problem or another.  The solution we have now is the only portable,
reliable solution I know of.  We're unlikely to change it.

Original comment by [email protected] on 12 Aug 2009 at 6:53

[GoogleCodeExporter]

Sorry but it's impossible to discuss if you keep changing your position. You 
gave an
example of code which would be broken by changing the objects to pointers which 
did
_not_ involve threads. I gave a solution which is not MT-safe but which does 
solve
this use case. If you need a MT-safe solution please give another example of 
code 

a) works now
b) wouldn't work after this change

and uses threads.

And I won't even mention (again) that you do not have a "a portable, reliable
solution now" (you agreed that there was a bug in the comment 7 yourself) nor 
that
you explicitly said that you didn't want to support the case of creating threads
before main() (which is the only problem with GetFoo() approach as if you call 
it
from the main thread first, without creating any other ones, there is no 
problem) and
now you're worried about breaking it.


Anyhow, you seem to be seriously intent on making this issue much more 
complicated
than it is. I won't bother you any more, it's not a problem for me to keep this 
patch
in my local version, I only submitted it here as a courtesy. But as a last 
advice I
think it would be really useful for you to finally think of a real(istic) 
scenario
that wouldn't work with the objects that I asked for because solving the problem
without defining it first is just not a good idea.

Original comment by [email protected] on 13 Aug 2009 at 10:24

[GoogleCodeExporter]

} Sorry but it's impossible to discuss if you keep changing your position

I thought you might say this, and it indicates to me we're reaching the end of 
useful
conversation on this thread.  I will say that the issue here is we have multiple
requirements: thread-safety and global-constructor-safety.  (There is a third
requirement, threads-after-main-exit-safety, which as you point out none of our
solutions has yet; I think that shouldn't be hard to fix in the current code, 
but
luckily this is by far the least likely scenario to occur in actual code.)  You 
keep
proposing solutions that address one but violate the other.  Pointing this out 
is not
changing my position.

} which is the only problem with GetFoo() approach as if you call it
} from the main thread first

There's no guarantee users will do that.  As should be clear, some users will 
use
this functionality before main starts.  Others will not, and might not use it 
at all
until they have spawned multiple threads, and have used it from other threads.  
We
need to be safe in both cases.

} Anyhow, you seem to be seriously intent on making this issue much more 
complicated
} than it is.

Thank you for your input.

Original comment by [email protected] on 13 Aug 2009 at 3:42

• Changed state: WontFix