Library concurency refactoring

[MK-42]

#73

Trojans GSOC project (Async DB access) reworked to split TrackCollection to public and private part.

[MK-42]

This won't compile because of the Library-Scanner. It is not reworked now, as I don't understand It yet.

It uses its own DAOs - why? And sometimes it uses those from TrackCollection - Why?

[MK-42]

@troyane would you mind to look at my questions about your LibraryScanner changes? Some are not clear to me.

[daschuer]

It uses its own DAOs - why?

Because it uses currently a different thread. This way sq-light allows concurrent access.
But it is locking, which has introduced some deadlocks in the past, hopfully solved in master.

[daschuer]

My be it is a good idea to remove the extra instance for the library scanner and use always the lambda.
This will make the code clearer and should not suffer performance issues since the database access is locking anyway.
We need to verify this though.

[MK-42]

I'm currently facing some nested callSync calls which cause a deadlock. Need to investigate this to get it running again

[daschuer]

This seams to be also a common issue that may happens later as well.
Can we place flag and an assertion that guards against this condition?

[MK-42]

Can we place flag and an assertion that guards against this condition?

There is one.
TrackCollection::callSync(func lambda, QString where)

[MK-42]

well those nested calls get tricky... that will take a while

[daschuer]

Can you give an example?

[MK-42]

Maybe look at 3add7af there is one of them fixed
I'm also facing some problems with slots containing a lambda and beeing called from one. I thought Qt::QueuedConnection would help there but it doesn't

[MK-42]

Not its segfaulting. What a crappy little thing

[MK-42]

Well in fact it is a infinite loop in lookupCachedTrack, happening as soon as a track is in the library. screw...

[daschuer]

Do you have the deadlocks in the original files as well?
I have tried to track the issue from 3add7af, but I failed.
Qt should choose queued connections when it jumps across thread borders.
... Or does the connect call inside a Lambda confuse Qt?

Are you able to single step into the Qt code? If not it might be worth to set it up.

An other thought:
Be aware that the slots from the DAOs are also called directly from the GUI thread. Since they inherit the signal message queue from the parent Object.
If we need slots that are executed in the Track Collection thread, we may execute them from an new
signal message queue.
See: moveToThread() and QCoreApplication::processEvents()

[daschuer]

Does any of your commit build? I would like to test.
What is the receipt to get the deadlock?

[MK-42]

It builds. Just Start it. You will geht some Kind Of race condition to a segfault at the Moment i think if im not wrong. Cant look into it right now

Am 17. Juni 2015 22:47:21 MESZ, schrieb "Daniel Schürmann" [email protected]:

Does any of your commit build? I would like to test.
What is the receipt to get the deadlock?

---

Reply to this email directly or view it on GitHub:
#629 (comment)

Diese Nachricht wurde von meinem Android-Mobiltelefon mit K-9 Mail gesendet.

[daschuer]

Building fails on my site:

