From b09757bdb8a759bf7d3ed353719f59024aa445a2 Mon Sep 17 00:00:00 2001 From: Robert Osfield Date: Thu, 18 Feb 2010 21:21:12 +0000 Subject: [PATCH] Refactored the osg::Referenced observerset code so that it now uses a dedicated ObserverSet class, which utilises a global recursive mutex that is dedicated to manage Observer and ObserverSet. The new global mutex for observers avoids problems with deadlocks that were occurring previously when an osg::Refenced object was being deleted at the same time as on osg::ObserverNodePath. --- include/osg/Observer | 65 +++++++-- include/osg/ObserverNodePath | 1 - include/osg/Referenced | 36 +++-- src/osg/CMakeLists.txt | 1 + src/osg/Observer.cpp | 71 ++++++++++ src/osg/ObserverNodePath.cpp | 10 +- src/osg/Referenced.cpp | 265 +++++++++++------------------------ 7 files changed, 234 insertions(+), 215 deletions(-) create mode 100644 src/osg/Observer.cpp diff --git a/include/osg/Observer b/include/osg/Observer index 8faaa41a2..ebd3037d7 100644 --- a/include/osg/Observer +++ b/include/osg/Observer @@ -11,28 +11,63 @@ * OpenSceneGraph Public License for more details. */ -#ifndef OSG_OCCLUDER -#define OSG_OCCLUDER 1 +#ifndef OSG_OBSERVER +#define OSG_OBSERVER 1 + +#include +#include +#include namespace osg { /** Observer base class for tracking when objects are unreferenced (there reference count goes to 0) and are being deleted.*/ -class Observer +class OSG_EXPORT Observer { -public: - virtual ~Observer() {} + public: + virtual ~Observer() {} - /** objectUnreferenced(void*) is called when the observed object's referenced count goes to zero, indicating that - * the object will be deleted unless a new reference is made to it. If you wish to prevent deletion of the object - * then it's reference count should be incremented such as via taking a ref_ptr<> to it, if no refernce is taken - * by any of the observers of the object then the object will be deleted, and objectDeleted will in turn be called. - * return true if the Observer wishes to removed from the oberseved objects observer set.*/ - virtual bool objectUnreferenced(void*) { return false; } + /** Get the optional global observer mutex, this can be shared between all osg::Observer.*/ + static OpenThreads::Mutex* getGlobalObserverMutex(); - /** objectDeleted is called when the observed object is about to be deleted. The observer will be automatically - * removed from the observerd objects observer set so there is no need for the objectDeleted implementation - * to call removeObserver() on the observed object. */ - virtual void objectDeleted(void*) {} + inline OpenThreads::Mutex* getObserverMutex() const { return osg::Observer::getGlobalObserverMutex(); } + + /** objectUnreferenced(void*) is called when the observed object's referenced count goes to zero, indicating that + * the object will be deleted unless a new reference is made to it. If you wish to prevent deletion of the object + * then it's reference count should be incremented such as via taking a ref_ptr<> to it, if no refernce is taken + * by any of the observers of the object then the object will be deleted, and objectDeleted will in turn be called. + * return true if the Observer wishes to removed from the oberseved objects observer set.*/ + virtual bool objectUnreferenced(void*) { return false; } + + /** objectDeleted is called when the observed object is about to be deleted. The observer will be automatically + * removed from the observerd objects observer set so there is no need for the objectDeleted implementation + * to call removeObserver() on the observed object. */ + virtual void objectDeleted(void*) {} + +}; + +/** Class used by osg::Referenced to track the observers assoicated with it.*/ +class OSG_EXPORT ObserverSet +{ + public: + + ObserverSet(); + ~ObserverSet(); + + inline OpenThreads::Mutex* getObserverSetMutex() const { return osg::Observer::getGlobalObserverMutex(); } + + void addObserver(Observer* observer); + void removeObserver(Observer* observer); + + void signalObjectUnreferenced(void* ptr); + void signalObjectDeleted(void* ptr); + + typedef std::set Observers; + Observers& getObservers() { return _observers; } + const Observers& getObservers() const { return _observers; } + + protected: + + Observers _observers; }; } diff --git a/include/osg/ObserverNodePath b/include/osg/ObserverNodePath index 925f606cf..d79633c70 100644 --- a/include/osg/ObserverNodePath +++ b/include/osg/ObserverNodePath @@ -67,7 +67,6 @@ class OSG_EXPORT ObserverNodePath : public osg::Observer virtual bool objectUnreferenced(void* ptr); - mutable OpenThreads::Mutex _mutex; osg::NodePath _nodePath; bool _valid; }; diff --git a/include/osg/Referenced b/include/osg/Referenced index 3a0b776cc..ad6fe2743 100644 --- a/include/osg/Referenced +++ b/include/osg/Referenced @@ -14,8 +14,7 @@ #ifndef OSG_REFERENCED #define OSG_REFERENCED 1 -// When building OSG with Java need to derive from Noodle::CBridgable class, -#include +#include #include #include @@ -30,6 +29,7 @@ namespace osg { // forward declare, declared after Referenced below. class DeleteHandler; class Observer; +class ObserverSet; /** template class to help enforce static initialization order. */ template @@ -95,11 +95,25 @@ class OSG_EXPORT Referenced /** Return the number pointers currently referencing this object. */ inline int referenceCount() const { return _refCount; } - /** Add a Observer that is observing this object, notify the Observer when this object gets deleted.*/ - void addObserver(Observer* observer) const; + + /** Get the ObserverSet if one is attached, otherwise return NULL.*/ + ObserverSet* getObserverSet() const + { + #if defined(_OSG_REFERENCED_USE_ATOMIC_OPERATIONS) + return static_cast(_observerSet.get()); + #else + return static_cast(_observerSet); + #endif + } + + /** Get the ObserverSet if one is attached, otherwise create an ObserverSet, attach it, then return this newly created ObserverSet.*/ + ObserverSet* getOrCreateObserverSet() const; /** Add a Observer that is observing this object, notify the Observer when this object gets deleted.*/ - void removeObserver(Observer* observer) const; + void addObserver(Observer* observer) const { getOrCreateObserverSet()->addObserver(observer); } + + /** remove Observer that is observing this object.*/ + void removeObserver(Observer* observer) const { getOrCreateObserverSet()->removeObserver(observer); } public: @@ -128,9 +142,7 @@ class OSG_EXPORT Referenced void deleteUsingDeleteHandler() const; #if defined(_OSG_REFERENCED_USE_ATOMIC_OPERATIONS) - struct ObserverSetData; - - mutable OpenThreads::AtomicPtr _observerSetDataPtr; + mutable OpenThreads::AtomicPtr _observerSet; mutable OpenThreads::Atomic _refCount; #else @@ -139,7 +151,7 @@ class OSG_EXPORT Referenced mutable int _refCount; - mutable void* _observers; + mutable void* _observerSet; #endif }; @@ -163,17 +175,17 @@ inline void Referenced::ref() const inline void Referenced::unref() const { #if defined(_OSG_REFERENCED_USE_ATOMIC_OPERATIONS) - bool needDelete = (--_refCount <= 0); + bool needDelete = ((--_refCount) == 0); #else bool needDelete = false; if (_refMutex) { OpenThreads::ScopedLock lock(*_refMutex); - needDelete = (--_refCount)<=0; + needDelete = (--_refCount)==0; } else { - needDelete = (--_refCount)<=0; + needDelete = (--_refCount)==0; } #endif diff --git a/src/osg/CMakeLists.txt b/src/osg/CMakeLists.txt index d34d16396..7592c53e9 100644 --- a/src/osg/CMakeLists.txt +++ b/src/osg/CMakeLists.txt @@ -276,6 +276,7 @@ ADD_LIBRARY(${LIB_NAME} NodeVisitor.cpp Notify.cpp Object.cpp + Observer.cpp ObserverNodePath.cpp OccluderNode.cpp OcclusionQueryNode.cpp diff --git a/src/osg/Observer.cpp b/src/osg/Observer.cpp new file mode 100644 index 000000000..6c1d6a483 --- /dev/null +++ b/src/osg/Observer.cpp @@ -0,0 +1,71 @@ +/* -*-c++-*- OpenSceneGraph - Copyright (C) 1998-2006 Robert Osfield + * + * This library is open source and may be redistributed and/or modified under + * the terms of the OpenSceneGraph Public License (OSGPL) version 0.0 or + * (at your option) any later version. The full license is in LICENSE file + * included with this distribution, and on the openscenegraph.org website. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * OpenSceneGraph Public License for more details. +*/ + +#include +#include + +using namespace osg; + +ObserverSet::ObserverSet() +{ + //OSG_NOTICE<<"ObserverSet::ObserverSet() "< lock(*getObserverSetMutex()); + _observers.insert(observer); +} + +void ObserverSet::removeObserver(Observer* observer) +{ + //OSG_NOTICE<<"ObserverSet::removeObserver("< lock(*getObserverSetMutex()); + _observers.erase(observer); +} + +void ObserverSet::signalObjectUnreferenced(void* ptr) +{ + for(Observers::iterator itr = _observers.begin(); + itr != _observers.end(); + ) + { + if ((*itr)->objectUnreferenced(ptr)) + { + Observers::iterator orig_itr = itr; + ++itr; + _observers.erase(orig_itr); + } + else + { + ++itr; + } + } +} + +void ObserverSet::signalObjectDeleted(void* ptr) +{ + for(Observers::iterator itr = _observers.begin(); + itr != _observers.end(); + ++itr) + { + (*itr)->objectDeleted(ptr); + } + _observers.clear(); +} \ No newline at end of file diff --git a/src/osg/ObserverNodePath.cpp b/src/osg/ObserverNodePath.cpp index 794197264..2120c1ddd 100644 --- a/src/osg/ObserverNodePath.cpp +++ b/src/osg/ObserverNodePath.cpp @@ -82,7 +82,7 @@ void ObserverNodePath::setNodePathTo(osg::Node* node) void ObserverNodePath::setNodePath(const osg::NodePath& nodePath) { - OpenThreads::ScopedLock lock(_mutex); + OpenThreads::ScopedLock lock(*getObserverMutex()); _setNodePath(nodePath); } @@ -98,7 +98,7 @@ void ObserverNodePath::setNodePath(const osg::RefNodePath& refNodePath) void ObserverNodePath::clearNodePath() { - OpenThreads::ScopedLock lock(_mutex); + OpenThreads::ScopedLock lock(*getObserverMutex()); _clearNodePath(); } @@ -106,7 +106,7 @@ bool ObserverNodePath::getRefNodePath(RefNodePath& refNodePath) const { refNodePath.clear(); - OpenThreads::ScopedLock lock(_mutex); + OpenThreads::ScopedLock lock(*getObserverMutex()); if (!_valid) return false; for(osg::NodePath::const_iterator itr = _nodePath.begin(); @@ -123,7 +123,7 @@ bool ObserverNodePath::getNodePath(NodePath& nodePath) const { nodePath.clear(); - OpenThreads::ScopedLock lock(_mutex); + OpenThreads::ScopedLock lock(*getObserverMutex()); if (!_valid) return false; nodePath = _nodePath; return true; @@ -168,7 +168,7 @@ bool ObserverNodePath::objectUnreferenced(void* ptr) { osg::Node* node = reinterpret_cast(ptr); - OpenThreads::ScopedLock lock(_mutex); + OpenThreads::ScopedLock lock(*getObserverMutex()); _valid = false; diff --git a/src/osg/Referenced.cpp b/src/osg/Referenced.cpp index 1533e2831..ccc973fc8 100644 --- a/src/osg/Referenced.cpp +++ b/src/osg/Referenced.cpp @@ -15,7 +15,7 @@ #include #include #include -#include +#include #include #include @@ -87,14 +87,12 @@ OpenThreads::Mutex* Referenced::getGlobalReferencedMutex() return s_ReferencedGlobalMutext.get(); } -typedef std::set ObserverSet; - -#if defined(_OSG_REFERENCED_USE_ATOMIC_OPERATIONS) -struct Referenced::ObserverSetData { - OpenThreads::Mutex _mutex; - ObserverSet _observers; -}; -#endif +// we'll implement Observer::getGlobalObserverMutex() here for convinience as ResetPointer is available. +OpenThreads::Mutex* Observer::getGlobalObserverMutex() +{ + static GlobalMutexPointer s_ReferencedGlobalMutext = new OpenThreads::Mutex(OpenThreads::Mutex::MUTEX_RECURSIVE); + return s_ReferencedGlobalMutext.get(); +} #if !defined(_OSG_REFERENCED_USE_ATOMIC_OPERATIONS) static bool s_useThreadSafeReferenceCounting = getenv("OSG_THREAD_SAFE_REF_UNREF")!=0; @@ -142,12 +140,12 @@ static int s_numObjects = 0; Referenced::Referenced(): #if defined(_OSG_REFERENCED_USE_ATOMIC_OPERATIONS) - _observerSetDataPtr(0), + _observerSet(0), _refCount(0) #else _refMutex(0), _refCount(0), - _observers(0) + _observerSet(0) #endif { #if !defined(_OSG_REFERENCED_USE_ATOMIC_OPERATIONS) @@ -161,7 +159,7 @@ Referenced::Referenced(): { OpenThreads::ScopedLock lock(getNumObjectMutex()); ++s_numObjects; - osg::notify(osg::NOTICE)<<"Object created, total num="<(_observerSet.get()); +#else + if (_observerSet) delete static_cast(_observerSet); +#endif +} + +ObserverSet* Referenced::getOrCreateObserverSet() const +{ +#if defined(_OSG_REFERENCED_USE_ATOMIC_OPERATIONS) + ObserverSet* observerSet = static_cast(_observerSet.get()); + while (0 == observerSet) { + ObserverSet* newObserverSet = new ObserverSet; + if (!_observerSet.assign(newObserverSet, 0)) + delete newObserverSet; + observerSet = static_cast(_observerSet.get()); + } + return observerSet; +#else + if (_refMutex) + { + OpenThreads::ScopedLock lock(*_refMutex); + if (!_observerSet) _observerSet = new ObserverSet; + return static_cast(_observerSet); + } + else + { + if (!_observerSet) _observerSet = new ObserverSet; + return static_cast(_observerSet); + } +#endif } void Referenced::signalObserversAndDelete(bool signalUnreferened, bool signalDelete, bool doDelete) const { - if (signalUnreferened) +#if defined(_OSG_REFERENCED_USE_ATOMIC_OPERATIONS) + ObserverSet* observerSet = static_cast(_observerSet.get()); +#else + ObserverSet* observerSet = static_cast(_observerSet); +#endif + + if (observerSet) { - // tell all observers that we have been unreferenced so that they - // can do clean up or add their own reference to prevent deletion. - #if !defined(_OSG_REFERENCED_USE_ATOMIC_OPERATIONS) - ObserverSet* os = static_cast(_observers); - if (os) - { - if (_refMutex) - { - OpenThreads::ScopedLock lock(*_refMutex); - for(ObserverSet::iterator itr = os->begin(); - itr != os->end(); - ) - { - if ((*itr)->objectUnreferenced(const_cast(this))) - { - ObserverSet::iterator orig_itr = itr; - ++itr; - os.erase(orig_itr); - } - else - { - ++itr; - } - } - } - else - { - for(ObserverSet::iterator itr = os->begin(); - itr != os->end(); - ) - { - if ((*itr)->objectUnreferenced(const_cast(this))) - { - ObserverSet::iterator orig_itr = itr; - ++itr; - os.erase(orig_itr); - } - else - { - ++itr; - } - } - } - } - #else - ObserverSetData* observerSetData = static_cast(_observerSetDataPtr.get()); - if (observerSetData) - { - OpenThreads::ScopedLock lock(observerSetData->_mutex); - for(ObserverSet::iterator itr = observerSetData->_observers.begin(); - itr != observerSetData->_observers.end(); - ) - { - if ((*itr)->objectUnreferenced(const_cast(this))) - { - ObserverSet::iterator orig_itr = itr; - ++itr; - observerSetData->_observers.erase(orig_itr); - } - else - { - ++itr; - } - } - } - #endif + OpenThreads::ScopedLock lock(*(observerSet->getObserverSetMutex())); + if (signalUnreferened) + { + observerSet->signalObjectUnreferenced(const_cast(this)); + } + + if (_refCount!=0) + { + OSG_NOTICE<<"Referenced::signalObserversAndDelete(,,) disabling delete as _refCount="<<_refCount<signalObjectDeleted(const_cast(this)); + } } - if (_refCount!=0) return; - - if (signalDelete) - { - // tell all observers that we being delete so that they - // can do cleans up and remove any references they have. - #if !defined(_OSG_REFERENCED_USE_ATOMIC_OPERATIONS) - ObserverSet* os = static_cast(_observers); - if (os) - { - for(ObserverSet::iterator itr = os->begin(); - itr != os->end(); - ++itr) - { - (*itr)->objectDeleted(const_cast(this)); - } - delete os; - _observers = 0; - } - #else - ObserverSetData* observerSetData = static_cast(_observerSetDataPtr.get()); - if (observerSetData) - { - for(ObserverSet::iterator itr = observerSetData->_observers.begin(); - itr != observerSetData->_observers.end(); - ++itr) - { - (*itr)->objectDeleted(const_cast(this)); - } - _observerSetDataPtr.assign(0, observerSetData); - delete observerSetData; - } - #endif - } - - if (doDelete &&_refCount<=0) + if (doDelete && _refCount==0) { + //OSG_NOTICE<<"Referenced::signalObserversAndDelete(,,) doing delete as _refCount="<<_refCount< lock(*_refMutex); - needUnreferencedSignal = (--_refCount)<=0; + OpenThreads::ScopedLock lock(*_refMutex); + needUnreferencedSignal = ((--_refCount) == 0); } else { - needUnreferencedSignal = (--_refCount)<=0; + needUnreferencedSignal = ((--_refCount) == 0); } -#else - bool needUnreferencedSignal = (--_refCount)<=0; #endif if (needUnreferencedSignal) @@ -402,57 +354,6 @@ void Referenced::unref_nodelete() const } } -void Referenced::addObserver(Observer* observer) const -{ -#if defined(_OSG_REFERENCED_USE_ATOMIC_OPERATIONS) - ObserverSetData* observerSetData = static_cast(_observerSetDataPtr.get()); - while (0 == observerSetData) { - ObserverSetData* newObserverSetData = new ObserverSetData; - if (!_observerSetDataPtr.assign(newObserverSetData, 0)) - delete newObserverSetData; - observerSetData = static_cast(_observerSetDataPtr.get()); - } - OpenThreads::ScopedLock lock(observerSetData->_mutex); - observerSetData->_observers.insert(observer); -#else - if (_refMutex) - { - OpenThreads::ScopedLock lock(*_refMutex); - - if (!_observers) _observers = new ObserverSet; - if (_observers) static_cast(_observers)->insert(observer); - } - else - { - if (!_observers) _observers = new ObserverSet; - if (_observers) static_cast(_observers)->insert(observer); - } -#endif -} - -void Referenced::removeObserver(Observer* observer) const -{ -#if defined(_OSG_REFERENCED_USE_ATOMIC_OPERATIONS) - ObserverSetData* observerSetData = static_cast(_observerSetDataPtr.get()); - if (observerSetData) - { - OpenThreads::ScopedLock lock(observerSetData->_mutex); - observerSetData->_observers.erase(observer); - } -#else - if (_refMutex) - { - OpenThreads::ScopedLock lock(*_refMutex); - - if (_observers) static_cast(_observers)->erase(observer); - } - else - { - if (_observers) static_cast(_observers)->erase(observer); - } -#endif -} - void Referenced::deleteUsingDeleteHandler() const { getDeleteHandler()->requestDelete(this);