Make Log() instances only lock in ~Log(), and make the mutex non-recursive.

es-core/src/Log.cpp

@@ -12,14 +12,14 @@
 LogLevel Log::getReportingLevel()
 {
     // Static Log functions need to grab the lock.
-    std::unique_lock<std::recursive_mutex> lock {sLogMutex};
+    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::recursive_mutex> lock {sLogMutex};
+    std::unique_lock<std::mutex> lock {sLogMutex};
     sReportingLevel = level;
 }
 
@@ -49,7 +49,7 @@ void Log::init()
 void Log::open()
 {
     // Static Log functions need to grab the lock.
-    std::unique_lock<std::recursive_mutex> lock {sLogMutex};
+    std::unique_lock<std::mutex> lock {sLogMutex};
 #if defined(_WIN64)
     sFile.open(Utils::String::stringToWideString(getLogPath()).c_str());
 #else
@@ -57,9 +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, lock is guarded by the Log() instance.
+    // 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)
@@ -75,34 +91,19 @@ std::ostringstream& Log::get(LogLevel level)
     return mOutStringStream;
 }
 
-void Log::flush()
-{
-    // Flush file. Static Log functions need to grab the lock.
-    std::unique_lock<std::recursive_mutex> lock {sLogMutex};
-    sFile.flush();
-}
-
-void Log::close()
-{
-    // Static Log functions need to grab the lock.
-    std::unique_lock<std::recursive_mutex> lock {sLogMutex};
-    if (sFile.is_open())
-        sFile.close();
-}
-
-Log::Log()
-{
-    // Log instance created. We grab the lock until destruction.
-    // This permits `Log().get(...) << msg << msg << msg;` to
-    // function as expected.
-    sLogMutex.lock();
-}
-
 Log::~Log()
 {
-    // sLogMutex was (and still is) locked from the constructor 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, "
@@ -116,8 +117,4 @@ Log::~Log()
     // If it's an error or the --debug flag has been set, then print to the console as well.
     if (mMessageLevel == LogError || sReportingLevel >= LogDebug)
         std::cerr << mOutStringStream.str();
-
-    // Release the lock, after any and all operations have been performed
-    // on mOutStringStream or sFile.
-    sLogMutex.unlock();
 }

es-core/src/Log.h

@@ -34,11 +34,14 @@ enum LogLevel {
 class Log
 {
 public:
-    // Constructor/deconstructor handle a lock, making get() thread-safe.
-    Log();
+    // 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);
 
+    // Lock is grabbed for sReportingLevel.
+    // This means level > Log::getReportingLevel() still requires the lock.
     static LogLevel getReportingLevel();
     static void setReportingLevel(LogLevel level);
 
@@ -46,10 +49,11 @@ public:
     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 open();
     static void close();
 
 protected:
@@ -63,7 +67,7 @@ private:
                                                   {LogDebug, "Debug"}};
     static inline std::ofstream sFile;
     static inline LogLevel sReportingLevel = LogInfo;
-    static inline std::recursive_mutex sLogMutex;
+    static inline std::mutex sLogMutex;
     LogLevel mMessageLevel;
 };