Eject crash fix #1924

daschuer · 2018-11-28T23:05:32Z

This PRs should fix some crashes that may occur during switching or ejecting tracks.
https://bugs.launchpad.net/mixxx/+bug/1801874

The main fix is that we now that we obtain a strong reference to the track and beat pointers changed by the engine worker thread.
I have also included some related changes that was helpful to have a clear look on the code during development.

…ability.

… sample rate

uklotzde · 2018-11-28T23:17:33Z

This looks like a minefield, completely broken by design 🙈

uklotzde · 2018-11-29T08:12:11Z

src/engine/bpmcontrol.h


    TapFilter m_tapFilter;

-    TrackPointer m_pTrack;
-    BeatsPointer m_pBeats;
+    TrackPointer m_pTrack; // is witten from an engine worker thread


Typo. Use a single line comment for both elements. What about the other concurrently accessed members? We should at least group them together.

uklotzde · 2018-11-29T08:20:08Z

OMG, what an ugly patch for evil code! Not your fault, @daschuer ;)

I didn't notice any obvious mistakes while quickly scrolling over all the changes.

rryan · 2018-11-29T12:27:13Z

I'll be the first to say the engine needs to be rewritten from scratch due to flawed designs around mutable shared state, but let's avoid talking about blame b/c a culture of shaming isn't healthy. People who wrote this code are receiving your comments :). https://codeascraft.com/2012/05/22/blameless-postmortems/

…

On Thu, Nov 29, 2018, 3:20 AM Uwe Klotz ***@***.***> wrote: OMG, what an ugly patch for evil code! Not your fault, @daschuer <https://github.com/daschuer> ;) I didn't notice any obvious mistakes while quickly scrolling over all the changes. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1924 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABnn4gLJKcwH8lrvCt40X97W0--P-E3ks5uz5i4gaJpZM4Y4nAt> .

uklotzde · 2018-11-29T12:47:55Z

Sorry for the comment, @rryan, it was not my intention to blame anyone!

Every project gets bigger with time and design decisions that were reasonable at the start might turn out as unhandy later. That's daily business, no matter how experienced you (think you) are. Primarily I struggle with the code that I have written myself in the past ;)

What I actually should have expressed is that @daschuer's fixes are very brittle and I'm uncomfortable with this code or coding style. But unfortunately we have no choice other than to mitigate the issues.

daschuer · 2018-12-01T00:09:09Z

OK, improved even more,

uklotzde · 2018-12-02T21:02:05Z

I vote for 2.2 instead of master, because the threat is real and the successor of 2.2 is months away. Due to the conflicts I would like to avoid a fix for 2.1 that will go out of support soon.

daschuer · 2018-12-02T21:07:17Z

Most changes in enginebuffer.cpp are white space changes due to the function extraction.
If you use Meld as diff tool, you can filter out these changes and it becomes less scary, hopefully.

Here is a eject crash report:
https://www.mixxx.org/forums/viewtopic.php?f=3&t=12218&p=40200&hilit=crash#p40200
But it is unclear if this PR will actually fix this.

I think the crash condition is not very likely for normal users, because you need to to use controls during ejecting the track. I can only imagine that this happens when using AUTO DJ, or some automation in scripts,

daschuer · 2018-12-02T21:37:57Z

It only conflicts with some typo fixes when merging to 2.2. The conflict is displayed that big, because of surrounding white space changes.

uklotzde · 2018-12-08T10:17:02Z

src/engine/bpmcontrol.cpp

-        m_sGroup(group) {
+                       UserSettingsPointer pConfig)
+        : EngineControl(group, pConfig),
+          m_tapFilter(this, kFilterLength, kMaxInterval),


It doesn't hurt to explicitly initialize the atomic values. Here they are used on a higher level of abstraction. We should not rely on the implicit default values that are pure numbers without any meaning in the context of BPM control.

uklotzde · 2018-12-08T10:20:50Z

src/engine/bpmcontrol.cpp

