From 3ce649504a9f7340ccd0529121c2092b83143513 Mon Sep 17 00:00:00 2001 From: Tyagiquamar Date: Thu, 1 Oct 2026 21:49:46 +0530 Subject: [PATCH] Foundation: Synchronize channel replacement and prevent FileChannel data races --- Foundation/include/Poco/FileChannel.h | 1 + Foundation/include/Poco/FormattingChannel.h | 6 +- Foundation/include/Poco/Logger.h | 60 ++++++++++------ Foundation/src/FileChannel.cpp | 9 ++- Foundation/src/FormattingChannel.cpp | 34 ++++++--- Foundation/src/Logger.cpp | 22 ++++-- Foundation/testsuite/src/LoggerTest.cpp | 76 +++++++++++++++++++++ Foundation/testsuite/src/LoggerTest.h | 2 + 8 files changed, 170 insertions(+), 40 deletions(-) diff --git a/Foundation/include/Poco/FileChannel.h b/Foundation/include/Poco/FileChannel.h index af9fc14f33..c8559bd04e 100644 --- a/Foundation/include/Poco/FileChannel.h +++ b/Foundation/include/Poco/FileChannel.h @@ -261,6 +261,7 @@ class Foundation_API FileChannel: public Channel [[nodiscard]] RotateStrategy* createRotationStrategy(const std::string& rotation, const std::string& times) const; [[nodiscard]] ArchiveStrategy* createArchiveStrategy(const std::string& archive, const std::string& times) const; + void unsafeOpen(); std::string _path; std::string _times; diff --git a/Foundation/include/Poco/FormattingChannel.h b/Foundation/include/Poco/FormattingChannel.h index e9d1fd438d..b19e338513 100644 --- a/Foundation/include/Poco/FormattingChannel.h +++ b/Foundation/include/Poco/FormattingChannel.h @@ -22,6 +22,7 @@ #include "Poco/Channel.h" #include "Poco/Formatter.h" #include "Poco/AutoPtr.h" +#include "Poco/Mutex.h" namespace Poco { @@ -89,8 +90,9 @@ class Foundation_API FormattingChannel: public Channel ~FormattingChannel() override; private: - Formatter::Ptr _pFormatter; - Channel::Ptr _pChannel; + Formatter::Ptr _pFormatter; + Channel::Ptr _pChannel; + mutable FastMutex _mutex; }; diff --git a/Foundation/include/Poco/Logger.h b/Foundation/include/Poco/Logger.h index 3c79311e23..65dfb02d72 100644 --- a/Foundation/include/Poco/Logger.h +++ b/Foundation/include/Poco/Logger.h @@ -23,6 +23,7 @@ #include "Poco/Message.h" #include "Poco/Format.h" #include "Poco/AutoPtr.h" +#include "Poco/Mutex.h" #include #include #include @@ -498,9 +499,10 @@ class Foundation_API Logger: public Channel void logAlways(const std::string& text, Message::Priority prio); void logAlways(std::string&& text, Message::Priority prio); - std::string _name; - Channel::Ptr _pChannel; - int _level; + std::string _name; + Channel::Ptr _pChannel; + int _level; + mutable FastMutex _channelMutex; // definitions in Foundation.cpp static LoggerMapPtr _pLoggerMap; @@ -724,72 +726,92 @@ inline int Logger::getLevel() const inline void Logger::log(const std::string& text, Message::Priority prio) { - if (_level >= prio && _pChannel) + if (_level >= prio) { - _pChannel->log(Message(_name, text, prio)); + Channel::Ptr pChannel = getChannel(); + if (pChannel) + { + pChannel->log(Message(_name, text, prio)); + } } } inline void Logger::log(std::string&& text, Message::Priority prio) { - if (_level >= prio && _pChannel) + if (_level >= prio) { - _pChannel->log(Message(_name, std::move(text), prio)); + Channel::Ptr pChannel = getChannel(); + if (pChannel) + { + pChannel->log(Message(_name, std::move(text), prio)); + } } } inline void Logger::logNPC(const std::string& text, Message::Priority prio) { - if (_pChannel) + Channel::Ptr pChannel = getChannel(); + if (pChannel) { - _pChannel->log(Message(_name, text, prio)); + pChannel->log(Message(_name, text, prio)); } } inline void Logger::logNPC(std::string&& text, Message::Priority prio) { - if (_pChannel) + Channel::Ptr pChannel = getChannel(); + if (pChannel) { - _pChannel->log(Message(_name, std::move(text), prio)); + pChannel->log(Message(_name, std::move(text), prio)); } } inline void Logger::log(const std::string& text, Message::Priority prio, const char* file, LineNumber line) { - if (_level >= prio && _pChannel) + if (_level >= prio) { - _pChannel->log(Message(_name, text, prio, file, line)); + Channel::Ptr pChannel = getChannel(); + if (pChannel) + { + pChannel->log(Message(_name, text, prio, file, line)); + } } } inline void Logger::log(std::string&& text, Message::Priority prio, const char* file, LineNumber line) { - if (_level >= prio && _pChannel) + if (_level >= prio) { - _pChannel->log(Message(_name, std::move(text), prio, file, line)); + Channel::Ptr pChannel = getChannel(); + if (pChannel) + { + pChannel->log(Message(_name, std::move(text), prio, file, line)); + } } } inline void Logger::logAlways(const std::string& text, Message::Priority prio) { - if (_pChannel) + Channel::Ptr pChannel = getChannel(); + if (pChannel) { - _pChannel->log(Message(_name, text, prio)); + pChannel->log(Message(_name, text, prio)); } } inline void Logger::logAlways(std::string&& text, Message::Priority prio) { - if (_pChannel) + Channel::Ptr pChannel = getChannel(); + if (pChannel) { - _pChannel->log(Message(_name, std::move(text), prio)); + pChannel->log(Message(_name, std::move(text), prio)); } } diff --git a/Foundation/src/FileChannel.cpp b/Foundation/src/FileChannel.cpp index 73437125db..6abd5d4ff7 100644 --- a/Foundation/src/FileChannel.cpp +++ b/Foundation/src/FileChannel.cpp @@ -87,7 +87,12 @@ FileChannel::~FileChannel() void FileChannel::open() { FastMutex::ScopedLock lock(_mutex); + unsafeOpen(); +} + +void FileChannel::unsafeOpen() +{ if (!_pFile) { _pFile = new LogFile(_path); @@ -122,10 +127,10 @@ void FileChannel::close() void FileChannel::log(const Message& msg) { - open(); - FastMutex::ScopedLock lock(_mutex); + unsafeOpen(); + if (_pRotateStrategy->mustRotate(_pFile)) { try diff --git a/Foundation/src/FormattingChannel.cpp b/Foundation/src/FormattingChannel.cpp index 87d1eedf64..9b3a8f9445 100644 --- a/Foundation/src/FormattingChannel.cpp +++ b/Foundation/src/FormattingChannel.cpp @@ -48,41 +48,47 @@ FormattingChannel::~FormattingChannel() void FormattingChannel::setFormatter(Formatter::Ptr pFormatter) { + FastMutex::ScopedLock lock(_mutex); _pFormatter = pFormatter; } Formatter::Ptr FormattingChannel::getFormatter() const { + FastMutex::ScopedLock lock(_mutex); return _pFormatter; } void FormattingChannel::setChannel(Channel::Ptr pChannel) { + FastMutex::ScopedLock lock(_mutex); _pChannel = pChannel; } Channel::Ptr FormattingChannel::getChannel() const { + FastMutex::ScopedLock lock(_mutex); return _pChannel; } void FormattingChannel::log(const Message& msg) { - if (_pChannel) + Channel::Ptr pChannel = getChannel(); + if (pChannel) { - if (_pFormatter) + Formatter::Ptr pFormatter = getFormatter(); + if (pFormatter) { std::string text; - _pFormatter->format(msg, text); - _pChannel->log(Message(msg, text)); + pFormatter->format(msg, text); + pChannel->log(Message(msg, text)); } else { - _pChannel->log(msg); + pChannel->log(msg); } } } @@ -94,22 +100,28 @@ void FormattingChannel::setProperty(const std::string& name, const std::string& setChannel(LoggingRegistry::defaultRegistry().channelForName(value)); else if (name == "formatter") setFormatter(LoggingRegistry::defaultRegistry().formatterForName(value)); - else if (_pChannel) - _pChannel->setProperty(name, value); + else + { + Channel::Ptr pChannel = getChannel(); + if (pChannel) + pChannel->setProperty(name, value); + } } void FormattingChannel::open() { - if (_pChannel) - _pChannel->open(); + Channel::Ptr pChannel = getChannel(); + if (pChannel) + pChannel->open(); } void FormattingChannel::close() { - if (_pChannel) - _pChannel->close(); + Channel::Ptr pChannel = getChannel(); + if (pChannel) + pChannel->close(); } diff --git a/Foundation/src/Logger.cpp b/Foundation/src/Logger.cpp index 601c411c01..5fac2ac1e2 100644 --- a/Foundation/src/Logger.cpp +++ b/Foundation/src/Logger.cpp @@ -41,12 +41,14 @@ Logger::~Logger() void Logger::setChannel(Channel::Ptr pChannel) { + FastMutex::ScopedLock lock(_channelMutex); _pChannel = pChannel; } Channel::Ptr Logger::getChannel() const { + FastMutex::ScopedLock lock(_channelMutex); return _pChannel; } @@ -77,9 +79,13 @@ void Logger::setProperty(const std::string& name, const std::string& value) template void Logger::logImpl(M&& msg) { - if (_level >= msg.getPriority() && _pChannel) + if (_level >= msg.getPriority()) { - _pChannel->log(std::forward(msg)); + Channel::Ptr pChannel = getChannel(); + if (pChannel) + { + pChannel->log(std::forward(msg)); + } } } @@ -110,11 +116,15 @@ void Logger::log(const Exception& exc, const char* file, LineNumber line) void Logger::dump(const std::string& msg, const void* buffer, std::size_t length, Message::Priority prio) { - if (_level >= prio && _pChannel) + if (_level >= prio) { - std::string text(msg); - formatDump(text, buffer, length); - _pChannel->log(Message(_name, text, prio)); + Channel::Ptr pChannel = getChannel(); + if (pChannel) + { + std::string text(msg); + formatDump(text, buffer, length); + pChannel->log(Message(_name, text, prio)); + } } } diff --git a/Foundation/testsuite/src/LoggerTest.cpp b/Foundation/testsuite/src/LoggerTest.cpp index 8d46102bdf..d87bd790f5 100644 --- a/Foundation/testsuite/src/LoggerTest.cpp +++ b/Foundation/testsuite/src/LoggerTest.cpp @@ -18,6 +18,7 @@ #include "Poco/Event.h" #include "Poco/PatternFormatter.h" #include "Poco/FormattingChannel.h" +#include "Poco/NullChannel.h" #include #include #include @@ -28,6 +29,7 @@ using Poco::Message; using Poco::AutoPtr; using Poco::PatternFormatter; using Poco::FormattingChannel; +using Poco::NullChannel; using Poco::Event; using Poco::Thread; @@ -411,6 +413,78 @@ void LoggerTest::testLoggerRefSurvivesShutdown() } +void LoggerTest::testConcurrentChannelReplacement() +{ + Logger& logger = Logger::get("TestLogger.ConcurrentReplace"); + logger.setLevel(Message::PRIO_INFORMATION); + + std::atomic stop{false}; + std::vector threads; + + // Worker threads logging concurrently + for (int i = 0; i < 4; ++i) + { + threads.emplace_back([&logger, &stop, i]() { + while (!stop) + { + logger.information("concurrent message from thread " + std::to_string(i)); + } + }); + } + + // Channel replacement thread + for (int i = 0; i < 200; ++i) + { + AutoPtr pChan = new NullChannel; + logger.setChannel(pChan); + std::this_thread::yield(); + } + + stop = true; + for (auto& t : threads) + { + t.join(); + } + logger.setChannel(nullptr); +} + + +void LoggerTest::testConcurrentShutdown() +{ + Logger& logger = Logger::get("TestLogger.ConcurrentShutdown"); + AutoPtr pChannel = new NullChannel; + logger.setChannel(pChannel); + logger.setLevel(Message::PRIO_INFORMATION); + + std::atomic stop{false}; + std::vector threads; + + for (int i = 0; i < 4; ++i) + { + threads.emplace_back([&logger, &stop]() { + while (!stop) + { + logger.information("message during shutdown"); + } + }); + } + + for (int i = 0; i < 50; ++i) + { + Logger::shutdown(); + logger.setChannel(new NullChannel); + } + + stop = true; + for (auto& t : threads) + { + t.join(); + } + Logger::shutdown(); +} + + + void LoggerTest::setUp() { Logger::shutdown(); @@ -433,6 +507,8 @@ CppUnit::Test* LoggerTest::suite() CppUnit_addTest(pSuite, LoggerTest, testFormatThreadName); CppUnit_addTest(pSuite, LoggerTest, testFormatStdThreadName); CppUnit_addTest(pSuite, LoggerTest, testLoggerRefSurvivesShutdown); + CppUnit_addTest(pSuite, LoggerTest, testConcurrentChannelReplacement); + CppUnit_addTest(pSuite, LoggerTest, testConcurrentShutdown); return pSuite; } diff --git a/Foundation/testsuite/src/LoggerTest.h b/Foundation/testsuite/src/LoggerTest.h index c454dd9138..f76432caad 100644 --- a/Foundation/testsuite/src/LoggerTest.h +++ b/Foundation/testsuite/src/LoggerTest.h @@ -31,6 +31,8 @@ class LoggerTest: public CppUnit::TestCase void testFormatThreadName(); void testFormatStdThreadName(); void testLoggerRefSurvivesShutdown(); + void testConcurrentChannelReplacement(); + void testConcurrentShutdown(); void setUp(); void tearDown();