From cb9c984aa7c1044660e6e901dc35946eb3159ec0 Mon Sep 17 00:00:00 2001
From: gabime <gmelman1@gmail.com>
Date: Tue, 24 Jul 2018 22:59:34 +0300
Subject: [PATCH] registery and periodic flusher fixes.

---
 example/example.vcxproj                  |   6 +-
 include/spdlog/async.h                   |  24 ++---
 include/spdlog/details/periodic_worker.h |  39 ++++----
 include/spdlog/details/registry.h        | 119 ++++++++++++-----------
 include/spdlog/spdlog.h                  |   6 ++
 include/spdlog/tweakme.h                 |   9 --
 tests/tests.vcxproj.user                 |   4 +
 7 files changed, 100 insertions(+), 107 deletions(-)
 create mode 100644 tests/tests.vcxproj.user

diff --git a/example/example.vcxproj b/example/example.vcxproj
index 12cbcf6b..0cedd620 100644
--- a/example/example.vcxproj
+++ b/example/example.vcxproj
@@ -31,7 +31,7 @@
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'" Label="Configuration">
     <ConfigurationType>Application</ConfigurationType>
     <UseDebugLibraries>true</UseDebugLibraries>
-    <PlatformToolset>v120</PlatformToolset>
+    <PlatformToolset>v141</PlatformToolset>
     <CharacterSet>Unicode</CharacterSet>
   </PropertyGroup>
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'" Label="Configuration">
@@ -43,7 +43,7 @@
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|Win32'" Label="Configuration">
     <ConfigurationType>Application</ConfigurationType>
     <UseDebugLibraries>false</UseDebugLibraries>
-    <PlatformToolset>v120</PlatformToolset>
+    <PlatformToolset>v141</PlatformToolset>
     <WholeProgramOptimization>true</WholeProgramOptimization>
     <CharacterSet>Unicode</CharacterSet>
   </PropertyGroup>
@@ -86,7 +86,7 @@
     <ClCompile>
       <PrecompiledHeader>
       </PrecompiledHeader>
-      <WarningLevel>Level4</WarningLevel>
+      <WarningLevel>Level3</WarningLevel>
       <Optimization>Disabled</Optimization>
       <PreprocessorDefinitions>WIN32;_DEBUG;_CONSOLE;_LIB;%(PreprocessorDefinitions)</PreprocessorDefinitions>
       <AdditionalIncludeDirectories>..\include;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
