Hi Robert,
>From our SVN log I've extracted a unified diff of the modifications I've
had to make on our branch of OSG (i.e. a stabilized trunk snapshot taken
around OSG 2.9.3). This should give a fair idea of the places where I
found a potentially dangerous getenv(). See attached file.
Be careful! It's not impossible I've missed a couple (especially in
other modules than osg.dll).
But as importantly, I've probably commented out a couple false positives
(i.e. getenv() that are perfectly safe). I can probably help you making
a more exhaustive and safer list by using the debugger to see exactly
which getenv() are really getting called in DllMain() (note that's what
I already did for most, but not all).
> Init is usually done single threaded, is there something specific
> about the threading model you have that concerns you? There may be
> cases which threading may be an issue, but most cases I'd guess as not
> being threaded, we'll want to avoid adding mutex locks across the
> board.
The problem with the getSingleton() solution is that you cannot tell
when they will be called. Hence, even with the best intentions in the
world you cannot guarantee that two threads won't call getSingleton() at
the same time. I'm afraid that if we don't make the getSingleton()
functions thread-safe, the fix will actually introduce more threading
bugs than it actually fixes.
I guess that what is needed to reduce the impact of locking in
getSingleton is to:
1. Check whether a particular getSingleton() is potentially called
frequently (if it is not, we can probably afford the lock).
2. Check whether the call to getSingleton() can be cached by the
object(s) using it. For example, if an object needs to call getSingleton
quite enough, then it should call it in its constructor and cache the
returned address/reference in a member variable.
3. See whether the call to getSingleton() is really necessary and
whether it could be moved somewhere else where it does not have to be
called that often. (same idea as 2.)
Cheers,
Tanguy
-----Original Message-----
From: [email protected]
[mailto:[email protected]] On Behalf Of Robert
Osfield
Sent: 07 August 2009 15:51
To: OpenSceneGraph Users
Subject: Re: [osg-users] Deadlock when loading osg.dll, singletons are
evil
Hi Tanguy,
On Fri, Aug 7, 2009 at 3:14 PM, Tanguy
Fautre<[email protected]> wrote:
> I had the chance to test your patch. As expected, it fixes the
problem.
> The debugger shows the singleton is only initialized well after
DllMain
> has been called. No deadlock to report.
Great, a good first step then.
> As you stated, we need to fix all the getenv() that can be called from
> constructing a global singleton. We've seen our app deadlocking on
them
> before.
With your other work I presume you've not enumerated all the files
that have this global static's that use getenv? Do you have a list?
My thought is to look at the usage pattern and then come up with
standard implementation pattern for these variables. Clearly it'll
involve a singleton access method/function, but this might be
something we can use macro's or templates to help keep regular.
> Not sure if you want to apply the patch as it stands though, as
> thread-safety is quite important to us.
Init is usually done single threaded, is there something specific
about the threading model you have that concerns you? There may be
cases which threading may be an issue, but most cases I'd guess as not
being threaded, we'll want to avoid adding mutex locks across the
board.
Robert.
_______________________________________________
osg-users mailing list
[email protected]
http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.or
g
Index:
D:/svn/ThirdParty/build/OpenSceneGraph-2.9.4-vc8/OpenSceneGraph/src/osgUtil/RenderBin.cpp
===================================================================
---
D:/svn/ThirdParty/build/OpenSceneGraph-2.9.4-vc8/OpenSceneGraph/src/osgUtil/RenderBin.cpp
(revision 282)
+++
D:/svn/ThirdParty/build/OpenSceneGraph-2.9.4-vc8/OpenSceneGraph/src/osgUtil/RenderBin.cpp
(revision 287)
@@ -114,7 +114,12 @@
if (!s_defaultBinSortModeInitialized)
{
s_defaultBinSortModeInitialized = true;
-
+
+ /* ARIS: this function could be called in DllMain due to
singletons construction.
+ * Unfortunately, getenv() uses an internal lock, which causes a
deadlock (see DllMain doc).
+ * http://forum.openscenegraph.org/viewtopic.php?p=15636
+ */
+ /*
const char* str = getenv("OSG_DEFAULT_BIN_SORT_MODE");
if (str)
{
@@ -123,6 +128,7 @@
else if (strcmp(str,"SORT_FRONT_TO_BACK")==0) s_defaultBinSortMode
= RenderBin::SORT_FRONT_TO_BACK;
else if (strcmp(str,"SORT_BACK_TO_FRONT")==0) s_defaultBinSortMode
= RenderBin::SORT_BACK_TO_FRONT;
}
+ */
}
return s_defaultBinSortMode;
Index:
D:/svn/ThirdParty/build/OpenSceneGraph-2.9.4-vc8/OpenSceneGraph/src/osg/Texture.cpp
===================================================================
---
D:/svn/ThirdParty/build/OpenSceneGraph-2.9.4-vc8/OpenSceneGraph/src/osg/Texture.cpp
(revision 282)
+++
D:/svn/ThirdParty/build/OpenSceneGraph-2.9.4-vc8/OpenSceneGraph/src/osg/Texture.cpp
(revision 287)
@@ -1683,8 +1683,12 @@
glGetIntegerv(GL_MAX_TEXTURE_SIZE,&_maxTextureSize);
+ /* ARIS: this function could be called in DllMain due to singletons
construction.
+ * Unfortunately, getenv() uses an internal lock, which causes a
deadlock (see DllMain doc).
+ * http://forum.openscenegraph.org/viewtopic.php?p=15636
+ */
char *ptr;
- if( (ptr = getenv("OSG_MAX_TEXTURE_SIZE")) != 0)
+ if( (ptr = 0/*getenv("OSG_MAX_TEXTURE_SIZE")*/) != 0)
{
GLint osg_max_size = atoi(ptr);
Index:
D:/svn/ThirdParty/build/OpenSceneGraph-2.9.4-vc8/OpenSceneGraph/src/osg/Notify.cpp
===================================================================
---
D:/svn/ThirdParty/build/OpenSceneGraph-2.9.4-vc8/OpenSceneGraph/src/osg/Notify.cpp
(revision 282)
+++
D:/svn/ThirdParty/build/OpenSceneGraph-2.9.4-vc8/OpenSceneGraph/src/osg/Notify.cpp
(revision 287)
@@ -48,8 +48,12 @@
g_NotifyLevel = osg::NOTICE; // Default value
- char* OSGNOTIFYLEVEL=getenv("OSG_NOTIFY_LEVEL");
- if (!OSGNOTIFYLEVEL) OSGNOTIFYLEVEL=getenv("OSGNOTIFYLEVEL");
+ /* ARIS: this function could be called in DllMain due to singletons
construction.
+ * Unfortunately, getenv() uses an internal lock, which causes a
deadlock (see DllMain doc).
+ * http://forum.openscenegraph.org/viewtopic.php?p=15636
+ */
+ char* OSGNOTIFYLEVEL=0;//getenv("OSG_NOTIFY_LEVEL");
+ if (!OSGNOTIFYLEVEL) OSGNOTIFYLEVEL=0;//getenv("OSGNOTIFYLEVEL");
if(OSGNOTIFYLEVEL)
{
Index:
D:/svn/ThirdParty/build/OpenSceneGraph-2.9.4-vc8/OpenSceneGraph/src/osg/GLExtensions.cpp
===================================================================
---
D:/svn/ThirdParty/build/OpenSceneGraph-2.9.4-vc8/OpenSceneGraph/src/osg/GLExtensions.cpp
(revision 282)
+++
D:/svn/ThirdParty/build/OpenSceneGraph-2.9.4-vc8/OpenSceneGraph/src/osg/GLExtensions.cpp
(revision 287)
@@ -39,7 +39,11 @@
static osg::buffered_object<std::string> s_gluRendererList;
static osg::buffered_value<int> s_gluInitializedList;
-static const char* envVar = getenv("OSG_GL_EXTENSION_DISABLE");
+/* ARIS: this function is being call in DllMain due to singletons construction.
+ * Unfortunately, getenv() uses an internal lock, which causes a deadlock (see
DllMain doc).
+ * http://forum.openscenegraph.org/viewtopic.php?p=15636
+ */
+static const char* envVar = 0; //getenv("OSG_GL_EXTENSION_DISABLE");
static std::string s_GLExtensionDisableString(envVar?envVar:"Nothing defined");
float osg::getGLVersionNumber()
Index:
D:/svn/ThirdParty/build/OpenSceneGraph-2.9.4-vc8/OpenSceneGraph/src/osg/Geometry.cpp
===================================================================
---
D:/svn/ThirdParty/build/OpenSceneGraph-2.9.4-vc8/OpenSceneGraph/src/osg/Geometry.cpp
(revision 282)
+++
D:/svn/ThirdParty/build/OpenSceneGraph-2.9.4-vc8/OpenSceneGraph/src/osg/Geometry.cpp
(revision 287)
@@ -903,6 +903,11 @@
bool Geometry::computeFastPathsUsed()
{
+ /* ARIS: this function could be called in DllMain due to singletons
construction.
+ * Unfortunately, getenv() uses an internal lock, which causes a
deadlock (see DllMain doc).
+ * http://forum.openscenegraph.org/viewtopic.php?p=15636
+ */
+ /*
static bool s_DisableFastPathInDisplayLists =
getenv("OSG_DISABLE_FAST_PATH_IN_DISPLAY_LISTS")!=0;
if (_useDisplayList && s_DisableFastPathInDisplayLists)
{
@@ -910,6 +915,7 @@
_supportsVertexBufferObjects = _fastPath = false;
return _fastPath;
}
+ */
///////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
//
Index:
D:/svn/ThirdParty/build/OpenSceneGraph-2.9.4-vc8/OpenSceneGraph/src/osg/GraphicsContext.cpp
===================================================================
---
D:/svn/ThirdParty/build/OpenSceneGraph-2.9.4-vc8/OpenSceneGraph/src/osg/GraphicsContext.cpp
(revision 282)
+++
D:/svn/ThirdParty/build/OpenSceneGraph-2.9.4-vc8/OpenSceneGraph/src/osg/GraphicsContext.cpp
(revision 287)
@@ -100,11 +100,17 @@
void GraphicsContext::ScreenIdentifier::readDISPLAY()
{
+ /* ARIS: this function could be called in DllMain due to singletons
construction.
+ * Unfortunately, getenv() uses an internal lock, which causes a
deadlock (see DllMain doc).
+ * http://forum.openscenegraph.org/viewtopic.php?p=15636
+ */
+ /*
const char* ptr = 0;
if ((ptr=getenv("DISPLAY")) != 0)
{
setScreenIdentifier(ptr);
}
+ */
}
void GraphicsContext::ScreenIdentifier::setScreenIdentifier(const std::string&
displayName)
Index:
D:/svn/ThirdParty/build/OpenSceneGraph-2.9.4-vc8/OpenSceneGraph/src/osg/State.cpp
===================================================================
---
D:/svn/ThirdParty/build/OpenSceneGraph-2.9.4-vc8/OpenSceneGraph/src/osg/State.cpp
(revision 282)
+++
D:/svn/ThirdParty/build/OpenSceneGraph-2.9.4-vc8/OpenSceneGraph/src/osg/State.cpp
(revision 287)
@@ -47,7 +47,11 @@
_checkGLErrors = ONCE_PER_FRAME;
- const char* str = getenv("OSG_GL_ERROR_CHECKING");
+ /* ARIS: this function could be called in DllMain due to singletons
construction.
+ * Unfortunately, getenv() uses an internal lock, which causes a
deadlock (see DllMain doc).
+ * http://forum.openscenegraph.org/viewtopic.php?p=15636
+ */
+ const char* str = 0;//getenv("OSG_GL_ERROR_CHECKING");
if (str && (strcmp(str,"ONCE_PER_ATTRIBUTE")==0 || strcmp(str,"ON")==0 ||
strcmp(str,"on")==0))
{
_checkGLErrors = ONCE_PER_ATTRIBUTE;
Index:
D:/svn/ThirdParty/build/OpenSceneGraph-2.9.4-vc8/OpenSceneGraph/src/osg/DisplaySettings.cpp
===================================================================
---
D:/svn/ThirdParty/build/OpenSceneGraph-2.9.4-vc8/OpenSceneGraph/src/osg/DisplaySettings.cpp
(revision 282)
+++
D:/svn/ThirdParty/build/OpenSceneGraph-2.9.4-vc8/OpenSceneGraph/src/osg/DisplaySettings.cpp
(revision 287)
@@ -188,6 +188,12 @@
void DisplaySettings::readEnvironmentalVariables()
{
+ /* ARIS: this function is being call in DllMain due to singletons
construction.
+ * Unfortunately, getenv() uses an internal lock, which causes a
deadlock (see DllMain doc).
+ * http://forum.openscenegraph.org/viewtopic.php?p=15636
+ */
+
+ /*
const char* ptr = 0;
if ((ptr = getenv("OSG_DISPLAY_TYPE")) != 0)
@@ -380,7 +386,7 @@
if( (ptr = getenv("OSG_MULTI_SAMPLES")) != 0)
{
_numMultiSamples = atoi(ptr);
- }
+ }*/
}
void DisplaySettings::readCommandLine(ArgumentParser& arguments)
Index:
D:/svn/ThirdParty/build/OpenSceneGraph-2.9.4-vc8/OpenSceneGraph/src/osgDB/Output.cpp
===================================================================
---
D:/svn/ThirdParty/build/OpenSceneGraph-2.9.4-vc8/OpenSceneGraph/src/osgDB/Output.cpp
(revision 282)
+++
D:/svn/ThirdParty/build/OpenSceneGraph-2.9.4-vc8/OpenSceneGraph/src/osgDB/Output.cpp
(revision 287)
@@ -56,11 +56,17 @@
_writeOutDefaultValues = false;
+ /* ARIS: this function could be called in DllMain due to singletons
construction.
+ * Unfortunately, getenv() uses an internal lock, which causes a
deadlock (see DllMain doc).
+ * http://forum.openscenegraph.org/viewtopic.php?p=15636
+ */
+ /*
const char* env = getenv("OSG_WRITE_OUT_DEFAULT_VALUES");
if (env)
{
_writeOutDefaultValues = strcmp(env,"ON")==0;
}
+ */
}
void Output::setOptions(const ReaderWriter::Options* options)
Index:
D:/svn/ThirdParty/build/OpenSceneGraph-2.9.4-vc8/OpenSceneGraph/src/osgDB/DatabasePager.cpp
===================================================================
---
D:/svn/ThirdParty/build/OpenSceneGraph-2.9.4-vc8/OpenSceneGraph/src/osgDB/DatabasePager.cpp
(revision 282)
+++
D:/svn/ThirdParty/build/OpenSceneGraph-2.9.4-vc8/OpenSceneGraph/src/osgDB/DatabasePager.cpp
(revision 287)
@@ -808,6 +808,11 @@
_numFramesActive = 0;
_frameNumber = 0;
+ /* ARIS: this function could be called in DllMain due to singletons
construction.
+ * Unfortunately, getenv() uses an internal lock, which causes a
deadlock (see DllMain doc).
+ * http://forum.openscenegraph.org/viewtopic.php?p=15636
+ */
+ /*
const char* str = getenv("OSG_DATABASE_PAGER_PRIORITY");
if (str)
{
@@ -836,6 +841,7 @@
setSchedulePriority(OpenThreads::Thread::THREAD_PRIORITY_MAX);
}
}
+ */
#if __APPLE__
// OSX really doesn't like compiling display lists, and performs poorly
when they are used,
@@ -845,6 +851,11 @@
_drawablePolicy = DO_NOT_MODIFY_DRAWABLE_SETTINGS;
#endif
+ /* ARIS: this function could be called in DllMain due to singletons
construction.
+ * Unfortunately, getenv() uses an internal lock, which causes a
deadlock (see DllMain doc).
+ * http://forum.openscenegraph.org/viewtopic.php?p=15636
+ */
+ /*
str = getenv("OSG_DATABASE_PAGER_GEOMETRY");
if (!str) str = getenv("OSG_DATABASE_PAGER_DRAWABLE");
if (str)
@@ -866,6 +877,7 @@
_drawablePolicy = USE_VERTEX_ARRAYS;
}
}
+ */
_changeAnisotropy = false;
_valueAnisotropy = 1.0f;
@@ -875,27 +887,50 @@
const char* ptr=0;
_deleteRemovedSubgraphsInDatabaseThread = true;
+ /* ARIS: this function could be called in DllMain due to singletons
construction.
+ * Unfortunately, getenv() uses an internal lock, which causes a
deadlock (see DllMain doc).
+ * http://forum.openscenegraph.org/viewtopic.php?p=15636
+ */
+ /*
if( (ptr = getenv("OSG_DELETE_IN_DATABASE_THREAD")) != 0)
{
_deleteRemovedSubgraphsInDatabaseThread = strcmp(ptr,"yes")==0 ||
strcmp(ptr,"YES")==0 ||
strcmp(ptr,"on")==0 || strcmp(ptr,"ON")==0;
}
+ */
_expiryDelay = 10.0;
- if( (ptr = getenv("OSG_EXPIRY_DELAY")) != 0)
+ /* ARIS: this function could be called in DllMain due to singletons
construction.
+ * Unfortunately, getenv() uses an internal lock, which causes a
deadlock (see DllMain doc).
+ * http://forum.openscenegraph.org/viewtopic.php?p=15636
+ */
+ /*
+ if( (ptr = getenv("OSG_EXPIRY_DELAY")) != 0)
{
_expiryDelay = atof(ptr);
osg::notify(osg::NOTICE)<<"Expiry delay = "<<_expiryDelay<<std::endl;
}
+ */
_expiryFrames = 1; // Last frame will not be expired
- if( (ptr = getenv("OSG_EXPIRY_FRAMES")) != 0)
+ /* ARIS: this function could be called in DllMain due to singletons
construction.
+ * Unfortunately, getenv() uses an internal lock, which causes a
deadlock (see DllMain doc).
+ * http://forum.openscenegraph.org/viewtopic.php?p=15636
+ */
+ /*
+ if( (ptr = getenv("OSG_EXPIRY_FRAMES")) != 0)
{
_expiryFrames = atoi(ptr);
osg::notify(osg::NOTICE)<<"Expiry frames = "<<_expiryFrames<<std::endl;
}
+ */
+ /* ARIS: this function could be called in DllMain due to singletons
construction.
+ * Unfortunately, getenv() uses an internal lock, which causes a
deadlock (see DllMain doc).
+ * http://forum.openscenegraph.org/viewtopic.php?p=15636
+ */
+ /*
if( (ptr = getenv("OSG_RELEASE_DELAY")) != 0)
{
if (strcmp(ptr,"OFF")==0 || strcmp(ptr,"Off")==0 ||
strcmp(ptr,"off")==0)
@@ -910,37 +945,60 @@
osg::notify(osg::NOTICE)<<"Release delay = "<<_releaseDelay<<std::endl;
}
else
+ */
{
setReleaseDelay(DBL_MAX);
}
_releaseFrames = 1; // Last frame will not be release
+ /* ARIS: this function could be called in DllMain due to singletons
construction.
+ * Unfortunately, getenv() uses an internal lock, which causes a
deadlock (see DllMain doc).
+ * http://forum.openscenegraph.org/viewtopic.php?p=15636
+ */
+ /*
if( (ptr = getenv("OSG_RELEASE_FRAMES")) != 0)
{
_releaseFrames = atoi(ptr);
osg::notify(osg::NOTICE)<<"Release frames =
"<<_releaseFrames<<std::endl;
}
+ */
_targetMaximumNumberOfPageLOD = 300;
+ /* ARIS: this function could be called in DllMain due to singletons
construction.
+ * Unfortunately, getenv() uses an internal lock, which causes a
deadlock (see DllMain doc).
+ * http://forum.openscenegraph.org/viewtopic.php?p=15636
+ */
+ /*
if( (ptr = getenv("OSG_MAX_PAGEDLOD")) != 0)
{
_targetMaximumNumberOfPageLOD = atoi(ptr);
osg::notify(osg::NOTICE)<<"_targetMaximumNumberOfPageLOD =
"<<_targetMaximumNumberOfPageLOD<<std::endl;
}
+ */
-
_doPreCompile = false;
+ /* ARIS: this function could be called in DllMain due to singletons
construction.
+ * Unfortunately, getenv() uses an internal lock, which causes a
deadlock (see DllMain doc).
+ * http://forum.openscenegraph.org/viewtopic.php?p=15636
+ */
+ /*
if( (ptr = getenv("OSG_DO_PRE_COMPILE")) != 0)
{
_doPreCompile = strcmp(ptr,"yes")==0 || strcmp(ptr,"YES")==0 ||
strcmp(ptr,"on")==0 || strcmp(ptr,"ON")==0;
}
+ */
_targetFrameRate = 100.0;
_minimumTimeAvailableForGLCompileAndDeletePerFrame = 0.001; // 1ms.
_maximumNumOfObjectsToCompilePerFrame = 4;
+ /* ARIS: this function could be called in DllMain due to singletons
construction.
+ * Unfortunately, getenv() uses an internal lock, which causes a
deadlock (see DllMain doc).
+ * http://forum.openscenegraph.org/viewtopic.php?p=15636
+ */
+ /*
if( (ptr = getenv("OSG_MINIMUM_COMPILE_TIME_PER_FRAME")) != 0)
{
_minimumTimeAvailableForGLCompileAndDeletePerFrame = atof(ptr);
@@ -950,6 +1008,7 @@
{
_maximumNumOfObjectsToCompilePerFrame = atoi(ptr);
}
+ */
// initialize the stats variables
resetStats();
_______________________________________________
osg-users mailing list
[email protected]
http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org
