views:

189

answers:

4

Hi,

In a recent project I had to create a Singleton class and after a lot of digging around on Google I came up with this template class definition. The idea is to derive from this template class and make the derived class' constructor protected / private. It seems to work well but I have only used it with a single class in one project so I was hoping some of you could point out if I've made mistakes in the implementation. Here it is:

/**
 * @brief 
 *    Singleton design pattern implementation using a dynamically allocated singleton instance.
 *
 * The SingletonDynamic class is intended for use as a base for classes implementing the Singleton
 * design pattern and require lazy initialization of the singleton object. The default 
 * implementation is not thread-safe, however, the derived classes can make it so by reinitializing
 * the function pointers SingletonDynamic<T>::pfnLockMutex, SingletonDynamic<T>::pfnUnlockMutex
 * and SingletonDynamic<T>::pfnMemoryBarrier. The member function pointers are initialized by 
 * default to point to placeholder functions that do not perform any function. The derived class
 * must provide alternate implementations for SingletonDynamic<T>::lock_mutex(),
 * SingletonDynamic<T>::unlock_mutex() and SingletonDynamic<T>::memory_barrier() respectively
 * and reinitialize the respective function pointer members to these alternate implementations.
 *
 * @tparam T
 *    The type name of the derived (singleton) class
 *
 * @note The derived class must have a no-throw default constructor and a no-throw destructor.
 * @note The derived class must list this class as a friend, since, by necessity, the derived class'
 *       constructors must be protected / private.
 */
template< typename T >
class SingletonDynamic
{
public:
  /**
   * Factory function for vending mutable references to the sole instance of the singleton object.
   *
   * @return A mutable reference to the one and only instance of the singleton object.
   */
  static T &instance()
  {
    return *SingletonDynamic< T >::get_instance();
  }


  /**
   * Factory function for vending constant references to the sole instance of the singleton object.
   *
   * @return A constant reference to the one and only instance of the singleton object.
   */
  static const T &const_instance()
  {
    return *SingletonDynamic< T >::get_instance();
  }

protected:
  /** Default constructor */
  SingletonDynamic() {}

  /** Destructor */
  virtual ~SingletonDynamic() 
  {
    delete SingletonDynamic< T >::pInstance_;
  }

  /** Defines an alias for a function pointer type for executing functions related to thread-safety */
  typedef void(*coherence_callback_type)();

  /** 
   * Pointer to a function that will lock a mutex denying access to threads other that the current 
   * 
   * @note The function must have the signature void foo()
   * @note The derived class must never set this variable to NULL, doing so will cause a crash. The 
   *       default value must be left unchanged if this functionality is not desired.
   */
  static coherence_callback_type  pfnLockMutex;

  /** 
   * Pointer to a function that will unlock a mutex allowing access to other threads 
   * 
   * @note The function must have the signature void foo()
   * @note The derived class must never set this variable to NULL, doing so will cause a crash. The 
   *       default value must be left unchanged if this functionality is not desired.
   */
  static coherence_callback_type  pfnUnlockMutex;

  /** 
   * Pointer to a function that executes a memory barrier instruction that prevents the compiler
   * from reordering reads and writes across this boundary.
   * 
   * @note The function must have the signature void foo()
   * @note The derived class must never set this variable to NULL, doing so will cause a crash. The 
   *       default value must be left unchanged if this functionality is not desired.
   */
  static coherence_callback_type  pfnMemoryBarrier;

private:
  /** The sole instance of the singleton object */
  static T *pInstance_;

  /** Flag indicating whether the singleton object has been created */
  static volatile bool flag_;

  /** Private copy constructor to prevent copy construction */
  SingletonDynamic( SingletonDynamic const & );

  /** Private operator to prevent assignment */
  SingletonDynamic &operator=( SingletonDynamic const & );


