WIP: Eliminate a "C++ initialization order fiasco" for geometryregister #125
Conversation
Current initialization of the global geometryregister suffers from a classic 'initialization order fiasco'. Depending on the order the compilation units are loaded/linked, the initialization of the global geometryregisterarray is not guaranteed to happen (and indeed often does not happen) before it is used. This leads to entries being appended before it's initialized (usually 'suceeding, but potentially causing memory corruption if the segment at that point isn't zeroed), initialization then happening halfway through (wiping the initial entries) and then the last entries being the only ones that show up. The net effect is either a crash at startup, or several geometry types seeming to be missing. Eg, step files will oad, but STL files are just ignored. The bug is actively observed on, eg, Linux. This patch implements a simple 'initialize at first access' convention for the array, eliminating the ordering problem. I've not reviewed the rest of the source for other potential examples of the fiasco pattern; this fixes only the geometryregister, since that was actively biting.
| } | ||
|
|
||
| virtual shared_ptr<NetgenGeometry> LoadFromMeshFile (istream & ist) const; |
There was a problem hiding this comment.
LoadFromMeshFile is not reimplemented, but delegated to the registered GeometryRegister::LoadFromMeshFile.
| virtual shared_ptr<NetgenGeometry> LoadFromMeshFile (istream & ist) const; | |
| shared_ptr<NetgenGeometry> LoadFromMeshFile (istream & ist) const; |
| public: | ||
| virtual ~GeometryRegisterArray() |
There was a problem hiding this comment.
Avoid accidental misuse (copying) of the singleton.
Make non-virtual, singleton should never be reimplemented.
| public: | |
| virtual ~GeometryRegisterArray() | |
| public: | |
| GeometryRegisterArray(const GeometryRegisterArray &) = delete; | |
| GeometryRegisterArray & operator=(const GeometryRegisterArray &) = delete; | |
| private: | |
| GeometryRegisterArray() = default; | |
| ~GeometryRegisterArray() = default; |
|
This pattern is known as Meyers Singleton (after the C++ guru Scott Meyers). For a small overview of thread-safe singleton implementations, see https://www.modernescpp.com/index.php/thread-safe-initialization-of-a-singleton. Bottom line:
|
| if(stlgeometry){ // don't let the default initializer crash init | ||
| stlgeometry->SetSelectTrig(selecttrig); | ||
| stlgeometry->SetNodeOfSelTrig(nodeofseltrig); | ||
| } |
There was a problem hiding this comment.
This change is unrelated, and incorrect/insufficient (dito below).
stlgeometry is a private member of VisualSceneSTLMeshing, and it must be a nullptr here. (It unfortunately lacks an initializer, and thus may point to arbitrary memory). stlgeometry can only be set with SetGeometry().
See #128
xiphmont
changed the title
Current initialization of the global geometryregister suffers from a
classic 'initialization order fiasco'. Depending on the order the
compilation units are loaded/linked, the initialization of the global
GeometryRegisterArray is not guaranteed to happen (and indeed often
does not happen) before it is used. This leads to entries being
appended before it's initialized (usually 'succeeding', but potentially
causing memory corruption if the segment at that point isn't zeroed),
initialization then happening halfway through (wiping the initial
entries) and then the last entries being the only ones that show up.
The net effect is either a crash at startup, or several geometry types
seeming to be missing. Eg, step files will oad, but STL files are
just ignored. The bug is actively observed on, eg, Linux.
This patch implements a simple 'initialize at first access' convention
for the array, eliminating the ordering problem.
I've not reviewed the rest of the source for other potential examples
of the fiasco pattern; this fixes only the global geometryregister, since
that was actively biting.