diff --git a/include/spdlog/async.h b/include/spdlog/async.h
index 469e47f1..2f4de8f3 100644
--- a/include/spdlog/async.h
+++ b/include/spdlog/async.h
@@ -38,17 +38,11 @@ struct async_factory_impl
     template<typename Sink, typename... SinkArgs>
     static std::shared_ptr<async_logger> create(const std::string &logger_name, SinkArgs &&... args)
     {
-        using details::registry;
-
-        std::lock_guard<std::recursive_mutex> lock(registry::instance().tp_mutex());
-        auto tp = registry::instance().get_thread_pool();
-        if (tp == nullptr)
-        {
-            tp = std::make_shared<details::thread_pool>(details::default_async_q_size, 1);
-            registry::instance().set_thread_pool(tp);
-        }
-
+        using details::registry;				
         auto sink = std::make_shared<Sink>(std::forward<SinkArgs>(args)...);
+
+		// create default tp if not already exists.
+		auto tp = registry::instance().create_tp_once(details::default_async_q_size, 1);
         auto new_logger = std::make_shared<async_logger>(logger_name, std::move(sink), std::move(tp), OverflowPolicy);
         registry::instance().register_and_init(new_logger);
         return new_logger;
@@ -72,16 +66,14 @@ inline std::shared_ptr<spdlog::logger> create_async_nb(const std::string &logger
 
 // set global thread pool.
 inline void init_thread_pool(size_t q_size, size_t thread_count)
-{
-    using details::registry;
-    using details::thread_pool;
-    auto tp = std::make_shared<thread_pool>(q_size, thread_count);
-    registry::instance().set_thread_pool(std::move(tp));
+{        
+    auto tp = std::make_shared<details::thread_pool>(q_size, thread_count);
+	details::registry::instance().set_tp(std::move(tp));
 }
 
 // get the global thread pool.
 inline std::shared_ptr<spdlog::details::thread_pool> thread_pool()
 {
-    return details::registry::instance().get_thread_pool();
+    return details::registry::instance().get_tp();
 }
 } // namespace spdlog
diff --git a/include/spdlog/details/periodic_worker.h b/include/spdlog/details/periodic_worker.h
index 0fa55b89..96ec8db3 100644
--- a/include/spdlog/details/periodic_worker.h
+++ b/include/spdlog/details/periodic_worker.h
@@ -17,7 +17,6 @@
 #include <functional>
 #include <mutex>
 #include <thread>
-
 namespace spdlog {
 namespace details {
 
@@ -26,22 +25,19 @@ class periodic_worker
 public:
     periodic_worker(std::function<void()> callback_fun, std::chrono::seconds interval)
     {
-        if (interval == std::chrono::seconds::zero())
+        active_ = (interval > std::chrono::seconds::zero());
+        if (!active_)
         {
-            active_ = false;
             return;
         }
-        active_ = true;
-        flusher_thread_ = std::thread([callback_fun, interval, this]() {
+
+        worker_thread_ = std::thread([this, callback_fun, interval]() {
             for (;;)
             {
                 std::unique_lock<std::mutex> lock(this->mutex_);
-                bool should_terminate = this->cv_.wait_for(lock, interval, [this] {
-                    return !this->active_;
-                });
-                if (should_terminate)
+                if (this->cv_.wait_for(lock, interval, [this] { return !this->active_; }))
                 {
-                    break;
+                    return; // active_ == false, so exit this thread
                 }
                 callback_fun();
             }
@@ -51,24 +47,23 @@ public:
     periodic_worker(const periodic_worker &) = delete;
     periodic_worker &operator=(const periodic_worker &) = delete;
 
-    // stop the back thread and join it
+    // stop the worker thread and join it
     ~periodic_worker()
     {
-        if (!active_)
-        {
-           return;
-        }
-        {
-            std::lock_guard<std::mutex> lock(mutex_);
-            active_ = false;
-        }
-        cv_.notify_one();   
-        flusher_thread_.join();
+		if (worker_thread_.joinable())
+		{
+			{
+				std::lock_guard<std::mutex> lock(mutex_);
+				active_ = false;				
+			}			
+			cv_.notify_one();
+			worker_thread_.join();
+		}
     }
 
 private:
     bool active_;
-    std::thread flusher_thread_;
+    std::thread worker_thread_;
     std::mutex mutex_;
     std::condition_variable cv_;
 };
diff --git a/include/spdlog/details/registry.h b/include/spdlog/details/registry.h
index bff70690..d5244ce3 100644
--- a/include/spdlog/details/registry.h
+++ b/include/spdlog/details/registry.h
@@ -24,18 +24,15 @@ namespace spdlog {
 namespace details {
 class thread_pool;
 
-template<typename Mutex>
-class registry_t
+class registry
 {
 public:
-    using MutexT = Mutex;
-
-    registry_t<Mutex>(const registry_t<Mutex> &) = delete;
-    registry_t<Mutex> &operator=(const registry_t<Mutex> &) = delete;
+    registry(const registry &) = delete;
+    registry &operator=(const registry &) = delete;
 
     void register_logger(std::shared_ptr<logger> new_logger)
     {
-        std::lock_guard<Mutex> lock(loggers_mutex_);
+        std::lock_guard<std::mutex> lock(logger_map_mutex_);
         auto logger_name = new_logger->name();
         throw_if_exists_(logger_name);
         loggers_[logger_name] = new_logger;
@@ -43,7 +40,7 @@ public:
 
     void register_and_init(std::shared_ptr<logger> new_logger)
     {
-        std::lock_guard<Mutex> lock(loggers_mutex_);
+        std::lock_guard<std::mutex> lock(logger_map_mutex_);
         auto logger_name = new_logger->name();
         throw_if_exists_(logger_name);
 
@@ -58,33 +55,44 @@ public:
         new_logger->set_level(level_);
         new_logger->flush_on(flush_level_);
 
-        // Add to registry
+        // add to registry
         loggers_[logger_name] = new_logger;
     }
 
     std::shared_ptr<logger> get(const std::string &logger_name)
     {
-        std::lock_guard<Mutex> lock(loggers_mutex_);
+        std::lock_guard<std::mutex> lock(logger_map_mutex_);
         auto found = loggers_.find(logger_name);
         return found == loggers_.end() ? nullptr : found->second;
     }
 
-    void set_thread_pool(std::shared_ptr<thread_pool> tp)
+    void set_tp(std::shared_ptr<thread_pool> tp)
     {
-        std::lock_guard<decltype(tp_mutex_)> lock(tp_mutex_);
+        std::lock_guard<std::mutex> lock(tp_mutex_);
         tp_ = std::move(tp);
     }
 
-    std::shared_ptr<thread_pool> get_thread_pool()
+	// create tp only if not exists already 
+	std::shared_ptr<thread_pool> create_tp_once(size_t queue_size, size_t n_threads)
+	{
+		std::lock_guard<std::mutex> lock(tp_mutex_);
+		if (tp_ == nullptr)
+		{
+			tp_ = std::make_shared<details::thread_pool>(queue_size, n_threads);
+		}		
+		return tp_;
+	}
+
+    std::shared_ptr<thread_pool> get_tp()
     {
-        std::lock_guard<decltype(tp_mutex_)> lock(tp_mutex_);
+        std::lock_guard<std::mutex> lock(tp_mutex_);
         return tp_;
     }
 
     // Set global formatter. Each sink in each logger will get a clone of this object
     void set_formatter(std::unique_ptr<formatter> formatter)
     {
-        std::lock_guard<Mutex> lock(loggers_mutex_);
+        std::lock_guard<std::mutex> lock(logger_map_mutex_);
         formatter_ = std::move(formatter);
         for (auto &l : loggers_)
         {
@@ -94,7 +102,7 @@ public:
 
     void set_level(level::level_enum log_level)
     {
-        std::lock_guard<Mutex> lock(loggers_mutex_);
+        std::lock_guard<std::mutex> lock(logger_map_mutex_);
         for (auto &l : loggers_)
         {
             l.second->set_level(log_level);
@@ -104,7 +112,7 @@ public:
 
     void flush_on(level::level_enum log_level)
     {
-        std::lock_guard<Mutex> lock(loggers_mutex_);
+        std::lock_guard<std::mutex> lock(logger_map_mutex_);
         for (auto &l : loggers_)
         {
             l.second->flush_on(log_level);
@@ -114,13 +122,14 @@ public:
 
     void flush_every(std::chrono::seconds interval)
     {
-        std::lock_guard<Mutex> lock(loggers_mutex_);
-        std::function<void()> clbk(std::bind(&registry_t::flush_all, this));
+        std::lock_guard<std::mutex> lock(flusher_mutex_);
+        std::function<void()> clbk(std::bind(&registry::flush_all, this));
         periodic_flusher_.reset(new periodic_worker(clbk, interval));
     }
 
     void set_error_handler(log_err_handler handler)
     {
+        std::lock_guard<std::mutex> lock(logger_map_mutex_);
         for (auto &l : loggers_)
         {
             l.second->set_error_handler(handler);
@@ -130,52 +139,66 @@ public:
 
     void apply_all(std::function<void(std::shared_ptr<logger>)> fun)
     {
-        std::lock_guard<Mutex> lock(loggers_mutex_);
+        std::lock_guard<std::mutex> lock(logger_map_mutex_);
         for (auto &l : loggers_)
         {
             fun(l.second);
         }
     }
 
+	void flush_all()
+	{				
+		std::lock_guard<std::mutex> lock(logger_map_mutex_);
+		for (auto &l : loggers_)
+		{
+			l.second->flush();
+		}
+	}
+
     void drop(const std::string &logger_name)
     {
-        std::lock_guard<Mutex> lock(loggers_mutex_);
+        std::lock_guard<std::mutex> lock(logger_map_mutex_);
         loggers_.erase(logger_name);
     }
 
     void drop_all()
+    {
+        std::lock_guard<std::mutex> lock(logger_map_mutex_);
+        loggers_.clear();
+    }
+
+    // clean all reasources and threads started by the registry
+    void shutdown()
     {
         {
-            std::lock_guard<Mutex> lock(loggers_mutex_);
-            loggers_.clear();
+            std::lock_guard<std::mutex> lock(flusher_mutex_);
+            periodic_flusher_.reset();
         }
 
+		drop_all();
+
         {
-            std::lock_guard<decltype(tp_mutex_)> lock(tp_mutex_);
+            std::lock_guard<std::mutex> lock(tp_mutex_);
             tp_.reset();
         }
     }
-
-    std::recursive_mutex &tp_mutex()
+    
+    static registry &instance()
     {
-        return tp_mutex_;
-    }
-
-    static registry_t<Mutex> &instance()
-    {
-        static registry_t<Mutex> s_instance;
+        static registry s_instance;
         return s_instance;
     }
 
 private:
-    registry_t<Mutex>()
+    registry()
         : formatter_(new pattern_formatter("%+"))
     {
     }
 
-    ~registry_t<Mutex>()
+    ~registry()
     {
-        periodic_flusher_.reset();
+        /*std::lock_guard<std::mutex> lock(flusher_mutex_);
+        periodic_flusher_.reset();*/
     }
 
     void throw_if_exists_(const std::string &logger_name)
@@ -185,20 +208,10 @@ private:
             throw spdlog_ex("logger with name '" + logger_name + "' already exists");
         }
     }
-
-    void flush_all()
-    {
-        std::lock_guard<Mutex> lock(loggers_mutex_);
-        for (auto &l : loggers_)
-        {
-            l.second->flush();
-        }
-    }
-
-    Mutex loggers_mutex_;
-    std::recursive_mutex tp_mutex_;
-    std::unordered_map<std::string, std::shared_ptr<logger>> loggers_;
-    // std::unique_ptr<formatter> formatter_{ new pattern_formatter("%+") };
+   
+    std::mutex logger_map_mutex_, flusher_mutex_;
+    std::mutex tp_mutex_;
+    std::unordered_map<std::string, std::shared_ptr<logger>> loggers_;    
     std::unique_ptr<formatter> formatter_;
     level::level_enum level_ = level::info;
     level::level_enum flush_level_ = level::off;
@@ -207,13 +220,5 @@ private:
     std::unique_ptr<periodic_worker> periodic_flusher_;
 };
 
-#ifdef SPDLOG_NO_REGISTRY_MUTEX
-#include "spdlog/details/null_mutex.h"
-using registry = registry_t<spdlog::details::null_mutex>;
-#else
-#include <mutex>
-using registry = registry_t<std::mutex>;
-#endif
-
 } // namespace details
 } // namespace spdlog
diff --git a/include/spdlog/spdlog.h b/include/spdlog/spdlog.h
index cc039062..cf4cac2d 100644
--- a/include/spdlog/spdlog.h
+++ b/include/spdlog/spdlog.h
@@ -117,6 +117,12 @@ inline void drop_all()
     details::registry::instance().drop_all();
 }
 
+// stop any running threads started by spdlog and clean registry loggers
+void shutdown()
+{
+	details::registry::instance().shutdown();
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 //
 // Trace & Debug can be switched on/off at compile time for zero cost debug
diff --git a/include/spdlog/tweakme.h b/include/spdlog/tweakme.h
index 1eb26cd2..53256450 100644
--- a/include/spdlog/tweakme.h
+++ b/include/spdlog/tweakme.h
@@ -68,15 +68,6 @@
 // #define SPDLOG_TRACE_ON
 ///////////////////////////////////////////////////////////////////////////////
 
-///////////////////////////////////////////////////////////////////////////////
-// Uncomment to avoid locking in the registry operations (spdlog::get(),
-// spdlog::drop() spdlog::register()).
-// Use only if your code never modifies concurrently the registry.
-// Note that upon creating a logger the registry is modified by spdlog..
-//
-// #define SPDLOG_NO_REGISTRY_MUTEX
-///////////////////////////////////////////////////////////////////////////////
-
 ///////////////////////////////////////////////////////////////////////////////
 // Uncomment to avoid spdlog's usage of atomic log levels
 // Use only if your code never modifies a logger's log levels concurrently by
diff --git a/tests/tests.vcxproj.user b/tests/tests.vcxproj.user
new file mode 100644
index 00000000..be250787
--- /dev/null
+++ b/tests/tests.vcxproj.user
@@ -0,0 +1,4 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Project ToolsVersion="15.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <PropertyGroup />
+</Project>
\ No newline at end of file