Increase robustness of the logging subsystem

Tools/MCADefrag/MCADefrag.cpp

@@ -22,26 +22,26 @@ static const Byte g_Zeroes[4096] = {0};
 
 int main(int argc, char ** argv)
 {
-	cLogger::cListener * consoleLogListener = MakeConsoleListener(false);
+	auto consoleLogListener = MakeConsoleListener(false);
-	cLogger::cListener * fileLogListener = new cFileListener();
+	auto consoleAttachment = cLogger::GetInstance().AttachListener(std::move(consoleLogListener));
-	cLogger::GetInstance().AttachListener(consoleLogListener);
+	auto fileLogListenerRet = MakeFileListener();
-	cLogger::GetInstance().AttachListener(fileLogListener);
+	if (!fileLogListenerRet.first)
+	{
+		LOGERROR("Failed to open log file, aborting");
+		return EXIT_FAILURE;
+	}
+	auto fileAttachment = cLogger::GetInstance().AttachListener(std::move(fileLogListenerRet.second));
 	
 	cLogger::InitiateMultithreading();
 	
 	cMCADefrag Defrag;
 	if (!Defrag.Init(argc, argv))
 	{
-		return 1;
+		return EXIT_FAILURE;
 	}
 	
 	Defrag.Run();
 	
-	cLogger::GetInstance().DetachListener(consoleLogListener);
-	delete consoleLogListener;
-	cLogger::GetInstance().DetachListener(fileLogListener);
-	delete fileLogListener;
-	
 	return 0;
 }
 

Tools/ProtoProxy/ProtoProxy.cpp

