fix(Core/Threading): Modernize/Improve thread safety, performance, and maintainability of the MapUpdater class (#21081)

Co-authored-by: Anton Popovichenko <walkline.ua@gmail.com>

1 files changed, 25 insertions, 31 deletions

diff --git a/src/common/Threading/PCQueue.h b/src/common/Threading/PCQueue.h
index 1efb494ba0..57ea0b2d34 100644
--- a/src/common/Threading/PCQueue.h
+++ b/src/common/Threading/PCQueue.h
@@ -20,51 +20,51 @@
 
 #include <condition_variable>
 #include <queue>
+#include <atomic>
+#include <mutex>
 
 template <typename T>
 class ProducerConsumerQueue
 {
 private:
-    std::mutex _queueLock;
+    mutable std::mutex _queueLock;
     std::queue<T> _queue;
     std::condition_variable _condition;
-    std::atomic<bool> _cancel;
-    std::atomic<bool> _shutdown;
+    std::atomic<bool> _cancel{};
+    std::atomic<bool> _shutdown{};
 
 public:
-    ProducerConsumerQueue() : _cancel(false), _shutdown(false) { }
+    ProducerConsumerQueue() = default;
 
     void Push(const T& value)
     {
-        std::lock_guard<std::mutex> lock(_queueLock);
-        _queue.push(std::move(value));
-
+        {
+            std::lock_guard<std::mutex> lock(_queueLock);
+            _queue.push(std::move(value));
+        }
         _condition.notify_one();
     }
 
-    bool Empty()
+    bool Empty() const
     {
         std::lock_guard<std::mutex> lock(_queueLock);
-
         return _queue.empty();
     }
 
     [[nodiscard]] std::size_t Size() const
     {
+        std::lock_guard<std::mutex> lock(_queueLock);
         return _queue.size();
     }
 
     bool Pop(T& value)
     {
         std::lock_guard<std::mutex> lock(_queueLock);
-
         if (_queue.empty() || _cancel)
             return false;
 
-        value = _queue.front();
-
+        value = std::move(_queue.front());
         _queue.pop();
-
         return true;
     }
 
@@ -72,39 +72,30 @@ public:
     {
         std::unique_lock<std::mutex> lock(_queueLock);
 
-        // we could be using .wait(lock, predicate) overload here but it is broken
-        // https://connect.microsoft.com/VisualStudio/feedback/details/1098841
-        while (_queue.empty() && !_cancel && !_shutdown)
-            _condition.wait(lock);
+        // Wait for the queue to have an element or the cancel/shutdown flag
+        _condition.wait(lock, [this] { return !_queue.empty() || _cancel || _shutdown; });
 
         if (_queue.empty() || _cancel)
             return;
 
-        value = _queue.front();
-
+        value = std::move(_queue.front());
         _queue.pop();
     }
 
-    // Clears the queue and will immediately stop any consumers
+    // Clears the queue and immediately stops any consumers.
     void Cancel()
     {
-        std::unique_lock<std::mutex> lock(_queueLock);
-
-        while (!_queue.empty())
-        {
+        std::lock_guard<std::mutex> lock(_queueLock);
+        while (!_queue.empty()) {
             T& value = _queue.front();
-
             DeleteQueuedObject(value);
-
             _queue.pop();
         }
-
         _cancel = true;
-
         _condition.notify_all();
     }
 
-    // Graceful stop, will wait for queue to become empty before stopping consumers
+    // Graceful stop: waits for the queue to become empty before stopping consumers.
     void Shutdown()
     {
         _shutdown = true;
@@ -113,10 +104,13 @@ public:
 
 private:
     template<typename E = T>
-    typename std::enable_if<std::is_pointer<E>::value>::type DeleteQueuedObject(E& obj) { delete obj; }
+    typename std::enable_if<std::is_pointer<E>::value>::type DeleteQueuedObject(E& obj)
+    {
+        delete obj;
+    }
 
     template<typename E = T>
-    typename std::enable_if<!std::is_pointer<E>::value>::type DeleteQueuedObject(E const& /*packet*/) { }
+    typename std::enable_if<!std::is_pointer<E>::value>::type DeleteQueuedObject(E const& /*obj*/) { }
 };
 
 #endif