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

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 killthem 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 killthem 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();

src/track/track.cpp

@@ -923,7 +923,7 @@ const ConstWaveformPointer& Track::getWaveform() const {
 }
 
 void Track::setWaveform(ConstWaveformPointer pWaveform) {
-    m_waveform = pWaveform;
+    m_waveform = std::move(pWaveform);
     emit waveformUpdated();
 }
 

src/util/assert.h

@@ -49,10 +49,10 @@ inline void mixxx_release_assert(const char* assertion, const char* file, int li
 /// MIXXX_DEBUG_ASSERTIONS_FATAL then the warning message is fatal. Compiles
 /// to nothing in release builds.
 ///
-/// Be careful of the common mistake with assertions:
+/// In release builds, it marks cond as used and checks if it can be converted
+/// to bool. Be careful of the common mistake with assertions:
 ///   DEBUG_ASSERT(doSomething());
-///
-/// In release builds, doSomething() is never called!
+/// doSomething() is never called, in In release builds.
 #ifdef MIXXX_DEBUG_ASSERTIONS_ENABLED
 #define DEBUG_ASSERT(cond)                                                  \
     do                                                                      \
@@ -61,8 +61,9 @@ inline void mixxx_release_assert(const char* assertion, const char* file, int li
         }                                                                   \
     while (0)
 #else
-#define DEBUG_ASSERT(cond) \
-    do {                   \
+#define DEBUG_ASSERT(cond)                                                \
+    do {                                                                  \
+        [[maybe_unused]] std::size_t s = sizeof(static_cast<bool>(cond)); \
     } while (0)
 #endif
 

src/util/parented_ptr.h

@@ -1,13 +1,10 @@
 #pragma once
 
-#ifdef MIXXX_DEBUG_ASSERTIONS_ENABLED
 #include <QObject>
-#endif
 #include <QPointer>
 #include <memory>
 
 #include "util/assert.h"
-#include "util/parented_ptr.h"
 
 // Use this wrapper class to clearly represent a raw pointer that is owned by the QT object tree.
 // Objects which both derive from QObject AND have a parent object, have their lifetime governed by