[Bug] Database Access in ClientManager leads to DeadLock (at least with) SQLiteDatabase (1 Viewer)

[MJGraf]

While I was hunting down a bug in the UPnP system I found another bug, which looked like it was a bug in SQLiteDatabase, but to me it seams that the code in ClientManager is at least "not ideal" if not wrong.

Steps to reproduce:
1. Start MP2 Server (with SQLiteDatabase)
2. Start MP2 Client
3. Attach the MP2 Client to the MP2 Server
4. Detach the MP2 Client from the MP2 Server
5. press ESC.
Now the client hangs and in the server log you see that SQLiteDatabase uses three more connections and nothing else happens.

The reason can be found here: https://github.com/MediaPortal/Medi...ces/ClientCommunication/ClientManager.cs#L242
We first start one transaction and execute a delete command.
But before we commit this transaction we call

mediaLibrary.DeleteMediaItemOrPath(clientSystemId, null, true);
mediaLibrary.RemoveSharesOfSystem(clientSystemId);

Each of these calls starts another transaction with each another connection to the database in the same thread. Both transactions are "write" transactions because they try to delete. And in the respective delete statements they access the same table as our very first transaction.
This does not work, because the very first transaction holds a write lock to the respective table. This lock is held until a commit is issued to the first transaction. The two calls to media library also try to aquire a write lock on the same table, but they don't get it because we have a dead lock with the first transaction.

In gerenal, this code in ClientManager is IMHO bad because

• nested write transaction in the same thread are evil in general (AFAIK we avoid them anywhere else in our code); and
• to my understanding all the database changes which occurr in ClientManager.DetachClientAndRemoveShares should happen in one single transaction - not in three transactions as it is now, because either we remove all the traces of that client from the database or we rollback completely.

While the latter point would be ideal to achieve, this would require a bigger change to the MediaLibrary (just one method in MediaLibrary that does all the three jobs and uses a single transaction for it). Although I consider this change necessary sooner or later, I would propose a simple solutioin for now: Move transaction.commit() right below command.ExecuteNonQuery(). That should solve the actual bug although it does not (yet) solve the problem that we should have only one transaction.

Any opinions on this? My proposal is easy so I'm happy to implement a patch if there is no objection...

Michael

PS: I also found the UPnP bug by the way. But I couldn't reproduce it due to this bug - so one thing after the other

 

[breese]

Would that also apply if there were 2 clients and the Client Only had an issue?
1-Server/Client (Bugsbunny)
1-Client (Sylvester)

I ask because there was a disconnect of my Sylvester Client when doing my last MP2 Import Scan on Sunday.
[2013-11-16 18:30:54,673] [10702382] [56 ] [DEBUG] - UPnPServerControlPoint: MP2 client '
' (system ID '0fe91daa-3467-44d1-bbf2-dce706f747c3') at host 'sylvester.acme.local' was removed from the network

 

[MJGraf]

yep, same thing. But I'm not talking about connects and disconnects (i.e. when you just quit the MP2 client application or shutdown the client). These are handled correctly.
The problem occurs on a client detach. I.e. you go to settings, general , server... detach from home-server to permanently detach a MP2 client from the MP2 server (i.e. the client is not treated as part of the MP2 system anymore and you don't see Audio, Movie, Series in the client anymore).

 

[MJGraf]

Just added a Jira issue for this (MP2-405) and made a patch (source will be pushed to a new BUG-branch in a few seconds).
Could someone please try whether
(a) he can reproduce the bug according to the steps to reproduce above with the latest snapshot and
(b) whether the attached dll solves the bug?
Thanks,
Michael

 

[breese]

Grabbing the 11/17 version and will install....
back in ohhh 30 minutes

 

[breese]

All done and tested.
The fix did the trick BUT...
Are you aware that a ? comes up in the Client when you detach?
Sometimes it takes a while for the Client to detach but when it does you can click the ? and then a popup asks you if you want to Handle the problem



anyways, after the 2nd detach and ESC, I verified the server log for the SQL multi connections.
I then used the Task Manager to kill off the Client.
then I stopped the server, installed the patch, restarted the server and went back into MP2 Client.
When I went back into the Client, and went to reattach, I again was presented the Handle Popup.
I told it to Handle it and it connected.
I then disconnected, pressed ESC and checked the server log
All good....

Also, my client.1.log has 3 exceptions you might want to see....

Bob

 

[MJGraf]

Thanks bob, much appreciated as usual
Will have a look at your exceptions tomorrow.
Michael

 

[MJGraf]

Oh, and the ? Is expected. You can switch it off in the settings (think its called networktracker) but I wouldn't know why you want to switch it off. It's not a bug it's a feature

 

[MJGraf]

Bob can you still remember what you did when client.1.log was generated? The time in it does not fit the time in the server.log and the SQLHang.bug.
Was that exceptions thrown while you were testing with or without this bugfix-dll? Thanks!

 

[breese]

Right after I built the machine I went into the Client and expected to connect to the server but it was already connected.
I did a disconnect and it took a very long time, when I got the disconnect I realized there was a ? on the page and I clicked it.
Getting the Handle or Discard popup I think I told it to Discard and then did the ESC.

Once I verified the Server log showed the multiple SQL connections I used the Task Manager to kill off the Client. It was the restarting of the Client (after applying the patch) that created the new log file.