Fixed tests

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

@@ -945,7 +945,7 @@ protected:
     static std::string format(const std::string & fmt, int argc, std::string argv[]);
 
 private:
-    static std::pair<Logger::LoggerMapIterator, bool> unsafeGet(const std::string & name);
+    static std::pair<Logger::LoggerMapIterator, bool> unsafeGet(const std::string & name, bool get_shared);
     static Logger * unsafeGetRawPtr(const std::string & name);
     static std::pair<LoggerMapIterator, bool> unsafeCreate(const std::string & name, Channel * pChannel, int level = Message::PRIO_INFORMATION);
     static Logger & parent(const std::string & name);

base/poco/Foundation/src/Logger.cpp

@@ -314,18 +314,7 @@ Logger& Logger::get(const std::string& name)
 {
 	std::lock_guard<std::mutex> lock(getLoggerMutex());
 
-	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.
-	  */
-	if (it->second.owned_by_shared_ptr)
-	{
-		it->second.logger->duplicate();
-		it->second.owned_by_shared_ptr = false;
-	}
-
+	auto [it, inserted] = unsafeGet(name, false /*get_shared*/);
 	return *it->second.logger;
 }
 
@@ -333,35 +322,47 @@ Logger& Logger::get(const std::string& name)
 LoggerPtr Logger::getShared(const std::string & name, bool should_be_owned_by_shared_ptr_if_created)
 {
 	std::lock_guard<std::mutex> lock(getLoggerMutex());
-	auto [it, inserted] = unsafeGet(name);
+	auto [it, inserted] = unsafeGet(name, true /*get_shared*/);
 
 	/** 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 (inserted && should_be_owned_by_shared_ptr_if_created)
+	if (inserted)
 	{
-		it->second.owned_by_shared_ptr = true;
-	}
-	else
-	{
-		it->second.logger->duplicate();
+		if (should_be_owned_by_shared_ptr_if_created)
+			it->second.owned_by_shared_ptr = true;
+		else
+			it->second.logger->duplicate();
 	}
 
 	return makeLoggerPtr(*it->second.logger);
 }
 
 
-std::pair<Logger::LoggerMapIterator, bool> Logger::unsafeGet(const std::string& name)
+std::pair<Logger::LoggerMapIterator, bool> Logger::unsafeGet(const std::string& name, bool get_shared)
 {
 	std::optional<Logger::LoggerMapIterator> optional_logger_it = find(name);
 
 	bool should_recreate_logger = false;
 
-	/// Other thread already deleted this logger, but did not yet remove it from map
-	if (optional_logger_it && (*optional_logger_it)->second.logger->referenceCount() == 0)
+	if (optional_logger_it)
 	{
-		assert((*optional_logger_it)->second.owned_by_shared_ptr);
-		should_recreate_logger = true;
+		auto & logger_it = *optional_logger_it;
+		std::optional<size_t> reference_count_before;
+
+		if (get_shared)
+		{
+			reference_count_before = logger_it->second.logger->duplicate();
+		}
+		else if (logger_it->second.owned_by_shared_ptr)
+		{
+			reference_count_before = logger_it->second.logger->duplicate();
+			logger_it->second.owned_by_shared_ptr = false;
+		}
+
+		/// Other thread already decided to delete this logger, but did not yet remove it from map
+		if (reference_count_before && reference_count_before == 0)
+			should_recreate_logger = true;
 	}
 
 	if (!optional_logger_it || should_recreate_logger)
@@ -393,7 +394,7 @@ std::pair<Logger::LoggerMapIterator, bool> Logger::unsafeGet(const std::string&
 
 Logger * Logger::unsafeGetRawPtr(const std::string & name)
 {
-	return unsafeGet(name).first->second.logger;
+	return unsafeGet(name, false /*get_shared*/).first->second.logger;
 }
 
 
@@ -496,12 +497,11 @@ void intrusive_ptr_release(Logger * ptr)
 		if (_pLoggerMap)
 		{
 			auto it = _pLoggerMap->find(ptr->name());
-			assert(it != _pLoggerMap->end());
 
 			/** It is possible that during release other thread created logger and
 			  * updated iterator in map.
 			  */
-			if (it->second.logger == ptr)
+			if (it != _pLoggerMap->end() && ptr == it->second.logger)
 			{
 				/** If reference count is 0, this means this intrusive pointer owns logger
 				  * and need destroy it.

src/Common/tests/gtest_log.cpp

@@ -9,6 +9,7 @@
 #include <Poco/NullChannel.h>
 #include <Poco/StreamChannel.h>
 #include <sstream>
+#include <thread>
 
 
 TEST(Logger, Log)
@@ -100,3 +101,75 @@ TEST(Logger, SideEffects)
 
     LOG_TRACE(log, "test no throw {}", getLogMessageParamOrThrow());
 }
+
+TEST(Logger, SharedRawLogger)
+{
+    {
+        std::ostringstream stream; // STYLE_CHECK_ALLOW_STD_STRING_STREAM
+        auto stream_channel = Poco::AutoPtr<Poco::StreamChannel>(new Poco::StreamChannel(stream));
+
+        auto shared_logger = getLogger("Logger_1");
+        shared_logger->setChannel(stream_channel.get());
+        shared_logger->setLevel("trace");
+
+        LOG_TRACE(shared_logger, "SharedLogger1Log1");
+        LOG_TRACE(getRawLogger("Logger_1"), "RawLogger1Log");
+        LOG_TRACE(shared_logger, "SharedLogger1Log2");
+
+        auto actual = stream.str();
+        EXPECT_EQ(actual, "SharedLogger1Log1\nRawLogger1Log\nSharedLogger1Log2\n");
+    }
+    {
+        std::ostringstream stream; // STYLE_CHECK_ALLOW_STD_STRING_STREAM
+        auto stream_channel = Poco::AutoPtr<Poco::StreamChannel>(new Poco::StreamChannel(stream));
+
+        auto * raw_logger = getRawLogger("Logger_2");
+        raw_logger->setChannel(stream_channel.get());
+        raw_logger->setLevel("trace");
+
+        LOG_TRACE(getLogger("Logger_2"), "SharedLogger2Log1");
+        LOG_TRACE(raw_logger, "RawLogger2Log");
+        LOG_TRACE(getLogger("Logger_2"), "SharedLogger2Log2");
+
+        auto actual = stream.str();
+        EXPECT_EQ(actual, "SharedLogger2Log1\nRawLogger2Log\nSharedLogger2Log2\n");
+    }
+}
+
+TEST(Logger, SharedLoggersThreadSafety)
+{
+    static size_t threads_count = std::thread::hardware_concurrency();
+    static constexpr size_t loggers_count = 10;
+    static constexpr size_t logger_get_count = 1000;
+
+    Poco::Logger::root();
+
+    std::vector<std::string> names;
+
+    Poco::Logger::names(names);
+    size_t loggers_size_before = names.size();
+
+    std::vector<std::thread> threads;
+
+    for (size_t thread_index = 0; thread_index < threads_count; ++thread_index)
+    {
+        threads.emplace_back([]()
+        {
+            for (size_t logger_index = 0; logger_index < loggers_count; ++logger_index)
+            {
+                for (size_t iteration = 0; iteration < logger_get_count; ++iteration)
+                {
+                    getLogger("Logger_" + std::to_string(logger_index));
+                }
+            }
+        });
+    }
+
+    for (auto & thread : threads)
+        thread.join();
+
+    Poco::Logger::names(names);
+    size_t loggers_size_after = names.size();
+
+    EXPECT_EQ(loggers_size_before, loggers_size_after);
+}