Skip to content
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

Improve pageant process handling #211

Merged
merged 3 commits into from
Sep 8, 2015

Conversation

GrahamTheCoder
Copy link

Don't wait indefinitely for pageant (given that it's a service-like process that adds a tray icon and waits for input), and don't error when there are already 2 pageant processes.

Hanging bug
This is the old behaviour I observed (that I believe f28bb54 removes):
Start state:

  • I am on Windows 10 with pageant 0.63 running. GitUtils runs as part of my powershell profile.
  • I start powershell.exe and run $env:GIT_SSH = "plink"
  • I kill any pageant.exe that is running

Repro:

  • I run Start-Process powershell.exe which creates a new powershell window, starts pageant, then hangs forever (waiting for it to return).
  • I close the hanging powershell window, pageant stays running
  • I run Start-Process powershell.exe in my original powershell window which starts fine since pageant is already running

Multiple-pageant bug
When using GitExtensions on my machine I end up with two pageant processes (one is created by ShimGen), 99021d4 fixes the powershell error thrown in Start-SshAgent due to having an array returned. Stop-SshAgent also uses this, so should work with multiple pageant processes, but you need to call Stop-SshAgent once for each running SshAgent process which could be better, but is an improvement over doing nothing and erroring.

GrahamTheCoder added 2 commits August 26, 2015 12:11
It's supposed to be a background service style process, so this otherwise waits indefinitely.
Suspect this is true for other `$sshAgent`s too (see a few lines below) but have no test cases so haven't changed it.
@theaquamarine
Copy link
Collaborator

Do we actually end up waiting for pageant? It always returns immediately for me, even if it's asking for a key to be unlocked. Some quick testing timing the options for launching pageant looks like there's not much difference.

@@ -303,7 +303,7 @@ function Start-SshAgent([switch]$Quiet) {
$pageant = Get-Command pageant -TotalCount 1 -Erroraction SilentlyContinue
$pageant = if ($pageant) {$pageant} else {Guess-Pageant}
if (!$pageant) { Write-Warning "Could not find Pageant."; return }
& $pageant
Start-Process -NoNewWindow $pageant
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whitespace czar has come to visit - extra space here.

@dahlbyk
Copy link
Owner

dahlbyk commented Aug 27, 2015

Is the waiting indefinitely an artifact of the instance already running?

@theaquamarine
Copy link
Collaborator

I don't think makes a difference, since an existing process would have caused Start-SshAgent to exit by then. Attempting to launch pageant while it's already running manually (without specifying a key to add) causes it to immediately return as normal, but with a "Pageant is already running" alert.

@GrahamTheCoder
Copy link
Author

I've updated the description with an explanation of what I'm seeing.

@dahlbyk
Copy link
Owner

dahlbyk commented Aug 29, 2015

I'm not a Pageant user, so I'll defer to whatever you come up with. I don't see a down-side to switching from & to Start-Process.

@GrahamTheCoder
Copy link
Author

I briefly thought it might not work in Powershell 2.0 and below due to not being in these docs. Turns out the docs are just somewhere else.

@theaquamarine
Copy link
Collaborator

No downsides and fixes issues, let's get this merged.

theaquamarine added a commit that referenced this pull request Sep 8, 2015
Improve pageant process handling
@theaquamarine theaquamarine merged commit ea6533d into dahlbyk:master Sep 8, 2015
@dahlbyk
Copy link
Owner

dahlbyk commented Sep 9, 2015

💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants