skindesigner - bad locking order for vdr-2.3.x

    Hi. I'm not sure if this has been reported yet but It seems that skindesigner (1.2.3-git+c8d54db) has a bad locking sequence. Apparently it's locking Channels and then Timers. Does anyone happen to have a patch to fix this?


    Thanks!


    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

    HowTo: APT pinning

    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.

    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 ... :rolleyes:


    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

    HowTo: APT pinning

    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


    Thanks for your reply, I must have missed your post somehow. Skindesigner is a great plugin, very popular, hopefully fixing the problem is fairly simple and he'll give it some tlc. I share your fear though since no development has occured in quite some time now. :(

    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:

    I only get this with my clients.

    This is fixing the problem for me:

    Files

    skindesigner_invalid-lock-fix.patch 860 Byte – 398 Downloads

    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.

    Code
    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. :thumbup:


    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!

    Like 2

    @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...

Participate now!

Don’t have an account yet? Register yourself now and be a part of our community!

Register Yourself Login