Fix killing Analyzis worker threads during shutdown, possible data loss. by daschuer · Pull Request #16064 · mixxxdj/mixxx

daschuer · 2026-02-28T00:13:43Z

The issue was that they where auto deleted after finish via deleteLater() however it can happen that the Qt event cue is already down and the object is never deleted.
The fix is to add a synchronous delete call in the owners destructor that stops the workers, waits for finish and deletes the threads manually.

The issue has been found by Valgrind:

==43819== 4,194,304 bytes in 1 blocks are still reachable in loss record 9,796 of 9,796
==43819==    at 0x5E01F95: operator new(unsigned long) (vg_replace_malloc.c:488)
==43819==    by 0x4AF3016: allocate (new_allocator.h:127)
==43819==    by 0x4AF3016: allocate (allocator.h:185)
==43819==    by 0x4AF3016: allocate (alloc_traits.h:464)
==43819==    by 0x4AF3016: _M_allocate (stl_vector.h:346)
==43819==    by 0x4AF3016: _M_default_append (vector.tcc:635)
==43819==    by 0x4AF3016: resize (stl_vector.h:940)
==43819==    by 0x4AF3016: resize (waveform.cpp:209)
==43819==    by 0x4AF3016: Waveform::readByteArray(QByteArray const&) (waveform.cpp:169)
==43819==    by 0x4AF378C: Waveform::Waveform(QByteArray const&) (waveform.cpp:29)
==43819==    by 0x4DB5659: WaveformFactory::loadWaveformFromAnalysis(AnalysisDao::AnalysisInfo const&) (waveformfactory.cpp:7)
==43819==    by 0x4C2863C: AnalyzerWaveform::shouldAnalyze(std::shared_ptr<Track>) const (analyzerwaveform.cpp:116)
==43819==    by 0x4C28D15: AnalyzerWaveform::initialize(AnalyzerTrack const&, mixxx::audio::SampleRate, long) (analyzerwaveform.cpp:48)
==43819==    by 0x4C23452: initialize (analyzer.h:73)
==43819==    by 0x4C23452: AnalyzerThread::doRun() (analyzerthread.cpp:144)
==43819==    by 0x4AC804C: WorkerThread::run() (workerthread.cpp:72)
==43819==    by 0xACA2FCE: operator() (qthread_unix.cpp:356)
==43819==    by 0xACA2FCE: terminate_on_exception<QThreadPrivate::start(void*)::<lambda()> > (qthread_unix.cpp:292)
==43819==    by 0xACA2FCE: QThreadPrivate::start(void*) (qthread_unix.cpp:315)
==43819==    by 0xB42AAC2: start_thread (pthread_create.c:442)
==43819==    by 0xB4BBA83: clone (clone.S:100)

acolombier · 2026-02-28T00:18:13Z

+        ~Worker() {
+            if (m_pThread) {
+                m_pThread->stop();
+                m_pThread->wait();


Should we add a timeout(+assert) to prevent risk of infinite wait?

… active during application exit. This happens because the deleteLater() signal might not reach the thread early enough before the event cue is destroyed.

daschuer · 2026-02-28T00:35:00Z

I have debugged this with extra qDebug() messages and it seems to work now. However the is still a related memory left over, slightly different

==68283== 1,048,576 bytes in 1 blocks are still reachable in loss record 9,571 of 9,571
==68283==    at 0x5E01F95: operator new(unsigned long) (vg_replace_malloc.c:488)
==68283==    by 0x4AF49AD: allocate (new_allocator.h:127)
==68283==    by 0x4AF49AD: allocate (allocator.h:185)
==68283==    by 0x4AF49AD: allocate (alloc_traits.h:464)
==68283==    by 0x4AF49AD: _M_allocate (stl_vector.h:346)
==68283==    by 0x4AF49AD: _M_create_storage (stl_vector.h:361)
==68283==    by 0x4AF49AD: _Vector_base (stl_vector.h:305)
==68283==    by 0x4AF49AD: vector (stl_vector.h:524)
==68283==    by 0x4AF49AD: std::vector<WaveformData, std::allocator<WaveformData> >::_M_fill_assign(unsigned long, WaveformData const&) (vector.tcc:262)
==68283==    by 0x4AF2167: assign (stl_vector.h:750)
==68283==    by 0x4AF2167: assign (waveform.cpp:215)
==68283==    by 0x4AF2167: Waveform::Waveform(int, long, int, int) (waveform.cpp:70)
==68283==    by 0x4C292A0: AnalyzerWaveform::initialize(AnalyzerTrack const&, mixxx::audio::SampleRate, long) (analyzerwaveform.cpp:64)
==68283==    by 0x4C24132: initialize (analyzer.h:73)
==68283==    by 0x4C24132: AnalyzerThread::doRun() (analyzerthread.cpp:144)
==68283==    by 0x4AC8D2C: WorkerThread::run() (workerthread.cpp:72)
==68283==    by 0xACA2FCE: operator() (qthread_unix.cpp:356)
==68283==    by 0xACA2FCE: terminate_on_exception<QThreadPrivate::start(void*)::<lambda()> > (qthread_unix.cpp:292)
==68283==    by 0xACA2FCE: QThreadPrivate::start(void*) (qthread_unix.cpp:315)
==68283==    by 0xB42AAC2: start_thread (pthread_create.c:442)
==68283==    by 0xB4BBA83: clone (clone.S:100)

daschuer · 2026-04-03T21:39:22Z

The remaining memory leak is unrelated. It happens because Waveform is created as a QSharedPointer in the Analyzer thread, but then distributed in Mixxx. So there is one owner left that is not deleted. It is no longer the Analyzer thread.

daschuer · 2026-04-23T05:24:27Z

Done.

Copilot

Pull request overview

This PR updates analyzer shutdown handling so analysis worker threads are stopped, waited for, and deleted synchronously instead of relying entirely on Qt deferred deletion during application shutdown. It also includes related cleanup enabled by the adjusted release-build behavior of DEBUG_ASSERT.

Changes:

Adds synchronous cleanup of TrackAnalysisScheduler and its worker AnalyzerThreads in shutdown paths.
Updates DEBUG_ASSERT release behavior to type-check/mark conditions as used, then removes several now-unneeded Q_UNUSED/#ifdef workarounds.
Applies small cleanup changes such as moving waveform pointers and simplifying includes.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
`src/analyzer/trackanalysisscheduler.h`	Adds `Worker` destructor that stops, waits for, and deletes analyzer threads synchronously.
`src/analyzer/trackanalysisscheduler.cpp`	Updates worker thread state check and adds destructor comment.
`src/mixer/playermanager.cpp`	Deletes the track analysis scheduler synchronously during player manager shutdown.
`src/library/analysis/analysisfeature.h`	Declares a custom destructor for synchronous scheduler cleanup.
`src/library/analysis/analysisfeature.cpp`	Implements synchronous deletion of the analysis scheduler.
`src/util/assert.h`	Changes release-build `DEBUG_ASSERT` to compile-check the condition without evaluating it.
`src/util/parented_ptr.h`	Simplifies includes around QObject usage.
`src/track/track.cpp`	Moves waveform shared pointer into the track field.
`src/track/beats.cpp`	Removes debug-only guards around values now referenced by release `DEBUG_ASSERT`.
`src/sources/soundsourcemp3.cpp`	Removes debug-assertion-only unused annotations/guards.
`src/qml/qmlwaveformoverview.cpp`	Removes now-unneeded `Q_UNUSED`.
`src/library/dao/autodjcratesdao.cpp`	Unguards constants used by `DEBUG_ASSERT`.
`src/control/controlobject.cpp`	Removes now-unneeded `Q_UNUSED`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

daschuer · 2026-05-31T08:45:38Z

+                bool success = m_pThread->wait(5000); // 5 s
+                DEBUG_ASSERT(success);
+                m_pThread->wait();


That only happen with DEBUG_ASSERTIONS_FATAL and is no issue.

+/// 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.


-        m_pTrackAnalysisScheduler.reset();
-    }
+    // We need to delete m_pTrackAnalysisScheduler here immediately synchronously,
+    // waiting for pending threads to have finished, to not killthem during exit



+AnalysisFeature::~AnalysisFeature() {
+    // We need to delete m_pTrackAnalysisScheduler here immediately synchronously,
+    // waiting for pending threads to have finished, to not killthem during exit



 TrackAnalysisScheduler::~TrackAnalysisScheduler() {
    kLogger.debug() << "Destroying";
+    // Here the workers in m_workers are deleted after waiting for the associated thread is finished


daschuer · 2026-05-31T09:09:19Z

Done.

JoergAtGithub

LGTM!

daschuer added 2 commits February 27, 2026 21:57

use p prefix for AnalyzerThread::Pointer

853e007

Introduce Worker::hasThread() replacing bool operator

5ffd4c4

github-actions Bot added the analyzer label Feb 28, 2026

acolombier reviewed Feb 28, 2026

View reviewed changes

daschuer added 2 commits February 28, 2026 01:21

Add a synchrony Worker destructor that avoids that workers are killed…

db9c0d7

… active during application exit. This happens because the deleteLater() signal might not reach the thread early enough before the event cue is destroyed.

Add a synchrony Worker destructor that avoids that workers are killed…

649614c

… active during application exit. This happens because the deleteLater() signal might not reach the thread early enough before the event cue is destroyed.

daschuer force-pushed the valigrind_analyzerthread branch from 765eacd to 649614c Compare February 28, 2026 00:25

github-actions Bot added the library label Feb 28, 2026

acolombier added this to the 2.6.0 milestone Mar 13, 2026

acolombier changed the base branch from 2.5 to 2.6 March 13, 2026 16:32

daschuer added 2 commits April 3, 2026 23:46

Analyzer thread: Assert for waiting not more than 5 s during shutdown

42a3acb

Avoid extra reference counting by std:move

8825b22

github-actions Bot added the qml label Apr 4, 2026

daschuer force-pushed the valigrind_analyzerthread branch 5 times, most recently from 6966808 to cf66ba4 Compare April 4, 2026 15:15

github-actions Bot added the soundsource label Apr 4, 2026

daschuer force-pushed the valigrind_analyzerthread branch from cf66ba4 to 0e51f94 Compare April 5, 2026 10:06

daschuer changed the base branch from 2.6 to 2.5 April 5, 2026 12:13

daschuer added 2 commits April 5, 2026 14:18

Fix includes in parented_ptr.h

3df5704

DEBUG_ASSERT condition is now considered as used.

e869551

daschuer force-pushed the valigrind_analyzerthread branch from 28b1f0c to e869551 Compare April 5, 2026 12:19

JoergAtGithub requested a review from Copilot May 30, 2026 17:48

Copilot started reviewing on behalf of JoergAtGithub May 30, 2026 17:48 View session

Copilot AI reviewed May 30, 2026

View reviewed changes

Improve the description of DEBUG_ASSERT() and fix typos.

9fbbf7b

daschuer force-pushed the valigrind_analyzerthread branch from caa3aa6 to 9fbbf7b Compare May 31, 2026 09:09

JoergAtGithub approved these changes Jun 17, 2026

View reviewed changes

JoergAtGithub merged commit 4f87b4f into mixxxdj:2.5 Jun 17, 2026
15 checks passed

Navigation Menu

Uh oh!

Uh oh!

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

Fix killing Analyzis worker threads during shutdown, possible data loss. #16064
JoergAtGithub merged 9 commits into
mixxxdj:2.5from
daschuer:valigrind_analyzerthread

daschuer commented Feb 28, 2026

Uh oh!

acolombier Feb 28, 2026

Uh oh!

daschuer commented Feb 28, 2026

Uh oh!

daschuer commented Apr 3, 2026

Uh oh!

daschuer commented Apr 23, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

daschuer May 31, 2026

Uh oh!

daschuer commented May 31, 2026

Uh oh!

JoergAtGithub left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Uh oh!

Conversation

daschuer commented Feb 28, 2026

Uh oh!

acolombier Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

daschuer commented Feb 28, 2026

Uh oh!

daschuer commented Apr 3, 2026

Uh oh!

daschuer commented Apr 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

daschuer May 31, 2026

Choose a reason for hiding this comment

Uh oh!

daschuer commented May 31, 2026

Uh oh!

JoergAtGithub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants