search

sbmlteam / libsbml

LibSBML is a native library for reading, writing and manipulating files and data streams containing the Systems Biology Markup Language (SBML). It offers language bindings for C, C++, C#, Java, JavaScript, MATLAB, Perl, PHP, Python, R and Ruby.
https://sbml.org/software/libsbml
Other
41 stars 29 forks source link

Deleting a document crashes when SBMLTransforms is already gone #320

Open luciansmith opened 1 year ago

luciansmith commented 1 year ago

I'm getting crashes at the end of my program when an SBML document is deleted, at the line:

SBMLTransforms::clearComponentValues(mModel);

at line 240 of SBMLDocument.

I think this is because the SBMLTransforms object happened to be deleted first, so when I delete this document, it's already gone (the doc in question is itself attached to a global object that is going out of scope).

I couldn't figure out a good way to fix this. Is there an actual object that SBMLTransforms uses? Can a document have a pointer to it, so it won't go out of scope, maybe?

fbergmann commented 1 year ago

It used to be, that SBMLTransforms would just reuse the same map for all models, which caused issues if you had multiple ones loaded at the same time, so now it has a static map. So it would not depend on any specific transform instance.

Unfortunately your bug report does not have enough information to see what precisely went wrong. Do you have a stack trace or ideally an example that crashes so i can reproduce it to look into this?

luciansmith commented 1 year ago

Unfortunately, my example is just 'An Antimony run of test_antimony in Windows debug mode', which seems like kind of a lot to put together on your end for this. I think the basic component you'd need to reproduce it is just 'a global object with an SBMLDocument child' (in Antimony's case it's 'the registry object') but you'd have to happen to arrange things so that the global objects were torn down in a particular order. You might get lucky! But it seems a little fraught.

I can at least give you the errors I get when I run it: Here's what I get for the details of the exception:

Exception thrown: read access violation. _Pnode was 0xFFFFFFFFFFFFFFE6.

This is at line 1687 of xtree

Here's the stack trace:

test_antimony.exe!std::_Tree<std::_Tmap_traits<libsbml::Model const ,std::map<std::string const ,std::pair<double,bool>,std::less,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>,std::less<libsbml::Model const >,std::allocator<std::pair<libsbml::Model const const,std::map<std::string const ,std::pair<double,bool>,std::less,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>,0>>::_Eqrange<libsbml::Model const >(const libsbml::Model const & _Keyval) Line 1687 C++ test_antimony.exe!std::_Tree<std::_Tmap_traits<libsbml::Model const ,std::map<std::string const ,std::pair<double,bool>,std::less,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>,std::less<libsbml::Model const >,std::allocator<std::pair<libsbml::Model const const,std::map<std::string const ,std::pair<double,bool>,std::less,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>,0>>::erase(const libsbml::Model const & _Keyval) Line 1332 C++ test_antimony.exe!libsbml::SBMLTransforms::clearComponentValues(const libsbml::Model m) Line 557 C++ test_antimony.exe!libsbml::SBMLDocument::~SBMLDocument() Line 241 C++ test_antimony.exe!Module::~Module() Line 256 C++ [External Code] test_antimony.exe!Registry::~Registry() Line 66 C++

The key bit of libsbml that's the problem is 'mModelValues.erase(m)'. I tried changing this to not erase if there was no 'm' in it, but 'mModelValues.find(m)' also crashed. The problem is (I believe) that mModelValues has already been deleted, so doing anything with it results in a crash. Here's what VisualStudio shows me:

<HTML>
<head>
<title>Document</title></head>
<body>
<!--StartFragment-->

  | Name | Value | Type
-- | -- | -- | --
◢ | mModelValues | { size=0 } | std::map<libsbml::Model const *,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>,std::less<libsbml::Model const *>,std::allocator<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>>
  | ◢ [comparator] | less | std::_Compressed_pair<std::less<libsbml::Model const *>,std::_Compressed_pair<std::allocator<std::_Tree_node<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>,void *>>,std::_Tree_val<std::_Tree_simple_types<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>>,1>,1>
  | ▶ [Raw View] | {_Myval2=allocator } | std::_Compressed_pair<std::less<libsbml::Model const *>,std::_Compressed_pair<std::allocator<std::_Tree_node<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>,void *>>,std::_Tree_val<std::_Tree_simple_types<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>>,1>,1>
  | ◢ [allocator] | allocator | std::_Compressed_pair<std::allocator<std::_Tree_node<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>,void *>>,std::_Tree_val<std::_Tree_simple_types<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>>,1>
  | ▶ [Raw View] | {_Myval2={_Myhead=0x000001a6e7b32a40 {_Left=0xdddddddddddddddd {_Left=??? _Parent=??? _Right=??? ...} ...} ...} } | std::_Compressed_pair<std::allocator<std::_Tree_node<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>,void *>>,std::_Tree_val<std::_Tree_simple_types<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>>,1>
  | ◢ [Raw View] | {...} | std::map<libsbml::Model const *,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>,std::less<libsbml::Model const *>,std::allocator<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>>
  | ◢ std::_Tree<std::_Tmap_traits<libsbml::Model const *,std::map<std::basic_string<char,std::char_traits<char>,std::allocator<char> > const ,std::pair<double,bool>,std::less<std::basic_string<char,std::char_traits<char>,std::allocator<char> > const >,std::allocator<std::pair<std::basic_string<char,std::char_traits<char>,std::allocator<char> > const ,std::pair<double,bool> > > >,std::less<libsbml::Model const *>,std::allocator<std::pair<libsbml::Model const * const,std::map<std::basic_string<char,std::char_traits<char>,std::allocator<char> > const ,std::pair<double,bool>,std::less<std::basic_string<char,std::char_traits<char>,std::allocator<char> > const >,std::allocator<std::pair<std::basic_string<char,std::char_traits<char>,std::allocator<char> > const ,std::pair<double,bool> > > > > >,0> > | {_Mypair=less } | std::_Tree<std::_Tmap_traits<libsbml::Model const *,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>,std::less<libsbml::Model const *>,std::allocator<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>,0>>
  | ◢ _Mypair | less | std::_Compressed_pair<std::less<libsbml::Model const *>,std::_Compressed_pair<std::allocator<std::_Tree_node<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>,void *>>,std::_Tree_val<std::_Tree_simple_types<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>>,1>,1>
  | ◢ [Raw View] | {_Myval2=allocator } | std::_Compressed_pair<std::less<libsbml::Model const *>,std::_Compressed_pair<std::allocator<std::_Tree_node<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>,void *>>,std::_Tree_val<std::_Tree_simple_types<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>>,1>,1>
  | std::less<libsbml::Model const *> | {...} | std::less<libsbml::Model const *>
  | ◢ _Myval2 | allocator | std::_Compressed_pair<std::allocator<std::_Tree_node<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>,void *>>,std::_Tree_val<std::_Tree_simple_types<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>>,1>
  | ◢ [Raw View] | {_Myval2={_Myhead=0x000001a6e7b32a40 {_Left=0xdddddddddddddddd {_Left=??? _Parent=??? _Right=??? ...} ...} ...} } | std::_Compressed_pair<std::allocator<std::_Tree_node<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>,void *>>,std::_Tree_val<std::_Tree_simple_types<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>>,1>
  | std::allocator<std::_Tree_node<std::pair<libsbml::Model const * const,std::map<std::basic_string<char,std::char_traits<char>,std::allocator<char> > const ,std::pair<double,bool>,std::less<std::basic_string<char,std::char_traits<char>,std::allocator<char> > const >,std::allocator<std::pair<std::basic_string<char,std::char_traits<char>,std::allocator<char> > const ,std::pair<double,bool> > > > >,void *> > | {...} | std::allocator<std::_Tree_node<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>,void *>>
  | ◢ _Myval2 | {_Myhead=0x000001a6e7b32a40 {_Left=0xdddddddddddddddd {_Left=??? _Parent=??? _Right=??? ...} _Parent=...} ...} | std::_Tree_val<std::_Tree_simple_types<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>>
  | ◢ std::_Container_base12 | {_Myproxy=0x000001a6e7b85320 {_Mycont=0xdddddddddddddddd {_Myproxy=??? } _Myfirstiter=0xdddddddddddddddd {...} } } | std::_Container_base12
  | ▶ _Myproxy | 0x000001a6e7b85320 {_Mycont=0xdddddddddddddddd {_Myproxy=??? } _Myfirstiter=0xdddddddddddddddd {_Myproxy=...} } | std::_Container_proxy *
  | ◢ _Myhead | 0x000001a6e7b32a40 {_Left=0xdddddddddddddddd {_Left=??? _Parent=??? _Right=??? ...} _Parent=0xdddddddddddddddd {...} ...} | std::_Tree_node<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>,void *> *
  | ▶ _Left | 0xdddddddddddddddd {_Left=??? _Parent=??? _Right=??? ...} | std::_Tree_node<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>,void *> *
  | ▶ _Parent | 0xdddddddddddddddd {_Left=??? _Parent=??? _Right=??? ...} | std::_Tree_node<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>,void *> *
  | ▶ _Right | 0xdddddddddddddddd {_Left=??? _Parent=??? _Right=??? ...} | std::_Tree_node<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>,void *> *
  | _Color | -35 'Ý' | char
  | _Isnil | -35 'Ý' | char
  | ▶ _Myval | (0xdddddddddddddddd {mSubstanceUnits={...} mTimeUnits={...} mVolumeUnits={...} ...}, { size=15987178197214944733 }) | std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>
  | _Mysize | 0 | unsigned __int64

<!--EndFragment-->
</body>
</HTML>
luciansmith commented 1 year ago

If it helps:

https://dev.azure.com/TheRoadrunnerProject/Antimony/_build/results?buildId=2091&view=logs&j=15314346-96fc-526a-0d4a-1584374d24fd&t=6ef87d2e-fa5c-53e0-1ab5-aed8f1ea3d98&s=5eb25f97-233a-54a9-28a6-afcff88f1eb7

I could also just send an executable that crashes, if that would help?

fbergmann commented 1 year ago

An executable would only work if it would pick up libSBML from a shared library, so I could try. It is really odd that the static map would have already been deleted. That happens when the c++ runtime goes down not before.

Does the patch below help?

diff --git a/src/sbml/SBMLTransforms.cpp b/src/sbml/SBMLTransforms.cpp
index b6187c1c4..04971e774 100644
--- a/src/sbml/SBMLTransforms.cpp
+++ b/src/sbml/SBMLTransforms.cpp
@@ -545,6 +545,9 @@ SBMLTransforms::getComponentIds(const Model* m)
 void 
 SBMLTransforms::clearComponentValues(const Model* m)
 {
+  if (mModelValues.empty())
+    return;
+
   if (!m)
   {
     // clear all maps if no model specified

thank you

luciansmith commented 1 year ago

Exactly: the C++ runtime is going down, and your static map is being deleted and my static SBML document is also being deleted; it's just a question of which one is deleted first.

That said, your patch works for me! I'm kind of surprised at that, and worry that it might not work in all situations. Is 'empty' guaranteed to work on a deleted object? That seems kind of fraught.

fbergmann commented 1 year ago

the object was not deleted, you saw in your stack trace, that there were no objects in side: 

◢ | mModelValues | { size=0 }

the bad ptr stuff below is just the interpretation of the map::end iterator, which does not (and never does) contain a valid element. So it definitely was not deleted in your case. What is odd is that .find() and erase() threw an invalid read exception I did not think that in the STL they were supposed to.

What you could try is to register an std::atexit handler for clearing the antimony registry, so you have better control when it will go down.

luciansmith commented 1 year ago

No, I can confirm that even when a thing is deleted, sometimes bits of it happen to hang around in memory so if you ask it some questions, it's fine.

I just tried calling SBMLTransforms::mapComponentValues(m_sbml.getModel()) when I create new models, and even with the fix, we're back to crashing again, but only when the program is done and the Registry object is being deleted. 'clearComponentValues' is called many many times before that with zero problems.

Here's a stack trace at the end:

test_antimony.exe!std::_Tree<std::_Tmap_traits<libsbml::Model const ,std::map<std::string const ,std::pair<double,bool>,std::less,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>,std::less<libsbml::Model const >,std::allocator<std::pair<libsbml::Model const const,std::map<std::string const ,std::pair<double,bool>,std::less,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>,0>>::_Eqrange<libsbml::Model const >(const libsbml::Model const & _Keyval) Line 1687 C++ test_antimony.exe!std::_Tree<std::_Tmap_traits<libsbml::Model const ,std::map<std::string const ,std::pair<double,bool>,std::less,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>,std::less<libsbml::Model const >,std::allocator<std::pair<libsbml::Model const const,std::map<std::string const ,std::pair<double,bool>,std::less,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>,0>>::erase(const libsbml::Model const & _Keyval) Line 1332 C++ test_antimony.exe!libsbml::SBMLTransforms::clearComponentValues(const libsbml::Model m) Line 560 C++ test_antimony.exe!libsbml::SBMLDocument::~SBMLDocument() Line 241 C++ test_antimony.exe!Module::~Module() Line 256 C++ [External Code] test_antimony.exe!Registry::~Registry() Line 66 C++ [External Code]

and here's what can be gleaned from mModelValues:

<HTML>
<head>
<title>Document</title></head>
<body>
<!--StartFragment-->

  | Name | Value | Type
-- | -- | -- | --
◢ | mModelValues | { size=1 } | std::map<libsbml::Model const *,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>,std::less<libsbml::Model const *>,std::allocator<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>>
  | ▶ [comparator] | less | std::_Compressed_pair<std::less<libsbml::Model const *>,std::_Compressed_pair<std::allocator<std::_Tree_node<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>,void *>>,std::_Tree_val<std::_Tree_simple_types<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>>,1>,1>
  | ▶ [allocator] | allocator | std::_Compressed_pair<std::allocator<std::_Tree_node<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>,void *>>,std::_Tree_val<std::_Tree_simple_types<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>>,1>
  | ◢ [Raw View] | {...} | std::map<libsbml::Model const *,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>,std::less<libsbml::Model const *>,std::allocator<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>>
  | ◢ std::_Tree<std::_Tmap_traits<libsbml::Model const *,std::map<std::basic_string<char,std::char_traits<char>,std::allocator<char> > const ,std::pair<double,bool>,std::less<std::basic_string<char,std::char_traits<char>,std::allocator<char> > const >,std::allocator<std::pair<std::basic_string<char,std::char_traits<char>,std::allocator<char> > const ,std::pair<double,bool> > > >,std::less<libsbml::Model const *>,std::allocator<std::pair<libsbml::Model const * const,std::map<std::basic_string<char,std::char_traits<char>,std::allocator<char> > const ,std::pair<double,bool>,std::less<std::basic_string<char,std::char_traits<char>,std::allocator<char> > const >,std::allocator<std::pair<std::basic_string<char,std::char_traits<char>,std::allocator<char> > const ,std::pair<double,bool> > > > > >,0> > | {_Mypair=less } | std::_Tree<std::_Tmap_traits<libsbml::Model const *,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>,std::less<libsbml::Model const *>,std::allocator<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>,0>>
  | ▶ _Mypair | less | std::_Compressed_pair<std::less<libsbml::Model const *>,std::_Compressed_pair<std::allocator<std::_Tree_node<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>,void *>>,std::_Tree_val<std::_Tree_simple_types<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>>,1>,1>
  | <Error> |   |  

<!--EndFragment-->
</body>
</HTML>

The 'Error' is the sort of thing I often see on technically-deleted-but-the-residue-is-still-in-memory objects.

fbergmann commented 1 year ago

did you at the std::atexit handler as I asked? That way you can control that your stuff gets destroyed whenever you choose to, not when the runtime does. Most compilers destroy static objects in the order they are created otherwise. We should also add one to libsbml, but let's see whether the one for antimony works first.

otherwise someone using lib antimony would / could have the same issue when static objects get destroyed. an example is in the extension registry (SBMLExtensionRegistry.cpp) where we do that already.

luciansmith commented 1 year ago

The registry is an object, not a pointer, so I don't know how to destroy it; it just goes out of scope at some point. Is there some way to destroy it explicitly in an atexit routine?

fbergmann commented 1 year ago

well if whatever happened on the registry object destructor was moved into a function, that could be called atexit and would free stuff

luciansmith commented 1 year ago

The SBML Document itself is also just an object child of the registry; not a pointer.

fbergmann commented 1 year ago

ok, then try this patch too: 

diff --git a/src/sbml/extension/SBMLExtensionRegistry.cpp b/src/sbml/extension/SBMLExtensionRegistry.cpp
index 1c20e7a20..283dcf768 100644
--- a/src/sbml/extension/SBMLExtensionRegistry.cpp
+++ b/src/sbml/extension/SBMLExtensionRegistry.cpp
@@ -50,6 +50,7 @@
 #include <string>

 #include <sbml/extension/RegisterExtensions.h>
+#include <sbml/SBMLTransforms.h>

 #ifdef __cplusplus

@@ -75,6 +76,8 @@ SBMLExtensionRegistry::deleteRegistry()
     mInstance = NULL;
     registered = false;
   }
+
+  SBMLTransforms::clearComponentValues();
 }

 /** @cond doxygenLibsbmlInternal */

does that help?

luciansmith commented 1 year ago

That still fails--the 'deleteRegistry' function isn't called before ~Registry is called on my end.

Maybe mModelValues can be changed to a pointer instead of an object? Then things that delete it can set it to NULL, and the 'clear' function could check if it's NULL?

luciansmith commented 1 year ago

Actually, that causes its own problems: once the 'deleteRegistry' function is called, the mModelValues object is already bad, so it's now the deleteRegistry function that crashes:

test_antimony.exe!std::_Tree_val<std::_Tree_simple_types<std::pair<libsbml::Model const const,std::map<std::string const ,std::pair<double,bool>,std::less,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>>::_Orphan_ptr(std::_Tree_node<std::pair<libsbml::Model const const,std::map<std::string const ,std::pair<double,bool>,std::less,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>,void > const _Ptr) Line 718 C++ test_antimony.exe!std::_Tree<std::_Tmap_traits<libsbml::Model const ,std::map<std::string const ,std::pair<double,bool>,std::less,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>,std::less<libsbml::Model const >,std::allocator<std::pair<libsbml::Model const const,std::map<std::string const ,std::pair<double,bool>,std::less,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>,0>>::clear() Line 1342 C++ test_antimony.exe!libsbml::SBMLTransforms::clearComponentValues(const libsbml::Model m) Line 555 C++ test_antimony.exe!libsbml::SBMLExtensionRegistry::deleteRegistry() Line 79 C++

This from a version of libsbml that doesn't call 'clearComponentValues' when deleting an SBMLDocument. At that point, mModelValues looks like this:

<HTML>
<head>
<title>Document</title></head>
<body>
<!--StartFragment-->

  | Name | Value | Type
-- | -- | -- | --
◢ | mModelValues | { size=19 } | std::map<libsbml::Model const *,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>,std::less<libsbml::Model const *>,std::allocator<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>>
  | ▶ [comparator] | less | std::_Compressed_pair<std::less<libsbml::Model const *>,std::_Compressed_pair<std::allocator<std::_Tree_node<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>,void *>>,std::_Tree_val<std::_Tree_simple_types<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>>,1>,1>
  | ▶ [allocator] | allocator | std::_Compressed_pair<std::allocator<std::_Tree_node<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>,void *>>,std::_Tree_val<std::_Tree_simple_types<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>>,1>
  | ▶ [Raw View] | {...} | std::map<libsbml::Model const *,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>,std::less<libsbml::Model const *>,std::allocator<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>>
  | <Error> |   |  
  | <Error> |   |  
  | <Error> |   |  
  | <Error> |   |  
  | <Error> |   |  
  | <Error> |   |  
  | <Error> |   |  
  | <Error> |   |  
  | <Error> |   |  
  | <Error> |   |  
  | <Error> |   |  
  | <Error> |   |  
  | <Error> |   |  
  | <Error> |   |  
  | <Error> |   |  
  | <Error> |   |  
  | <Error> |   |  
  | <Error> |   |  
  | <Error> |   |  

