Alternative to the Singleton for a Logger class

Question

I am designing a shared library that will mainly provide a class with specific logging capabilities for client applications. I would rather not presume how this class might be used to be more generic.

The responsibility of the Logger class is to encapsulate application datas with a header and a tailer and write a line to a single file where the file path is specified by the application but not its name.

A first (simplified) skeleton of the Logger class could be :

class Logger
{
public:
     Logger(const QString &filepath);
     bool log(const QString &datas);
     ...

};

As the application can use this Logger class anywhere and in any thread, i would like to ensure that the filepath is set only once by the application and that it is thread safe to protected concurrent file access.

Singleton pattern seems a good candidate for this class but i ask myself 3 questions :

How to make it thread safe without using C++11 or more recent compilers ?

The second point is that filepath should be set only once by the application. With Singleton, we still can add a filepath in getInstance(const &QString filepath) but even if this ensure that the filepath won't change, it might be called in the application with different arguments leading to

// let's say this is the firstcall so filepath = filepath1 and won't change
Logger::getInstance(filepath1); 
...
Logger::getInstance(filepath2); // Ok we still have filepath = filepath1 
... // and so on from anywhere in the code and possibly from different threads

This will work. But from the application point of view, i find this not clear because a developer might think that it creates a log in filepath1 and filepath2 but he is not. Moreover, each time the client has to call the singleton, he has to pass all arguments. Ideally, filepath should be set only once in the client code, and then it simply call the log method. How to make it more clear for the client application ?

The last point is about the singleton pattern itself : i would rather avoid it especially in a multi threaded context as a good thread safe implementation is not as easy as it seems to be. Am i missing something obvious ?

Thank you.

p-a-o-l-o · Answer 1 · 2018-02-01T21:25:00.610

Since you're using Qt, the multithreading issue can be solved this way:

class Logger
{
public:
     bool log(const QString &datas)
     {
         QMutexLocker lock(&Logger::mutex);
         //log ...
     }

private:
    static QMutex mutex;
};

So you can just do:

Logger().log("some data");

About the path, you can have a static private QString to hold it

private:
    static QString path;

and a method to set its value

 bool setPath(QString path)
 {
     QMutexLocker lock(&Logger::mutex);
     if(Logger::path.isEmpty())
     {
        Logger::path = path;
        return true;
     }
     return false;
 }

The method has a bool return type, just to notify if the path was already set, and it could be static to remark its application-wide nature.

A better solution using a configuration file:

     bool log(const QString &datas)
     {
         QMutexLocker lock(&Logger::mutex);
         if(Logger::path.isEmpty())
              loadPathFromConfiguration();
         //log ...
     }

A more complicated pattern:

class Logger
{
public:
     bool log(const QString &datas)
     {
         QMutexLocker lock(&Logger::mutex);
         if(!path().isEmpty())
         {
             //log ...
         }
     }

     virtual QString path() const = 0;

private:
    static QMutex mutex;
};

This way the user is forced to provide its own implementation, and could not possibly think that there can be more than one log file.

So basically you protect the log function from concurrent calls but you don't consider that the Logger class should be a singleton is it right ? — Fryz, Feb 01 '18 at 20:14

Kuba hasn't forgotten Monica · Answer 2 · 2018-02-10T01:31:42.143

First of all, in order to prevent the fallout from the static order initialization fiasco, you should have full control over when objects come to life. The logger should be created in main; perhaps in a factory that builds your application from components but is instantiated from main. A dependency injection framework could help with that. The logger is then a singleton, but its point of instantiation is explicitly controlled. You avoid any threaded initialization issues, then:

int main(int argc, char ** argv) {
  QApplication app(argc, argv);
  app.setApplicationName(...);
  // set other application identification data
  Logger logger; // e.g. reads log location from QSettings
  ...
}

At this point, you can either use Logger methods, or can leverage the global qDebug/qWarning/qInfo system by installing a message handler.

One way of sending messages thread-safely would be via the thread-safe signal-slot mechanism:

// interface

class Logger : public QObject {
  Q_OBJECT
  static QReadWriteLock m_lock{QReadWriteLock::Recursive};
  static Logger * m_instance;
  static QThread * m_initialThread;
  QFile m_file;
  int m_flushInterval = 25, m_flushCountdown = {};
  // this function is thread-safe
  static void log(QtMsgType type, const QMessageLogContext &context, const QString &msg) {
     QString output;
     /* format the message into output here */
     QReadLock lock(&m_lock);
     if (m_instance) emit m_instance->log_req(output);
  }
  Q_SIGNAL void log_req(const QString &);
  Q_SIGNAL void finish_req();
  void log_ind(const QString & entry) {
    m_file.write(entry.toLocal8Bit());
    m_file.write("\r\n");
    if (--m_flushCountdown <= 0) {
      m_file.flush();
      m_flushCountdown = m_flushInterval;
    }
  }
  void finish_ind() {
    QReadLock lock(&m_lock);
    moveToThread(m_initialThread);
  }
public:
  Logger(const QString & path, QObject * parent = {}) : QObject(parent) {
    m_file.setFileName(path);
    // etc.
    connect(this, &Logger::log_req, this, &Logger::log_ind);
    connect(this, &Logger::finish_req, this, &Logger::finish_ind, Qt::BlockingQueuedConnection);
    QWriteLock lock(&m_lock);
    Q_ASSERT(!m_instance);
    m_instance = this;
    m_initialThread = QThread::currentThread();
    m_handler = qInstallMessageHandler(&Logger::log);
  }
  ~Logger() {
    Q_ASSERT(currentThread() == m_initialThread);
    if (thread() != m_initialThread)
      finish_req(); // move ourselves back to the instantiating thread
    QWriteLock lock(&m_lock);
    qInstallMessageHandler(m_handler);
    m_instance = {};
  }
  static void log(const QString & output) {
     QReadLock lock(&m_lock);
     if (m_instance) emit m_instance->log_req(output);
  }
};

// implementation

QReadWriteLock Logger::m_lock;
Logger * Logger::m_instance;
QThread * Logger::m_initialThread;

The above also needs some way of reacting to errors. Is operation without a usable log output acceptable? - the reaction to errors will depend on that.

Qt then handles thread-safety for you automatically, and the only thing you yourself have to handle is the thread-safe access to the instance pointer and other static members. Specifically, you have to avoid the race between test and use, i.e. the following is racy and usafe: if (m_instance) m_instance->method();

It is simple then to put the logger into its own dedicated thread:

class SafeThread : public Thread {
public:
  ~SafeThread() { quit(); wait(); }
};

int main(int argc, char ** argv) {
  ...
  SafeThread loggerThread;
  loggerThread.start();

  Logger logger;
  logger.moveToThread(&loggerThread);
  ... 
}

The logger instance will be destroyed first. The destructor will request the instance move itself out of its thread back to the thread where it was instantiated, and then proceed with destruction. The SafeThread is a RAII mod of QThread and will destruct itself safely as well.

Alternative to the Singleton for a Logger class

2 Answers2