[Freepooma-devel] [PATCH] Fix locking in RefCounted(Ptr)

[Richard Guenther]

This patch fixes design issues with RefCounted and RefCountedPtr.
It also makes objects managed through RefCountedPtr madatory to
be derived from RefCounted (there is no other way to provide safe
operation, other than pushing all of the RefCountedPtr implementation
to RefCounted -- and this changes just as much of the interface).

All the rationales of this patch (the changed locking behavior) is
embedded in the (new) documentation.  I provided a context diff
for easier reading.

I'll hold off applying this for some days, but finally will apply
it to HEAD if noone objects with good reasons.

Richard.


2005Apr14  Richard Guenther <address@hidden>

        * src/Utilities/RefCounted.h: Document tight integration with
        RefCountedPtr.  Document behavior wrt locking.
        (countUnlocked): Deprecate.
        (isShared): Make implementation unlocked.
        (count): Likewise.
        (removeReferenceUnlocked): New.
        src/Utilities/RefCountedPtr.h: Document requirement of
        deriving from RefCounted.  Document behavior wrt locking.
        (makeOwnCopy): Fix race condition.

Index: RefCounted.h
===================================================================
RCS file: /cvsroot/freepooma/freepooma/src/Utilities/RefCounted.h,v
retrieving revision 1.19
diff -c -3 -r1.19 RefCounted.h
*** RefCounted.h        1 Nov 2004 18:17:18 -0000       1.19
--- RefCounted.h        14 Apr 2005 09:32:03 -0000
***************
*** 45,50 ****
--- 45,55 ----
  #include "Utilities/PAssert.h"
  #include "Threads/PoomaMutex.h"

+
+ template <class T>
+ class RefCountedPtr;
+
+
  
///////////////////////////////////////////////////////////////////////////////
  // namespace Pooma {

***************
*** 55,60 ****
--- 60,69 ----
   *
   * When running in a threaded environment, RefCounted protects the
   * reference count with a mutex, so this class is thread safe.
+  *
+  * Note that this class is tightly integrated with the RefCountedPtr
+  * class as such that it should be used only through an object of
+  * the latter class.
   */

  class RefCounted
***************
*** 65,86 ****
    // Constructors, Destructor, Assignment...
    //============================================================

!   // Default constructor.
!   // Starts count at zero. The client that creates the RefCounted
!   // object is responsible for calling addReference().

    RefCounted()
      : count_m(0)
      { }

!   // Copy constructor.
!   // This may appear a bit odd. If a RefCounted object is
!   // copied, then this creates a NEW RefCounted object
!   // that must be reference counted separately from the
!   // old one. Thus its count must be initialized to zero.
!   // Ordinarily RefCounted objects aren't copied. However,
!   // clients may wish to implement a clone() operation
!   // that does explicitly make a (deep) copy.

    RefCounted(const RefCounted &)
      : count_m(0)
--- 74,95 ----
    // Constructors, Destructor, Assignment...
    //============================================================

!   /// Default constructor.
!   /// Starts count at zero. The client that creates the RefCounted
!   /// object is responsible for calling addReference().

    RefCounted()
      : count_m(0)
      { }

!   /// Copy constructor.
!   /// This may appear a bit odd. If a RefCounted object is
!   /// copied, then this creates a NEW RefCounted object
!   /// that must be reference counted separately from the
!   /// old one. Thus its count must be initialized to zero.
!   /// Ordinarily RefCounted objects aren't copied. However,
!   /// clients may wish to implement a clone() operation
!   /// that does explicitly make a (deep) copy.

    RefCounted(const RefCounted &)
      : count_m(0)
***************
*** 90,133 ****

    ~RefCounted() { }

!   //============================================================
!   // Accessors
!   //============================================================

!   bool isShared() const;

!   //============================================================
!   // Mutators
!   //============================================================

!   // These simply increment and decrement the reference count.

    void addReference();
    void removeReference();

!   // Remove reference and check if it leaves garbage.

    bool removeRefAndCheckGarbage();
!
!   // Expose lock and unlock:

    void lock() const
    {
      mutex_m.lock();
    }

    void unlock() const
    {
      mutex_m.unlock();
    }

