0

I am learning about the singleton design pattern with Chapter 29 of Professional C++, Second Edition1. It illustrates a singleton implementation of a Logger class that covers thread-safety requirements:

Header

#include <iostream>
#include <fstream>
#include <vector>
#include <string>
#include <mutex>
// Definition of a multithread safe singleton logger class
class Logger
{
    public:
        static const std::string kLogLevelDebug;
        static const std::string kLogLevelInfo;
        static const std::string kLogLevelError;
        // Returns a reference to the singleton Logger object
        static Logger& instance();
        // Logs a single message at the given log level
        void log(const std::string& inMessage,
                 const std::string& inLogLevel);
        // Logs a vector of messages at the given log level
        void log(const std::vector<std::string>& inMessages,
                 const std::string& inLogLevel);
    protected:
        // Static variable for the one-and-only instance
        static Logger* pInstance;
        // Constant for the filename
        static const char* const kLogFileName;
        // Data member for the output stream
        std::ofstream mOutputStream;
        // Embedded class to make sure the single Logger
        // instance gets deleted on program shutdown.
        friend class Cleanup;
        class Cleanup
        {
           public:
           ~Cleanup();
        };
        // Logs message. The thread should own a lock on sMutex
        // before calling this function.
        void logHelper(const std::string& inMessage,
                  const std::string& inLogLevel);
    private:
        Logger();
        virtual ~Logger();
        Logger(const Logger&);
        Logger& operator=(const Logger&);
        static std::mutex sMutex;
};

Implementation

#include <stdexcept>
#include "Logger.h"
using namespace std;

const string Logger::kLogLevelDebug = "DEBUG";
const string Logger::kLogLevelInfo = "INFO";
const string Logger::kLogLevelError = "ERROR";
const char* const Logger::kLogFileName = "log.out";
Logger* Logger::pInstance = nullptr;
mutex Logger::sMutex;

Logger& Logger::instance()
{
    static Cleanup cleanup;
    lock_guard<mutex> guard(sMutex);
    if (pInstance == nullptr)
        pInstance = new Logger();
    return *pInstance;
}
Logger::Cleanup::~Cleanup()
{
    lock_guard<mutex> guard(Logger::sMutex);
    delete Logger::pInstance;
    Logger::pInstance = nullptr;
}
Logger::~Logger()
{
    mOutputStream.close();
}
Logger::Logger()
{
    mOutputStream.open(kLogFileName, ios_base::app);
    if (!mOutputStream.good()) {
        throw runtime_error("Unable to initialize the Logger!");
    }
}
void Logger::log(const string& inMessage, const string& inLogLevel)
{
    lock_guard<mutex> guard(sMutex);
    logHelper(inMessage, inLogLevel);
}
void Logger::log(const vector<string>& inMessages, const string& inLogLevel)
{
    lock_guard<mutex> guard(sMutex);
    for (size_t i = 0; i < inMessages.size(); i++) {
        logHelper(inMessages[i], inLogLevel);
    }
}
void Logger::logHelper(const std::string& inMessage,
    const std::string& inLogLevel)
{
    mOutputStream << inLogLevel << ": " << inMessage << endl;
}

It goes on to explain why the friend class Cleanup is introduced:

The Cleanup class is there to make sure the single Logger instance gets deleted properly on program shutdown. This is necessary because this implementation is dynamically allocating the Logger instance by using the new operator in a block of code protected with a mutex. A static instance of the Cleanup class will be created the first time the instance() method is called. When the program terminates, the C++ runtime will destroy this static Cleanup instance, which will trigger the deletion of the Logger object and a call to the Logger destructor to close the file.

I find it very confusing that it states "This is necessary because...", as if there were no other alternative.

My questions:

1) Is it really necessary? Wouldn't it be enough to just have all the handling in the destructor?, like:

Logger::~Logger()
{
    {
        lock_guard<mutex> guard(Logger::sMutex);
        delete Logger::pInstance;
        Logger::pInstance = nullptr;
    }
    mOutputStream.close();
}

2) If the answer to 1) is "yes, it is indeed necessary!", I would like to know why.

1Professional C++, Second Edition by Marc Gregoire , Nicholas A. Solter , Scott J. Kleper Publisher: Wrox Published: October 2011

NathanOliver
  • 171,901
  • 28
  • 288
  • 402
levelont
  • 584
  • 4
  • 12

1 Answers1

1

Yes, this is needed in this case. Since the book used new and handed out a pointer there is no object that will go out of scope that will cause the destructor to fire. The only way to do so is to call delete somewhere on that pointer. Instead of requiring you to do that the Cleanup class was created to do that.

All of this though can be avoided if you use a Meyers Singleton. That uses a static variable of the singleton type and returns a pointer/reference to that. That will automatically be destroyed at the end of the program unlike the book version. A Meyers Singleton looks like:

class Singleton {
public:
    static Singleton* Instance() { static Singleton s; return &s; }
    Singleton(const Singleton&) = delete;
    void operator=(const Singleton&) = delete;
private:
    Singleton() = default;
};
NathanOliver
  • 171,901
  • 28
  • 288
  • 402
  • You answer leaves me wondering if the referred book is any good. I mean, I learned a lot from comparing its proposed variant of singleton against the Meyer's alternative, but when in need for a quick reference of design patterns, is there a book that documents implementations in their most accepted form? – levelont Sep 29 '17 at 12:58
  • @levelont Not sure but [here](http://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list) is a great place to start your search – NathanOliver Sep 29 '17 at 13:00