[Development] Q_GLOBAL_STATIC_WITH_INITIALIZER is harmful: please stop using it! (long)

[Thiago Macieira]

Ok, some people requested an explanation. I will provide one, but I don't 
expect many will understand it or even read until the end of this email, so if 
you don't get it, you'll have to trust me (which is the exact same result as 
not giving an explanation).

First, two public announcements:
1) The Q_GLOBAL_STATIC macros are *not* public API. Do not use them outside Qt 
modules. I *will* change its semantics soon and I will not hesitate to break 
your code if you're using it. I will fix all of Qt when making my changes, but 
I will not fix your non-Qt code.

For Qt 5.1, my rewritten Q_GLOBAL_STATIC can be made public API.

2) < bhughes> i'd agree with the blanket statement "global static initializers 
are evil"


Explanation (short):
Q_GLOBAL_STATIC_WITH_INITIALIZER is not thread-safe. Therefore, you must not 
use it: if you want thread-safety, use one of the other two macros. If you 
don't care about thread-safety, you can drop the macro altogether. For 
example:

Q_GLOBAL_STATIC_WITH_INITIALIZER(Type, object, {initializer})

can be "safely" replaced with:

Type *object()
{
    static Type *pointer;
    if (!pointer) {
        pointer = new Type;
        initializer
    }
}

that is equally as thread-safe as the macro. And it's a lot more readable.


Explanation (long):
Q_GLOBAL_STATIC_WITH_INIITIALIZER can be used thread-safely, just as the other 
macros can be used unsafely. The question we must judge is how often a given 
API is used incorrectly. As it happens, this particular macro, especially with 
the name that it has, is most often used incorrectly.

Q_GLOBAL_STATIC macros are lock-free. They inspect an atomic pointer[1] to see 
if it has been initialised or not. If it has not yet been initialised, a new 
object is created and then an atomic compare-and-swap (CAS) operation is 
performed. If two or more threads are concurrently trying to initialise the 
object, only one of them will succeed in the CAS operation. The other N-1 
threads will delete the object they had created and will use the object 
created by the thread that succeeded in setting.

It should be clear that an object can be created N times and deleted N-1 times 
through that process. That's how it must be if Q_GLOBAL_STATIC is to remain 
lock-free[2]. Unfortunately, that means the code run by the object's 
constructor and by the evaluation of the arguments in the case of 
Q_GLOBAL_STATIC_WITH_ARGS must be thread-safe too. Expensive operations, 
thread-safe or not, should be avoided in the initialisation of the global 
static. But if they are thread-safe, an expensive operation is acceptable.

Q_GLOBAL_STATIC_WITH_INITIALIZER modifies the macro slightly by adding a 
section of code that is run only in the thread that succeeded in the CAS 
operation. So, unlike the constructor code, this extra code is run exactly 
once. This is tempting.

However, note that the macros remain lock-free. That means this initialisation 
code is run completely unsynchronised with other threads. The absence of the 
lock means the N-1 threads that failed the CAS will simply loadAcquire the 
pointer again and proceed normally, possibly before the initialisation code 
gets run. They simply expect that the object is already fully initialised by 
then. Moreover, a proper Acquire/Release pair is missing. That means any 
further operations executed on the object may be visible to other threads in 
any order, including impossible conditions.

Therefore, we conclude this code MUST NOT touch the global static object! The 
*only* acceptable extra code is to run extra operations, like registering a 
cleanup function:

Q_GLOBAL_STATIC_WITH_INITIALIZER(QtCursorDatabase, cursorDatabase, 
qAddPostRoutine(clearCursorDatabase))

(found in qttools/src/shared/qtpropertybrowser/qtpropertymanager.cpp)

Unfortunately, that's not what this macro is being used for. It has a bad name 
and causes people to use the extra argument not for post-initialisation, but 
for actual initialisation of the object. More often than not, you're going to 
find something like:

Q_GLOBAL_STATIC_WITH_INITIALIZER(itemVectorType, staticItemVector, {
    *x = itemVector();
})

(found in Qt Creator c0e5060:src/plugins/qt4projectmanager/qtmodulesinfo.cpp)

The magic "x" variable is the object created by the current thread. The code 
above exhibits both failures I mentioned: it's not thread-safe and it lacks a 
proper Release operation. The type in question (a QVector<const item*>) is 
*not* designed for concurrent mutating operations.

The code above can be replaced by the more correct:

Q_GLOBAL_STATIC_WITH_ARGS(itemVectorType, staticItemVector, (itemVector()))

The above is thread-safe and has proper memory ordering semantics. However, 
due to its lock-free nature, itemVector() may be called N times and its result 
discarded N-1 times. Therefore, the itemVector() function must be thread-safe 
too.

More complex code like this one found right below the above:
Q_GLOBAL_STATIC_WITH_INITIALIZER(QStringList, staticModulesList, {
    const itemVectorType * const itemVector = staticItemVector();
    for (int i = 0; i < itemVector->count(); i++)
        x->append(QString::fromLatin1(itemVector->at(i)->config));
})

Can be replaced by the following:
namespace {
struct ModuleList: public QStringList
{
    ModuleList()
    {
        const itemVectorType * const itemVector = staticItemVector();
        for (int i = 0; i < itemVector->count(); i++)
            append(QString::fromLatin1(itemVector->at(i)->config));
    }
};
}

Q_GLOBAL_STATIC(ModuleList, staticModulesList)


[1] as per the C++11 standard, it's neither active nor radioactive
[2] my changes will make it use a lock, which means the initialisation will be 
performed exactly once too, obviating the need for the initialisation function 
to be thread-safe. However, my current solution is not deadlock-free.

-- 
Thiago Macieira - thiago.macieira (AT) intel.com
  Software Architect - Intel Open Source Technology Center
     Intel Sweden AB - Registration Number: 556189-6027
     Knarrarnäsgatan 15, 164 40 Kista, Stockholm, Sweden
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.qt-project.org/pipermail/development/attachments/20120316/59b8aba4/attachment.sig>