[osg-users] Using the notification API with multi-threading - heap corruption errors

[Matthias Schütze]

Hi,

I am actually working on a complex project which uses OSG 3.0.1 on a
Windows 7 Professional x64 SP1 platform with Visual Studio 2010
Premium SP1. Because I encountered random heap corruption errors, I
was testing and debugging with different configurations of my program
and could state the following:
- the invalid heap occurs mostly around one of the notification strings
- the problem occurs with all threading models except SingleThreaded
- the problem occurs still with an "empty" osg::NotifyHandler subclass
(which originally implements some custom logging mechanism)
- the problem occurs still without a custom osg::NotifyHandler
subclass, i.e. when using osg::StandardNotifyHandler by default

After all, I found the following forum thread which describes very
similar experiences:
http://forum.openscenegraph.org/viewtopic.php?t=9475 According to this
forum thread, I designed my custom osg::NotifyHandler subclass
thread-safe. Also, I checked out not to mix up incompatible binaries.
My actual working project is not portable. But in order to track the
problem on a non-Windows platform, I created another simple program
which is inspired by the forum thread mentioned above. Here it is:

#include <ctime>
#include <cstdlib>
#include <fstream>
#include <OpenThreads/Thread>
#include <osg/Notify>

// empty logging handler
class LogFileHandler : public osg::NotifyHandler
{
public:
LogFileHandler(const char *filename) { }
void notify(osg::NotifySeverity severity, const char *message) { }

protected:
~LogFileHandler() { }
};

// simple worker thread
class MyThread : public OpenThreads::Thread
{
public:
MyThread(int id)
: m_id(id)
{
OSG_INFO << "CREATE THREAD " << m_id << std::endl;
}

protected:
virtual void run()
{
int lines = 100000; // number of notifications generated by this thread
for (int i = 0; i < lines; ++i)
{
// generate a random notification, e.g. "HELLO FROM THREAD 2:
aaabbbcccccdde[...]zz..."
OSG_INFO << "HELLO FROM THREAD " << m_id << ": ";
int letters = 26;
for (int j = 0; j < letters; ++j)
{
char c = 'a' + j;
switch (std::rand() % 5 + 1)
{
case 1: OSG_INFO << c; break;
case 2: OSG_INFO << c << c; break;
case 3: OSG_INFO << c << c << c; break;
case 4: OSG_INFO << c << c << c << c; break;
case 5: OSG_INFO << c << c << c << c << c; break;
}
}
OSG_INFO << "..." << std::endl;
}
}

private:
int m_id;
};

// simple program
int main(int argc, char **argv)
{
srand(time(NULL));
osg::setNotifyLevel(osg::DEBUG_FP);
osg::setNotifyHandler(new LogFileHandler("log.txt")); // comment out
this line: stdout will contain mixed up strings

const int count = 2; // number of threads accessing the notification
API concurrently
MyThread *threads[count] = { };
for (int i = 0; i < count; ++i)
{
threads[i] = new MyThread(i);
threads[i]->start();
}
for (int i = 0; i < count; ++i)
{
while (threads[i]->isRunning()) { /* active wait until the worker
thread ends */ }
delete threads[i];
threads[i] = NULL;
}

return EXIT_SUCCESS;
}

The program essentially imitates a multi-threaded environment for the
notification API by creating some worker threads. Every thread
generates a bulk of random notifications and pushes them per OSG_INFO.
I think, one can compare this scenario with the case where a verbose
notify level is used in combination with a non-SingleThreaded
threading model, or am I wrong?
In my Win 7 x64 VS 2010 environment the result is the following: I got
frequent heap corruption errors. The problem can be reproduced more or
less depending on the number of concurrently running threads and if it
is a debug or release build. If I comment out the use of the custom
osg::NotifyHandler subclass (which defaults to
osg::StandardNotifyHandler), I recognized mixed up strings and missing
endl's in stdout.
Furthermore, I tested this program with Lubuntu 12.0.4 x64 and the
QtCreator from the QtSDK 4.8.1. At least in debug mode, I got frequent
heap corruption errors. If I comment out the use of the custom
osg::NotifyHandler subclass, I recognized mixed up strings (debug) and
missing endl's (debug and release) in stdout as well.

Concluding, for my actual working project I will run SingleThreaded to
avoid heap corruption as it fits my needs at the moment. But I cannot
overcome some questions:
- Do I miss some well-known restrictions, when using the notification
API with multi-threading?
- IMHO the heap corruption is caused by the non-thread-safe access to
the global static instance of osg::NotifyStream defined in Notify.cpp.
All over OSG, it is accessed by a reference to std::ostream which is
returned by the osg::notify() function in order to call operator << on
it. Can someone check these facts and explain to me, why there is no
locking mechanism or the like to make the stream thread-safe?

I appreciate every enlightenment or correction!

Matthias Schütze, Germany
_______________________________________________
osg-users mailing list
osg-...@lists.openscenegraph.org
http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org

[Robert Osfield]

Hi Matthias,

osg::notify() is written on the assumption that stream are thread
safe, avoiding adding extra mutexes is desirable as the performance
would really suffer if we had to add them to all osg::notify() calls.

I can see how that osg::initNotifyLevel() method might be problem if
multiple threads all call osg::notify() in parallel. To avoid this
perhaps a local proxy object to force initialization would be
sufficient so that it initialized prior to any calls to osg::notify().
To see if this works I've added the following to Notify.cpp:

struct InitNotifyProxy
{
InitNotifyProxy()
{
osg::initNotifyLevel();
}
};

static InitNotifyProxy s_initNotifyProxy;

The modified file is attached, could you try this out?

Robert.

[Matthias Schütze]

Hi Robert,

Thanks for your fast reply and revised code!

> osg::notify() is written on the assumption that stream are thread
> safe, avoiding adding extra mutexes is desirable as the performance
> would really suffer if we had to add them to all osg::notify() calls.

I agree with the argument not to add extra mutexes. But what do you
mean with "assumption that stream are thread safe"? Obviously neither
the stream implementation of the Visual compiler under Windows nor the
one of the GNU compiler under Lubuntu are thread-safe by nature. Do
you know any such implementations or platforms or maybe compiler flags
which fulfill this assumption?

> I can see how that osg::initNotifyLevel() method might be problem if
> multiple threads all call osg::notify() in parallel. To avoid this
> perhaps a local proxy object to force initialization would be
> sufficient so that it initialized prior to any calls to osg::notify().

I think, the proxy object to force early initialization is an
important improvement to protect the init phase of the global static
instance of osg::NotifyStream. But it cannot solve the whole problem,
if the stream itself is not thread-safe when used with operator <<
from parallel threads. I quickly tested your code change with my
sample program on my Windows machine: The heap corruption still
occurs, indeed some time after the init phase.

Beyond mutexes, I am less skilled with thread synchronisation. But in
other libraries like Qt sometimes the concept of thread local storage
is used to separate resource access from parallel threads. Do you
think this concept would be applicable to provide every thread with
its own stream? Would this concept be more efficient compared to
mutexes? I could imagine that every thread writes to an own stream
which then calls the *same* osg::NotifyHandler subclass. This way, the
notify handler would be responsible for thread-safeness again (as it
is now).

Cheers,

[Filip Arlet]

Hi,

"Beyond mutexes, I am less skilled with thread synchronisation. But in
other libraries like Qt sometimes the concept of thread local storage
is used to separate resource access from parallel threads. Do you
think this concept would be applicable to provide every thread with
its own stream? Would this concept be more efficient compared to
mutexes? I could imagine that every thread writes to an own stream
which then calls the *same* osg::NotifyHandler subclass. This way, the
notify handler would be responsible for thread-safeness again (as it
is now)."

It's always problem of standard library. Creating output streams for every thread is overkill and not safe either. What if internal implementation of new streams will call some static internal write() function that won't be threadsafe ?

"I agree with the argument not to add extra mutexes. But what do you
mean with "assumption that stream are thread safe"? Obviously neither
the stream implementation of the Visual compiler under Windows nor the
one of the GNU compiler under Lubuntu are thread-safe by nature. Do
you know any such implementations or platforms or maybe compiler flags
which fulfill this assumption?"

I agree with that too. Solution using custom osg::NotifyHandler with mutexes is easy and it dont have to be in osg core library, what about using NotifyHandler with mutexes if OSG_NOTIFY_LEVEL is set ?
Ok the problem will always persist, if someone use std::out directly, but that is not OSG problem ....
Cheers,
Filip

------------------
Read this topic online here:
http://forum.openscenegraph.org/viewtopic.php?p=48136#48136

[Matthias Schütze]

Hi,

> > Beyond mutexes, I am less skilled with thread synchronisation. But in
> > other libraries like Qt sometimes the concept of thread local storage
> > is used to separate resource access from parallel threads. Do you
> > think this concept would be applicable to provide every thread with
> > its own stream? Would this concept be more efficient compared to
> > mutexes? I could imagine that every thread writes to an own stream
> > which then calls the *same* osg::NotifyHandler subclass. This way, the
> > notify handler would be responsible for thread-safeness again (as it
> > is now).
>

> It's always problem of standard library. Creating output streams for every thread is overkill and not safe either. What if internal implementation of new streams will call some static internal write() function that won't be threadsafe ?

Thanks for your hints again! It is a good point that under certain
circumstances thread local streams would lack thread-safeness, too.

> > I agree with the argument not to add extra mutexes. But what do you
> > mean with "assumption that stream are thread safe"? Obviously neither
> > the stream implementation of the Visual compiler under Windows nor the
> > one of the GNU compiler under Lubuntu are thread-safe by nature. Do
> > you know any such implementations or platforms or maybe compiler flags
> > which fulfill this assumption?
>

> I agree with that too. Solution using custom osg::NotifyHandler with mutexes is easy and it dont have to be in osg core library, what about using NotifyHandler with mutexes if OSG_NOTIFY_LEVEL is set ?
> Ok the problem will always persist, if someone use std::out directly, but that is not OSG problem ....

Maybe, I missunderstand your argument. I'm sorry, if so. To clarify,
IMHO custom osg::NotifyHandler with mutexes seems to be *no* problem
at all. But as I noted earlier in this thread, heap corruption errors
are very likely a consequence of a race condition for the global
static instance of osg::NotifyStream (std::ostream). Those errors can
occur with every kind of notify handler, even empty ones and
thread-safe ones as well as osg::StandardNotifyHandler.
Because non-thread-safe implementations of std::ostream seems common
and OSG uses streams without protection, I think, the notification API
poses a wide problem, doesn't it? Theoretically, every OSG example
program executed on a multi-processor machine is affected by the race
condition by default. So, I would like to know, if this drawback is
commonly accepted in order to prefer OSG efficiency over guaranteed
thread-safeness?

I look forward to your feedback!

[Robert Osfield]

Hi Matthias,

On 14 June 2012 15:21, Matthias Schütze <matthi....@googlemail.com> wrote:
>> I agree with that too. Solution using custom osg::NotifyHandler with mutexes is easy and it dont have to be in osg core library, what about using NotifyHandler with mutexes if OSG_NOTIFY_LEVEL is set ?

Potentially we could provide a thread safe version of NotifyHandler as
part of the core OSG, just not have it enabled by default so to
prevent the performance consequences of it.

>> Ok the problem will always persist, if someone use std::out directly, but that is not OSG problem ....
>
> Maybe, I missunderstand your argument. I'm sorry, if so. To clarify,
> IMHO custom osg::NotifyHandler with mutexes seems to be *no* problem
> at all. But as I noted earlier in this thread, heap corruption errors
> are very likely a consequence of a race condition for the global
> static instance of osg::NotifyStream (std::ostream). Those errors can
> occur with every kind of notify handler, even empty ones and
> thread-safe ones as well as osg::StandardNotifyHandler.
> Because non-thread-safe implementations of std::ostream seems common
> and OSG uses streams without protection, I think, the notification API
> poses a wide problem, doesn't it? Theoretically, every OSG example
> program executed on a multi-processor machine is affected by the race
> condition by default. So, I would like to know, if this drawback is
> commonly accepted in order to prefer OSG efficiency over guaranteed
> thread-safeness?