@@ -15,11 +15,17 @@
 int main(int argc, char ** argv)
 {
 	// Initialize logging subsystem:
-	cLogger::InitiateMultithreading();
 	auto consoleLogListener = MakeConsoleListener(false);
-	auto fileLogListener = new cFileListener();
+	auto consoleAttachment = cLogger::GetInstance().AttachListener(std::move(consoleLogListener));
-	cLogger::GetInstance().AttachListener(consoleLogListener);
+	auto fileLogListenerRet = MakeFileListener();
-	cLogger::GetInstance().AttachListener(fileLogListener);
+	if (!fileLogListenerRet.first)
+	{
+		LOGERROR("Failed to open log file, aborting");
+		return EXIT_FAILURE;
+	}
+	auto fileAttachment = cLogger::GetInstance().AttachListener(std::move(fileLogListenerRet.second));
+
+	cLogger::InitiateMultithreading();
 
 	int ListenPort  = (argc > 1) ? atoi(argv[1]) : 25564;
 	int ConnectPort = (argc > 2) ? atoi(argv[2]) : 25565;

src/CompositeChat.h

@@ -5,6 +5,7 @@
 
 #include "Defines.h"
 #include "json/json.h"
+#include "Logger.h"
 
 
 

src/Globals.h

@@ -271,7 +271,25 @@ template class SizeChecker<UInt8,  1>;
 #include "OSSupport/StackTrace.h"
 
 #ifndef TEST_GLOBALS
-	#include "Logger.h"
+
+// These fiunctions are defined in Logger.cpp, but are declared here to avoid including all of logger.h
+extern void LOG       (const char * a_Format, ...) FORMATSTRING(1, 2);
+extern void LOGINFO   (const char * a_Format, ...) FORMATSTRING(1, 2);
+extern void LOGWARNING(const char * a_Format, ...) FORMATSTRING(1, 2);
+extern void LOGERROR  (const char * a_Format, ...) FORMATSTRING(1, 2);
+
+
+
+
+
+// In debug builds, translate LOGD to LOG, otherwise leave it out altogether:
+#ifdef _DEBUG
+	#define LOGD LOG
+#else
+	#define LOGD(...)
+#endif  // _DEBUG
+
+#define LOGWARN LOGWARNING
 #else
 	// Logging functions
 void inline LOGERROR(const char * a_Format, ...) FORMATSTRING(1, 2);

src/Logger.cpp

@@ -1,5 +1,6 @@
 
 #include "Globals.h"  // NOTE: MSVC stupidness requires this to be the same across all modules
+#include "Logger.h"
 
 #include "OSSupport/IsThread.h"
 #ifdef _WIN32
@@ -73,10 +74,14 @@ void cLogger::Log(const char * a_Format, eLogLevel a_LogLevel, va_list a_ArgList
 
 
 
-void cLogger::AttachListener(cListener * a_Listener)
+cLogger::cAttachment cLogger::AttachListener(std::unique_ptr<cListener> a_Listener)
 {
-	cCSLock Lock(m_CriticalSection);
+	auto nonOwning = a_Listener.get();
-	m_LogListeners.push_back(a_Listener);
+	{
+		cCSLock Lock(m_CriticalSection);
+		m_LogListeners.push_back(std::move(a_Listener));
+	}
+	return cAttachment{nonOwning};
 }
 
 
@@ -86,7 +91,16 @@ void cLogger::AttachListener(cListener * a_Listener)
 void cLogger::DetachListener(cListener * a_Listener)
 {
 	cCSLock Lock(m_CriticalSection);
-	m_LogListeners.erase(std::remove(m_LogListeners.begin(), m_LogListeners.end(), a_Listener));
+	m_LogListeners.erase(
+		std::remove_if(
+			m_LogListeners.begin(),
+			m_LogListeners.end(),
+			[=](std::unique_ptr<cListener> & a_OtherListener) -> bool
+			{
+				return a_OtherListener.get() == a_Listener;
+			}
+		)
+	);
 }
 
 
@@ -120,7 +134,7 @@ void LOGINFO(const char * a_Format, ...)
 
 
 
-void LOGWARN(const char * a_Format, ...)
+void LOGWARNING(const char * a_Format, ...)
 {
 	va_list argList;
 	va_start(argList, a_Format);

src/Logger.h

@@ -23,13 +23,35 @@ public:
 		virtual ~cListener(){}
 	};
 
+	class cAttachment
+	{
+		public:
+
+		cAttachment(cAttachment && a_other)
+			: m_listener(a_other.m_listener)
+		{
+		}
+
+		~cAttachment()
+		{
+			cLogger::GetInstance().DetachListener(m_listener);
+		}
+
+		private:
+
+		cListener * m_listener;
+
+		friend class cLogger;
+
+		cAttachment(cListener * a_listener) : m_listener(a_listener) {}
+	};
+
 	void Log  (const char * a_Format, eLogLevel a_LogLevel, va_list a_ArgList) FORMATSTRING(2, 0);
 
 	/** Logs the simple text message at the specified log level. */
 	void LogSimple(AString a_Message, eLogLevel a_LogLevel = llRegular);
 
-	void AttachListener(cListener * a_Listener);
+	cAttachment AttachListener(std::unique_ptr<cListener> a_Listener);
-	void DetachListener(cListener * a_Listener);
 	
 	static cLogger & GetInstance(void);
 	// Must be called before calling GetInstance in a multithreaded context
@@ -37,37 +59,20 @@ public:
 private:
 
 	cCriticalSection m_CriticalSection;
-	std::vector<cListener *> m_LogListeners;
+	std::vector<std::unique_ptr<cListener>> m_LogListeners;
+
+	void DetachListener(cListener * a_Listener);
 
 };
 
 
 
 
-
+// These declarations are duplicated in globals.h
-
+extern void LOG       (const char * a_Format, ...) FORMATSTRING(1, 2);
-extern void LOG     (const char * a_Format, ...) FORMATSTRING(1, 2);
+extern void LOGINFO   (const char * a_Format, ...) FORMATSTRING(1, 2);
-extern void LOGINFO (const char * a_Format, ...) FORMATSTRING(1, 2);
+extern void LOGWARNING(const char * a_Format, ...) FORMATSTRING(1, 2);
-extern void LOGWARN (const char * a_Format, ...) FORMATSTRING(1, 2);
+extern void LOGERROR  (const char * a_Format, ...) FORMATSTRING(1, 2);
-extern void LOGERROR(const char * a_Format, ...) FORMATSTRING(1, 2);
-
-
-
-
-
-// In debug builds, translate LOGD to LOG, otherwise leave it out altogether:
-#ifdef _DEBUG
-	#define LOGD LOG
-#else
-	#define LOGD(...)
-#endif  // _DEBUG
-
-
-
-
-
-#define LOGWARNING LOGWARN
-
 
 
 

src/LoggerListeners.cpp

@@ -251,11 +251,11 @@ class cNullConsoleListener
 
 
 
-cLogger::cListener * MakeConsoleListener(bool a_IsService)
+std::unique_ptr<cLogger::cListener> MakeConsoleListener(bool a_IsService)
 {
 	if (a_IsService)
 	{
-		return new cNullConsoleListener;
+		return cpp14::make_unique<cNullConsoleListener>();
 	}
 
 	#ifdef _WIN32
@@ -267,25 +267,25 @@ cLogger::cListener * MakeConsoleListener(bool a_IsService)
 			HANDLE Console = GetStdHandle(STD_OUTPUT_HANDLE);
 			GetConsoleScreenBufferInfo(Console, &sbi);
 			WORD DefaultConsoleAttrib = sbi.wAttributes;
-			return new cWindowsConsoleListener(Console, DefaultConsoleAttrib);
+			return cpp14::make_unique<cWindowsConsoleListener>(Console, DefaultConsoleAttrib);
 		}
 		else
 		{
-			return new cVanillaCPPConsoleListener;
+			return cpp14::make_unique<cVanillaCPPConsoleListener>();
 		}
 		
 	#elif defined (__linux) && !defined(ANDROID_NDK)
 		// TODO: lookup terminal in terminfo
 		if (isatty(fileno(stdout)))
 		{
-			return new cLinuxConsoleListener();
+			return cpp14::make_unique<cLinuxConsoleListener>();
 		}
 		else
 		{
-			return new cVanillaCPPConsoleListener();
+			return cpp14::make_unique<cVanillaCPPConsoleListener>();
 		}
 	#else
-		return new cVanillaCPPConsoleListener();
+		return cpp14::make_unique<cVanillaCPPConsoleListener>();
 	#endif
 }
 
