Laszlo Papp committed changes in [gluon] /:

Core/Singleton: Implement a proxy method for getting a mutex instance for need



The basic issue that this commit address is that the static mutex of the

singletons is created during the initialization stage, before the main entry

point. This would not cause any issue itself, but the gluonobjectfactory creates

objects for getting the mimetypes in case of the GluonObjects that are targetted

for registration.



The problem is also that C++ does not have virtual static method as we say in

the relevant comment of the factory code and as such we need to create objects.

The issues come into the picture when these objects, which are also created during

the initialization stage (before the main entry point), try to access the

singleton instance methods in their constructor. It might result that the mutex

tries locking in that method, but it can crash anytime without any guarantee for

the proper operation if the static mutex is not created. We cannot make the

order proper because there is no standard C/C++ way of defining these things.



The order of the global/static initialization is undefined as such during that

stage. I have tested on my i386 and the linker does it from the last included

source file, thus while creating *.o files the order is really not guaranteed

for the initializations.



There are essentially more approach for solving this issues.

1) Make static supportedMimeTypes methods for the GluonObjects that will be

registered by the factory. The problem is that with this approach we could not

provide a pure virtual static method-like way, thus we would force each class

that would like to be registered to implement it on their own which can be not

that user/programmer friendly. We can do just fine with virtual methods as

non-static if construct the objects there.



2) We could eliminate all the singleton instance method calls from the

constructor of the relevant GluonObjects that will be registered. It is not

something that handy either. However I eliminated the audio engine instance

method call (which is a singleton type) from the soundlistener component

constructor. It hsa not been still enough actually. For instance the

materialinstance constructor was also using the Graphics Engine (singleton)

instance in its constructor. That can be arguably eliminated, but the problem is

that we should take care about these things all the time when we have a class

that will be registered. In my humble opinion, that is why it is that good

approach either heretofore.



3) Guarantee that the static mutex of the singletons are created properly before

trying to use them for locking the Singleton creation. After a lot of thinking,

I chose this way. This would actually solve all the crash we experienced on

Windows, Nokia N9 (Harmattan/Maemo6) phone and the "random" crash on my Linux

desktop PC. Yes, "random" crash because the order of the collected cpp source

files in the CMakeLists.txt cmake filesmatters differently in different

environments.



After all, it is not that easy to write a thread-safe singleton, and people like

generally avoiding singletons in multi-threaded application as much as possible,

but we still ship a nice way of doing it.



The basic idea was to provide a proxy method (we called it later mutexInstance

after the agreement about it) that creates a mutex if it is needed. Imagine it

as the singleton instantiation happens (that is how the name comes from there).



There were more ideas actually how to accomplish it properly that I would like

share with you. The first idea has been to make a boost::scoped_lock like way

here, as in a critical section, but that would not have really solved the issue

because the basic issue would have remained how to "protect" a function local

static variable which is not thread safe by "purpose". There are more

discussions like that on the qt mailing list arhcive site, but the tryReadLock

and QMutexLocket, etc did not really cause the proper functionality and

operation.



There was also a very simple approach to just make the localfunction variable

static. That is not enough and not even thread-safe even if the compilers

nowadays has the proper options *by default* for that it is really not safe to

make guess work each architecture we ship. The C++ standard before C++11 or

C++0x were not really thread-safe scaled which is slighly fixed by the now

upcoming standard, thus we should not really make guess-works over there.

(even though gcc has -Wfno-global-static option, but it is enabled by default)



I was also considering using spinlock. The idea is not to lock the constructor,

the idea is to lock *before* the construction happens to ensure no duplicate

instance is created. If you locked inside the constructor, you could end up with

multiple instances. The idea of the lock is not just to ensure the constructor

body is synchronized. Which is a part of it, but there's obviously more into it.

It does not matter here if you have a spinlock or a sleeplock or whatever,

it's negligible. The lock acquisition only happens once, when you create the

instance and the only time ever the lock acquisition won't be instant is when

another thread tries to call instance() at the same time while other thread is

still constructing the singleton. The lock is only acquired during

initialization. Spinlock is just an alternative lock to sleeplock where instead

of going to sleep, it busy-waits with a loop. It is useful if you know you'd

only sleep for small amount of time, so there's no reason to do expensive

context switching or OS scheduling. Yes, going to sleep implies a context

switch, but like I said earlier: 1) the chances of two threads ever calling this

function at the same time are very very very slim. 2) the only time lock is ever

acquired is during initialization, which happens exactly once. 3) because of the

two afromentioned reasons, which kind of lock you use is negligible, VERY

negligible and making an assumption that spinlock is better for the situation is

bad: 1) you don't know HOW LONG the construction of the singleton takes, it can

take so long that a sleeplock would perform better. 2) this is a library, you

don't know on what kind of machine the code is going to be run on, spinlock on

single-core, single-cpu machine is a very very bad idea. There's no reason to

change it, it's negligible. Spinlock will be perform better in certain

situations and worse in others, but it does not matter at all.



So we are at the discussion of the final solution which was is a good idea

actually after discussing it with thiago IRC so that using QBasicAtomicPointer

and atomic swap in the proxy method. There is a good example for this uage

inside the Q_GLOBAL_STATIC internal Qt API macro, but since it is not documented

and not even planned to be a public API, they would not like to fix bugs about

it and they would like to make changes anytime without warning us, I could not

just simply use this nice macro written by thiago. That is true,

QBasicAtomicPointer is not documented either, but targetted for public usage as

I was pointed out by thiago. The documentation will happen by him as soon as

possible. There is a documented way, like QAtomicPointer, but the problem is

that with it is not really a POD type which will not solve the issue of having

another thing that should be defended. Hence after learning the internal

Q_GLOBAL_STATIC internal Qt API, I decided to implement our way similarly and

the result is awesome.



During the internal review we decided to put the class members into the private

section and also this proxy method. The only public method remained is the

instance method of the singleton.



Finally I could eliminate the hack in the gluon cmake file (CMakeLists.txt)

where the order mattered previously, so it is nice as expected by now.



About the testing: I could not test it windows, but I discussed it with the

windows guy so that he can try this patch out later today. Nevertheless I could

test on Linux and it works just neatly and I also tested on my Nokia N9 device

where it also worked. There are some graphics issues, but that is not relevant

to this change.