Skip to content

Use dedicated threads to read the redirected output and error streams from the child process for out-of-proc jobs #11713

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
iSazonov merged 8 commits into from
Mar 28, 2020
Merged

Use dedicated threads to read the redirected output and error streams from the child process for out-of-proc jobs #11713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
277ad1f
Use dedicated threads to read the redirected output and error streams…
daxian-dbw Jan 28, 2020
efddbff
Address Paul's comments
daxian-dbw Jan 29, 2020
9d930d1
Remove the 'stdInWriter == null' check and add try/catch to the reade…
daxian-dbw Jan 29, 2020
f133f94
Fix the CodeFactor issues
daxian-dbw Jan 30, 2020
b6b446a
Remove calls to 'CancelOutputRead' and 'CancelErrorRead'
daxian-dbw Mar 27, 2020
5f13eb0
Add tests to verify the background process is cleaned up after job is…
daxian-dbw Mar 27, 2020
a30faa0
Use 'Wait-UntilTrue' to avoid 'Start-Sleep'
daxian-dbw Mar 27, 2020
d133971
Fix test
daxian-dbw Mar 27, 2020
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
120 changes: 81 additions & 39 deletions src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1098,7 +1098,6 @@ internal override void CreateAsync()
{
_processCreated = false;
}
// _processInstance.Start();
}

PSEtwLog.LogAnalyticInformational(PSEventId.WSManCreateShell, PSOpcode.Connect,
Expand All @@ -1123,28 +1122,10 @@ internal override void CreateAsync()
_processInstance.RunspacePool.Dispose();
}

stdInWriter = _processInstance.StdInWriter;
// if (stdInWriter == null)
{
_serverProcess.OutputDataReceived += new DataReceivedEventHandler(OnOutputDataReceived);
_serverProcess.ErrorDataReceived += new DataReceivedEventHandler(OnErrorDataReceived);
}

_serverProcess.Exited += new EventHandler(OnExited);

// serverProcess.Start();
_processInstance.Start();

if (stdInWriter != null)
{
_serverProcess.CancelErrorRead();
_serverProcess.CancelOutputRead();
}

// Start asynchronous reading of output/errors
_serverProcess.BeginOutputReadLine();
_serverProcess.BeginErrorReadLine();

StartRedirectionReaderThreads(_serverProcess);
stdInWriter = new OutOfProcessTextWriter(_serverProcess.StandardInput);
_processInstance.StdInWriter = stdInWriter;
}
Expand Down Expand Up @@ -1172,6 +1153,86 @@ internal override void CreateAsync()
SendOneItem();
}

private void StartRedirectionReaderThreads(Process serverProcess)
{
Thread outputThread = new Thread(ProcessOutputData);
outputThread.IsBackground = true;
outputThread.Name = "Out-of-Proc Job Output Thread";

Thread errorThread = new Thread(ProcessErrorData);
errorThread.IsBackground = true;
errorThread.Name = "Out-of-Proc Job Error Thread";

outputThread.Start(serverProcess.StandardOutput);
errorThread.Start(serverProcess.StandardError);
}

private void ProcessOutputData(object arg)
{
if (arg is StreamReader reader)
{
try
{
string data = reader.ReadLine();
while (data != null)
{
HandleOutputDataReceived(data);
data = reader.ReadLine();
}
}
catch (IOException)
{
// Treat this as EOF, the same as what 'Process.BeginOutputReadLine()' does.
}
catch (Exception e)
{
_tracer.WriteMessage(
"OutOfProcessClientSessionTransportManager",
"ProcessOutputThread",
Guid.Empty,
"Transport manager output reader thread ended with error: {0}",
e.Message ?? string.Empty);
}
}
else
{
Dbg.Assert(false, "Invalid argument. Expecting a StreamReader object.");
}
}

