Merge pull request #16064 from daschuer/valigrind_analyzerthread

Fix killing Analyzis worker threads during shutdown, possible data loss.

Author: JoergAtGithub

src/analyzer/trackanalysisscheduler.cpp

@@ -92,6 +92,7 @@ TrackAnalysisScheduler::TrackAnalysisScheduler(
 
 TrackAnalysisScheduler::~TrackAnalysisScheduler() {
     kLogger.debug() << "Destroying";
+    // Here the workers in m_workers are deleted after waiting for the associated thread is finished
 }
 
 void TrackAnalysisScheduler::emitProgressOrFinished() {
@@ -228,7 +229,7 @@ void TrackAnalysisScheduler::onWorkerThreadProgress(
         DEBUG_ASSERT(!trackId.isValid());
         DEBUG_ASSERT(analyzerProgress == kAnalyzerProgressUnknown);
         worker.onThreadExit();
-        DEBUG_ASSERT(!worker);
+        DEBUG_ASSERT(!worker.hasThread());
         break;
     default:
         DEBUG_ASSERT(!"Unhandled signal from worker thread");

src/analyzer/trackanalysisscheduler.h

@@ -80,62 +80,72 @@ class TrackAnalysisScheduler : public QObject {
     // that runs the TrackAnalysisScheduler.
     class Worker {
       public:
-        explicit Worker(AnalyzerThread::Pointer thread = AnalyzerThread::NullPointer())
-            : m_thread(std::move(thread)),
-              m_analyzerProgress(kAnalyzerProgressUnknown) {
+        explicit Worker(AnalyzerThread::Pointer pThread = AnalyzerThread::NullPointer())
+                : m_pThread(std::move(pThread)),
+                  m_analyzerProgress(kAnalyzerProgressUnknown) {
         }
         Worker(const Worker&) = delete;
         Worker(Worker&&) = default;
 
-        operator bool() const {
-            return static_cast<bool>(m_thread);
+        ~Worker() {
+            if (m_pThread) {
+                m_pThread->stop();
+                bool success = m_pThread->wait(5000); // 5 s
+                DEBUG_ASSERT(success);
+                m_pThread->wait();
+                delete m_pThread.release();
+            }
+        }
+
+        bool hasThread() const {
+            return static_cast<bool>(m_pThread);
         }
 
         AnalyzerThread* thread() const {
-            DEBUG_ASSERT(m_thread);
-            return m_thread.get();
+            DEBUG_ASSERT(m_pThread);
+            return m_pThread.get();
         }
 
         AnalyzerProgress analyzerProgress() const {
             return m_analyzerProgress;
         }
 
         bool submitNextTrack(const AnalyzerTrack& track) {
-            DEBUG_ASSERT(m_thread);
-            return m_thread->submitNextTrack(std::move(track));
+            DEBUG_ASSERT(m_pThread);
+            return m_pThread->submitNextTrack(std::move(track));
         }
 
         void suspendThread() {
-            if (m_thread) {
-                m_thread->suspend();
+            if (m_pThread) {
+                m_pThread->suspend();
             }
         }
 
         void resumeThread() {
-            if (m_thread) {
-                m_thread->resume();
+            if (m_pThread) {
+                m_pThread->resume();
             }
         }
 
         void stopThread() {
-            if (m_thread) {
-                m_thread->stop();
+            if (m_pThread) {
+                m_pThread->stop();
             }
         }
 
         void onAnalyzerProgress(AnalyzerProgress analyzerProgress) {
-            DEBUG_ASSERT(m_thread);
+            DEBUG_ASSERT(m_pThread);
             m_analyzerProgress = analyzerProgress;
         }
 
         void onThreadExit() {
-            DEBUG_ASSERT(m_thread);
-            m_thread.reset();
+            DEBUG_ASSERT(m_pThread);
+            m_pThread.reset();
             m_analyzerProgress = kAnalyzerProgressUnknown;
         }
 
       private:
-        AnalyzerThread::Pointer m_thread;
+        AnalyzerThread::Pointer m_pThread;
         AnalyzerProgress m_analyzerProgress;
     };
 

src/control/controlobject.cpp

@@ -41,7 +41,6 @@ ControlObject::ControlObject(const ConfigKey& key,
 ControlObject::~ControlObject() {
     DEBUG_ASSERT(m_pControl);
     const bool success = m_pControl->resetCreatorCO(this);
-    Q_UNUSED(success);
     DEBUG_ASSERT(success);
 }
 

src/library/analysis/analysisfeature.cpp

@@ -55,6 +55,12 @@ AnalysisFeature::AnalysisFeature(
           m_title(m_baseTitle) {
 }
 
+AnalysisFeature::~AnalysisFeature() {
+    // We need to delete m_pTrackAnalysisScheduler here immediately synchronously,
+    // waiting for pending threads to have finished, to not kill them during exit
+    delete m_pTrackAnalysisScheduler.release();
+}
+
 void AnalysisFeature::resetTitle() {
     m_title = m_baseTitle;
     emit featureIsLoading(this, false);

src/library/analysis/analysisfeature.h

@@ -18,7 +18,7 @@ class AnalysisFeature : public LibraryFeature {
   public:
     AnalysisFeature(Library* pLibrary,
                     UserSettingsPointer pConfig);
-    ~AnalysisFeature() override = default;
+    ~AnalysisFeature() override;
 
     QVariant title() override {
         return m_title;

src/library/dao/autodjcratesdao.cpp

@@ -43,10 +43,8 @@ namespace {
 constexpr int kLeastPreferredPercent = 15;
 
 // These consts are only used for DEBUG_ASSERTs
-#ifdef MIXXX_DEBUG_ASSERTIONS_ENABLED
 constexpr int kLeastPreferredPercentMin = 0;
 constexpr int kLeastPreferredPercentMax = 50;
-#endif
 
 int bounded_rand(int highest) {
     return QRandomGenerator::global()->bounded(highest);

src/mixer/playermanager.cpp

@@ -151,11 +151,9 @@ PlayerManager::~PlayerManager() {
     m_samplers.clear();
     m_microphones.clear();
     m_auxiliaries.clear();
-
-    if (m_pTrackAnalysisScheduler) {
-        m_pTrackAnalysisScheduler->stop();
-        m_pTrackAnalysisScheduler.reset();
-    }
+    // We need to delete m_pTrackAnalysisScheduler here immediately synchronously,
+    // waiting for pending threads to have finished, to not kill them during exit
+    delete m_pTrackAnalysisScheduler.release();
 }
 
 void PlayerManager::bindToLibrary(Library* pLibrary) {

src/qml/qmlwaveformoverview.cpp

@@ -76,7 +76,6 @@ void QmlWaveformOverview::slotTrackLoaded(TrackPointer pTrack) {
 }
 
 void QmlWaveformOverview::slotTrackLoading(TrackPointer pNewTrack, TrackPointer pOldTrack) {
-    Q_UNUSED(pOldTrack); // only used in DEBUG_ASSERT
     DEBUG_ASSERT(m_pCurrentTrack == pOldTrack);
     setCurrentTrack(pNewTrack);
 }

src/sources/soundsourcemp3.cpp

@@ -105,15 +105,11 @@ void logFrameHeader(QDebug logger, const mad_header& madHeader) {
            << "flags:" << formatHeaderFlags(madHeader.flags);
 }
 
-inline bool isUnrecoverableError(mad_error error) {
+bool isUnrecoverableError(mad_error error) {
     return (MAD_ERROR_NONE != error) && !MAD_RECOVERABLE(error);
 }
 
-#ifndef MIXXX_DEBUG_ASSERTIONS_ENABLED
-[[maybe_unused]]
-#endif
-inline bool
-hasUnrecoverableError(const mad_stream* pMadStream) {
+bool hasUnrecoverableError(const mad_stream* pMadStream) {
     if (pMadStream) {
         return isUnrecoverableError(pMadStream->error);
     }

src/track/beats.cpp

@@ -60,9 +60,7 @@ Beats::ConstIterator Beats::ConstIterator::operator+=(Beats::ConstIterator::diff
     }
 
     DEBUG_ASSERT(n > 0);
-#ifdef MIXXX_DEBUG_ASSERTIONS_ENABLED
-    const auto origValue = m_value;
-#endif
+    const audio::FramePos origValue = m_value;
 
     // Detect integer overflow in `m_beatOffset + n`
     const int maxBeatOffset = std::numeric_limits<Beats::ConstIterator::difference_type>::max();
@@ -108,9 +106,7 @@ Beats::ConstIterator Beats::ConstIterator::operator-=(Beats::ConstIterator::diff
     }
 
     DEBUG_ASSERT(n > 0);
-#ifdef MIXXX_DEBUG_ASSERTIONS_ENABLED
-    const auto origValue = m_value;
-#endif
+    const audio::FramePos origValue = m_value;
 
     // Detect integer overflow
     const int minBeatOffset = std::numeric_limits<Beats::ConstIterator::difference_type>::lowest();