The OSG is used in many multi-threaded applications and the
notification system as it is hasn't caused problems, is a debug system
that mostly just sits behind the scenes doing nothing. If
notification system was causing lots of problems then we'd be hearing
about it much more, and the OSG wouldn't have got to 3.0 without
tackling the issue. Given this I would expect having a thread safe
NotifyHandler is likely to be niche tool going forward.

W.r.t static initialization, this is something we need to sort out,
and as long as there isn't a performance overhead in how we tackle it
then I have no problem with it being the only code path for
initialization of the singletons that manage the notification.

Robert.

[Robert Osfield]

Hi Mathias,

On 14 June 2012 17:29, Robert Osfield <robert....@gmail.com> wrote:
> W.r.t static initialization, this is something we need to sort out,
> and as long as there isn't a performance overhead in how we tackle it
> then I have no problem with it being the only code path for
> initialization of the singletons that manage the notification.

What I have gone for with src/osg/Notify.cpp is to use a
NotifySingleton class that holds all the global notify variables and
initializes these in it's constructor, then a static
getNotifySingleton() function returns a reference to locally defined
static NotifySingleton. Finally to force the NotifySingleton to
initialize automatically rather than on first invocation of
osg::Notify() I have used a static proxy class that simple calls the
getNotifySingleton() to make sure that it's called right after loading
the library.

My hope that this combination will ensure that the threading problems
relating to the initialization of the global notify variables you've
seen will not occur. This doesn't make the noitfy() handlers
themselves thread safe, I'll defer this to your own custom
implementation.

I have checked in my changes to svn/trunk and also attached the
modified Notify.cpp. Could you please test this and let me know if
this works.

Thanks,
Robert.

[Matthias Schütze]

Re: Using the notification API with multi-threading - heap corruption errors

Hi,

> What I have gone for with src/osg/Notify.cpp is to use a NotifySingleton class that holds all the global notify variables and initializes these in it's constructor, then a static getNotifySingleton() function returns a reference to locally defined static NotifySingleton. Finally to force the NotifySingleton to initialize automatically rather than on first invocation of osg::Notify() I have used a static proxy class that simple calls the getNotifySingleton() to make sure that it's called right after loading the library.
>
> My hope that this combination will ensure that the threading problems relating to the initialization of the global notify variables you've seen will not occur. This doesn't make the noitfy() handlers themselves thread safe, I'll defer this to your own custom implementation.
>
> I have checked in my changes to svn/trunk and also attached the modified Notify.cpp. Could you please test this and let me know if this works.

Again, thanks for the revised code, Robert. I think, the modified
version handles initialization of the notification mechanism more
clearly and the proxy prevents concurrent initialization of the global
resources. Anyway, both my working project and my test program crash
with heap corruption more or less frequently. For now, I tested it on
Win 7 x64.

Based on the svn trunk version of Notify.cpp, I experimented with
another approach. The solution is prototype, however, here is what
worked for me: I used a thread local storage to separate access to
osg::NotifyStream for each thread. As discussed earlier in this
thread, this assumes stream classes (including std::ostream and
std::stringbuf) to be at least reentrant. According to
http://msdn.microsoft.com/en-us/library/c9ceah3b%28v=vs.100%29.aspx
this is true at least for my Visual compiler implementation.
Currently, there are some other drawbacks with this solution: As I
used QThreadStorage for now, it requires QtCore library to be linked
in core osg. Maybe, another TLS implementation would be more suitable
(does OpenThreads has one?). Although I could not recognize capital
performance loss yet, this may be another critical issue.
Nevertheless, I tried the modified version upon OSG 3.0.1 on all my
Win 7 x64 machines and the heap corruption never showed up again.
For further inspection and interest, I attached my modified version of
Notify.cpp which has a preprocessor flag OSG_NOTIFY_STREAM_PER_THREAD
to toggle the use of thread local storage.

Cheers,
Matthias Schütze, Germany

[Robert Osfield]

HI Matthias,

OpenThreads doesn't expose thread local storage but does use it
internally to provide the Thread::CurrentThread() functionality.
Might it be possible to use this in your implementation?

Robert.