GlobalTrackCache: Fix edge cases and tests #2293

uklotzde · 2019-09-22T17:59:38Z

I still experience a crash caused by GlobalTrackCache once in a while when closing Mixxx. It happens very infrequently, mostly after loading just a single or a few tracks. These crashes may still be unsolved, nevertheless I discovered some other issues during my investigations:

Updated or removed outdated comments
Ensure that all cache items are only moved, never copied
Verify that functors are called only once
Fixed edge cases when evicting tracks that might occur due to expected race conditions
Disconnect receivers and block signals in GlobalTrackCache instead of in the Library code.
Fixed a memory leak in the concurrency test. Track objects were neither evicted nor deleted because no main event loop is executed! Note: deleteLater() is still not invoked even when calling QCoreApplication::processEvents() periodically! I have added a workaround for Qt >= 5.6.0 in the code, but this memory leak is still present for earlier versions (only in tests).

...to avoid that more track objects are created than destroyed.

uklotzde · 2019-09-22T18:00:09Z

Next try, functors need to be copyable.

uklotzde · 2019-09-22T21:58:35Z

I was not able to find out which test actually caused the crash and had to add a workaround. The WPushButtonTest can't be the culprit. Startup and shutdown of application components during tests don't seem to be sane.

uklotzde · 2019-09-22T22:21:19Z

The crash during the tests is hopefully fixed now. The insights gained from this analysis and the resulting fix might also eliminate the rare crashes on shutdown. Only time will tell.

src/test/globaltrackcache_test.cpp

src/test/librarytest.h

daschuer · 2019-09-24T20:23:31Z

src/track/globaltrackcache.cpp

+    DEBUG_ASSERT(plainPtr->signalsBlocked());
+#if QT_VERSION >= QT_VERSION_CHECK(5, 6, 0)
+    if (plainPtr->thread()->loopLevel() > 0) {
+        plainPtr->deleteLater();


https://doc.qt.io/qt-5/qthread.html:

Note: This can only be called within the thread itself, i.e. when it is the current thread.

Is this guaranteed?

~~Yes. Cache entries are always evicted on the main (event loop) thread, and then the track pointer goes out of scope.~~ ...or maybe not. I need to check this!

I found a correct, more explicit solution. The tests now provide a function pointer to override the default QObject::deleteLater() deletion.

This new solution is independent of the Qt version.

src/track/globaltrackcache.cpp

uklotzde · 2019-09-28T13:54:55Z

In a branch without these changes the spurious crashes are back. Seems like the fixes work as expected!

daschuer

The changes are looking sensible.
Unfortunately I do not understand which part is the crux for the crash fix here?
Can you give some additional hints.

src/test/globaltrackcache_test.cpp

daschuer · 2019-09-28T17:50:11Z

src/track/globaltrackcache.h

+        void operator()(Track* pTrack) const;
+
+    private:
+        deleteTrackFn m_deleteTrack;


How about deleteTrackFn m_deleteTrackFn ?

m_typeName? The name of the function is deleteTrack, the corresponding type is deleteTrackFn.

I have stumbled over this because it can also be bool m_deleteTrack;
The idea was to make it obvious from the variable name that this is a function pointer.

src/track/globaltrackcache.cpp

daschuer · 2019-09-28T18:18:07Z

src/track/globaltrackcache.cpp

-        if (trackById != m_tracksById.end()) {
-            DEBUG_ASSERT(trackById->second->getPlainPtr() == plainPtr);
+        if (trackById != m_tracksById.end() &&
+                trackById->second->getPlainPtr() == plainPtr) {


What happens if this fails?

I need to check this again

uklotzde · 2019-09-28T20:52:27Z

Possible crash fixes:

evict() -> tryEvict(): Due to a race conditions during evictAndSave() the wrong pointer could be removed from the maps. Renamed the function to better reflect these circumstances. The corresponding DEBUG_ASSERTION never triggered during the tests, but would fail now.
Invoke QObject::disconnect() when evicting a track from the cache (+ block signals as before, just in case)
Invoke QCoreApplication::processEvents() before destroying the cache singleton to finish the pending deletion of track objects

Not sure which of these actually fixed the issue. The tests never worked as intended and the issues slipped through unnoticed.

daschuer · 2019-09-30T06:14:13Z

LGTM, thank you.

uklotzde added 10 commits September 22, 2019 19:56

Handle an edge case when evicting cached tracks

dc891b6

Delete obsolete comment

427cf4a

Reword comment

cb3fb21

Replace getter/setter with dedicated functions

b3eefd5

Block track signals in cache instead of library

55703a4

Ensure that cache items are moved and not copied

0554943

Verify that the evict-and-save functor is only executed once

3ebfee8

Move code block

8266455

Balance load in test threads

2b1d429

...to avoid that more track objects are created than destroyed.

Fix memory leak in test

540c933

uklotzde added this to the 2.2.3 milestone Sep 22, 2019

uklotzde requested a review from daschuer September 22, 2019 17:59

uklotzde changed the title ~~GlobalTrackCache: Fix edge cases and tests~~ [WiP] GlobalTrackCache: Fix edge cases and tests Sep 22, 2019

Fix crash during tests

1181897

uklotzde changed the title ~~[WiP] GlobalTrackCache: Fix edge cases and tests~~ GlobalTrackCache: Fix edge cases and tests Sep 22, 2019

daschuer reviewed Sep 24, 2019

View reviewed changes

uklotzde added 4 commits September 25, 2019 00:21

Explain how the test is intended to work

b6056bd

Rename class member

1d9953a

Rename method and parameter

bf0fe4c

Use a custom function for deleting Track objects during tests

ecbf655

daschuer reviewed Sep 28, 2019

View reviewed changes

uklotzde added 2 commits September 28, 2019 21:42

Explain how the test works

b57be5a

Explain race conditions during evict and verify assumptions

83ea2ca

uklotzde added 2 commits September 29, 2019 00:01

Rename function pointer type and values

81304d2

Fix warning

313e0c1

daschuer merged commit 9688e04 into mixxxdj:2.2 Sep 30, 2019

uklotzde deleted the 2.2_globaltrackcache branch September 30, 2019 07:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GlobalTrackCache: Fix edge cases and tests #2293

GlobalTrackCache: Fix edge cases and tests #2293

uklotzde commented Sep 22, 2019

uklotzde commented Sep 22, 2019

uklotzde commented Sep 22, 2019

uklotzde commented Sep 22, 2019

daschuer Sep 24, 2019

uklotzde Sep 24, 2019 •

edited

Loading

uklotzde Sep 25, 2019

uklotzde Sep 25, 2019

uklotzde commented Sep 28, 2019

daschuer left a comment

daschuer Sep 28, 2019

uklotzde Sep 28, 2019

daschuer Sep 28, 2019

daschuer Sep 28, 2019

uklotzde Sep 28, 2019

uklotzde commented Sep 28, 2019 •

edited

Loading

daschuer commented Sep 30, 2019

GlobalTrackCache: Fix edge cases and tests #2293

GlobalTrackCache: Fix edge cases and tests #2293

Conversation

uklotzde commented Sep 22, 2019

uklotzde commented Sep 22, 2019

uklotzde commented Sep 22, 2019

uklotzde commented Sep 22, 2019

Choose a reason for hiding this comment

uklotzde Sep 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde commented Sep 28, 2019

daschuer left a 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 Sep 28, 2019 • edited Loading

daschuer commented Sep 30, 2019

uklotzde Sep 24, 2019 •

edited

Loading

uklotzde commented Sep 28, 2019 •

edited

Loading