Merge pull request ClickHouse#59273 from kitaisreal/atomic-logger

Added AtomicLogger

Author: KochetovNicolai

base/base/defines.h

@@ -155,6 +155,7 @@
 #    define TSA_ACQUIRE_SHARED(...) __attribute__((acquire_shared_capability(__VA_ARGS__)))          /// function acquires a shared capability, but does not release it
 #    define TSA_TRY_ACQUIRE_SHARED(...) __attribute__((try_acquire_shared_capability(__VA_ARGS__)))  /// function tries to acquire a shared capability and returns a boolean value indicating success or failure
 #    define TSA_RELEASE_SHARED(...) __attribute__((release_shared_capability(__VA_ARGS__)))          /// function releases the given shared capability
+#    define TSA_SCOPED_LOCKABLE __attribute__((scoped_lockable))                                     /// object of a class has scoped lockable capability
 
 /// Macros for suppressing TSA warnings for specific reads/writes (instead of suppressing it for the whole function)
 /// They use a lambda function to apply function attribute to a single statement. This enable us to suppress warnings locally instead of
@@ -182,6 +183,7 @@
 #    define TSA_ACQUIRE_SHARED(...)
 #    define TSA_TRY_ACQUIRE_SHARED(...)
 #    define TSA_RELEASE_SHARED(...)
+#    define TSA_SCOPED_LOCKABLE(...)
 
 #    define TSA_SUPPRESS_WARNING_FOR_READ(x) (x)
 #    define TSA_SUPPRESS_WARNING_FOR_WRITE(x) (x)

base/poco/Foundation/include/Poco/Logger.h

@@ -904,13 +904,6 @@ class Foundation_API Logger : public Channel
     /// Returns a pointer to the Logger with the given name if it
     /// exists, or a null pointer otherwise.
 
-    static bool destroy(const std::string & name);
-    /// Destroys the logger with the specified name. Does nothing
-    /// if the logger is not found.
-    ///
-    /// After a logger has been destroyed, all references to it
-    /// become invalid.
-
     static void shutdown();
     /// Shuts down the logging framework and releases all
     /// Loggers.
@@ -940,8 +933,6 @@ class Foundation_API Logger : public Channel
     static const std::string ROOT; /// The name of the root logger ("").
 
 protected:
-    typedef std::map<std::string, Logger *> LoggerMap;
-
     Logger(const std::string & name, Channel * pChannel, int level);
     ~Logger();
 
@@ -962,8 +953,6 @@ class Foundation_API Logger : public Channel
     std::string _name;
     Channel * _pChannel;
     std::atomic_int _level;
-
-    static LoggerMap * _pLoggerMap;
 };
 
 

base/poco/Foundation/src/Logger.cpp

@@ -20,6 +20,7 @@
 #include "Poco/NumberParser.h"
 #include "Poco/String.h"
 
+#include <cassert>
 #include <mutex>
 
 namespace
@@ -37,12 +38,20 @@ std::mutex & getLoggerMutex()
 	return *logger_mutex;
 }
 
+struct LoggerEntry
+{
+	Poco::Logger * logger;
+	bool owned_by_shared_ptr = false;
+};
+
+using LoggerMap = std::unordered_map<std::string, LoggerEntry>;
+LoggerMap * _pLoggerMap = nullptr;
+
 }
 
 namespace Poco {
 
 
-Logger::LoggerMap* Logger::_pLoggerMap = 0;
 const std::string Logger::ROOT;
 
 
@@ -134,12 +143,12 @@ void Logger::setLevel(const std::string& name, int level)
 	if (_pLoggerMap)
 	{
 		std::string::size_type len = name.length();
-		for (LoggerMap::iterator it = _pLoggerMap->begin(); it != _pLoggerMap->end(); ++it)
+		for (auto & it : *_pLoggerMap)
 		{
 			if (len == 0 ||
-				(it->first.compare(0, len, name) == 0 && (it->first.length() == len || it->first[len] == '.')))
+				(it.first.compare(0, len, name) == 0 && (it.first.length() == len || it.first[len] == '.')))
 			{
-				it->second->setLevel(level);
+				it.second.logger->setLevel(level);
 			}
 		}
 	}
@@ -153,12 +162,12 @@ void Logger::setChannel(const std::string& name, Channel* pChannel)
 	if (_pLoggerMap)
 	{
 		std::string::size_type len = name.length();
-		for (LoggerMap::iterator it = _pLoggerMap->begin(); it != _pLoggerMap->end(); ++it)
+		for (auto & it : *_pLoggerMap)
 		{
 			if (len == 0 ||
-				(it->first.compare(0, len, name) == 0 && (it->first.length() == len || it->first[len] == '.')))
+				(it.first.compare(0, len, name) == 0 && (it.first.length() == len || it.first[len] == '.')))
 			{
-				it->second->setChannel(pChannel);
+				it.second.logger->setChannel(pChannel);
 			}
 		}
 	}
@@ -172,12 +181,12 @@ void Logger::setProperty(const std::string& loggerName, const std::string& prope
 	if (_pLoggerMap)
 	{
 		std::string::size_type len = loggerName.length();
-		for (LoggerMap::iterator it = _pLoggerMap->begin(); it != _pLoggerMap->end(); ++it)
+		for (auto & it : *_pLoggerMap)
 		{
 			if (len == 0 ||
-				(it->first.compare(0, len, loggerName) == 0 && (it->first.length() == len || it->first[len] == '.')))
+				(it.first.compare(0, len, loggerName) == 0 && (it.first.length() == len || it.first[len] == '.')))
 			{
-				it->second->setProperty(propertyName, value);
+				it.second.logger->setProperty(propertyName, value);
 			}
 		}
 	}
@@ -304,35 +313,84 @@ struct LoggerDeleter
 {
 	void operator()(Poco::Logger * logger)
 	{
-		if (Logger::destroy(logger->name()))
+		std::lock_guard<std::mutex> lock(getLoggerMutex());
+
+		/// If logger infrastructure is destroyed just decrement logger reference count
+		if (!_pLoggerMap)
+		{
+			logger->release();
 			return;
+		}
+
+		auto it = _pLoggerMap->find(logger->name());
+		assert(it != _pLoggerMap->end());
 
-		logger->release();
+		/** If reference count is 1, this means this shared pointer owns logger
+		  * and need destroy it.
+		  */
+		size_t reference_count_before_release = logger->release();
+		if (reference_count_before_release == 1)
+		{
+			assert(it->second.owned_by_shared_ptr);
+			_pLoggerMap->erase(it);
+		}
 	}
 };
 
+
 inline LoggerPtr makeLoggerPtr(Logger & logger)
 {
-	logger.duplicate();
 	return std::shared_ptr<Logger>(&logger, LoggerDeleter());
 }
 
 }
 
+
 Logger& Logger::get(const std::string& name)
 {
 	std::lock_guard<std::mutex> lock(getLoggerMutex());
 
-	return unsafeGet(name);
+	Logger & logger = unsafeGet(name);
+
+	/** If there are already shared pointer created for this logger
+	  * we need to increment Logger reference count and now logger
+	  * is owned by logger infrastructure.
+	  */
+	auto it = _pLoggerMap->find(name);
+	if (it->second.owned_by_shared_ptr)
+	{
+		it->second.logger->duplicate();
+		it->second.owned_by_shared_ptr = false;
+	}
+
+	return logger;
 }
 
+
 LoggerPtr Logger::getShared(const std::string & name)
 {
 	std::lock_guard<std::mutex> lock(getLoggerMutex());
+	bool logger_exists = _pLoggerMap && _pLoggerMap->contains(name);
 
-	return makeLoggerPtr(unsafeGet(name));
+	Logger & logger = unsafeGet(name);
+
+	/** If logger already exists, then this shared pointer does not own it.
+	  * If logger does not exists, logger infrastructure could be already destroyed
+	  * or logger was created.
+	  */
+	if (logger_exists)
+	{
+		logger.duplicate();
+	}
+	else if (_pLoggerMap)
+	{
+		_pLoggerMap->find(name)->second.owned_by_shared_ptr = true;
+	}
+
+	return makeLoggerPtr(logger);
 }
 
+
 Logger& Logger::unsafeGet(const std::string& name)
 {
 	Logger* pLogger = find(name);
@@ -364,7 +422,10 @@ LoggerPtr Logger::createShared(const std::string & name, Channel * pChannel, int
 {
 	std::lock_guard<std::mutex> lock(getLoggerMutex());
 
-	return makeLoggerPtr(unsafeCreate(name, pChannel, level));
+	Logger & logger = unsafeCreate(name, pChannel, level);
+	_pLoggerMap->find(name)->second.owned_by_shared_ptr = true;
+
+	return makeLoggerPtr(logger);
 }
 
 Logger& Logger::root()
@@ -389,10 +450,14 @@ void Logger::shutdown()
 
 	if (_pLoggerMap)
 	{
-		for (LoggerMap::iterator it = _pLoggerMap->begin(); it != _pLoggerMap->end(); ++it)
+		for (auto & it : *_pLoggerMap)
 		{
-			it->second->release();
+			if (it.second.owned_by_shared_ptr)
+				continue;
+
+			it.second.logger->release();
 		}
+
 		delete _pLoggerMap;
 		_pLoggerMap = 0;
 	}
@@ -405,32 +470,12 @@ Logger* Logger::find(const std::string& name)
 	{
 		LoggerMap::iterator it = _pLoggerMap->find(name);
 		if (it != _pLoggerMap->end())
-			return it->second;
+			return it->second.logger;
 	}
 	return 0;
 }
 
 
-bool Logger::destroy(const std::string& name)
-{
-	std::lock_guard<std::mutex> lock(getLoggerMutex());
-
-	if (_pLoggerMap)
-	{
-		LoggerMap::iterator it = _pLoggerMap->find(name);
-		if (it != _pLoggerMap->end())
-		{
-			if (it->second->release() == 1)
-				_pLoggerMap->erase(it);
-
-			return true;
-		}
-	}
-
-	return false;
-}
-
-
 void Logger::names(std::vector<std::string>& names)
 {
 	std::lock_guard<std::mutex> lock(getLoggerMutex());
@@ -538,7 +583,8 @@ void Logger::add(Logger* pLogger)
 {
 	if (!_pLoggerMap)
 		_pLoggerMap = new LoggerMap;
-	_pLoggerMap->insert(LoggerMap::value_type(pLogger->name(), pLogger));
+
+	_pLoggerMap->emplace(pLogger->name(), LoggerEntry{pLogger, false /*owned_by_shared_ptr*/});
 }
 
 

src/Common/AtomicLogger.h

@@ -0,0 +1,51 @@
+#pragma once
+
+#include <memory>
+
+#include <Common/Logger.h>
+#include <Common/SharedMutex.h>
+#include <Common/SharedLockGuard.h>
+
+
+/** AtomicLogger allows to atomically change logger.
+  * Standard library does not have atomic_shared_ptr, and we do not use std::atomic* operations,
+  * because standard library implementation uses fixed table of mutexes, and it is better to avoid contention here.
+  */
+class AtomicLogger
+{
+public:
+    explicit AtomicLogger(LoggerPtr logger_)
+        : logger(std::move(logger_))
+    {}
+
+    explicit AtomicLogger(const std::string & log_name)
+        : AtomicLogger(::getLogger(log_name))
+    {}
+
+    void store(LoggerPtr new_logger)
+    {
+        std::lock_guard lock(log_mutex);
+        logger = std::move(new_logger);
+    }
+
+    void store(const std::string & new_log_name)
+    {
+        auto new_logger = ::getLogger(new_log_name);
+        store(std::move(new_logger));
+    }
+
+    LoggerPtr load() const
+    {
+        DB::SharedLockGuard lock(log_mutex);
+        return logger;
+    }
+
+    String loadName() const
+    {
+        DB::SharedLockGuard lock(log_mutex);
+        return logger->name();
+    }
+private:
+    mutable DB::SharedMutex log_mutex;
+    LoggerPtr logger;
+};

src/Common/Exception.cpp

@@ -228,6 +228,11 @@ void tryLogCurrentException(LoggerPtr logger, const std::string & start_of_messa
     tryLogCurrentException(logger.get(), start_of_message);
 }
 
+void tryLogCurrentException(const AtomicLogger & logger, const std::string & start_of_message)
+{
+    tryLogCurrentException(logger.load(), start_of_message);
+}
+
 static void getNoSpaceLeftInfoMessage(std::filesystem::path path, String & msg)
 {
     path = std::filesystem::absolute(path);
@@ -497,6 +502,11 @@ void tryLogException(std::exception_ptr e, LoggerPtr logger, const std::string &
     }
 }
 
+void tryLogException(std::exception_ptr e, const AtomicLogger & logger, const std::string & start_of_message)
+{
+    tryLogException(e, logger.load(), start_of_message);
+}
+
 std::string getExceptionMessage(const Exception & e, bool with_stacktrace, bool check_embedded_stacktrace)
 {
     return getExceptionMessageAndPattern(e, with_stacktrace, check_embedded_stacktrace).text;

src/Common/Exception.h

@@ -8,6 +8,11 @@
 #include <Poco/Exception.h>
 
 #include <base/defines.h>
+#include <base/errnoToString.h>
+#include <base/scope_guard.h>
+#include <Common/LoggingFormatStringHelpers.h>
+#include <Common/Logger.h>
+#include <Common/AtomicLogger.h>
 #include <Common/StackTrace.h>
 #include <Common/LoggingFormatStringHelpers.h>
 
@@ -212,6 +217,7 @@ using Exceptions = std::vector<std::exception_ptr>;
 void tryLogCurrentException(const char * log_name, const std::string & start_of_message = "");
 void tryLogCurrentException(Poco::Logger * logger, const std::string & start_of_message = "");
 void tryLogCurrentException(LoggerPtr logger, const std::string & start_of_message = "");
+void tryLogCurrentException(const AtomicLogger & logger, const std::string & start_of_message = "");
 
 
 /** Prints current exception in canonical format.
@@ -258,6 +264,7 @@ struct ExecutionStatus
 void tryLogException(std::exception_ptr e, const char * log_name, const std::string & start_of_message = "");
 void tryLogException(std::exception_ptr e, Poco::Logger * logger, const std::string & start_of_message = "");
 void tryLogException(std::exception_ptr e, LoggerPtr logger, const std::string & start_of_message = "");
+void tryLogException(std::exception_ptr e, const AtomicLogger & logger, const std::string & start_of_message = "");
 
 std::string getExceptionMessage(const Exception & e, bool with_stacktrace, bool check_embedded_stacktrace = false);
 PreformattedMessage getExceptionMessageAndPattern(const Exception & e, bool with_stacktrace, bool check_embedded_stacktrace = false);