-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #387 - getting stuck on key usage confirmation #394
Conversation
@dlech, there was a problem with my account (they flagged my innocent account as spammy, ha-ha). |
I'm not sure this is a valid premise. For example, if there is a modal dialog open, the main window will be blocked indefinitely.
In the past we had issues with the IOProtocolExt extension blocking the main window waiting for an SSH agent which, if it is KeeAgent, would cause a deadlock. |
8fa4e22
to
a7af99c
Compare
@dlech , I introduced a separate UI thread and marshaled the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think adding a new UI thread is a great solution! (Can't believe that never occurred to me since I did something like that on Linux with a GTK UI thread in a winforms app!)
It looks like this will solve some other long standing issues too. Just a few comments inline.
Merged, thanks again! |
I was going through old issue related to these dialogs and found #198. So it would be a good idea to test what happens when there are multiple concurrent requests for confirmation dialog or key selection dialog. |
@dlech , LGTM. Basically, simultaneous confirmations chain one after another and there is no change in observable behavior.
I tested it on the clean 0.13.6 release and it behaves the same with an exception that sometimes it gets stuck on usage confirmation due to the bug we were trying to solve in this PR. |
Thanks for testing. I guess if there is a way to get stuck, someone will let us know. But this sounds like it is good enough for now. |
Abstract
PR is supposed to fix #387 - getting stuck on key usage confirmation.
The idea is to marshal theMessageBox.Show
call to theMainWindow
's thread.The idea is to marshal the
MessageBox.Show
andKeyPicker.ShowDialog
calls to a separate UI thread dedicated to the KeeAgent plugin.Problem research
My research didn't show any confirmation on the point that we should marshal
MessageBox
calls to the UI thread, nor that we shouldn't.A random comment on SO says that
MessageBox
will spin up its own message pump, but I haven't found a confirmation in the documentation.Impact analysis
Changes are contained in the KeeAgent library and don't leak to the SshAgentLib.
Possible issue: getting stuck in other situations due to the changes introduced.
I haven't found a situation where the issue will be plausible. Here is my reasoning:
The only change introduced is marshaling from thePageant
's worker thread to theMainWindow
's threadIf theMainWindow
's thread is busy, then it is either getting released any time soon or it is stuck alreadyTheMainWindow
's thread shouldn't be doing any calls to thePageant
's worker thread, so we are on the safe side of possible dead-locksThe only change introduced is marshaling from the
Pageant
's worker thread to the UI thread dedicated for the KeeAgent pluginIf the
KeeAgentUi
's thread is busy, then it is either getting released any time soon or it is stuck alreadyThe
KeeAgentUi
's thread shouldn't be doing any calls to thePageant
's worker thread, so we are on the safe side of possible dead-locks. The work done on theKeeAgentUi
's thread is the following:KeyPicker
dialog during ssh-agent list commandMessageBox
dialog during key usage confirmationReproducibility
I'm not sure this PR fixes the root issue instead of adding one more trick around UI-synchronization problems.
The main reason why I'm not sure is because I can't really reproduce the issue.
The most "valuable" thing I brought from those unsuccessful attempts to reproduce the issue is a screenshot of stuck callstacks. Here it goes:
It reproduces on my machine if I put the plugin in
C:\Program Files\KeePass Password Safe 2\Plugins
and launch KeePass from program files. Usual behavior: I have a confirmation window on the first SSH attempt, and on the second attempt, I get stuck.It doesn't reproduce if I launch KeePass from KeeAgent
bin\Debug
. I tried to bump KeePass's version to match the version in program files - but it still doesn't reproduce.I had some luck reproducing some delays of the MessageBox's appearance when launched from
bin\Debug
. I have two displays. I initiated a git push over ssh on one display, and there were no confirmation windows. When I clicked on the debugger window on the second display, the confirmation window popped up immediately. But that behavior didn't last long. I moved some windows and rearranged the setup to be able to pause the debugger without switching windows, and the behavior had gone.Prebuilt plugin to give it a shot
If anyone wants to give it a shot and doesn't have time to build the plugin from sources, here is a prebuilt version:
KeeAgent_v0.13.5-fix387.zip