<!--EndFragment-->
</body>
</HTML>
fbergmann commented 1 year ago

very strange. Ok ... let's look at other things ... what converter / evaluate thingies are you calling that would use a map? anyway they could clean up after themselves whenever they are done? clearly that'd be better than keeping 19 model states in memory.

luciansmith commented 1 year ago

I only put in the calls to SBMLTransforms::mapComponentValues just now to test things, to see what would happen if the 'cheat' to the call to 'mModelValues.empty()' didn't work to bypass the bit that crashed. (My hypotheses was that 'empty' only coincidentally happened to work on this deleted object. I think that hypothesis has been shown to be true? It sort of works, but I don't think it's guaranteed to work, and I think valgrind or similar would complain about accessing deleted memory.)

I don't know what other users of mapComponentValues might have in mind when using the function. Perhaps if they were required to clean up after themselves with explicit calls to clearComponentValues, that would make sense? The problem arises when the SBMLDocument tries to clean up for them (at the end of the program).

fbergmann commented 1 year ago

indeed, thus my question what used the SBMLtransforms, in your case, so we could track and delete stuff there.

anyways ... going again with the order of initialisation theory. it seems that in your case first there is an SBMLtransform init, then SBML Registry ... lets try one more where the registry somehow inits the map before the atexit call .. not sure if NULL is fine but we'll see: 

diff --git a/src/sbml/extension/SBMLExtensionRegistry.cpp b/src/sbml/extension/SBMLExtensionRegistry.cpp
index 1c20e7a20..1ab8a0f5a 100644
--- a/src/sbml/extension/SBMLExtensionRegistry.cpp
+++ b/src/sbml/extension/SBMLExtensionRegistry.cpp
@@ -50,6 +50,7 @@
 #include <string>

 #include <sbml/extension/RegisterExtensions.h>
+#include <sbml/SBMLTransforms.h>

 #ifdef __cplusplus

@@ -75,12 +76,17 @@ SBMLExtensionRegistry::deleteRegistry()
     mInstance = NULL;
     registered = false;
   }