private void ProcessErrorData(object arg)
{
if (arg is StreamReader reader)
{
try
{
string data = reader.ReadLine();
while (data != null)
{
HandleErrorDataReceived(data);
data = reader.ReadLine();
}
}
catch (IOException)
{
// Treat this as EOF, the same as what 'Process.BeginErrorReadLine()' does.
}
catch (Exception e)
{
_tracer.WriteMessage(
"OutOfProcessClientSessionTransportManager",
"ProcessErrorThread",
Guid.Empty,
"Transport manager error reader thread ended with error: {0}",
e.Message ?? string.Empty);
}
}
else
{
Dbg.Assert(false, "Invalid argument. Expecting a StreamReader object.");
}
}

/// <summary>
/// Kills the server process and disposes other resources.
/// </summary>
Expand All @@ -1198,20 +1259,6 @@ protected override void CleanupConnection()

#endregion

#region Event Handlers

private void OnOutputDataReceived(object sender, DataReceivedEventArgs e)
{
HandleOutputDataReceived(e.Data);
}

private void OnErrorDataReceived(object sender, DataReceivedEventArgs e)
{
HandleErrorDataReceived(e.Data);
}

#endregion

#region Helper Methods

private void KillServerProcess()
Expand All @@ -1230,13 +1277,8 @@ private void KillServerProcess()

if (_processCreated)
{
_serverProcess.CancelOutputRead();
_serverProcess.CancelErrorRead();
_serverProcess.Kill();
}

_serverProcess.OutputDataReceived -= new DataReceivedEventHandler(OnOutputDataReceived);
_serverProcess.ErrorDataReceived -= new DataReceivedEventHandler(OnErrorDataReceived);
}
}
catch (System.ComponentModel.Win32Exception)
Expand Down
54 changes: 54 additions & 0 deletions test/powershell/engine/Job/Jobs.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,4 +338,58 @@ Describe 'Basic Job Tests' -Tags 'Feature' {
ValidateJobInfo -job $jobToStop -state 'Stopped' -hasMoreData $false
}
}

Context 'Background pwsh process should terminate after job is done' {
It "Can clean up background pwsh process after job is done" {
$job = Start-Job { $pid }
$processId = Receive-Job $job -Wait

try {
$process = Get-Process -Id $processId -ErrorAction Stop
Wait-UntilTrue { $process.HasExited } -IntervalInMilliseconds 300 | Should -BeTrue
} catch {
$_.FullyQualifiedErrorId | Should -BeExactly 'NoProcessFoundForGivenId,Microsoft.PowerShell.Commands.GetProcessCommand'
}

Remove-Job $job -Force
}

It "Can clean up background pwsh process when job is stopped" {
$job = Start-Job { $pid; Start-Sleep -Second 10 }

# Wait for the pid to be received.
Wait-UntilTrue { [bool](Receive-Job $job -Keep) } | Should -BeTrue
$processId = Receive-Job $job

# Stop the job and wait for the cleanup to finish.
Stop-Job $job

try {
$process = Get-Process -Id $processId -ErrorAction Stop
Wait-UntilTrue { $process.HasExited } -IntervalInMilliseconds 300 | Should -BeTrue
} catch {
$_.FullyQualifiedErrorId | Should -BeExactly 'NoProcessFoundForGivenId,Microsoft.PowerShell.Commands.GetProcessCommand'
}

Remove-Job $job -Force
}

It "Can clean up background pwsh process when job is removed" {
$job = Start-Job { $pid; Start-Sleep -Second 10 }

# Wait for the pid to be received.
Wait-UntilTrue { [bool](Receive-Job $job -Keep) } | Should -BeTrue
$processId = Receive-Job $job

# Remove the job and wait for the cleanup to finish.
Remove-Job $job -Force

try {
$process = Get-Process -Id $processId -ErrorAction Stop
Wait-UntilTrue { $process.HasExited } -IntervalInMilliseconds 300 | Should -BeTrue
} catch {
$_.FullyQualifiedErrorId | Should -BeExactly 'NoProcessFoundForGivenId,Microsoft.PowerShell.Commands.GetProcessCommand'
}
}
}
}