@@ -834,9 +841,9 @@ void BpmControl::setInstantaneousBpm(double instantaneousBpm) {

 void BpmControl::resetSyncAdjustment() {
    // Immediately edit the beat distance to reflect the new reality.
-    double new_distance = m_pThisBeatDistance->get() + m_dUserOffset;
+    double new_distance = m_pThisBeatDistance->get() + m_dUserOffset.getValue();


Why getValue/setValue instead of fetchAndStore?

It is ControlValueAtomic, which has no fetchAndStore().

uklotzde · 2018-12-08T10:40:30Z

src/engine/bpmcontrol.cpp

        // Note: dThisBeatLength is fractional frames count * 2 (stereo samples)
        pGroupFeatures->beat_length_sec = dThisBeatLength / kSamplesPerFrame
-                / m_pTrack->getSampleRate() / calcRateRatio();


Might getTrackSampleRate() return 0.0 due to race conditions if the track has just been unloaded? We better should add a check for this.

Unrelated (or maybe related): Is calcRateRatio() guaranteed to always return a value that is strictly != 0 in any situation?

uklotzde · 2018-12-08T10:45:02Z

src/engine/enginebuffer.cpp

@@ -171,8 +171,7 @@ EngineBuffer::EngineBuffer(QString group, UserSettingsPointer pConfig,
    m_pRepeat = new ControlPushButton(ConfigKey(m_group, "repeat"));
    m_pRepeat->setButtonMode(ControlPushButton::TOGGLE);

-    // Sample rate
-    m_pSampleRate = new ControlProxy("[Master]", "samplerate", this);
+    m_pMasterSampleRate = new ControlProxy("[Master]", "samplerate", this);


m_pKeylockEngine is also bound to "[Master]" and doesn't contain a prefix in its name. Do we really need to add "Master" just for this member?

uklotzde · 2018-12-08T10:45:54Z

src/engine/enginebuffer.cpp

@@ -372,7 +371,7 @@ double EngineBuffer::getLocalBpm() {
 }

 void EngineBuffer::setEngineMaster(EngineMaster* pEngineMaster) {
-    foreach (EngineControl* pControl, m_engineControls) {
+    for (const auto& pControl: m_engineControls) {


Please use qAsConst(m_engineControls) to avoid a copy.

uklotzde · 2018-12-08T10:46:18Z

src/engine/enginebuffer.cpp

-    for (QList<EngineControl*>::iterator it = m_engineControls.begin();
-         it != m_engineControls.end(); ++it) {
-        EngineControl *pControl = *it;
+    for (const auto& pControl: m_engineControls) {


Please use qAsConst(m_engineControls) to avoid a copy.

uklotzde · 2018-12-08T10:50:30Z

src/engine/enginebuffer.cpp

+        TrackPointer pNewTrack, TrackPointer pOldTrack) {
+    // First inform engineControls directly
+    // Note: we are still in a worker thread.
+    for (const auto& pControl: m_engineControls) {


qAsConst is missing

uklotzde · 2018-12-08T10:53:27Z

src/engine/enginecontrol.h

    double getCurrentSample() const;
    double getTotalSamples() const;
+    double getTrackSampleRate() const;


These 3 accessors should be replaced by a single function that returns the whole thing atomically. I haven't check how many callers are affected.

uklotzde · 2018-12-08T10:59:43Z

src/util/tapfilter.cpp

@@ -11,6 +11,7 @@ TapFilter::~TapFilter() {
 }

 void TapFilter::tap() {
+    QMutexLocker locker(&m_mutex);
    mixxx::Duration elapsed = m_timer.restart();
    if (elapsed <= m_maxInterval) {
        emit(tapped(m_mean.insert(elapsed.toDoubleMillis()), m_mean.size()));


Emitting signals from an event loop thread while holding a lock may cause deadlocks.

uklotzde · 2018-12-08T11:05:54Z

TODO: Just some technical code fixes, optimizations, and clarifications.

The changes in EngineBuffer are hard to review. I'm silently assuming that the implementation hasn't changed substantially and only code has been reordered, reformatted, and updated.

Are we able to agree on a target? The threats are real and at least 2.2 should receive these fixes.

…08222

daschuer · 2018-12-13T20:57:37Z

Ready!

uklotzde · 2018-12-14T11:33:06Z

LGTM.

A huge change set. Should we still merge this into 2.1?

daschuer · 2018-12-14T15:04:39Z

This was getting bigger due to fixing the usefully review comments. We can go back to an earlier version with less changes. But that feels odd.

2.1 and 2.2 can considered as stable.
So if we decide for one, we should merge the other too.
Since this fixes a crasher, I think we should add it. We have many tests around this code. So I a confident to merge it.

uklotzde · 2018-12-14T15:47:21Z

Excluding some of the changes is not an option. I'm confident that we don't introduce regressions, all changes are straightforward. Only the reformatting causes some noise.

Fixing 2.2 is mandatory, fixing 2.1 is optional.

daschuer · 2018-12-14T19:12:41Z

OK, so let's merge it. Thank you for review.

daschuer added 16 commits November 28, 2018 23:45

Replace connections to EngineControls by direct calls for better trac…

35aca68

…ability.

use range based loops

82afb2a

Axtracted processTrackLocked() for a save Track access.

3f6865b

removed unused pOldTrack from trackLoaded function

7c87fc9

rename m_pSampleRate to m_pMasterSampleRate to distinguish from track…

8345b0d

… sample rate

fix m_trackSampleRateOld handling

fb25a89

fix m_trackSamplesOld handling

51bd6b5

removed unused totalSamples from process call

e675160

store track sample rate with m_sampleOfTrack

b3fd646

use m_pTrack thread save

3681a99

use m_pBeats in a threadsave way

de532d3

LoopingControl: use m_pBeats and m_pTrack in a thread save way

44e80f1

QuantizeControl: use m_pTrack and m_pBeats thread save

eb79a3e

Removed unused m_pTrack from RateControl

5f2105f

VinylControlControl: use m_pTrack thread save

3382d2a

make BpmControl::resetSyncAdjustment thread save

9ea73ae

daschuer added this to the 2.1.6 milestone Nov 28, 2018

daschuer added the major bug label Nov 28, 2018

uklotzde reviewed Nov 29, 2018

View reviewed changes

daschuer added 2 commits November 30, 2018 23:32

Fix typo

fb706ca

fix a race condition in bpmcontrol and improve documentation

213ccb3

mixxxdj deleted a comment Nov 30, 2018

daschuer added 2 commits December 1, 2018 00:59

RateControl: Make config options atomic

c6a001a

remove unused variable

3a2fdc4

mixxxdj deleted a comment Dec 1, 2018

uklotzde reviewed Dec 8, 2018

View reviewed changes

uklotzde suggested changes Dec 8, 2018

View reviewed changes

uklotzde mentioned this pull request Dec 8, 2018

Pitchbend direction fix lp1770745 #1906

Merged

mixxxdj deleted a comment Dec 11, 2018

daschuer force-pushed the engine_controls branch from 69d2eea to c8c5883 Compare December 12, 2018 05:55

Initalize atomic values and check trackSamples for 0

0652d02

mixxxdj deleted a comment Dec 12, 2018

daschuer added 3 commits December 12, 2018 20:05

Revert rename of m_pSampleRate

df5653c

use qAsConst()

3e87069

reset play and cue indicator unconditionally after eject, fixing lp18…

562387c

…08222

mixxxdj deleted a comment Dec 12, 2018

daschuer added 2 commits December 13, 2018 21:45

replace single getter with one for the whol atomic struct.

94756f3

unlock mutex before emit a signal

d6b9f8b

mixxxdj deleted a comment Dec 13, 2018

uklotzde approved these changes Dec 14, 2018

View reviewed changes

daschuer merged commit fd15f29 into mixxxdj:2.1 Dec 14, 2018

daschuer deleted the engine_controls branch September 26, 2021 17:36

This was referenced Aug 23, 2022

Possible race condition when unloading a track #9513

Closed

Ejecting a track twice CUE starts blinking #9543

Closed

When a track with old BPM data is rescanned after it is loaded, the auto-sync value can get crazy #9550

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eject crash fix #1924

Eject crash fix #1924

daschuer commented Nov 28, 2018

uklotzde commented Nov 28, 2018

uklotzde Nov 29, 2018

uklotzde commented Nov 29, 2018

rryan commented Nov 29, 2018 via email

uklotzde commented Nov 29, 2018

daschuer commented Dec 1, 2018

uklotzde commented Dec 2, 2018

daschuer commented Dec 2, 2018

daschuer commented Dec 2, 2018

uklotzde Dec 8, 2018

uklotzde Dec 8, 2018

daschuer Dec 11, 2018

uklotzde Dec 8, 2018

uklotzde Dec 8, 2018

uklotzde Dec 8, 2018

uklotzde Dec 8, 2018

uklotzde Dec 8, 2018

uklotzde Dec 8, 2018

uklotzde Dec 8, 2018

uklotzde commented Dec 8, 2018

daschuer commented Dec 13, 2018

uklotzde commented Dec 14, 2018

daschuer commented Dec 14, 2018

uklotzde commented Dec 14, 2018

daschuer commented Dec 14, 2018

Eject crash fix #1924

Eject crash fix #1924

Conversation

daschuer commented Nov 28, 2018

uklotzde commented Nov 28, 2018

Choose a reason for hiding this comment

uklotzde commented Nov 29, 2018

rryan commented Nov 29, 2018 via email

uklotzde commented Nov 29, 2018

daschuer commented Dec 1, 2018

uklotzde commented Dec 2, 2018

daschuer commented Dec 2, 2018

daschuer commented Dec 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde commented Dec 8, 2018

daschuer commented Dec 13, 2018

uklotzde commented Dec 14, 2018

daschuer commented Dec 14, 2018

uklotzde commented Dec 14, 2018

daschuer commented Dec 14, 2018