Skip to content

Fix Start-Process -Credential -Wait to work on Windows #19082

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.

Sign up for GitHub

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

Merged
PaulHigin merged 2 commits into from
Feb 3, 2023
Merged

Fix Start-Process -Credential -Wait to work on Windows #19082

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3e8f71a
address Ilya's feedback, simplify to just WaitForExit() when cred is …
SteveL-MSFT Feb 2, 2023
5854118
Update src/Microsoft.PowerShell.Commands.Management/commands/manageme…
SteveL-MSFT Feb 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 20 additions & 12 deletions src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2039,21 +2039,29 @@ protected override void BeginProcessing()
#if UNIX
process.WaitForExit();
#else
_waithandle = new ManualResetEvent(false);

// Create and start the job object
ProcessCollection jobObject = new();
if (jobObject.AssignProcessToJobObject(process))
if (_credential is not null)
{
// Wait for the job object to finish
jobObject.WaitOne(_waithandle);
// If we are running as a different user, we cannot use a job object, so just wait on the process
process.WaitForExit();
Copy link
Contributor

@PaulHigin PaulHigin Feb 2, 2023

Choose a reason for hiding this comment

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

Is there any reason not to always use process.WaitForExit()? Also if the user provides explicit credentials for the current user account, process.WaitForExit() will be used instead of the job object. Is this Ok?

Copy link
Collaborator

@jborean93 jborean93 Feb 2, 2023

Choose a reason for hiding this comment

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

-Wait will wait not just for the new process but any processes that the new process spawns. By waiting on the job object it will wait until all of them have finished rather than just the current one.

Copy link
Member Author

@SteveL-MSFT SteveL-MSFT Feb 3, 2023

Choose a reason for hiding this comment

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

I think the intent of the job object is to also wait on any child processes, but processes in for a different user can't be added to the job object so in that case we can only WaitForExit() of the initial process. WG had discussed this and was ok with this difference.

Copy link
Collaborator

@jborean93 jborean93 Feb 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

but processes in for a different user can't be added to the job object

This is not true, did you read my last comment - #19082 (comment)?

}
else if (!process.HasExited)
else
{
// WinBlue: 27537 Start-Process -Wait doesn't work in a remote session on Windows 7 or lower.
process.Exited += myProcess_Exited;
process.EnableRaisingEvents = true;
process.WaitForExit();
_waithandle = new ManualResetEvent(false);

// Create and start the job object
ProcessCollection jobObject = new();
if (jobObject.AssignProcessToJobObject(process))
{
// Wait for the job object to finish
jobObject.WaitOne(_waithandle);
}
else if (!process.HasExited)
{
// WinBlue: 27537 Start-Process -Wait doesn't work in a remote session on Windows 7 or lower.
process.Exited += myProcess_Exited;
process.EnableRaisingEvents = true;
process.WaitForExit();
}
}
#endif
}
Expand Down