+
+  SBMLTransforms::clearComponentValues();
 }

 /** @cond doxygenLibsbmlInternal */
 SBMLExtensionRegistry& 
 SBMLExtensionRegistry::getInstance()
 {
+
+  SBMLTransforms::getComponentValues(NULL);
+  
   if (mInstance == NULL)
   {
     mInstance = new SBMLExtensionRegistry();
luciansmith commented 1 year ago

Heh, nope. The getComponentValues(NULL) crashes at startup.

    test_antimony.exe!std::_Tree<std::_Tmap_traits<libsbml::Model const *,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>,std::less<libsbml::Model const *>,std::allocator<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>,0>>::_Find_lower_bound<libsbml::Model const *>(const libsbml::Model * const & _Keyval) Line 1597    C++
    test_antimony.exe!std::map<libsbml::Model const *,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>,std::less<libsbml::Model const *>,std::allocator<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>>::_Try_emplace<libsbml::Model const * const &>(const libsbml::Model * const & _Keyval) Line 175 C++
    test_antimony.exe!std::map<libsbml::Model const *,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>,std::less<libsbml::Model const *>,std::allocator<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>>::operator[](const libsbml::Model * const & _Keyval) Line 323   C++
>   test_antimony.exe!libsbml::SBMLTransforms::getComponentValues(const libsbml::Model * m) Line 529    C++
    test_antimony.exe!libsbml::SBMLExtensionRegistry::getInstance() Line 85 C++
    test_antimony.exe!libsbml::SBase::loadPlugins(libsbml::SBMLNamespaces * sbmlns) Line 620    C++
    test_antimony.exe!libsbml::SBMLDocument::SBMLDocument(libsbml::SBMLNamespaces * sbmlns) Line 187    C++
    test_antimony.exe!Module::Module(std::string name) Line 58  C++
    test_antimony.exe!Registry::NewCurrentModule(const std::string * name, const std::string * displayname, bool ismain) Line 913   C++
    test_antimony.exe!Registry::Registry() Line 58  C++
    test_antimony.exe!`dynamic initializer for 'g_registry''() Line 56  C++
fbergmann commented 1 year ago

ok, can you please just add a if (m == NULL) return; at the top of SBMLTransforms::getComponentValues and try again? its just for testing right now anyways.

luciansmith commented 1 year ago

Done! And same phenotype as without the call; crash at:

>   test_antimony.exe!std::_Tree_val<std::_Tree_simple_types<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>>::_Orphan_ptr(std::_Tree_node<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>,void *> * const _Ptr) Line 718  C++
    test_antimony.exe!std::_Tree<std::_Tmap_traits<libsbml::Model const *,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>,std::less<libsbml::Model const *>,std::allocator<std::pair<libsbml::Model const * const,std::map<std::string const ,std::pair<double,bool>,std::less<std::string const>,std::allocator<std::pair<std::string const ,std::pair<double,bool>>>>>>,0>>::clear() Line 1342  C++
    test_antimony.exe!libsbml::SBMLTransforms::clearComponentValues(const libsbml::Model * m) Line 558  C++
    test_antimony.exe!libsbml::SBMLExtensionRegistry::deleteRegistry() Line 79  C++
fbergmann commented 1 year ago

well, we either need an antimony-less test case in libsbml to sort this, or you tell me which converters / evaluations were called, so we can clean up the memory sooner.

for giggles, if you attend an atexit call to antimony (like super soon before there is any registry stuff going on) that would clear the SBMLTransforms maps .. would that help?

luciansmith commented 1 year ago

I think it would allow the 'empty' hack, but not actually solve the problem, if the 'empty' hack stopped working in the future?

As it is, for Antimony in particular, all I need to do is use a branch of libsbml that doesn't call 'clearComponents' from the SBMLDocument destructor, and I'm fine. I never use the SBMLTransforms infrastructure at all, otherwise, so I don't even accumulate wasted memory.

It would also 'work' for me to use a version of libsbml with the 'empty' hack in it, though I worry that'll stop working in the future, so am inclined towards the 'don't call clear components from the document destructor' version.

I'm aware that we only have so many resources available to us right now, and this is indeed kind of an edge case. I'll see if I can create a libsbml-only version that demonstrates the problem.

Incidentally: there seems to no longer be a 'WITH_CHECK' option in CMake (at least not one listed in cmake-gui). How do you turn on 'build the tests' in libsbml nowadays?

fbergmann commented 1 year ago

the deletion in the destructor tried to avoid the scenario where you have gazillions of documents processed and all still in memory. Clearly the converters called, should clear up their stuff once they are done. so that ought to be fixed.

I'm not sure why the empty thing was needed for you, since erase on non-existing object according to C++ std should not do anything. (Neither should it crash with accessing on non-existing entry, nor should find). All your findings are puzzling and will need some changes. Is there a way to run your azure tests on other platforms but windows, just to see what they turn up?

as for with_check .. that does seem to be a thing. I never used a cmake gui and did not notice. For me running a check would be configured as before with cmake -DWITH_CHECK=ON ... thanks for noticing.

luciansmith commented 1 year ago

The other platform azure builds had no problems at all. It was only Windows/Debug that had the problem.

Thanks for letting me know WITH_CHECK still existed! I guess it's no longer initialized by default, or something.

skeating commented 2 months ago

Did this get resolved?

I just ran a program that read in a document, created the sbml transform map, deleted the map and then deleted the document with no issues

luciansmith commented 2 months ago

Nope! My antimony test program still crashes on SBMLTransforms::clearComponentValues(); I just deleted that line in my branch, and have been using that.

luciansmith commented 2 months ago

Frank's

if (mModelValues.empty())
   return;

trick does seem to work in my case, but it wouldn't work if I ever used mModelValues, and I'm suspicious of it anyway; I think it happens to work when reading a stale pointer.