diff --git a/es-core/src/Log.cpp b/es-core/src/Log.cpp index 02b9468c4..07dbb8740 100644 --- a/es-core/src/Log.cpp +++ b/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 lock {sLogMutex}; + return sReportingLevel; +} + +void Log::setReportingLevel(LogLevel level) +{ + // Static Log functions need to grab the lock. + std::unique_lock 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 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 lock {sLogMutex}; + sFile.flush(); +} + +void Log::close() +{ + // Static Log functions need to grab the lock. + std::unique_lock 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 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, " diff --git a/es-core/src/Log.h b/es-core/src/Log.h index 3319fe771..833de1603 100644 --- a/es-core/src/Log.h +++ b/es-core/src/Log.h @@ -15,6 +15,7 @@ #include #include #include +#include #include #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; };