Merge branch 'threadsafe_logging'

es-core/src/Log.cpp

@@ -9,8 +9,37 @@
 #include "Log.h"
 #include "utils/StringUtil.h"
 
+LogLevel Log::getReportingLevel()
+{
+    // Static Log functions need to grab the lock.
+    std::unique_lock<std::mutex> lock {sLogMutex};
+    return sReportingLevel;
+}
+
+void Log::setReportingLevel(LogLevel level)
+{
+    // Static Log functions need to grab the lock.
+    std::unique_lock<std::mutex> lock {sLogMutex};
+    sReportingLevel = level;
+}
+
+std::string Log::getLogPath()
+{
+    // No attempt is made to make this thread-safe.
+    // Currently getLogPath is public, and called in contexts with
+    // and without sLogMutex locked.
+
+    // getHomePath() currently does not generate any Log messages.
+    return Utils::FileSystem::getHomePath() + "/.emulationstation/es_log.txt";
+}
+
 void Log::init()
 {
+    // No attempt is made to make this thread-safe.
+    // It is unlikely to be called across multiple threads.
+    // Both removeFile and renameFile might generate log messages,
+    // so they might try to grab the lock.
+
     Utils::FileSystem::removeFile(getLogPath() + ".bak");
     // Rename the previous log file.
     Utils::FileSystem::renameFile(getLogPath(), getLogPath() + ".bak", true);
@@ -19,6 +48,8 @@ void Log::init()
 
 void Log::open()
 {
+    // Static Log functions need to grab the lock.
+    std::unique_lock<std::mutex> lock {sLogMutex};
 #if defined(_WIN64)
     sFile.open(Utils::String::stringToWideString(getLogPath()).c_str());
 #else
@@ -26,8 +57,25 @@ void Log::open()
 #endif
 }
 
+void Log::flush()
+{
+    // Flush file. Static Log functions need to grab the lock.
+    std::unique_lock<std::mutex> lock {sLogMutex};
+    sFile.flush();
+}
+
+void Log::close()
+{
+    // Static Log functions need to grab the lock.
+    std::unique_lock<std::mutex> lock {sLogMutex};
+    if (sFile.is_open())
+        sFile.close();
+}
+
 std::ostringstream& Log::get(LogLevel level)
 {
+    // This function is not-static, but only touches instance member variables.
+    // The lock is not needed.
     time_t t {time(nullptr)};
     struct tm tm;
 #if defined(_WIN64)
@@ -43,22 +91,19 @@ std::ostringstream& Log::get(LogLevel level)
     return mOutStringStream;
 }
 
-void Log::flush()
-{
-    // Flush file.
-    sFile.flush();
-}
-
-void Log::close()
-{
-    if (sFile.is_open())
-        sFile.close();
-}
-
 Log::~Log()
 {
     mOutStringStream << std::endl;
 
+    // Log() constructor does not need the lock.
+    // Log::get() does not need the lock.
+    // get(..) << msg << msg also does not need the lock,
+    // since get() returns the instance member mOutStringStream.
+
+    // It is only now that we need the lock, to guard access to
+    // both std::cerr and sFile.
+    std::unique_lock<std::mutex> lock {sLogMutex};
+
     if (!sFile.is_open()) {
         // Not open yet, print to stdout.
         std::cerr << "Error: Tried to write to log file before it was open, "

es-core/src/Log.h

@@ -15,6 +15,7 @@
 #include <iomanip>
 #include <iostream>
 #include <map>
+#include <mutex>
 #include <sstream>
 
 #define LOG(level)                                                                                 \
@@ -33,19 +34,26 @@ enum LogLevel {
 class Log
 {
 public:
+    // Deconstructor grabs the lock.
     ~Log();
+
+    // No lock needed for get() as it operates on and returns non-static members.
     std::ostringstream& get(LogLevel level = LogInfo);
 
-    static LogLevel getReportingLevel() { return sReportingLevel; }
-    static void setReportingLevel(LogLevel level) { sReportingLevel = level; }
-    static std::string getLogPath()
-    {
-        return Utils::FileSystem::getHomePath() + "/.emulationstation/es_log.txt";
-    }
+    // Lock is grabbed for sReportingLevel.
+    // This means level > Log::getReportingLevel() still requires the lock.
+    static LogLevel getReportingLevel();
+    static void setReportingLevel(LogLevel level);
 
-    static void flush();
+    // getLogPath() is not thread-safe.
+    static std::string getLogPath();
+    // init() is not thread-safe.
     static void init();
+    // open() is not fully thread-safe, as it uses getLogPath().
     static void open();
+
+    // The following static functions are thread-safe.
+    static void flush();
     static void close();
 
 protected:
@@ -59,6 +67,7 @@ private:
                                                   {LogDebug, "Debug"}};
     static inline std::ofstream sFile;
     static inline LogLevel sReportingLevel = LogInfo;
+    static inline std::mutex sLogMutex;
     LogLevel mMessageLevel;
 };