Singleton design pattern study

Question

I am learning design patterns and the singleton is one of them. What are your opinions on the code? Please feel free to share.

I know singletons are not popular because of complications in testing. I tested the program by running it repeatedly and checking the results manually.

During my study of singletons I found that, if the constructor of the singleton is slow, multiple instances of the singleton are created even when declared static in a method. One thread can be busy constructing while the next thread starts starts constructing, even when the variable is declared static in a function block. To me this was new and baffling. So I had to start using mutexes for thread safety. The goal of the CSleeper class is to have the threads catch up with each other.

Compiling:

g++ -I inc -Wall -std=gnu++11 -pthread -c CPrinter.cpp -o dbg/bin/CPrinter.o
g++ -I inc -Wall -std=gnu++11 -pthread -c CSingleton.cpp -o dbg/bin/CSingleton.o
g++ -I inc -Wall -std=gnu++11 -pthread -c CSleeper.cpp -o dbg/bin/CSleeper.o
g++ -I inc -Wall -std=gnu++11 -pthread -c main.cpp -o dbg/bin/main.o
g++ -g -I inc -Wall -std=gnu++11 -pthread -lm -lpthread dbg/bin/CPrinter.o dbg/bin/CSingleton.o dbg/bin/CSleeper.o dbg/bin/main.o -o Singleton_dbg.exe

CPrinter.cpp

#include <mutex>
#include <iostream>

#include "CPrinter.h"

static std::mutex gMutex;

void CPrinter::Printer(std::stringstream& aStringStream)
{
  std::lock_guard<std::mutex> lock(gMutex);

  std::cout << aStringStream.str().c_str();
  aStringStream.str("");
}

void CPrinter::Printer(std::string& aString)
{
  std::stringstream ss;
  ss << aString << std::endl;
  Printer(ss);
}

CPrinter.h

#ifndef PRINTER_H
#define PRINTER_H

#include <sstream>

struct CPrinter
{
  static void Printer(std::stringstream& aStringStream);
  static void Printer(std::string& aString);
};

#endif // PRINTER_H

CSingleton.cpp

#include <mutex>
#include <string>

#include "CSleeper.h"
#include "CSingleton.h"
#include "CPrinter.h"

static std::mutex gStdMutexPublic;    // Entry level to the singleton
static std::mutex gStdMutexConstruct; // Around the construction if the singleton.

//////////////////////////////////////////////////////

CSingleton::CSingleton() : iTestMember("empty")
{
  std::stringstream stringStream;

  stringStream << __PRETTY_FUNCTION__ << " Started." << std::endl;
  CPrinter::Printer(stringStream);

  CSleeper::Sleep() ;

  stringStream << __PRETTY_FUNCTION__ << " Finished." << std::endl;
  CPrinter::Printer(stringStream);
}

//////////////////////////////////////////////////////

CSingleton& CSingleton::GetSingleton()
{
  std::lock_guard<std::mutex> lock(gStdMutexConstruct);

  CSingleton& mySingletonRef = Construct();
  CSleeper::Sleep();

  return mySingletonRef;
}

//////////////////////////////////////////////////////

CSingleton& CSingleton::Construct()
{
  // This method should be protected by a mutex outside of the method.
  static CSingleton myCSingleton;
  std::stringstream stringStream;

  stringStream << __PRETTY_FUNCTION__ << ": &myCSingleton= " << &myCSingleton << std::endl;
  CPrinter::Printer(stringStream);
  CSleeper::Sleep();

  return myCSingleton;
}

//////////////////////////////////////////////////////

std::string CSingleton::GetCopyOfTestMember() const
{
  std::lock_guard<std::mutex> lock(gStdMutexPublic);

  std::string r;

  for (size_t index = 0; index < iTestMember.length(); index++)
  {
    CSleeper::Sleep();
    // SLow copy for checking of thread safety
    r += iTestMember[index];
    CPrinter::Printer(r);
  }

  return r;
}

//////////////////////////////////////////////////////

void CSingleton::SetTestMember(const std::string& aTestMember)
{
  std::lock_guard<std::mutex> lock(gStdMutexPublic);
  std::stringstream stringStream;
  stringStream << __PRETTY_FUNCTION__ << ": changing from " << iTestMember << " to " << aTestMember << "." <<  std::endl;
  CPrinter::Printer(stringStream);

  iTestMember = "";
  for (size_t index = 0; index < aTestMember.length(); index++)
  {
    CSleeper::Sleep();
    // SLow copy for checking of thread safety
    iTestMember += aTestMember[index];
    CPrinter::Printer(iTestMember);
  }
}