@@ -296,60 +296,82 @@ cLogger::cListener * MakeConsoleListener(bool a_IsService)
 ////////////////////////////////////////////////////////////////////////////////
 // cFileListener:
 
-cFileListener::cFileListener(void)
+class cFileListener
+	: public cLogger::cListener
 {
-	cFile::CreateFolder(FILE_IO_PREFIX + AString("logs"));
+public:
-	m_File.Open(
+
-		FILE_IO_PREFIX + Printf(
+	cFileListener(void) {}
-			"logs/LOG_%d.txt",
+
-			std::chrono::duration_cast<std::chrono::duration<int, std::ratio<1>>>(
+	bool Open()
-				std::chrono::system_clock::now().time_since_epoch()
+	{
-			).count()
+		// Assume creation succeeds, as the API does not provide a way to tell if the folder exists.
-		),
+		cFile::CreateFolder(FILE_IO_PREFIX + AString("logs"));
-		cFile::fmAppend
+		bool success = m_File.Open(
-	);
+			FILE_IO_PREFIX + Printf(
+				"logs/LOG_%d.txt",
+				std::chrono::duration_cast<std::chrono::duration<int, std::ratio<1>>>(
+					std::chrono::system_clock::now().time_since_epoch()
+				).count()
+			),
+			cFile::fmAppend
+		);
+		return success;
+	}
+
+	virtual void Log(AString a_Message, cLogger::eLogLevel a_LogLevel) override
+	{
+		const char * LogLevelPrefix = "Unkn ";
+		bool ShouldFlush = false;
+		switch (a_LogLevel)
+		{
+			case cLogger::llRegular:
+			{
+				LogLevelPrefix = "     ";
+				break;
+			}
+			case cLogger::llInfo:
+			{
+				LogLevelPrefix = "Info ";
+				break;
+			}
+			case cLogger::llWarning:
+			{
+				LogLevelPrefix = "Warn ";
+				ShouldFlush = true;
+				break;
+			}
+			case cLogger::llError:
+			{
+				LogLevelPrefix = "Err  ";
+				ShouldFlush = true;
+				break;
+			}
+		}
+		m_File.Printf("%s%s", LogLevelPrefix, a_Message.c_str());
+		if (ShouldFlush)
+		{
+			m_File.Flush();
+		}
+	}
+	
+private:
+
+	cFile m_File;
+};
+
+
+
+
+
+std::pair<bool, std::unique_ptr<cLogger::cListener>> MakeFileListener()
+{
+	auto listener = cpp14::make_unique<cFileListener>();
+	if (!listener->Open())
+	{
+		return {false, nullptr};
+	}
+	return {true, std::move(listener)};
 }
 
 
-
-
-
-void cFileListener::Log(AString a_Message, cLogger::eLogLevel a_LogLevel)
-{
-	const char * LogLevelPrefix = "Unkn ";
-	bool ShouldFlush = false;
-	switch (a_LogLevel)
-	{
-		case cLogger::llRegular:
-		{
-			LogLevelPrefix = "     ";
-			break;
-		}
-		case cLogger::llInfo:
-		{
-			LogLevelPrefix = "Info ";
-			break;
-		}
-		case cLogger::llWarning:
-		{
-			LogLevelPrefix = "Warn ";
-			ShouldFlush = true;
-			break;
-		}
-		case cLogger::llError:
-		{
-			LogLevelPrefix = "Err  ";
-			ShouldFlush = true;
-			break;
-		}
-	}
-	m_File.Printf("%s%s", LogLevelPrefix, a_Message.c_str());
-	if (ShouldFlush)
-	{
-		m_File.Flush();
-	}
-}
-
-
-
-

src/LoggerListeners.h

@@ -2,30 +2,8 @@
 #include "Logger.h"
 #include "OSSupport/File.h"
 
-
+std::unique_ptr<cLogger::cListener> MakeConsoleListener(bool a_IsService);
-
+std::pair<bool, std::unique_ptr<cLogger::cListener>> MakeFileListener();
-
-
-class cFileListener
-	: public cLogger::cListener
-{
-public:
-
-	cFileListener();
-	cFileListener(AString a_Filename);
-
-	virtual void Log(AString a_Message, cLogger::eLogLevel a_LogLevel) override;
-	
-private:
-
-	cFile m_File;
-};
-
-
-
-
-
-cLogger::cListener * MakeConsoleListener(bool a_IsService);
 
 
 

src/Root.cpp

@@ -22,6 +22,7 @@
 #include "SettingsRepositoryInterface.h"
 #include "OverridesSettingsRepository.h"
 #include "SelfTests.h"
+#include "Logger.h"
 
 #include <iostream>
 
@@ -107,10 +108,15 @@ void cRoot::Start(std::unique_ptr<cSettingsRepositoryInterface> a_OverridesRepo)
 		EnableMenuItem(ConsoleMenu, SC_CLOSE, MF_GRAYED);  // Disable close button when starting up; it causes problems with our CTRL-CLOSE handling
 	#endif
 
-	cLogger::cListener * consoleLogListener = MakeConsoleListener(m_RunAsService);
+	auto consoleLogListener = MakeConsoleListener(m_RunAsService);
-	cLogger::cListener * fileLogListener = new cFileListener();
+	auto consoleAttachment = cLogger::GetInstance().AttachListener(std::move(consoleLogListener));
-	cLogger::GetInstance().AttachListener(consoleLogListener);
+	auto fileLogListenerRet = MakeFileListener();
-	cLogger::GetInstance().AttachListener(fileLogListener);
+	if (!fileLogListenerRet.first)
+	{
+		LOGERROR("Failed to open log file, aborting");
+		return;
+	}
+	auto fileAttachment = cLogger::GetInstance().AttachListener(std::move(fileLogListenerRet.second));
 
 	LOG("--- Started Log ---");
 
@@ -317,11 +323,6 @@ void cRoot::Start(std::unique_ptr<cSettingsRepositoryInterface> a_OverridesRepo)
 		LOG("Shutdown successful - restarting...");
 	}
 	LOG("--- Stopped Log ---");
-
-	cLogger::GetInstance().DetachListener(consoleLogListener);
-	delete consoleLogListener;
-	cLogger::GetInstance().DetachListener(fileLogListener);
-	delete fileLogListener;
 }
 
 

src/World.cpp

@@ -255,8 +255,6 @@ int cWorld::GetDefaultWeatherInterval(eWeather a_Weather)
 			return 2400 + (m_TickRand.randInt() % 4800);   // 2 - 6 minutes
 		}
 	}
-	LOGWARNING("%s: Missing default weather interval for weather %d.", __FUNCTION__, a_Weather);
-	return -1;
 }
 
 
@@ -646,10 +644,6 @@ eWeather cWorld::ChooseNewWeather()
 			return ((m_TickRand.randInt() % 256) < 32) ? eWeather_ThunderStorm : eWeather_Sunny;
 		}
 	}
-	
-	LOGWARNING("Unknown current weather: %d. Setting sunny.", m_Weather);
-	ASSERT(!"Unknown weather");
-	return eWeather_Sunny;
 }
 
 

src/main.cpp

@@ -14,6 +14,7 @@
 
 #include "OSSupport/NetworkSingleton.h"
 #include "BuildInfo.h"
+#include "Logger.h"
 
 #include "MemorySettingsRepository.h"