skindesigner - bad locking order for vdr-2.3.x

jinx

Yepp, I postet/reported it from: [ANNOUNCE] VDR developer version 2.3.6

Generally I do see that report only on VDR startup, where skindesigner loads everything, so not too bad from my point of view.

I guess "louis" is able to fix it, but I fear his coding motivation may be low actually ...

Regards
fnu

Quote from fnu

Generally I do see that report only on VDR startup, where skindesigner loads everything, so not too bad from my point of view.

Every wrong lock sequence is bad, because it can deadlock. There don't exist different levels of "bad" with a deadlock...

Lars.

Also note that VDR only reports the very first invalid locking order. That's because if there were lots of them in normal operation, the system's performance might be compromised.

Klaus

If it is only reportet once, ther should be a small notice ablot that fact. For example: "Note: There is no more logging for thist plugin locking issues until restart of vdr"

Quote from mini73

Every wrong lock sequence is bad

Didn't see me saying it's good ...

It seems to be reported on VDR start time, have only seen one while runtime, compared to uncounted VDR starts the last days.

If I don't see it at runtime, how big is the chance for a deadlock ... ? Albeit it's most likely possible ...

Regards
fnu

Just for clarity: VDR reports only the very first invalid locking sequence, no matter if it's from the VDR core or a plugin. So if you see this report at startup and then no more during runtime, this doesn't necessarily mean that there are no more invalid lock sequences at runtime. The idea behind this was that too many reports might slow down the system, and it's best practice to fix the first error, which may implicitly fix others, too.

If you want every invalid lock sequence logged, you can remove the line

dumped = true;

from cStateLockLog::Check().

Klaus

Hi,

i have looked into this problem. As Jinx already described, the locking sequence in this case is first channels and then timers. However, it is not the skindesigner plugin that locks the channels, but the vdr core.
The channels are locked in cDisplayChannel::cDisplayChannel(int, bool) which afterwards calls cDisplayChannel::DisplayChannel(void) which in turn calls the SetChannel(cChannel, int) method of the current skin.
Within that call, the skindesigner plugin then locks the timers.

I don`t see how that could be fixed in the plugin. If the vdr core requests a lock and does a virtual call into a plugin in the scope of that lock, the plugin is only allowed to request locks further down in the sequence.
My suggestion would be to ensure that in situations like this, the core not only requests a specific lock but all previous locks in the sequence as well (e.g. when the core wants to lock the channels, it should lock the timers as well, if there is a virtual function call in the current scope).

For this particular sequence violation, one might also argue that the lock on the channels is not correct in the first place, since the lifetime of the cChannel* it should guard is longer than the lifetime of the lock (e.g. the lock is in the scope of the constructor while the pointer is a class member). So currently there is actually no guarantee that the channel pointer remains valid with or without the lock. Or did I get something wrong here?

Regards,
Stefan

Quote from stefan.r

I don`t see how that could be fixed in the plugin. If the vdr core requests a lock and does a virtual call into a plugin in the scope of that lock, the plugin is only allowed to request locks further down in the sequence.
My suggestion would be to ensure that in situations like this, the core not only requests a specific lock but all previous locks in the sequence as well (e.g. when the core wants to lock the channels, it should lock the timers as well, if there is a virtual function call in the current scope).

SetChannel() is given a cChannel and is supposed to set that channel. It has no business handling timers!
If a plugin wants to do something with timers as a reaction to SetChannel, it shall do so in a separate thread.

Quote

For this particular sequence violation, one might also argue that the lock on the channels is not correct in the first place, since the lifetime of the cChannel* it should guard is longer than the lifetime of the lock (e.g. the lock is in the scope of the constructor while the pointer is a class member). So currently there is actually no guarantee that the channel pointer remains valid with or without the lock. Or did I get something wrong here?

Should the member cChannel ever be deleted while the cDisplayChannel still lives, it will remain valid in the list's garbage collector, until the cDisplayChannel is closed. So calling SetChannel with that member is OK.

Klaus

Quote

SetChannel() is given a cChannel and is supposed to set that channel. It has no business handling timers!
If a plugin wants to do something with timers as a reaction to SetChannel, it shall do so in a separate thread.

Hm, ok, you definitely got a point there ;-). Seems like the plugin authors intention behind this is similar to the case described here. If I can find the time, I will have a lock at the solution you described there.

Quote

Should the member cChannel ever be deleted while the cDisplayChannel still lives, it will remain valid in the list's garbage collector, until the cDisplayChannel is closed. So calling SetChannel with that member is OK.

Understood. Thanks for the clarification.

Ten month later but the problem is still present:

Apr  9 18:35:17 raspberrypi vdr: [21556] SVDRP raspberrypi > 192.168.178.101:6419 client created for 'vdr'
Apr  9 18:35:17 raspberrypi vdr: [21384] OSD size changed to 1920x1080 @ 1
Apr  9 18:35:17 raspberrypi vdr: [21384] timer 0 (28 2013-2240 'Serien~Germany's next Topmodel - by Heidi Klum') set to event Do. 12.04.2018 20:15-22:30 'Germany's next Topmodel - by Heidi Klum'
Apr  9 18:35:17 raspberrypi vdr: [21384] timer 0 (28 2013-2240 'Serien~Germany's next Topmodel - by Heidi Klum') set to event Do. 19.04.2018 20:15-22:30 'Germany's next Topmodel - by Heidi Klum'
Apr  9 18:35:17 raspberrypi vdr: [21384] --- begin invalid lock sequence report
Apr  9 18:35:17 raspberrypi vdr: [21384] 21384  -  R  -  -  -  -  -  -  -  -  L
Apr  9 18:35:17 raspberrypi vdr: [21384] 21384  -  U  -  -  -  -  -  -  -  -  U
Apr  9 18:35:17 raspberrypi vdr: [21384] 21384  -  R  -  -  -  -  -  -  -  -  L
Apr  9 18:35:17 raspberrypi vdr: [21384] 21384  -  *  -  -  -  -  -  -  -  -  L
Apr  9 18:35:17 raspberrypi vdr: [21384] 21384  -  *  -  -  -  -  -  -  -  -  L
Apr  9 18:35:17 raspberrypi vdr: [21384] 21384  -  *  -  -  -  -  -  -  -  -  U
Apr  9 18:35:17 raspberrypi vdr: [21384] 21384  -  *  -  -  -  -  -  -  -  -  L
Apr  9 18:35:17 raspberrypi vdr: [21384] 21384  -  *  -  -  -  -  -  -  -  -  U
Apr  9 18:35:17 raspberrypi vdr: [21384] 21384  -  *  -  -  R  -  -  -  -  -  L
Apr  9 18:35:17 raspberrypi vdr: [21384] 21384  -  *  -  -  U  -  -  -  -  -  U
Apr  9 18:35:17 raspberrypi vdr: [21384] 21384  -  *  -  -  -  -  -  -  -  -  U
Apr  9 18:35:17 raspberrypi vdr: [21384] 21384  -  *  -  -  -  -  -  -  -  -  L
Apr  9 18:35:17 raspberrypi vdr: [21384] 21384  -  *  -  -  -  -  -  -  -  -  L
Apr  9 18:35:17 raspberrypi vdr: [21384] 21384  -  *  -  -  -  -  -  -  -  -  U
Apr  9 18:35:17 raspberrypi vdr: [21384] 21384  -  *  -  -  -  -  -  -  -  -  L
Apr  9 18:35:17 raspberrypi vdr: [21384] 21384  -  *  -  -  -  -  -  -  -  -  U
Apr  9 18:35:17 raspberrypi vdr: [21384] 21384  -  *  -  -  R  -  -  -  -  -  L
Apr  9 18:35:17 raspberrypi vdr: [21384] 21384  -  *  -  -  U  -  -  -  -  -  U
Apr  9 18:35:17 raspberrypi vdr: [21384] 21384  -  *  -  -  -  -  -  -  -  -  U
Apr  9 18:35:17 raspberrypi vdr: [21384] 21384  R  *  -  -  -  -  -  -  -  -  L
Apr  9 18:35:17 raspberrypi vdr: [21384] 21384 invalid lock sequence: 1 Timers
Apr  9 18:35:17 raspberrypi vdr: [21384] full backtrace:
Apr  9 18:35:17 raspberrypi vdr: [21384] /usr/bin/vdr cStateLock::Lock(cStateKey&, bool, int) at ??:?
Apr  9 18:35:17 raspberrypi vdr: [21384] /usr/bin/vdr cTimers::GetTimersRead(cStateKey&, int) at ??:?
Apr  9 18:35:17 raspberrypi vdr[21384]: invalid lock sequence at Mo. 09.04. 18:35
Apr  9 18:35:17 raspberrypi vdr: [21384] /usr/lib/vdr/plugins/libvdr-skindesigner.so.2.3.9 cGlobalTimers::SetLocalTimers() at ??:?
Apr  9 18:35:17 raspberrypi vdr: [21384] /usr/lib/vdr/plugins/libvdr-skindesigner.so.2.3.9 cGlobalTimers::LoadTimers() at ??:?
Apr  9 18:35:17 raspberrypi vdr: [21384] /usr/lib/vdr/plugins/libvdr-skindesigner.so.2.3.9 cViewChannel::SetChannel(cChannel const*, int) at ??:?
Apr  9 18:35:17 raspberrypi vdr: [21384] /usr/bin/vdr cDisplayChannel::DisplayChannel() at ??:?
Apr  9 18:35:17 raspberrypi vdr: [21384] /usr/bin/vdr cDisplayChannel::cDisplayChannel(int, bool) at ??:?
Apr  9 18:35:17 raspberrypi vdr: [21384] /usr/bin/vdr main at ??:?
Apr  9 18:35:17 raspberrypi vdr: [21384] /lib/arm-linux-gnueabihf/libc.so.6 __libc_start_main at libc-start.c:325
Apr  9 18:35:17 raspberrypi vdr: [21384] --- end invalid lock sequence report
Apr  9 18:35:17 raspberrypi vdr: [21384] --- THERE WILL BE NO FURTHER REPORTS UNTIL VDR IS RESTARTED!

I only get this with my clients.

This is skindesigner's fault. The channel display has no business setting a lock on the Timers - at least not in the SetChannel() function.

Klaus

This is fixing the problem for me:

--- vdr-plugin-skindesigner-1.2.7.orig/coreengine/viewdisplaychannel.c
+++ vdr-plugin-skindesigner-1.2.7/coreengine/viewdisplaychannel.c
@@ -206,10 +206,12 @@ void cViewChannel::ClearVariables(void)
 }
 
 void cViewChannel::SetChannel(const cChannel *channel, int number) {
+/*
     if (!timersLoaded) {
         timersLoaded = true;
         globalTimers.LoadTimers();
     }
+*/
     channelChange = true;
 
     //check if channelgroups have to be cleared
@@ -252,10 +254,12 @@ void cViewChannel::SetChannel(const cCha
 }
 
 void cViewChannel::SetEvents(const cEvent *present, const cEvent *following) {
+/*
     if (!timersLoaded) {
         timersLoaded = true;
         globalTimers.LoadTimers();
     }
+*/
     Clear((int)eVeDisplayChannel::epginfo);
     Clear((int)eVeDisplayChannel::progressbar);
     Clear((int)eVeDisplayChannel::scrapercontent);

This "fix" breaks functionality

I didn't miss any functionality with this fix, so I don't care about it.

But anybody is welcome to do it all right.

Quote from Saman

I didn't miss any functionality with this fix, so I don't care about it.

Tokens {currentrecording}, {nextrecording}, {isRecording} and {devices[recording]} in displaychannel will not work anymore.

class cSkinDisplayChannel : public cSkinDisplay {
...
public:
  ...
  virtual void SetChannel(const cChannel *Channel, int Number) = 0;
       ///< Sets the current channel to Channel. If Number is not 0, the
       ///< user is in the process of entering a channel number, which must
       ///< be displayed accordingly.

This function is called to set the current channel. Of course the caller has a lock on the Channels list, because it hands over a pointer to a cChannel. Since at this time the Channels are locked, this function can't lock the Timers, because those would have to be locked before the channels in order to avoid a possible deadlock.

If a skin wants to display timer related things in its channel display, it can do so in its Flush() function, which is called without holding any locks on global lists.

Klaus

louis,

Du, als Entwickler vom skindesigner-Plugin, hast ja leider zum Bedauern vieler VDR-User, aus sicherlich wichtigen privaten Gründen Dich entschlossen, die Weiterentwicklung bzw. Fehlerbehebung des Plugins (wie z.B. die bekannten Bug-Tickets) nicht weiter zu verfolgen.

Das ist wirklich Schade, denn Dein skindesigner-Plugin ist wirklich ein Super-Plugin, was praktisch fast jeder VDRler nutzt und nicht mehr missen möchte.

Da ja KLS in den nächsten Tagen einen neuen Stable-VDR, nämlich den vdr-2.4 veröffentlichen will, sind die Anpassungen schon für viele User wichtig und sicherlich nicht nur ich persönlich würden es sehr begrüßen, wenn Du Dich doch nochmals entschließen könntest, etwas an der Anpassung und Fehlerbehebung des Plugins weiterzumachen.

Paul

PS: Aber wie schon gesagt, ich respektiere auch Deinen Entschluss, nicht mehr weiter aktiv hier mitarbeiten zu wollen.

Der VDR ist ja immer noch ein Hobby- und Freizeit-Projekt, an dem man mit Spaß und Freude arbeiten soll und nicht unter irgendwelchen Zwängen!

@Klaus: ich habe es schon verstanden, wie es korrekt funktionieren würde. Ich wollte nur ausdrücken, dass der "Holzhammer Patch" von Saman Mist ist

Paulaner: naja, private Gründe waren es eher nicht, vielmehr hat es mich genervt, wie die Entwicklung im VDR Umfeld abläuft und wie die "VDR Community" so tickt. Deshalb habe ich mich dazu entschlossen, meine Zeit anderweilig für mich sinnvoller zu investieren...