#84 Fix xsel deadlock when using a single threaded window manager on Mono

KeePass_2.x
closed
nobody
None
5
2013-10-26
2013-10-18
Will R
No

When running under Mono using a single-threaded window manager like Cinnamon it's frequently possible to dead-lock the entire window manager when KeePass launches xsel. The symptom is that xsel fails to daemonize itself to the background after its invoked.

This appears to be the result of a race-condition between xsel, the window manager's clipboard and Mono/Winforms redraw, wherein while Mono is waiting for xsel, it will not allow the window manager to process clipboard events, which in turn blocks xsel from daemonizing and thus deadlocks the system.

While this should probably be fixed on the window manager end, I have found that it seems it can be solved in a practical sense in KeePass by waiting for xsel to terminate in a separate thread, which allows the main KeePass thread to respond to the window manager and avoid the deadlock condition.

Patch attached.

1 Attachments

Discussion

  • Dominik Reichl

    Dominik Reichl - 2013-10-18
    • status: open --> closed
     
  • Dominik Reichl

    Dominik Reichl - 2013-10-18

    In your patch, WaitForExit is called in a new thread and the main thread continues to run. This completely defeats the purpose of the WaitForExit call; its purpose is to let the main thread wait until the started application has terminated.

    Theoretically the main thread could wait for the new thread being terminated, but then the whole threading approach would be useless (simply calling WaitForExit on the main thread has the same effect).

    Best regards,
    Dominik

     
    • Will R

      Will R - 2013-10-19

      In a practical sense it's not a problem with xsel and other clipboard application calls (which seems to be the only thing the function is used for at the moment). All the processing is finished by the time WaitForExit() is called since stdin and stdout have been ReadToEnd() so the applications results have already been obtained.

      RunConsoleApp doesn't collect the exit code of the terminating app in the first place, so it's not like it can rigorously depend on the application succeeding - but the problem at the moment is if the external app hangs for any reason, KeePass at minimum freezes (and thanks to Mono, on a single-threaded WM that freezes the whole desktop it seems).

       
  • Will R

    Will R - 2013-10-19

    I've attached a new version of the patch which addresses the over-generality. This version targets the specific xsel invocation, and nothing else - this should be generally applicable to the situations under which xsel is used with Mono systems, which I can confirm has no negative effects on KeePass.

     
  • Dominik Reichl

    Dominik Reichl - 2013-10-19

    Looks better, thanks. Do we really need the WaitForExit call in the new thread? Would there occur any problems with xsel if I'd remove this (i.e. call WaitForExit only if bWaitForExit is true)?

    Best regards,
    Dominik

     
    • Will R

      Will R - 2013-10-19

      Yes - when I was testing while debugging this problem originally I tried just dropping the WaitForExit call, but the result was that I got a lot of <defunct> xsel processes hanging about (but no hard lockups).

      If WaitForExit() isn't there, then when the function exits, and the garbage collector reaps all the resources including the pipes - which seems to cause xsel to again start blocking waiting for read/write.

       
  • Dominik Reichl

    Dominik Reichl - 2013-10-19

    Ok, thanks! I've implemented this now; basically your code, just a bit generalized with a flags enumeration and the WaitForExit in the new thread wrapped in a try-catch.

    The latest development snapshot (including the 3 modified source code files) is here:
    http://keepass.info/filepool/KeePass_131019.zip

    It would be great if you could confirm that it works on your machine now, too. Do we need this approach also for other xsel invokations (for reading and clearing the clipboard)?

    Best regards,
    Dominik

     
    • Will R

      Will R - 2013-10-19

      My attached patch included the bWaitForExit = false change for the read and clear modes as well. I've not much insight on how this bug manifests, but if it's a race-condition with the window manager then I suppose it could happen anytime xsel needs to manipulate the clipboard - I probably only noticed because copying entries to the clipboard is my most common action.

      It should be completely safe for read and clear modes of operation since the stdout stream is always fully read before the function returns.

       
  • Dominik Reichl

    Dominik Reichl - 2013-10-20

    I've now implemented this for the other xsel invocations, too.

    Here's the latest development snapshot (including the modified source code file) for testing:
    http://keepass.info/filepool/KeePass_131020.zip

    Thanks again for your help!

    Best regards,
    Dominik

     
  • Will R

    Will R - 2013-10-24

    Well, it looks like I was premature with this patch - at best it seems to improve the situation - deadlocks are not so frequent, but the underlying issue seems to be bound up in something else in gnome3 derived window managers.

     
  • Will R

    Will R - 2013-10-26

    I've just added a ticket for the proper fix for this problem. I can confirm that it resolves the reproducible forms of this issue completely and builds upon this patch to do it.

    https://sourceforge.net/p/keepass/patches/85/

    However I've accidentally posted it under the KeePass 1.x milestones, so it could be moved to 2.x that would be great (I don't seem to have permissions to edit my own patch tickets).

     

Log in to post a comment.