From 78118b7b1d3379881af2785d2d4ecc9d646616c3 Mon Sep 17 00:00:00 2001
From: tamasmeszaros <meszaros.q@gmail.com>
Date: Thu, 2 Dec 2021 14:55:52 +0100
Subject: [PATCH] Fix issue with non atomic transition to running state

After popping a job from input queue
---
 src/slic3r/GUI/Jobs/BoostThreadWorker.cpp | 13 +++++++------
 src/slic3r/GUI/Jobs/ThreadSafeQueue.hpp   | 12 ++++++++++--
 tests/slic3rutils/slic3r_jobs_tests.cpp   |  2 +-
 3 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/src/slic3r/GUI/Jobs/BoostThreadWorker.cpp b/src/slic3r/GUI/Jobs/BoostThreadWorker.cpp
index 40729a0e8..9e7e9c15a 100644
--- a/src/slic3r/GUI/Jobs/BoostThreadWorker.cpp
+++ b/src/slic3r/GUI/Jobs/BoostThreadWorker.cpp
@@ -45,7 +45,6 @@ void BoostThreadWorker::run()
                 stop = true;
             else {
                 m_canceled.store(false);
-                m_running.store(true);
 
                 try {
                     e.job->process(*this);
@@ -55,9 +54,9 @@ void BoostThreadWorker::run()
 
                 e.canceled = m_canceled.load();
                 m_output_queue.push(std::move(e)); // finalization message
-                m_running.store(false);
             }
-        });
+            m_running.store(false);
+        }, &m_running);
     };
 }
 
@@ -94,14 +93,16 @@ constexpr int ABORT_WAIT_MAX_MS = 10000;
 
 BoostThreadWorker::~BoostThreadWorker()
 {
+    bool joined = false;
     try {
         cancel_all();
         m_input_queue.push(JobEntry{nullptr});
-        join(ABORT_WAIT_MAX_MS);
-    } catch(...) {
+        joined = join(ABORT_WAIT_MAX_MS);
+    } catch(...) {}
+
+    if (!joined)
         BOOST_LOG_TRIVIAL(error)
             << "Could not join worker thread '" << m_name << "'";
-    }
 }
 
 bool BoostThreadWorker::join(int timeout_ms)
diff --git a/src/slic3r/GUI/Jobs/ThreadSafeQueue.hpp b/src/slic3r/GUI/Jobs/ThreadSafeQueue.hpp
index f01698cb2..3b1c4abea 100644
--- a/src/slic3r/GUI/Jobs/ThreadSafeQueue.hpp
+++ b/src/slic3r/GUI/Jobs/ThreadSafeQueue.hpp
@@ -5,6 +5,7 @@
 #include <queue>
 #include <mutex>
 #include <condition_variable>
+#include <atomic>
 
 namespace Slic3r { namespace GUI {
 
@@ -21,7 +22,7 @@ class ThreadSafeQueueSPSC
 public:
 
     // Consume one element, block if the queue is empty.
-    template<class Fn> void consume_one_blk(Fn &&fn)
+    template<class Fn> void consume_one_blk(Fn &&fn, std::atomic<bool> *pop_flag = nullptr)
     {
         static_assert(!std::is_reference_v<T>, "");
         static_assert(std::is_default_constructible_v<T>, "");
@@ -38,6 +39,9 @@ public:
                 el = m_queue.front();
 
             m_queue.pop();
+
+            if (pop_flag) // The optional atomic is set before the lock us unlocked
+                pop_flag->store(true);
         }
 
         fn(el);
@@ -50,7 +54,11 @@ public:
         {
             std::unique_lock lk{m_mutex};
             if (!m_queue.empty()) {
-                el = std::move(m_queue.front());
+                if constexpr (std::is_move_assignable_v<T>)
+                    el = std::move(m_queue.front());
+                else
+                    el = m_queue.front();
+
                 m_queue.pop();
             } else
                 return false;
diff --git a/tests/slic3rutils/slic3r_jobs_tests.cpp b/tests/slic3rutils/slic3r_jobs_tests.cpp
index 1d4f165af..9c75d79ac 100644
--- a/tests/slic3rutils/slic3r_jobs_tests.cpp
+++ b/tests/slic3rutils/slic3r_jobs_tests.cpp
@@ -86,7 +86,7 @@ TEST_CASE("Cancellation should be recognized be the worker", "[Jobs]") {
     REQUIRE(pri->pr != 100);
 }
 
-TEST_CASE("Cancel_all should remove all pending jobs", "[Jobs]") {
+TEST_CASE("cancel_all should remove all pending jobs", "[Jobs]") {
     using namespace Slic3r;
     using namespace Slic3r::GUI;