  /** 
   * Fetches a pointer to the singleton object, after creating it if necessary
   *
   * @return A pointer to the one and only instance of the singleton object.
   */
  static T *get_instance()
  {
    if( SingletonDynamic< T >::flag_ == false ) {
      /* acquire lock */
      (*SingletonDynamic< T >::pfnLockMutex)();

      if( SingletonDynamic< T >::pInstance_ == NULL ) {
        pInstance_ = new T();
      }

      /* release lock */
      (*SingletonDynamic< T >::pfnUnlockMutex)();

      /* enforce all prior I/O to be completed */
      (*SingletonDynamic< T >::pfnMemoryBarrier)();

      SingletonDynamic< T >::flag_ = true;

      return SingletonDynamic< T >::pInstance_;
    } else {
      /* enforce all prior I/O to be completed */
      (*SingletonDynamic< T >::pfnMemoryBarrier)();

      return SingletonDynamic< T >::pInstance_;
    }
  }


  /**
   * Placeholder function for locking a mutex, thereby preventing access to other threads. This 
   * default implementation does not perform any function, the derived class must provide an 
   * implementation if this functionality is desired.
   */
  inline static void lock_mutex()
  {
    /* default implementation does nothing */
    return;
  }


  /**
   * Placeholder function for unlocking a mutex, thereby allowing access to other threads. This 
   * default implementation does not perform any function, the derived class must provide an 
   * implementation if this functionality is desired.
   */
  inline static void unlock_mutex()
  {
    /* default implementation does nothing */
    return;
  }


  /**
   * Placeholder function for executing a memory barrier instruction, thereby preventing the 
   * compiler from reordering read and writes across this boundary. This default implementation does 
   * not perform any function, the derived class must provide an implementation if this 
   * functionality is desired.
   */
  inline static void memory_barrier()
  {
    /* default implementation does nothing */
    return;
  }
};

/* Initialize the singleton instance pointer */
template< typename T >
T *SingletonDynamic<T>::pInstance_        = NULL;

/* Initialize the singleton flag */
template< typename T >
volatile bool SingletonDynamic<T>::flag_  = false;

/* Initialize the function pointer that locks the mutex */
template< typename T >
typename SingletonDynamic<T>::coherence_callback_type SingletonDynamic<T>::pfnLockMutex  
                                                              = &SingletonDynamic<T>::lock_mutex;

/* Initialize the function pointer that unlocks the mutex */
template< typename T >
typename SingletonDynamic<T>::coherence_callback_type SingletonDynamic<T>::pfnUnlockMutex  
                                                              = &SingletonDynamic<T>::unlock_mutex;

/* Initialize the function pointer that executes the memory barrier instruction */
template< typename T >
typename SingletonDynamic<T>::coherence_callback_type SingletonDynamic<T>::pfnMemoryBarrier
                                                              = &SingletonDynamic<T>::memory_barrier;

I'm particularly worried about the static member initializations in the header file and whether that will cause multiple definition errors when the header file of the class that is derived from the SingleDynamic is included in several files. I already tried that out and it seems to work, but I can't figure out why its working :).

Thanks in advance, Ashish.

EDIT: Modified implementation using a policy based design as suggested in the accepted solution.

/**
 * This is the default ConcurrencyPolicy implementation for the SingletonDynamic class. This 
 * implementation does not provide thread-safety and is merely a placeholder. Classes deriving from
 * SingletonDynamic must provide alternate ConcurrencyPolicy implementations if thread-safety is
 * desired.
 */
struct DefaultSingletonConcurrencyPolicy
{
  /**
   * Placeholder function for locking a mutex, thereby preventing access to other threads. This 
   * default implementation does not perform any function, the derived class must provide an 
   * alternate implementation if this functionality is desired.
   */
  static void lock_mutex() 
  { 
    /* default implementation does nothing */
    return;
  }