CSingleton.h

#ifndef CSINGLETON_H
#define CSINGLETON_H

#include <string>

// This class should be the ultimate, thread-safe, failure proof
// singleton implementation in C++.

class CSingleton
{
public:
  std::string GetCopyOfTestMember() const;
  void SetTestMember(const std::string& aTestMember);
  static CSingleton& GetSingleton();

private:
  CSingleton();
  static CSingleton& Construct();

  ~CSingleton() = default; // dtor
  CSingleton(const CSingleton&) = delete; // copy ctor
  CSingleton(CSingleton&&) = delete; // move ctor
  CSingleton& operator=(CSingleton&) = default; // copy assignment op
  CSingleton& operator=(CSingleton&&) = delete; // move assignment op

  std::string iTestMember;
};


#endif // CSINGLETON_H

CSleeper.cpp

#include <unistd.h>
#include <stdlib.h>
#include <ctime>

#include "CSleeper.h"

static unsigned int g_seed = static_cast<unsigned int>(std::time(nullptr) % 0xFFFFFFFF);

void CSleeper::Sleep()
{
  rand_r(&g_seed);
  usleep(g_seed % 1000);
}

CSleeper.h

#ifndef CSLEEPER_H
#define CSLEEPER_H

struct CSleeper
{
  static void Sleep();
};

#endif // CSLEEPER_H

main.cpp

#include <thread>         // std::thread
#include <sstream>
#include <string>

#include "CSleeper.h"
#include "CSingleton.h"
#include "CPrinter.h"

void SingletonTester(const int aThreadId, const std::string& aSetMember)
{
  std::stringstream ss;
  ss << __PRETTY_FUNCTION__ << ": Thread " << aThreadId << " started. " << std::endl;
  CPrinter::Printer(ss);

  CSleeper::Sleep();

  CSingleton& mySingleton = CSingleton::GetSingleton();
  CSleeper::Sleep();

  ss << __PRETTY_FUNCTION__ << ": Thread " << aThreadId << " is going to set singleton member to " << aSetMember << "." << std::endl;
  CPrinter::Printer(ss);
  mySingleton.SetTestMember(aSetMember);
  CSleeper::Sleep();

  ss << __PRETTY_FUNCTION__ << ": Thread " << aThreadId << " has received singleton member value " << mySingleton.GetCopyOfTestMember() << "." << std::endl;
  CPrinter::Printer(ss);

  CSleeper::Sleep();

  ss << __PRETTY_FUNCTION__ << ": Finished." << std::endl;
  CPrinter::Printer(ss);
}

int main()
{
  std::stringstream ss;
  std::string thread1string = "twenty";
  std::string thread2string = "thirty";

  ss << "Singleton tester started." << std::endl;
  CPrinter::Printer(ss);

  std::thread threadOne(SingletonTester, 20,  thread1string);
  std::thread threadTwo(SingletonTester, 30,  thread2string);

  threadOne.join();
  threadTwo.join();

  ss << "Singleton tester finished." << std::endl;
  CPrinter::Printer(ss);
}

Answer 1

First note. Since C++11 the construction of static members has ben thread safe. So there is no need for locking.

There should only be one singelton. So all copying should be disabled.

// Having copy assignment enabled seems to go against his.
CSingleton& operator=(CSingleton&) = default; // copy assignment op

I know singletons are not popular because of complications in testing. I tested the program by running it repeatedly and checking the results manually.

Globally mutable state is not good practice. Because of tight coupling and side affect driven code (code that has side affects outside of the parameters is difficult to handle in real life as well as for testing).

During my study of singletons I found that, if the constructor of the singleton is slow, multiple instances of the singleton are created even when declared static in a method.

The you must be using a pre C++11 compiler or a compiler that has a bug (or the bug in your code I pointed to above is causing you problems).

One thread can be busy constructing while the next thread starts starts constructing, even when the variable is declared static in a function block.

Check the version of your compiler. That should not happen in C++11 compliant compiler. So I suspect that this is something you are doing wrong.