Library refactoring #55
Conversation
NOTE: DB is still locked while tranaction is open but not commited.
Added pause feature (according to Steven Boswell patch)
Added dao.cpp file. This example is working in restricted conditions: - when you do library scanning you can pause it (by clicking pause button); - when you cancel library scanning (by clicking pause button); - when you (ONLY!) add new playlist while library scanning in process (no need to click pause button).
|
Thank you for the pull request. Please merge from master and resolve the conflicts, so we will get a clean diff. |
| s_semaphoreTransaction.acquire("DAO::enterLockState"); | ||
| qDebug() << "\t\t entering LockState transaction time = " << QString::number( static_cast<double>(t.elapsed()) / 1e9, 'f', 4); | ||
| } | ||
| ++s_semaphoresLockedCount; |
There was a problem hiding this comment.
In the current implementation you have a possible race condition, if a thread switch occurs between checking the s_semaphoresLockedCount and incrementing it. Semaphores are already counting, will it be possible to use them?
There was a problem hiding this comment.
What do you mean already counting?
There was a problem hiding this comment.
Oh sorry, I miss-read your code. Why it is required that only the first call is locking? This means if we have three concurenting DAOs only one of them is blocked.
There was a problem hiding this comment.
This method is for LibraryScanner thread, so it is required to acquire semaphores only once. It is not used for simultaneous calls from other threads. Maybe method name is not clear, mean it to lock semaphores in library-scan time.
|
I don't think this is the right way to go. I'd be more comfortable if we had just one thread for DB access that is decoupled from the GUI-thread. Their is still a good chance for short GUI freezes with your code. You are now adding code so that the trackdao will act differently according to what thread it is generated in. For good readability and maintenance a class should always behave the same way no matter what thread called it. Also I don't see (from the code) that it is now possible to edit playlists/crates while libraray scanner is running unless the user clicks pause. I envisioned a solution that the libraryscanner runs in the background and mixxx automatically takes care of pausing transactions if the user wants to edit other parts of the DB. Did you check how other programs handle the library access? |
| DAO::pauseSemaphore().acquire("TrackCollection"); | ||
| } | ||
|
|
||
| if (transactionFinished) { |
There was a problem hiding this comment.
I think you can get rid of "transactionFinished" if you put the following code in the block above.
There was a problem hiding this comment.
No, that behavior is different from what you recommend. We open (reopen) transaction when it was finished before. It means, that once we tried to acquire pause semaphore and it was locked (we can't acquire it), so we are in pause -- need to close transaction, release transaction semaphore, say that we finished transaction and wait here for "end" of pause. Then, if we finished transaction we must reopen it...
Otherwise, if we acquire pause semaphore in DAO::pauseSemaphore().tryAcquire(), we just move on and do not close transaction.
There was a problem hiding this comment.
I was wrong. Added new commit to apply Daniel's changes.
|
@daschuer I've done merge with trunk. |
|
@kain88-de Deriving To avoid any freezing GUI we do really need to rewrite all database access |
|
@kain88-de Here is blueprint on migration https://blueprints.launchpad.net/mixxx/+spec/qt5-c++11. Problem on using C++11 is also presented in Windows, because Qt4 uses MinGW 4.4. We must discuss migration issue (at list to use C++11, but not Qt5). By the way, that is interesting idea to use CLang instead of GCC. |
|
Yes, right. |
This is code for review.
Here you can find extended explanation of idea of usage 2 semaphores.