  /**
   * Placeholder function for unlocking a mutex, thereby allowing access to other threads. This 
   * default implementation does not perform any function, the derived class must provide an 
   * alternate implementation if this functionality is desired.
   */
  static void unlock_mutex()
  {
    /* default implementation does nothing */
    return;
  }

  /**
   * Placeholder function for executing a memory barrier instruction, thereby preventing the 
   * compiler from reordering read and writes across this boundary. This default implementation does 
   * not perform any function, the derived class must provide an alternate implementation if this 
   * functionality is desired.
   */
  static void memory_barrier()
  {
    /* default implementation does nothing */
    return;
  }
};


/**
 * @brief 
 *    Singleton design pattern implementation using a dynamically allocated singleton instance.
 *
 * The SingletonDynamic class is intended for use as a base for classes implementing the Singleton
 * design pattern and that dynamic allocation of the singleton object. The default implementation 
 * is not thread-safe; however, the class uses a policy-based design pattern that allows the derived 
 * classes to achieve threaad-safety by providing an alternate implementation of the 
 * ConcurrencyPolicy.
 *
 * @tparam T
 *    The type name of the derived (singleton) class
 * @tparam ConcurrencyPolicy
 *    The policy implementation for providing thread-safety
 *
 * @note The derived class must have a no-throw default constructor and a no-throw destructor.
 * @note The derived class must list this class as a friend, since, by necessity, the derived class'
 *       constructors must be protected / private.
 */
template< typename T, typename ConcurrencyPolicy = DefaultSingletonConcurrencyPolicy >
class SingletonDynamic : public ConcurrencyPolicy
{
public:
  /**
   * Factory function for vending mutable references to the sole instance of the singleton object.
   *
   * @return A mutable reference to the one and only instance of the singleton object.
   */
  static T &instance()
  {
    return *SingletonDynamic< T, ConcurrencyPolicy >::get_instance();
  }


  /**
   * Factory function for vending constant references to the sole instance of the singleton object.
   *
   * @return A constant reference to the one and only instance of the singleton object.
   */
  static const T &const_instance()
  {
    return *SingletonDynamic< T, ConcurrencyPolicy >::get_instance();
  }

protected:
  /** Default constructor */
  SingletonDynamic() {}

  /** Destructor */
  virtual ~SingletonDynamic() 
  {
    delete SingletonDynamic< T, ConcurrencyPolicy >::pInstance_;
  }

private:
  /** The sole instance of the singleton object */
  static T *pInstance_;

  /** Flag indicating whether the singleton object has been created */
  static volatile bool flag_;

  /** Private copy constructor to prevent copy construction */
  SingletonDynamic( SingletonDynamic const & );

  /** Private operator to prevent assignment */
  SingletonDynamic &operator=( SingletonDynamic const & );


  /** 
   * Fetches a pointer to the singleton object, after creating it if necessary
   *
   * @return A pointer to the one and only instance of the singleton object.
   */
  static T *get_instance()
  {
    if( SingletonDynamic< T, ConcurrencyPolicy >::flag_ == false ) {
      /* acquire lock */
      ConcurrencyPolicy::lock_mutex();

      /* create the singleton object if this is the first time */
      if( SingletonDynamic< T, ConcurrencyPolicy >::pInstance_ == NULL ) {
        pInstance_ = new T();
      }

      /* release lock */
      ConcurrencyPolicy::unlock_mutex();

      /* enforce all prior I/O to be completed */
      ConcurrencyPolicy::memory_barrier();

      /* set flag to indicate singleton has been created */
      SingletonDynamic< T, ConcurrencyPolicy >::flag_ = true;

      return SingletonDynamic< T, ConcurrencyPolicy >::pInstance_;
    } else {
      /* enforce all prior I/O to be completed */
      ConcurrencyPolicy::memory_barrier();

      return SingletonDynamic< T, ConcurrencyPolicy >::pInstance_;
    }
  }
};

/* Initialize the singleton instance pointer */
template< typename T, typename ConcurrencyPolicy >
T *SingletonDynamic< T , ConcurrencyPolicy >::pInstance_        = NULL;

/* Initialize the singleton flag */
template< typename T, typename ConcurrencyPolicy >
volatile bool SingletonDynamic< T , ConcurrencyPolicy >::flag_  = false;
+1  A: 

Doesn't need to be that complex.
http://stackoverflow.com/questions/1701149/simple-c-logger-by-using-singleton-pattern

Arkaitz Jimenez
The suggestion at the end of that link relies on compiler specific behavior to handle the concurrency issue.
Omnifarious
@Omnifarious, the link above just uses a static singleton object. Why is that compiler specific? Isn't it true for all compilers that all static objects are constructed before `main()` executes? So that implementation should be thread-safe as long as all your calling threads being execution after `main()` starts executing.
Praetorian
@Praetorian: No. static function objects are only created on first use (allowing lazy evaluation). What Omnifarious is refering too is that the creation of this static is not thread safe unless you are using gcc which has implemented the compiler in a specific way to make sure it is thread safe.
Martin York
@Martin: I did not know that about static objects within functions. But what I said earlier does hold true for static objects declared at file scope, doesn't it?
Praetorian
@Praetorian - Yes, it does. But the example being linked to does not use a static at file scope.
Omnifarious
+1  A: 

The correctness of the concurrency related code here is difficult to evaluate. The implementation is trying to be a little too clever, in my opinion.

OTOH, all the concurrency related code basically has stubs behind it that do nothing. If this is used in a non-threaded environment, I think it should be fine.

But, I also think your worry is well founded. Those external definitions of static members seem like they would violate the one definition rule.

Personally, I think this template should be re-written to have the concurrency stuff as a policy argument to the template itself, and to require derived classes to declare their own versions of pInstance in an appropriate .cpp file.

Somebody else has suggested relying on compiler specific behavior with regards to the initialization of static local variables. I don't think that's a horrible suggestion, but it might be nice to have an option when you can't rely on the compiler to do the right thing.

Omnifarious
I like the policy based design idea; then I can have the default policy that does nothing, and derived classes can provide their own implementations that deal with concurrency. I'll try it out tonight. Thanks!
Praetorian
+1  A: 

Static members of template classes must be initialized in the header file and recent C++ compilers and their linkers must handle this correctly.

But you are right, some very old compiler have problems with this.

In these cases it is a workaround to initialize the static members exactly once in an arbitrary compilation unit for every type the singleton template is used for.

The gcc Documentation has same details about this: http://gcc.gnu.org/onlinedocs/gcc/Template-Instantiation.html.

I remember one embedded project (not long ago), where a old compiler was still in use and that one silently created multiple instances of template's static members. Obviously a very bad idea when it come's to store a Singleton in it....

Even worse, the only Singleton in use in the library (3rd-party framework) was some Configuration object that usually was initialized in the same way, so the bug only occured when configuration was changed at runtime. It took several days to track the bug down, until we finally saw in the disassembly that the "same" member is accessed at different memory regions.

IanH
+1  A: 

It's currently impossible to lazily create a Singleton in a multithreaded environment in C++.

It's been acknowledged by a number of gurus (among which Herb Sutter) that the current state of the standard did not guarantee anything. There are hacks for a variety of compilers, and boost provides the once facility for this very purpose, however it's a motley collection of compilers specific instructions... it's not standard C++ (which is thread unaware).

The only solution currently working (as per the standard) is to initialize the Singleton before launching the multiple threads or in a part of the process that guarantees only one thread will be accessing it.

C++0x brings threads into the standard, and notably guarantees that local static variables will only be created once even in the presence of multiple threads (in case of multiple simultaneous calls all block until the creation ends). Therefore the following method:

static MyType& Instance() { static Instance MExemplar; return MExemplar; }

works, and in this case there's no need for a singleton template class at all.

Matthieu M.