src/test/searchqueryparsertest.cpp: In constructor ‘SearchQueryParserTest::SearchQueryParserTest()’:
src/test/searchqueryparsertest.cpp:12:34: error: no matching function for call to ‘SearchQueryParser::SearchQueryParser(QSqlDatabase&)’
               m_parser(m_database) {
                                  ^
src/test/searchqueryparsertest.cpp:12:34: note: candidates are:
In file included from src/library/searchqueryparser.h:8:0,
                 from src/test/searchqueryparsertest.cpp:6:
src/library/searchqueryparser.h:54:30: note: SearchQueryParser::SearchQueryParser(const SearchQueryParser&)
     DISALLOW_COPY_AND_ASSIGN(SearchQueryParser);
                              ^
src/util.h:5:3: note: in definition of macro ‘DISALLOW_COPY_AND_ASSIGN’
   TypeName(const TypeName&);               \
   ^
src/library/searchqueryparser.h:54:30: note:   no known conversion for argument 1 from ‘QSqlDatabase’ to ‘const SearchQueryParser&’
     DISALLOW_COPY_AND_ASSIGN(SearchQueryParser);
                              ^
src/util.h:5:3: note: in definition of macro ‘DISALLOW_COPY_AND_ASSIGN’
   TypeName(const TypeName&);               \
   ^
In file included from src/test/searchqueryparsertest.cpp:6:0:
src/library/searchqueryparser.h:13:5: note: SearchQueryParser::SearchQueryParser(TrackCollection*)
     SearchQueryParser(TrackCollection* pTrackCollection);
     ^
src/library/searchqueryparser.h:13:5: note:   no known conversion for argument 1 from ‘QSqlDatabase’ to ‘TrackCollection*’
scons: *** [lin64_build/test/searchqueryparsertest.o] Error 1
scons: building terminated because of errors.

[daschuer]

The information in between is not really worth the mess.

We will see, how hard it is to fix all bugs. Have this branch in place can't hurt.
Probably we are able to find a solution for a hard problem using it.

Squashing all merge commits to one, in a second branch is a good idea. Finally no one needs these hug number of merge commits in the master branch.

[MK-42]

Looking back at this.. wow. From my current perspective, I think its not feasible to go that way with merging further without loosing the head in there. IIRC, the library scanner and other stuff is already heavily broken by that merging here, as one is not able to test that stuff after each merge.
Thinking about how that situation here could be resolved I will collect my thoughts, looking for feedback:

IIRC we have to make sure that only a single thread accesses QSql related stuff, and allow to put some more work in asyncronous ways out of the GUI-thread.

Maybe we should think about a more step-by-step approach. This PR is aiming to push the db-access to a single thread and also introducing async db access. Maybe we should do it step by step, first consolidate the db-access in a single entry point somewhere in the code, and after that is proven to work implement some asynchronous access methods where needed. I think that will also make the diff smaller and easier to review.

Looking at @uklotzde's work on the crate database access in #1099, it could also be a possible solution, to handle the threading problems inside of the FwdSqlQuery or subclasses of it. We could move the execution of that query into a dedicated Database-thread, introduce proper locking there, and get the rest of the code use such derived QSqlQuery classes. Probably not that much has to change, as we can keep the api stable for the query (as long as we stay with syncronus access). We only need to make sure that the code doesn't use plain QSqlQueries from that point on. That point could be merged as is, making smaller steps into master, keeping the diff small.

After that is done, we could introduce a Async(Fwd)SqlQuery, that tells it's caller when it is done (probably via callback), and allow him to fetch the data then. I don't know if the real bottleneck for the current responsibility issues with mass-actions is the sql-query or the postprocessing of it. If its postprocessing, we could also allow other code than sql-queries (lambdas?) to be queued into the library-thread, to allow sorting, building tables and stuff from non-gui-thread. For that, trojans queuing and locking code could be reused.

Btw.: as 2.1 is on i'ts way I don't want to get anything in there, and don't want to bind dev-ressources anywhere non-necessary for 2.1. But starting to think on that and get to a conclusion how to do it in small, master-mergeble steps would be good I think.

[uklotzde]

I don't think that there is a chance to merge this PR. And I'm not convinced that the approach will work as expected.

[daschuer]

Yes, this one was rotting for too long, I am really sorry.
Do you mean that the lambda approach will not work? We had good results during the GSoC project.
And on top of the latest refactorings, it should work even better.
What could be an alternative solution?

[uklotzde]

Even with lambdas you still need to care about thread-safety and concurrency issues. Wrapping arbitrary code with references to GUI classes into lambdas and executing them in another thread's context sometimes later does not sound like a good idea to me. It may be fast, but is is correct and safe?

Qt's queued signal/slot connections might be useful here. Passing immutable data from one thread is at least safe, albeit with some overhead. I already started to separate the main track select query with such an approach, but didn't have time to complete it, yet. Stay tuned ;)

[Be-ing]

This pull request has not been updated in years, has accumulated tons of merge conflicts, and there seem to be concerns about its fundamental approach so I am closing it to reduce the clutter of open PRs.