Do not use complex typed static variables
Dynamic Python binding for Qt Applications
Brought to you by:
florianlink,
marcusbarann
The pythonqt.dll crashes when you try to load it dynamically on Windows. The crash happens at this point:
https://github.com/commontk/PythonQt/blob/503597ba3274f59cd68347f11926340ec4f33471/src/PythonQtConversion.h#L166
The static function refers to a static variable that has complex type (QHash). The static variable is not initialised.
The issue is described here:
http://stackoverflow.com/questions/5114683/loading-dll-not-initializing-static-c-classes
Basically, complex typed static variables should be avoided in dll-s.
I raised an issue and sent a pull request to the commontk fork on GitHub:
https://github.com/commontk/PythonQt/issues/34
https://github.com/commontk/PythonQt/pull/35
I tried to get rid of every references to complex typed static variables. The object typed (non pointer) static data members I replaced by static getters that declare the variable as pointer, assignes NULL to it, and then does a NULL check and assigns a 'new' instance at the first time.
There were many static locals declared with type QByteArray but used only as const char * later. I changed the types to 'const char *' wherever I could.
In other cases the static locals were used only as performance tweak. There, I simply removed the 'static' keyword. All of them were trivial functions, the 'static' keyword was not necessary and I doubt that it made noticeable difference in execution time.
Do you agree with the pull request? Could you integrate it to the upstream?
I don't really see the problem. The initialization order of these static variables in undefined,
but nevertheless they are intialized by DLLMain and since PythonQt does not define it's own entry function, they should be initialized correctly. We are loading PythonQt dynamically on Windows/Mac/Linux on various compilers and never had this issue, so I am not sure if you are on the right track.
What compiler do you use on Windows? I used VS2012.
The crash was at the first reference to the map (QHash) that means the static instance has not been created. I tried to move the definition of the static function (that refers to the static variable) to the cpp file, but it did not help. So, it is not about the order of the initialisation of static variables. They are not initialised at all.
Have a look at "Recommendations" section on this page:
https://msdn.microsoft.com/en-us/library/windows/desktop/dn633971%28v=vs.85%29.aspx
"Perform simple initializations statically at compile time, rather than in DllMain."
"Defer initialization tasks that can wait until later. Certain error conditions must be detected early so that the application can handle errors gracefully. However, there are tradeoffs between this early detection and the loss of robustness that can result from it. Deferring initialization is often best."
We are using VS2010, VS2012, VS2013 and VS2015, all without any problems.
The problem happens when you load a library dynamically that depends on PythonQt. The crash occurs when this library tries to initialise Python. The static initialisers of PythonQt.dll have not been executed at this point, probably because the dynamic loading of the dependent library (the one that you are loading dynamically and which depends on PythonQt) has not been finished.
We do exactly that, we load a dll dynamically that depends on PythonQt using Qlibrary and we never ran into that problem. Which makes me wonder what is different in your scenario.
The difference might be that in my case the invocation is in static code, while in yours it is probably not.
Do you guys agree to fix this?
It is easy to see that the current use of static maps goes against the Microsoft recommendation about dll-s, and it does actually cause a crash when PythonQt is called from a static initialiser of another library.
I think your patch changes too much. Static variables in functions should not be affected,
since they will be initialized when the function is called the fist time and not on DLL load time.
Regarding the static global hash tables I agree that we can change that,
I think it might make sense to put all that state into a single struct and create that lazily.
I will not be able to do this soon, but I will put it on my TODO list.
I still think that you are having a strange problem, since it alwasy worked for us on all platfroms.
I have not actually checked if changing the static locals is necessary to avoid the crash. You are probably right. There were some cases when you created a static QByteArray but you used it only as
const char*later, so you might want to keep those changes, although that's an unrelated issue.Do you have a test case when PythonQt is called from a statically initialised code from another library?
Ah, yes that has to be the reason, we always use PythonQt as a dynamic library.
I created a slimmer pull request to the CTK fork. I tested it, this is sufficient to avoid the crash.
https://github.com/commontk/PythonQt/pull/37