From 88f13817398972e57a069b9eaa326c9146fc6042 Mon Sep 17 00:00:00 2001 From: Maksim Kita Date: Tue, 30 Jan 2024 14:43:53 +0300 Subject: [PATCH] Poco Logger small refactoring --- base/poco/Foundation/include/Poco/Logger.h | 32 ++--- base/poco/Foundation/src/Logger.cpp | 111 ++++++++++-------- .../ObjectStorages/ObjectStorageFactory.cpp | 2 +- .../ObjectStorages/Web/WebObjectStorage.cpp | 2 +- 4 files changed, 83 insertions(+), 64 deletions(-) diff --git a/base/poco/Foundation/include/Poco/Logger.h b/base/poco/Foundation/include/Poco/Logger.h index cf202718662..5bc099bbb42 100644 --- a/base/poco/Foundation/include/Poco/Logger.h +++ b/base/poco/Foundation/include/Poco/Logger.h @@ -876,16 +876,6 @@ public: /// If the Logger does not yet exist, it is created, based /// on its parent logger. - static Logger & unsafeGet(const std::string & name); - /// Returns a reference to the Logger with the given name. - /// If the Logger does not yet exist, it is created, based - /// on its parent logger. - /// - /// WARNING: This method is not thread safe. You should - /// probably use get() instead. - /// The only time this method should be used is during - /// program initialization, when only one thread is running. - static Logger & create(const std::string & name, Channel * pChannel, int level = Message::PRIO_INFORMATION); /// Creates and returns a reference to a Logger with the /// given name. The Logger's Channel and log level as set as @@ -932,6 +922,16 @@ public: static const std::string ROOT; /// The name of the root logger (""). +public: + struct LoggerEntry + { + Poco::Logger * logger; + bool owned_by_shared_ptr = false; + }; + + using LoggerMap = std::unordered_map; + using LoggerMapIterator = LoggerMap::iterator; + protected: Logger(const std::string & name, Channel * pChannel, int level); ~Logger(); @@ -940,12 +940,16 @@ protected: void log(const std::string & text, Message::Priority prio, const char * file, int line); static std::string format(const std::string & fmt, int argc, std::string argv[]); - static Logger & unsafeCreate(const std::string & name, Channel * pChannel, int level = Message::PRIO_INFORMATION); - static Logger & parent(const std::string & name); - static void add(Logger * pLogger); - static Logger * find(const std::string & name); private: + static std::pair unsafeGet(const std::string & name); + static Logger * unsafeGetRawPtr(const std::string & name); + static std::pair unsafeCreate(const std::string & name, Channel * pChannel, int level = Message::PRIO_INFORMATION); + static Logger & parent(const std::string & name); + static std::pair add(Logger * pLogger); + static std::optional find(const std::string & name); + static Logger * findRawPtr(const std::string & name); + Logger(); Logger(const Logger &); Logger & operator=(const Logger &); diff --git a/base/poco/Foundation/src/Logger.cpp b/base/poco/Foundation/src/Logger.cpp index cfc063c8979..7813faf136a 100644 --- a/base/poco/Foundation/src/Logger.cpp +++ b/base/poco/Foundation/src/Logger.cpp @@ -38,14 +38,7 @@ std::mutex & getLoggerMutex() return *logger_mutex; } -struct LoggerEntry -{ - Poco::Logger * logger; - bool owned_by_shared_ptr = false; -}; - -using LoggerMap = std::unordered_map; -LoggerMap * _pLoggerMap = nullptr; +Poco::Logger::LoggerMap * _pLoggerMap = nullptr; } @@ -350,64 +343,71 @@ Logger& Logger::get(const std::string& name) { std::lock_guard lock(getLoggerMutex()); - Logger & logger = unsafeGet(name); + auto [it, inserted] = 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; + return *it->second.logger; } LoggerPtr Logger::getShared(const std::string & name) { std::lock_guard lock(getLoggerMutex()); - bool logger_exists = _pLoggerMap && _pLoggerMap->contains(name); + auto [it, inserted] = 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 during `unsafeGet` logger was created, then this shared pointer owns it. + * If logger was already created, then this shared pointer does not own it. */ - if (logger_exists) + if (inserted) { - logger.duplicate(); + it->second.owned_by_shared_ptr = true; } - else if (_pLoggerMap) + else { - _pLoggerMap->find(name)->second.owned_by_shared_ptr = true; + it->second.logger->duplicate(); } - return makeLoggerPtr(logger); + return makeLoggerPtr(*it->second.logger); } -Logger& Logger::unsafeGet(const std::string& name) +std::pair Logger::unsafeGet(const std::string& name) { - Logger* pLogger = find(name); - if (!pLogger) + std::optional optional_logger_it = find(name); + + if (!optional_logger_it) { + Logger * logger = nullptr; + if (name == ROOT) { - pLogger = new Logger(name, 0, Message::PRIO_INFORMATION); + logger = new Logger(name, nullptr, Message::PRIO_INFORMATION); } else { Logger& par = parent(name); - pLogger = new Logger(name, par.getChannel(), par.getLevel()); + logger = new Logger(name, par.getChannel(), par.getLevel()); } - add(pLogger); + + return add(logger); } - return *pLogger; + + return std::make_pair(*optional_logger_it, false); +} + + +Logger * Logger::unsafeGetRawPtr(const std::string & name) +{ + return unsafeGet(name).first->second.logger; } @@ -415,24 +415,24 @@ Logger& Logger::create(const std::string& name, Channel* pChannel, int level) { std::lock_guard lock(getLoggerMutex()); - return unsafeCreate(name, pChannel, level); + return *unsafeCreate(name, pChannel, level).first->second.logger; } LoggerPtr Logger::createShared(const std::string & name, Channel * pChannel, int level) { std::lock_guard lock(getLoggerMutex()); - Logger & logger = unsafeCreate(name, pChannel, level); - _pLoggerMap->find(name)->second.owned_by_shared_ptr = true; + auto [it, inserted] = unsafeCreate(name, pChannel, level); + it->second.owned_by_shared_ptr = true; - return makeLoggerPtr(logger); + return makeLoggerPtr(*it->second.logger); } Logger& Logger::root() { std::lock_guard lock(getLoggerMutex()); - return unsafeGet(ROOT); + return *unsafeGetRawPtr(ROOT); } @@ -440,7 +440,11 @@ Logger* Logger::has(const std::string& name) { std::lock_guard lock(getLoggerMutex()); - return find(name); + auto optional_it = find(name); + if (!optional_it) + return nullptr; + + return (*optional_it)->second.logger; } @@ -459,22 +463,33 @@ void Logger::shutdown() } delete _pLoggerMap; - _pLoggerMap = 0; + _pLoggerMap = nullptr; } } -Logger* Logger::find(const std::string& name) +std::optional Logger::find(const std::string& name) { if (_pLoggerMap) { LoggerMap::iterator it = _pLoggerMap->find(name); if (it != _pLoggerMap->end()) - return it->second.logger; + return it; + + return {}; } - return 0; + + return {}; } +Logger * Logger::findRawPtr(const std::string & name) +{ + auto optional_it = find(name); + if (!optional_it) + return nullptr; + + return (*optional_it)->second.logger; +} void Logger::names(std::vector& names) { @@ -490,13 +505,11 @@ void Logger::names(std::vector& names) } } -Logger& Logger::unsafeCreate(const std::string & name, Channel * pChannel, int level) +std::pair Logger::unsafeCreate(const std::string & name, Channel * pChannel, int level) { if (find(name)) throw ExistsException(); Logger* pLogger = new Logger(name, pChannel, level); - add(pLogger); - - return *pLogger; + return add(pLogger); } Logger& Logger::parent(const std::string& name) @@ -505,13 +518,13 @@ Logger& Logger::parent(const std::string& name) if (pos != std::string::npos) { std::string pname = name.substr(0, pos); - Logger* pParent = find(pname); + Logger* pParent = findRawPtr(pname); if (pParent) return *pParent; else return parent(pname); } - else return unsafeGet(ROOT); + else return *unsafeGetRawPtr(ROOT); } @@ -579,12 +592,14 @@ namespace } -void Logger::add(Logger* pLogger) +std::pair Logger::add(Logger* pLogger) { if (!_pLoggerMap) - _pLoggerMap = new LoggerMap; + _pLoggerMap = new Logger::LoggerMap; - _pLoggerMap->emplace(pLogger->name(), LoggerEntry{pLogger, false /*owned_by_shared_ptr*/}); + auto result = _pLoggerMap->emplace(pLogger->name(), LoggerEntry{pLogger, false /*owned_by_shared_ptr*/}); + assert(result.second); + return result; } diff --git a/src/Disks/ObjectStorages/ObjectStorageFactory.cpp b/src/Disks/ObjectStorages/ObjectStorageFactory.cpp index ec6f7081c85..4a6bb924bdc 100644 --- a/src/Disks/ObjectStorages/ObjectStorageFactory.cpp +++ b/src/Disks/ObjectStorages/ObjectStorageFactory.cpp @@ -102,7 +102,7 @@ void checkS3Capabilities( if (s3_capabilities.support_batch_delete && !checkBatchRemove(storage, key_with_trailing_slash)) { LOG_WARNING( - &Poco::Logger::get("S3ObjectStorage"), + getLogger("S3ObjectStorage"), "Storage for disk {} does not support batch delete operations, " "so `s3_capabilities.support_batch_delete` was automatically turned off during the access check. " "To remove this message set `s3_capabilities.support_batch_delete` for the disk to `false`.", diff --git a/src/Disks/ObjectStorages/Web/WebObjectStorage.cpp b/src/Disks/ObjectStorages/Web/WebObjectStorage.cpp index 0223c24973e..786b23caf48 100644 --- a/src/Disks/ObjectStorages/Web/WebObjectStorage.cpp +++ b/src/Disks/ObjectStorages/Web/WebObjectStorage.cpp @@ -82,7 +82,7 @@ WebObjectStorage::loadFiles(const String & path, const std::unique_lock