Analyze queue view can freeze mixxx UI

[Ilkka Tuohela]

In 1.11 trunk, you can completely freeze mixxx UI when analysis is running.

How to reproduce:
- Sort analysis queue by BPM to be able to jump to position where bpm is 0 (not analyzed)
- Select tracks for analysis, I usually have 100-200 tracks in the queue
- Jump in UI to the first track selected
- Start analysis
- Use scroll wheel to browse up and down in analysis view list
- The UI hangs when a track is done and the view is updated with new bpm while you scroll it

There is plenty of work to do for analysis, as discussed on mailing list, but I think this type of bugs are critical to fix for 1.11 release.

[Max Linke (max-linke)]

I think this is the same problem as reported here https://bugs.launchpad.net/mixxx/+bug/1010365

[Ilkka Tuohela (hile)]

Running this under lldb debugger in OS/X, the cause of deadlock is easy to see.

Deadlock in analyzer I reported:
- TrackDAO::getTrack QMutexLocker calls m_trackCache.insert()
- which triggers TrackDAO::deleteTrack (there is a duplicate?)
- which is trying to lock mutex within mutex set in first step

What might happen is that analyzer queue adds or removes the weak cache pointer from another thread between the two if (m_trackCache.contains(id)) statements in TrackDAO::getTrack.

These kinds of errors might be reason for many other database access and update related bugs as well. I try to fix myself TrackDAO::getTrack locking, but here is the info if I did not succeed :)

[Ilkka Tuohela (hile)]

And more information. The actual reason is that m_trackCache is qCache with maximum size TRACK_CACHE_SIZE, and qCache will automatically remove old objects from cache if it is full.

Meaning. m_trackCache.insert() must never be called inside a mutex, as we do in getTrack, otherwise qCache automatic cleanup may call deleteTrack and deadlocks.

[Ilkka Tuohela (hile)]

Attached is a simple patch, which moves the cache insert out of the query mutex, fixing the deadlock.

[Max Linke (max-linke)]

This fixes the deadlock issue for me

[Daniel Schürmann (daschuer)]

Thank you Ilkka, for you investigations and the solution!
Committed to lp:mixxx/1.11 #3308

[RJ Skerry-Ryan (rryan)]

The fix may resolve the deadlock but it makes the access of m_sTracks non-thread safe which can cause crashes.

Another more troubling issue is that TrackDAO has not been written to be thread safe at all.

A shared track cache has inherently unsafe consequences. Calling pTrack->doSave() from TrackDAO::deleteTrack invokes TrackDAO::slotTrackSave via a DirectConnection. The particular TrackDAO that is called is the one that created the TrackPointer in question. This means that if the library scanner thread causes a track that was created by the main-thread TrackDAO to be dropped from m_sTracks, then the main thread's TrackDAO::slotDeleteTrack method will be called from the library scanner thread. This is bad because TrackDAO is not thread safe so if the main thread is also calling a TrackDAO method at the same time, Mixxx will crash.

I think that we will need to get rid of the shared track cache and figure out a new way to prevent memory usage from increasing. Maybe we can make use of a track cache optional and only enable it for the main-thread TrackDAO?

[Daniel Schürmann (daschuer)]

Hi RJ,

Thank you for your review!

I cannot get the point where the access of m_sTracks is non-threadsafe it should be already protected properly by m_sTracksMutex.

I do agree, that we should do something to to make the code more easy to understand and to maintain, but at least we have currently no race condition with TrackDAO:
Mixxx has two instances of TrackDAO one in LibraryScanner and one in TrackCollection.
The LibrarySanner does not use the Track cache, because it creates a private TrackPointer without the TrackDAO.
see TrackCollection::importDirectory.
The events of TrackDAO are all handled in the "Main" Thread.

[RJ Skerry-Ryan (rryan)]

Oops sorry! You're right I mixed up m_sTracks and m_trackCache. I agree m_sTracks usage is all safe throughout TrackDAO since it's just a QHash.

If the library scanner does not use m_sTracks then does it still need to be static?

[Daniel Schürmann (daschuer)]

It is not required to be be static. I think that was a result from my attempts to grantee each track exists once.
Two instances of TrackDAO are caring two separate caches, even though one of it is always empty.
I think we should consider to re-sort and split TrackDAO in a way that its more easy to maintain.
Unfortunately its a lot of work with no user improvements.

[Swiftb0y (swiftb0y)]

Mixxx now uses GitHub for bug tracking. This bug has been migrated to:
https://github.com/mixxxdj/mixxx/issues/6566