Deleting a document crashes when SBMLTransforms is already gone #320
Comments
|
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? |
|
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. This is at line 1687 of xtree Here's the stack trace:
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> |
|
If it helps: I could also just send an executable that crashes, if that would help? |
|
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 specifiedthank you |
|
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. |
|
the object was not deleted, you saw in your stack trace, that there were no objects in side: 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 |
|
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:
and here's what can be gleaned from mModelValues: The 'Error' is the sort of thing I often see on technically-deleted-but-the-residue-is-still-in-memory objects. |
|
did you at the 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. |
|
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? |
|
well if whatever happened on the registry object destructor was moved into a function, that could be called |
|
The SBML Document itself is also just an object child of the registry; not a pointer. |
|
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? |
|
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? |
|
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:
This from a version of libsbml that doesn't call 'clearComponentValues' when deleting an SBMLDocument. At that point, mModelValues looks like this: |
|
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. |
|
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). |
|
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 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();
|
|
Heh, nope. The getComponentValues(NULL) crashes at startup. |
|
ok, can you please just add a |
|
Done! And same phenotype as without the call; crash at: |
|
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 |
|
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? |
|
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 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 |
|
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. |
|
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 |
|
Nope! My antimony test program still crashes on SBMLTransforms::clearComponentValues(); I just deleted that line in my branch, and have been using that. |
|
Frank's 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. |
I'm getting crashes at the end of my program when an SBML document is deleted, at the line:
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?
The text was updated successfully, but these errors were encountered: