[pvr] workaround a deadlock if no pvr addons exist and you activate the pvr manager #5004

Merged
merged 1 commit into from Jul 13, 2014

Projects

None yet

7 participants

@xhaggi
Member
xhaggi commented Jul 11, 2014

Found by @mkortstiege, thanks ;) The modal dialog introduces a deadlock if called after the settings change. We don't know the root cause of this deadlock, but this shift workaround the issue.

Remove all PVR addons and disable/enable the PVR manager and you get the freeze.

@xhaggi xhaggi added Fix PVR labels Jul 11, 2014
Member
xhaggi commented Jul 11, 2014

@opdenkamp ping

@xhaggi xhaggi changed the title from [pvr] workaround a deadlock if no pvr addons exist or be enabled and you activate the pvr manager to [pvr] workaround a deadlock if no pvr addons exist and you activate the pvr manager Jul 11, 2014
Member

It would be most useful to know the cause - let's not bandaid these things.

We need backtraces, plus clear reasons for why it happens (i.e. what callbacks does the setting of that bool fire, and what do they fire etc?)

Member
xhaggi commented Jul 11, 2014

sure, backtrace: http://xbmclogs.com/show.php?id=244792 (thread 1 and 19)

Member
xhaggi commented Jul 11, 2014

the callback which is called after setting change https://github.com/xbmc/xbmc/blob/master/xbmc/pvr/PVRManager.cpp#L110

Member

Right, makes sense. CPVRClients sets the setting which fires a call to XBMC.StopPVRManager via app-messenger (adds a threadmessage) and then goes on to call into ApplicationMessenger (another threadmessage is added) via the DoModal and sits waiting on the threadmessage's event.

Meanwhile, app thread runs the first threadmessage which calls CPVRClients::Stop() but ofcourse it can't stop as it's waiting on the event of the next threadmessage in the queue.

The fix solves this by reordering those threadmessages (second isn't done until DoModal() returns).

@Montellese, @opdenkamp comments/suggestions on alternates?

Seems to me we don't need to be waiting at all on this dialog - it's basically a "you did something wrong" to the user, but setting the bool setting like this is kinda asking for trouble?

Member

@jmarshallnz i was wondering if this kind of deadlock could happen in other code parts as well. Always thought the root cause is outside of PVRClients/Manager.

Contributor

Patterns used in PVR heavily couples GUI and core. Coarse grained locking
combined with raising dialogs all over the show is a recipe for disaster.
The window rewrite should alleviate the major culprit of the observers
raising dialogs off app thread. I'm sure there are plenty of other places,
but the window rewrite PR was too much of a pig to justify digging deeper
until it was in.

Member
xhaggi commented Jul 12, 2014

@jmarshallnz good to hear, now we are a little bit more familiar how the thread messages stuff works ;)

Member

So this one is good to go?

Member

I'd like to hear from @Montellese first.

Member

this will do the trick

@t-nelson with the original code, observers never held locks and notifications were fired async, as I've explained to you on IRC already ;-) raising dialogs was fine in that situation.

Owner

I don't see a cleaner/easier solution around this. In this case it probably doesn't make much sense to stop the PVR manager through an app thread message (we're blocking anyway with the dialog) but that's probably not that easy to change.

Member
xhaggi commented Jul 13, 2014

jenkins build this please

@xhaggi xhaggi merged commit b0a9373 into xbmc:master Jul 13, 2014

1 check passed

default Merged build #1019 succeeded in 1 hr 3 min
Details
@MartijnKaijser MartijnKaijser added this to the Helix 14.0-alpha1 milestone Jul 16, 2014
@xhaggi xhaggi deleted the xhaggi:pvr-fix-deadlock-on-no-active-addons branch Jul 17, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment