static RunningThreadsList& getInstance()
{
static RunningThreadsList runningThreads;
return runningThreads;
}This depends on a hidden static boolean that the compiler inserts, to check to see if the variable with static duration needs to be initialized. Normally its not a problem but if you create a whole bunch of threads at once, I believe this thing can screw up. I’m working on a fix.
I have developed a StaticCriticalSection object. This object can be declared with static storage duration, and depends on it’s data members getting filled with zeroes (as per the C++ spec). Using this object it is possible to fix any code that depends on a variable with static storage duration inside a function. It can completely solve order of initialization issues, just protect any “getInstance()” type function with the mutex:
// Copyright (C) 2008-2011 by Vincent Falco, All rights reserved worldwide.
// This file is released under the MIT License:
// http://www.opensource.org/licenses/mit-license.php
//
// Wraps an object up so it can exist at file scope with static storage
// duration. The object is initialized with zeroes as per the C++ spec.
// Non-trivial constructors will silently fail with undefined behavior.
//
// Spec: N2914=09-0104
//
// [3.6.2] Initialization of non-local objects
//
// Objects with static storage duration (3.7.1) or thread storage
// duration (3.7.2) shall be zero-initialized (8.5) before any
// other initialization takes place.
//
template <class Object>
class StaticData
{
public:
Object* operator-> () { return getObject (); }
Object& operator* () { return *getObject (); }
const Object* operator-> () const { return getObject (); }
const Object& operator* () const { return *getObject (); }
Object* getObject ()
{
return reinterpret_cast <Object*> (m_storage);
}
const Object* getObject () const
{
return reinterpret_cast <Object*> (m_storage);
}
private:
char m_storage [sizeof (Object)];
};
// CriticalSection with static storage duration. Guaranteed to work correctly no matter
// what the order of initialization of constructors is, for all current and future versions of C++.
//
template <class Tag>
class StaticCriticalSection
{
public:
class ScopedLock
{
public:
inline explicit ScopedLock () throw() { StaticCriticalSection::enter(); }
inline ~ScopedLock() throw() { StaticCriticalSection::exit(); }
private:
JUCE_DECLARE_NON_COPYABLE (ScopedLock);
};
typedef ScopedLock ScopedLockType;
static void enter ()
{
if (inited->get() == 0)
{
if (initing->compareAndSetBool (1, 0))
{
new (lock.getObject()) CriticalSection;
inited->set (1);
}
else
{
do
{
Thread::yield ();
}
while (inited->get() == 0);
}
}
lock->enter ();
}
static void exit ()
{
lock->exit ();
}
private:
// Cleans up the CriticalSection on exit if it was inited
class CleanerUpper
{
public:
~CleanerUpper ()
{
if (StaticCriticalSection::inited->get() != 0)
StaticCriticalSection::lock->~CriticalSection ();
}
};
static StaticData <Atomic <int> > inited;
static StaticData <Atomic <int> > initing;
static StaticData <CriticalSection> lock;
static CleanerUpper cleanerUpper;
};
template <class Tag>
StaticData <Atomic <int> > StaticCriticalSection <Tag>::inited;
template <class Tag>
StaticData <Atomic <int> > StaticCriticalSection <Tag>::initing;
template <class Tag>
StaticData <CriticalSection> StaticCriticalSection <Tag>::lock;
template <class Tag>
typename StaticCriticalSection <Tag>::CleanerUpper StaticCriticalSection <Tag>::cleanerUpper;Usage example:
class MySingleton : DeletedAtShutdown
{
public:
static MySingleton* getInstance ()
{
StaticCriticalSection::ScopedLock lock_ (lock);
if (!instance)
instance = new MySingleton;
return instance;
}
private:
static MySingleton* instance;
static StaticCriticalSection lock;
};
MySingleton* MySingleton::instance; // initialized to zero as per C++ spec
StaticCriticalSection MySingleton::lock; // initialized to zero as per C++ specIt’s perfectly ok to use a normal CriticalSection object statically, AFAIK there’s no need for a special class…
100% sure this is not the case, unless you are using C++0x, which provides stronger thread safety assurances for initialization of objects with static storage duration.
When you write:
static RunningThreadsList& getInstance()
{
static RunningThreadsList runningThreads;
return runningThreads;
}Here’s what the compiler is outputting
static RunningThreadsList& getInstance()
{
static bool _runningThreads_ctor_called; // initialized to zero as per C++ spec
static RunningThreadsList runningThreads;
if (!_runningThreads_ctor_called)
{
new (&runningThreads) RunningThreadsList;
_runningThreads_ctor_called = true;
}
return runningThreads;
}See the problem?
gcc has command line options to control this behavior. Alas, Microsoft Visual C++ does not. But regardless, thread safety assurances are not part of the spec before C++0x.
Check it out:
http://blogs.msdn.com/b/oldnewthing/archive/2004/03/08/85901.aspx
I know this is a bit of an outlier, because this issue with the RunningThreadsList first of all would almost never show up. You have to create a bunch of threads, right on startup. But it is definitely a problem.
I only stumbled into it because I wrote a benchmark command-line app to test some multithreaded code, and created 32 juce threads on launch in a tight loop.
gcc:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=13684
-fno-threadsafe-statics
http://gcc.gnu.org/onlinedocs/gcc/C_002b_002b-Dialect-Options.html
No, you misunderstood - I didn’t mean that it’s thread-safe to use static variables, I just meant that it’s ok to use a static CriticalSection object rather than needing your own class to do that. (That’s assuming you don’t have any other static constructors that use the CriticalSection before its static initialiser has been run)
Uhh…not sure what you mean. The static CriticalSection in the function RunningThreadsList::getInstance() is definitely not thread-safe. If two threads get into getInstance() at the same time, it is vulnerable to double construction. I can give you a test case if you want, but it’s really simple. Just create 100 juce Threads in a tight loop from main(). When they get destroyed, there will be an assertion about the thread not being found in the running threads list (because one or more of them went into a different list, from the double construction).
So…RunningThreadsList::getInstance(), is not thread safe on all environments (note that gcc versions 4.0 and later fix this problem, while visual studio 2008 does not).
(That's assuming you don't have any other static constructors that use the CriticalSection before its static initialiser has been run)Well that’s exactly my problem! I have implemented a singleton that makes its own thread to signal a periodic garbage collection, and I have another object with static storage duration that implements a fixed memory allocator. The allocator tries to access the singleton, which exposes me to order-of-initialization issues.
But this is orthogonal to the juce::Thread issue (in my own singleton implementation, I use a StaticCriticalSection to ensure thread safety).
To summarize, juce::RunningThreadsList::getInstance() is not thread safe, and this can be demonstrated by creating a bunch of juce::Thread in a tight loop, then terminating all of them. If it seems like this is not a problem on the Mac, well that’s because gcc 4.0 and later invisibly add the necessary “two-bool atomic initialization” that I have manually coded out in my StaticCriticalSection (and this behavior can be disabled with the -fno-threadsafe-statics flag).
My fix to juce_Thread.cpp, assuming that you have the StaticObject and StaticCriticalSection classes, is the following:
class RunningThreadsList
{
public:
static StaticCriticalSection s_lock;
//...
static RunningThreadsList& getInstance()
{
// protect the "invisible bool" added by some compilers
StaticCriticalSection::ScopedLock lock_ (s_lock);
{
static RunningThreadsList runningThreads;
return runningThreads;
}
}
//...
};
StaticCriticalSection RunningThreadsList::s_lock;As a side note, juce_DeclareSingleton is vulnerable to order of initialization issues because _singletonLock requires the constructor execute in order to function correctly. Unlike the problem with function-scope objects with static storage duration, that will not be solved by C++0x, but is solved using StaticCriticalSection.
Oh, I see - sorry, I just looked at the code. I thought you were saying that I hadn’t given it a critical section, but in fact you meant that it has a critical section which isn’t thread-safe, which is true. (Actually, the easiest fix for this is probably just to make the static RunningThreadsList a file static rather than an inline static).
Something I’ve been meaning to write for a while is a simple SpinLock based around an Atomic, which would be handy for low-contention situations like this, and which could easily be created statically with no overhead…
That solves the immediate problem but then opens the door to order-of-initialization issues because the CriticalSection has a non trivial constructor.
My “StaticCriticalSection” object does not suffer from this problem, because it has no constructor. It relies on the C++ spec of filling its region of memory with zeroes, and then leveraging the atomic int flags to perform a “two-bool thread safe initialization”.
I agree, and that’s exactly what I did in StaticCriticalSection::enter():
void enter () const {
if (inited->get() == 0) {
if (initing->compareAndSetBool (1, 0)) {
new (lock.getObject()) CriticalSection;
inited->set (1);
} else
do { Thread::yield (); } while (inited->get() == 0); // <-- SPIN
}
lock->enter ();
}I just used yield() because it was simple and available.
Again, note that even with a static level CriticalSection, and even with a c++0x conforming compiler, there is still the order-of-initialization issue - StaticCriticalSection solves this problem with the existing C++ spec (and help from the wonderful juce::Atomic<>).
Also, juce_DeclareSingleton is vulnerable to order of initialization, for all the same reasons.
UPDATED I improved the original implementation of the StaticCriticalSection to prevent undefined usage and enforce the static storage duration requirement, by turning it into a template with a “Tag” and only static members. I updated the original post and I will also post it in a separate new message since it may solve order of initialization issues for people.
If you dissamble what GCC does for static object construction, you’ll see that GCC prevent any multiple thread to start initializing this at a same time.
So, if you have a Thread derived class you’re constructing on the global scope, you’ll likely deadlock under GCC, even before your main is started.
You can read http://gcc.gnu.org/ml/gcc-patches/2004-08/msg01598.html
In reality, your pseudo code for a static initialization is not complete, it’s more like:
static RunningThreadsList& getInstance()
{
static bool _runningThreads_ctor_called; // initialized to zero as per C++ spec
// HERE, you'll call malloc, which is protected by __cxa_guard_acquire, so no 2 threads can call this at the same time.
static RunningThreadsList runningThreads;
if (!_runningThreads_ctor_called)
{
// Or HERE
new (&runningThreads) RunningThreadsList;
_runningThreads_ctor_called = true;
}
// Here __cxa_guard_acquire is released
return runningThreads;
}The issue you’re experiencing is certainly not linked with static & concurrent access, unless you’re calling getInstance() after the main () function is entered, which can be solved way easier by providing your global CriticalSection that’ll be constructed before main() get called, and any other thread is started.
I’m using Visual Studio 2008
Doesn’t mean it is not standard, does it ?
The 98 C++ standard says:
6.7/4 (relevant to static local object initialization):
"[…] Otherwise such an object is initialized the first time control passes through its declaration; such an
object is considered initialized upon the completion of its initialization. […] If control reenters
the declaration (recursively) while the object is being initialized, the behavior
is undefined."
Since 1x version, it’s clear that such pattern MUST be thread safe.
Anyway, what about the other remark, [quote=“X-Ryl669”]The issue you’re experiencing is certainly not linked with static & concurrent access, unless you’re calling getInstance() after the main () function is entered, which can be solved way easier by providing your global CriticalSection that’ll be constructed before main() get called, and any other thread is started.
[/quote], wouldn’t that best suited for your issue ?
Not really entirely sure what you mean but it’s right there in the spec, if control re-enters the declaration the behavior is undefined. GCC goes a step farther and outputs the necessary code to make it thread safe by default. Not all compilers do this. C++0x is more specific with thread-safety assurances for function level objects with static storage duration (as I mentioned).
No, not at all and like I said it is easily reproduced with a test program, just make a bunch of juce::Thread in a tight loop. getInstance() will initialize the RunningThreadsList twice (under Visual Studio 2008).
No because we are talking about more than one object at file scope with static storage duration, in different translation units. The order of constructor calls is not specified in the current C++ spec, nor is it specified in the new spec C++0x.
To summarize, there are two issues, only the first of which is a defect in the current Juce tip:
Construction of objects at function scope with static storage duration is not thread-safe on all juce-supported platforms
The order of construction for objects at file scope with static storage duration, in different translation units, is undefined.
StaticCriticalSection avoids both problems because it does not have or require a constructor. It can be safely used both at function scope, or at file scope, and works correctly regardless of the thread access pattern or the order of construction of objects that depend on it.
