search

schrodinger / pymol-open-source

Open-source foundation of the user-sponsored PyMOL molecular visualization system.
https://pymol.org/
Other
1.2k stars 281 forks source link

pymol --testing destroys SingletonPyMOLGlobals #335

Open speleo3 opened 9 months ago

speleo3 commented 9 months ago

Summary

Looks like the C++ tests can destroy SingletonPyMOLGlobals. I suspect since 4e3097fa10b4ed495b5638f19bde6de29ffd5c19.

Steps to reproduce

Observed output

python: layer1/P.cpp:1914: void PInit(PyMOLGlobals*, int): Assertion `SingletonPyMOLGlobals' failed.

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
pymol is a Catch v2.13.10 host application.
Run with -? for options

-------------------------------------------------------------------------------
ExecutiveManageObject
-------------------------------------------------------------------------------
layerCTest/Test_Executive.cpp:27
...............................................................................

layerCTest/Test_Executive.cpp:27: FAILED:
due to a fatal error condition:
  SIGABRT - Abort (abnormal termination) signal

===============================================================================
test cases:  16 |  15 passed | 1 failed
assertions: 150 | 149 passed | 1 failed

Aborted (core dumped)

Crash observed on

Possible root cause

PInit() overwrites pymol.cmd._COb (without a destructor).

https://github.com/schrodinger/pymol-open-source/blob/3faec30816c8fe4d5cbe3ddfed490e8eb1ec72e1/layer1/P.cpp#L1916-L1917

The overwritten _COb was probably previously initialized in Cmd_New, with a destructor.

https://github.com/schrodinger/pymol-open-source/blob/3faec30816c8fe4d5cbe3ddfed490e8eb1ec72e1/layer4/Cmd.cpp#L3457-L3458

JarrettSJohnson commented 9 months ago

Thanks for opening this issue. There was a commit that I forgot to transfer here which includes the following

   PyMOLInstance::PyMOLInstance()
   {
     auto options = PyMOLOptions_New();
     options->show_splash = false;
     m_Inst = PyMOL_NewWithOptions(options);
     PyMOLOptions_Free(options);
     m_G = PyMOL_GetGlobals(m_Inst);
+    SingletonPyMOLGlobals = m_G;
     PInit(m_G, true);
     PyMOL_Start(m_Inst);
   }

   PyMOLInstance::~PyMOLInstance()
   {
     PyMOL_Stop(m_Inst);
     PFree(m_G);
     PyMOL_Free(m_Inst);
+    SingletonPyMOLGlobals = nullptr;
   }

was written some time ago, and looking back on it now, I'm not confident that it's the correct strategy.

Edit: if I don't have a proper solution by the end of the weekend, I'll disable this test for the time being.

JarrettSJohnson commented 8 months ago

I've silenced the tests that use PyMOLInstance for now just so that the suite can pass on these machines. I'll look into fixing the underlying issue soon when I get some time.