!   // Return the current value of the reference count.

-   int count() const;
-
-   // Ditto, but without locking the mutex while copying it.
-
-   int countUnlocked() const;

  private:

--- 99,187 ----

    ~RefCounted() { }

! #if !POOMA_NO_TEMPLATE_FRIENDS

!   template <class T>
!   friend class RefCountedPtr;

!   // private accessors ensure we only access this from RefCountedPtr
! private:
!
! #endif

!   /// @name Accessors
!   //@{
!
!   /// Return if the object has more than one reference.
!   /// You need to protect yourself against modifications from other
!   /// threads using lock and unlock.  Only if this returns false
!   /// (i.e. a private object), and thus you are the only reference
!   /// holder, you can rely on this property to stay true even without
!   /// locking.  So, a value of true cannot be trusted further without
!   /// extra locking by the caller.
!
!   bool isShared() const { return count_m > 1; }
!
!   /// Return the current value of the reference count.
!   /// You need to protect yourself against modifications from other
!   /// threads using lock and unlock.
!
!   int count() const { return count_m; }
!
!   // Ditto, but without locking the mutex while copying it.
!   // Redundant and deprecated.
!
!   int countUnlocked() const { return count_m; }
!
!   //@}
!
!
!   ///@name Mutators
!   //@{
!
!   /// Increment the reference counter.

    void addReference();
+
+   /// Decrement the reference counter.
+
    void removeReference();

!   /// Decrement the reference counter and return true if it
!   /// was the last reference (count_m is zero now).  If this
!   /// returns true (it was the last reference removed), this
!   /// could happened only exactly once and thus you can, without
!   /// further locking, assume the object will be not tampered
!   /// with from other threads from now on.  Same like if
!   /// isShared returned false.

    bool removeRefAndCheckGarbage();
!
!   //@}
!
!
!   ///@name Locking
!   //@{
!
!   /// Lock the reference counter.

    void lock() const
    {
      mutex_m.lock();
    }

+   /// Unlock the reference counter.
+
    void unlock() const
    {
      mutex_m.unlock();
    }

!   //@}
!
!
!   void removeReferenceUnlocked() { --count_m; }


  private:

***************
*** 150,165 ****
  // Inline functions
  
//-----------------------------------------------------------------------------

- inline bool
- RefCounted::isShared() const
- {
-   mutex_m.lock();
-   bool test = count_m > 1;
-   mutex_m.unlock();
-   return test;
- }
-
-
  inline void
  RefCounted::addReference()
  {
--- 204,209 ----
***************
*** 187,207 ****
    return test;
  }

- inline int
- RefCounted::count() const
- {
-   mutex_m.lock();
-   int count = count_m;
-   mutex_m.unlock();
-   return count;
- }
-
- inline int
- RefCounted::countUnlocked() const
- {
-   return count_m;
- }
-

  /**
   *  Simple template class encapsulating a single data item and
--- 231,236 ----
Index: RefCountedPtr.h
===================================================================
RCS file: /cvsroot/freepooma/freepooma/src/Utilities/RefCountedPtr.h,v
retrieving revision 1.18
diff -c -3 -r1.18 RefCountedPtr.h
*** RefCountedPtr.h     1 Nov 2004 18:17:18 -0000       1.18
--- RefCountedPtr.h     14 Apr 2005 09:32:03 -0000
***************
*** 39,53 ****
  #include "Pooma/Configuration.h"


  
///////////////////////////////////////////////////////////////////////////////
  // namespace Pooma {


  /**
   * RefCountedPtr<T> is a smart-pointer class that provides reference
!  * counting for objects of type T. T must provide the same interface
!  * and semantics as RefCounted, which is usually accomplished by
!  * inheriting from RefCounted.
   */

  template <class T>
--- 39,54 ----
  #include "Pooma/Configuration.h"


+ class RefCounted;
+
+
  
///////////////////////////////////////////////////////////////////////////////
  // namespace Pooma {


  /**
   * RefCountedPtr<T> is a smart-pointer class that provides reference
!  * counting for objects of type T.  T must be inheriting from RefCounted.
   */

  template <class T>
***************
*** 75,88 ****

    RefCountedPtr(T * const pT)
      : ptr_m(pT)
!     { if (isValid()) ptr_m->addReference(); }

    // Copy constructors copy the raw pointer and increment
    // the reference count.

    RefCountedPtr(const This_t &model)
      : ptr_m(model.ptr_m)
!     { if (isValid()) ptr_m->addReference(); }


    //============================================================
--- 76,89 ----

    RefCountedPtr(T * const pT)
      : ptr_m(pT)
!     { if (isValid()) ptr_m->RefCounted::addReference(); }

    // Copy constructors copy the raw pointer and increment
    // the reference count.

    RefCountedPtr(const This_t &model)
      : ptr_m(model.ptr_m)
!     { if (isValid()) ptr_m->RefCounted::addReference(); }


    //============================================================
***************
*** 100,151 ****
    RefCountedPtr & operator=(const RefCountedPtr &);
    RefCountedPtr & operator=(T *);

!   //============================================================
!   // Accessors and Mutators
!   //============================================================

!   // Two ways to use these pointers:
!   // member selection and dereferencing.

    inline T * operator->() const { return ptr_m;  }
    inline T & operator*()  const { return *ptr_m; }

!   // Comparison functions

    bool operator==(const This_t& a) const
    { return ptr_m == a.ptr_m; }

    bool operator!=(const This_t& a) const
    { return ptr_m != a.ptr_m; }

!   // Removes reference and sets pointer field to NULL.

    void invalidate();

!   // Check to see if a pointer is valid:

    inline bool isValid() const { return ptr_m != 0; }

!   // Check to see if the pointer is shared.

!   inline bool isShared() const { return ptr_m->isShared(); }

!   // Return the current value of the reference count.

!   inline int count() const { return ptr_m->count(); }

!   // Make private copy of data pointed to by this reference.

    RefCountedPtr<T> & makeOwnCopy();

!   // Interoperability with non-POOMA code may require access to the
!   // raw data pointer. Thus we provide the following accessor
!   // functions. These should be used with care as the returned pointer
!   // is not reference counted.

    inline T * rawPointer() { return ptr_m; }
    inline const T * rawPointer() const { return ptr_m; }


  #if ! POOMA_NO_TEMPLATE_FRIENDS

--- 101,164 ----
    RefCountedPtr & operator=(const RefCountedPtr &);
    RefCountedPtr & operator=(T *);

!   ///@name Accessors and Mutators
!   //@{

!   /// Use the pointer through member selection.

    inline T * operator->() const { return ptr_m;  }
+
+   /// Use the pointer by dereferencing.
+
    inline T & operator*()  const { return *ptr_m; }

!   /// Pointer equality check

    bool operator==(const This_t& a) const
    { return ptr_m == a.ptr_m; }

+   /// Pointer in-equality check
+
    bool operator!=(const This_t& a) const
    { return ptr_m != a.ptr_m; }

!   //@}
!
!   /// Removes reference and sets pointer field to NULL.

    void invalidate();

!   /// Check to see if the pointer is valid (non-NULL).

    inline bool isValid() const { return ptr_m != 0; }

!   /// Check to see if the pointer is shared.  Usable only to
!   /// reliably check, if the object is _not_ shared.  Do not
!   /// trust the return value true.

!   inline bool isShared() const { return ptr_m->RefCounted::isShared(); }

!   /// Return the current value of the reference count.  Values
!   /// other than one are not stable.

!   inline int count() const { return ptr_m->RefCounted::count(); }

!   /// Make private copy of data pointed to by this reference.

    RefCountedPtr<T> & makeOwnCopy();

!   ///@name Interoperability
!   /// Interoperability with non-POOMA code may require access to the
!   /// raw data pointer. Thus we provide the following accessor
!   /// functions. These should be used with care as the returned pointer
!   /// is not reference counted.
!   //@{

    inline T * rawPointer() { return ptr_m; }
    inline const T * rawPointer() const { return ptr_m; }

+   //@}
+

  #if ! POOMA_NO_TEMPLATE_FRIENDS

***************
*** 158,164 ****

  #endif

!   // The pointer itself.

    T * ptr_m;

--- 171,177 ----

  #endif

!   /// The pointer itself.

    T * ptr_m;

***************
*** 177,183 ****
  template <class T>
  inline void RefCountedPtr<T>::invalidate()
  {
!   if ( isValid() && ptr_m->removeRefAndCheckGarbage() )
      delete ptr_m;
    ptr_m = 0;
  }
--- 190,196 ----
  template <class T>
  inline void RefCountedPtr<T>::invalidate()
  {
!   if ( isValid() && ptr_m->RefCounted::removeRefAndCheckGarbage() )
      delete ptr_m;
    ptr_m = 0;
  }
***************
*** 210,222 ****
        // existence], so the fact that *this will be left in an
        // inconsistent state isn't a big deal.)

!       if ( isValid() && ptr_m->removeRefAndCheckGarbage() )
        delete ptr_m;

        // Now assign the new one.

        ptr_m = rhs.ptr_m;
!       if ( isValid() ) ptr_m->addReference();
      }

    return *this;
--- 223,235 ----
        // existence], so the fact that *this will be left in an
        // inconsistent state isn't a big deal.)

!       if ( isValid() && ptr_m->RefCounted::removeRefAndCheckGarbage() )
        delete ptr_m;

        // Now assign the new one.

        ptr_m = rhs.ptr_m;
!       if ( isValid() ) ptr_m->RefCounted::addReference();
      }

    return *this;
***************
*** 241,253 ****
        // existence], so the fact that *this will be left in an
        // inconsistent state isn't a big deal.)

!       if ( isValid() && ptr_m->removeRefAndCheckGarbage() )
        delete ptr_m;

        // Now assign the new one.

        ptr_m = pp;
!       if ( isValid() ) ptr_m->addReference();
      }

    return *this;
--- 254,266 ----
        // existence], so the fact that *this will be left in an
        // inconsistent state isn't a big deal.)

!       if ( isValid() && ptr_m->RefCounted::removeRefAndCheckGarbage() )
        delete ptr_m;

        // Now assign the new one.

        ptr_m = pp;
!       if ( isValid() ) ptr_m->RefCounted::addReference();
      }

    return *this;
***************
*** 261,269 ****
  inline RefCountedPtr<T> &
  RefCountedPtr<T>::makeOwnCopy()
  {
!   // If more than one thing is referring to this one

!   if ( isValid() && ptr_m->isShared() )
      {
        // First try to allocate new memory and assign to a temporary
        // pointer. If this throws, *this will still be in a consistent
--- 274,292 ----
  inline RefCountedPtr<T> &
  RefCountedPtr<T>::makeOwnCopy()
  {
!   // If the object is already private, we need not do anything
!
!   if (isValid() && !ptr_m->RefCounted::isShared())
!     return *this;
!
!   // Now, potentially(!), more than one reference is attached
!   // to the object.

!   // Lock the reference count to ensure atomicity.
!
!   ptr_m->RefCounted::lock();
!
!   if (ptr_m->RefCounted::isShared())
      {
        // First try to allocate new memory and assign to a temporary
        // pointer. If this throws, *this will still be in a consistent
***************
*** 275,295 ****

        T * temp = ElementProperties<T>::clone(*ptr_m);

!       // Remove reference from copy-ee. It was shared so there is no
!       // garbage to collect. (This can throw, but only if *ptr_m was
!       // already corrupt.)

!       ptr_m->removeReference();

        // Assign pointer to new object.

        ptr_m = temp;

!       // Increment reference for new copy.
!
!       ptr_m->addReference();
      }

    return *this;
  }

--- 298,325 ----

        T * temp = ElementProperties<T>::clone(*ptr_m);

!       // Increment reference for new copy.
!
!       temp->RefCounted::addReference();
!
!       // Remove reference from copy-ee. It is shared so there is no
!       // garbage to collect.

!       ptr_m->RefCounted::removeReferenceUnlocked();
!
!       // Unlock old object.
!
!       ptr_m->RefCounted::unlock();

        // Assign pointer to new object.

        ptr_m = temp;

!       return *this;
      }

+   ptr_m->RefCounted::unlock();
+
    return *this;
  }