Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eliminate complex typed static variables #35

Closed

Conversation

espakm
Copy link

@espakm espakm commented Feb 9, 2016

Complex typed static variables (e.g. maps) in dll-s are to avoid.
On Windows they are not initialised in time (or at all) when the dll
is dynamically loaded. The current use of static QHash data members
results in crash on Windows when the PythonQt.dll is loaded.

See details here:
http://stackoverflow.com/questions/5114683/loading-dll-not-initializing-static-c-classes

@jcfr
Copy link
Member

jcfr commented Feb 9, 2016

Would is also be possible to provide a minimum example allowing to reproduce the problem on windows ?

@espakm
Copy link
Author

espakm commented Feb 9, 2016

It happens for us when we use it through the MITK Python console, compiled on Windows with VS2012. (We have not tried other VS versions.)

The static map data members definitely cause a crash. Just moving them inside a getter function does not help, I had to turn them into pointers, assign NULL to them and do a NULL check. This was in PythonQtConversion and PythonQtMethondInfo.

The other changes I made just to be consistent with the pattern but have not actually run into a crash.

@jcfr
Copy link
Member

jcfr commented Feb 9, 2016

through the MITK Python console, compiled on Windows with VS2012

  • Is the issue reproducible building CTK with the ctkSimplePythonShell application enabled ?
  • Could you also confirm that both python and mitk were build in Release ?

Since commontk/PythonQt is just a staging area to conveniently host patches before integrating them upstream, we should make sure we have a simple use case to integrate to the test suite.

@espakm
Copy link
Author

espakm commented Feb 10, 2016

Python, PythonQt, CTK and MITK were all in release mode. Later I rebuilt PythonQt and CTK with adding debug symbols (/Zi) to the release build, so that I can step through the code to find the bug.

I will try to reproduce the issue with ctkSimplePythonShell later.

Complex typed static variables (e.g. maps) in dll-s are to avoid.
On Windows they are not initialised in time (or at all) when the dll
is dynamically loaded. The current use of static QHash data members
results in crash on Windows when the PythonQt.dll is loaded.

See details here:
http://stackoverflow.com/questions/5114683/loading-dll-not-initializing-static-c-classes
@espakm espakm force-pushed the 34-no-statics-with-complex-type branch from 6a29110 to 127d894 Compare February 10, 2016 13:21
@jcfr
Copy link
Member

jcfr commented Feb 10, 2016

I will try to reproduce the issue with ctkSimplePythonShell later.

Great. Keep us posted of your findings.

@espakm
Copy link
Author

espakm commented Feb 10, 2016

ctkSimplePythonShell works fine.

@jcfr
Copy link
Member

jcfr commented Feb 10, 2016

Good. Then, could the issue be in how PythonQt/CTK is integrated in MITK ?

It would be good if we can make a good case and get your changes upstreamed.

@espakm
Copy link
Author

espakm commented Feb 11, 2016

I do not think so. I built MITK with the Python console plugin yesterday. The exact same plugin that we use in our MITK based application. PythonQt commit hash was the same as well. And it does not crash there. The only difference I see is that we make our release build with debug symbols, while the clean MITK build was without symbols.

It does not make much sense, I know, but do not see any difference. I will rebuild MITK with debug symbols to see if it crashes.

@espakm
Copy link
Author

espakm commented Feb 12, 2016

I got it. There are a few differences in the MITK plugin that we use compared to their latest release. We applied some later fixes from their master branch. The changes factor out the MitkPythonService code to a separate module that is loaded dynamically through an 'autoload' mechanism when the core library of the application (MitkCore.dll) is loaded. PythonQt is initialised from the activator of the MitkPythonService module, that is probably a called from a static initialiser. (It's a macro generated code, did not decypher it.)

So, it seems that the crash occurs when PythonQt is called from a static initiliser of a library that is being loaded. (And PythonQt has not been loaded before.)

Florian agreed to incorporate part of my patch that turns the static data members to pointers and initialises them lazily from a getter.

@jcfr
Copy link
Member

jcfr commented Feb 12, 2016

Outstanding investigation 👍

Thanks for the detailed report

Florian agreed to incorporate part of my patch that turns the static data members to pointers and initialises them lazily from a getter.

Sounds good. When you have a patch close to what you would contribute upstream, we will integrate it in the CTK fork.

Let me know what you think,

@espakm
Copy link
Author

espakm commented Feb 12, 2016

I pushed a new, slimmer PR, based on the discussion with Florian:

#37

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants