From 277ad1f0e1bde29a8670832541f2ca80cbb60b23 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Tue, 28 Jan 2020 14:51:56 -0800 Subject: [PATCH 1/8] Use dedicated threads to read the redirected output and error streams from the child process --- .../fanin/OutOfProcTransportManager.cs | 76 +++++++++++-------- 1 file changed, 43 insertions(+), 33 deletions(-) diff --git a/src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs b/src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs index 9bd00bed2c6..5e4be7ff82e 100644 --- a/src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs +++ b/src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs @@ -1123,28 +1123,15 @@ 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) + stdInWriter = _processInstance.StdInWriter; + if (stdInWriter == null) { - _serverProcess.CancelErrorRead(); - _serverProcess.CancelOutputRead(); + StartOutputAndErrorReaderThreads(_serverProcess); } - // Start asynchronous reading of output/errors - _serverProcess.BeginOutputReadLine(); - _serverProcess.BeginErrorReadLine(); - stdInWriter = new OutOfProcessTextWriter(_serverProcess.StandardInput); _processInstance.StdInWriter = stdInWriter; } @@ -1172,6 +1159,46 @@ internal override void CreateAsync() SendOneItem(); } + private void StartOutputAndErrorReaderThreads(Process serverProcess) + { + ParameterizedThreadStart func = arg => { + var tuple = arg as Tuple; + if (tuple == null) + { + return; + } + + StreamReader reader = tuple.Item1; + bool isStandardOutput = tuple.Item2; + + string data = reader.ReadLine(); + while (data != null) + { + if (isStandardOutput) + { + HandleOutputDataReceived(data); + } + else + { + HandleErrorDataReceived(data); + } + + data = reader.ReadLine(); + } + }; + + Thread outputThread = new Thread(func); + outputThread.IsBackground = true; + outputThread.Name = "Out-of-Proc Job Transport Output Thread"; + + Thread errorThread = new Thread(func); + errorThread.IsBackground = true; + errorThread.Name = "Out-of-Proc Job Transport Error Thread"; + + outputThread.Start(Tuple.Create(serverProcess.StandardOutput, /*is-stdout*/ true)); + errorThread.Start(Tuple.Create(serverProcess.StandardError, /*is-stdout*/ false)); + } + /// /// Kills the server process and disposes other resources. /// @@ -1198,20 +1225,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() @@ -1234,9 +1247,6 @@ private void KillServerProcess() _serverProcess.CancelErrorRead(); _serverProcess.Kill(); } - - _serverProcess.OutputDataReceived -= new DataReceivedEventHandler(OnOutputDataReceived); - _serverProcess.ErrorDataReceived -= new DataReceivedEventHandler(OnErrorDataReceived); } } catch (System.ComponentModel.Win32Exception) From efddbff4520df48befa8d284386f944b16e6ae33 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Tue, 28 Jan 2020 16:46:35 -0800 Subject: [PATCH 2/8] Address Paul's comments --- .../fanin/OutOfProcTransportManager.cs | 68 +++++++++++-------- 1 file changed, 38 insertions(+), 30 deletions(-) diff --git a/src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs b/src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs index 5e4be7ff82e..d6a3755737b 100644 --- a/src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs +++ b/src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs @@ -1129,7 +1129,7 @@ internal override void CreateAsync() stdInWriter = _processInstance.StdInWriter; if (stdInWriter == null) { - StartOutputAndErrorReaderThreads(_serverProcess); + StartRedirectionReaderThreads(_serverProcess); } stdInWriter = new OutOfProcessTextWriter(_serverProcess.StandardInput); @@ -1159,44 +1159,52 @@ internal override void CreateAsync() SendOneItem(); } - private void StartOutputAndErrorReaderThreads(Process serverProcess) + private void StartRedirectionReaderThreads(Process serverProcess) { - ParameterizedThreadStart func = arg => { - var tuple = arg as Tuple; - if (tuple == null) - { - return; - } + Thread outputThread = new Thread(ProcessOutputData); + outputThread.IsBackground = true; + outputThread.Name = "Out-of-Proc Job Transport Output Thread"; - StreamReader reader = tuple.Item1; - bool isStandardOutput = tuple.Item2; + Thread errorThread = new Thread(ProcessErrorData); + errorThread.IsBackground = true; + errorThread.Name = "Out-of-Proc Job Transport Error Thread"; + outputThread.Start(serverProcess.StandardOutput); + errorThread.Start(serverProcess.StandardError); + } + + private void ProcessOutputData(object arg) + { + if (arg is StreamReader reader) + { string data = reader.ReadLine(); while (data != null) { - if (isStandardOutput) - { - HandleOutputDataReceived(data); - } - else - { - HandleErrorDataReceived(data); - } - + HandleOutputDataReceived(data); data = reader.ReadLine(); } - }; - - Thread outputThread = new Thread(func); - outputThread.IsBackground = true; - outputThread.Name = "Out-of-Proc Job Transport Output Thread"; - - Thread errorThread = new Thread(func); - errorThread.IsBackground = true; - errorThread.Name = "Out-of-Proc Job Transport Error Thread"; + } + else + { + Dbg.Assert(false, "Invalid argument. Expecting a StreamReader object."); + } + } - outputThread.Start(Tuple.Create(serverProcess.StandardOutput, /*is-stdout*/ true)); - errorThread.Start(Tuple.Create(serverProcess.StandardError, /*is-stdout*/ false)); + private void ProcessErrorData(object arg) + { + if (arg is StreamReader reader) + { + string data = reader.ReadLine(); + while (data != null) + { + HandleErrorDataReceived(data); + data = reader.ReadLine(); + } + } + else + { + Dbg.Assert(false, "Invalid argument. Expecting a StreamReader object."); + } } /// From 9d930d12e629b21ccb326d745e1befa46f8a6ff0 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Wed, 29 Jan 2020 10:07:09 -0800 Subject: [PATCH 3/8] Remove the 'stdInWriter == null' check and add try/catch to the reader threads --- .../fanin/OutOfProcTransportManager.cs | 53 +++++++++++++------ 1 file changed, 37 insertions(+), 16 deletions(-) diff --git a/src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs b/src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs index d6a3755737b..afdb83d7c40 100644 --- a/src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs +++ b/src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs @@ -1126,12 +1126,7 @@ internal override void CreateAsync() _serverProcess.Exited += new EventHandler(OnExited); _processInstance.Start(); - stdInWriter = _processInstance.StdInWriter; - if (stdInWriter == null) - { - StartRedirectionReaderThreads(_serverProcess); - } - + StartRedirectionReaderThreads(_serverProcess); stdInWriter = new OutOfProcessTextWriter(_serverProcess.StandardInput); _processInstance.StdInWriter = stdInWriter; } @@ -1163,11 +1158,11 @@ private void StartRedirectionReaderThreads(Process serverProcess) { Thread outputThread = new Thread(ProcessOutputData); outputThread.IsBackground = true; - outputThread.Name = "Out-of-Proc Job Transport Output Thread"; + outputThread.Name = "Out-of-Proc Job Output Thread"; Thread errorThread = new Thread(ProcessErrorData); errorThread.IsBackground = true; - errorThread.Name = "Out-of-Proc Job Transport Error Thread"; + errorThread.Name = "Out-of-Proc Job Error Thread"; outputThread.Start(serverProcess.StandardOutput); errorThread.Start(serverProcess.StandardError); @@ -1177,11 +1172,24 @@ private void ProcessOutputData(object arg) { if (arg is StreamReader reader) { - string data = reader.ReadLine(); - while (data != null) + try + { + string data = reader.ReadLine(); + while (data != null) + { + HandleOutputDataReceived(data); + data = reader.ReadLine(); + } + } + catch (IOException) { - HandleOutputDataReceived(data); - data = reader.ReadLine(); + // Treat this as EOF, the same as what 'Process.BeginOutputReadLine()' does. + } + catch (Exception e) + { + string errorMsg = e.Message ?? string.Empty; + _tracer.WriteMessage("OutOfProcessClientSessionTransportManager", "ProcessOutputThread", Guid.Empty, + "Transport manager output reader thread ended with error: {0}", errorMsg); } } else @@ -1194,11 +1202,24 @@ private void ProcessErrorData(object arg) { if (arg is StreamReader reader) { - string data = reader.ReadLine(); - while (data != null) + 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) { - HandleErrorDataReceived(data); - data = reader.ReadLine(); + string errorMsg = e.Message ?? string.Empty; + _tracer.WriteMessage("OutOfProcessClientSessionTransportManager", "ProcessErrorThread", Guid.Empty, + "Transport manager error reader thread ended with error: {0}", errorMsg); } } else From f133f942293442b66c44e233af5f63c92637c527 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Thu, 30 Jan 2020 09:22:31 -0800 Subject: [PATCH 4/8] Fix the CodeFactor issues --- .../fanin/OutOfProcTransportManager.cs | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs b/src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs index afdb83d7c40..d81ab3aaaca 100644 --- a/src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs +++ b/src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs @@ -1187,9 +1187,12 @@ private void ProcessOutputData(object arg) } catch (Exception e) { - string errorMsg = e.Message ?? string.Empty; - _tracer.WriteMessage("OutOfProcessClientSessionTransportManager", "ProcessOutputThread", Guid.Empty, - "Transport manager output reader thread ended with error: {0}", errorMsg); + _tracer.WriteMessage( + "OutOfProcessClientSessionTransportManager", + "ProcessOutputThread", + Guid.Empty, + "Transport manager output reader thread ended with error: {0}", + e.Message ?? string.Empty); } } else @@ -1217,9 +1220,12 @@ private void ProcessErrorData(object arg) } catch (Exception e) { - string errorMsg = e.Message ?? string.Empty; - _tracer.WriteMessage("OutOfProcessClientSessionTransportManager", "ProcessErrorThread", Guid.Empty, - "Transport manager error reader thread ended with error: {0}", errorMsg); + _tracer.WriteMessage( + "OutOfProcessClientSessionTransportManager", + "ProcessErrorThread", + Guid.Empty, + "Transport manager error reader thread ended with error: {0}", + e.Message ?? string.Empty); } } else From b6b446a9a3895ea292837019d3914cb70d722127 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Fri, 27 Mar 2020 10:10:49 -0700 Subject: [PATCH 5/8] Remove calls to 'CancelOutputRead' and 'CancelErrorRead' --- .../engine/remoting/fanin/OutOfProcTransportManager.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs b/src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs index d81ab3aaaca..fd55cd51bd3 100644 --- a/src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs +++ b/src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs @@ -1098,7 +1098,6 @@ internal override void CreateAsync() { _processCreated = false; } - // _processInstance.Start(); } PSEtwLog.LogAnalyticInformational(PSEventId.WSManCreateShell, PSOpcode.Connect, @@ -1278,8 +1277,6 @@ private void KillServerProcess() if (_processCreated) { - _serverProcess.CancelOutputRead(); - _serverProcess.CancelErrorRead(); _serverProcess.Kill(); } } From 5f13eb0572e6c2a378f57c7ea556528e807ceb99 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Fri, 27 Mar 2020 13:36:11 -0700 Subject: [PATCH 6/8] Add tests to verify the background process is cleaned up after job is done --- test/powershell/engine/Job/Jobs.Tests.ps1 | 44 +++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/test/powershell/engine/Job/Jobs.Tests.ps1 b/test/powershell/engine/Job/Jobs.Tests.ps1 index 13276e092c0..57dcd437217 100644 --- a/test/powershell/engine/Job/Jobs.Tests.ps1 +++ b/test/powershell/engine/Job/Jobs.Tests.ps1 @@ -338,4 +338,48 @@ 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 + + # The job is done, wait 2 seconds for the cleanup to finish. + Start-Sleep -Seconds 2 + { Get-Process -Id $processId -ErrorAction Stop } | Should -Throw ` + -ErrorId '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. + Start-Sleep -Seconds 2 + $processId = Receive-Job $job + + # Stop the job and wait 2 seconds for the cleanup to finish. + Stop-Job $job + Start-Sleep -Seconds 2 + { Get-Process -Id $processId -ErrorAction Stop } | Should -Throw ` + -ErrorId '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. + Start-Sleep -Seconds 2 + $processId = Receive-Job $job + + # Remove the job and wait 2 seconds for the cleanup to finish. + Remove-Job $job -Force + Start-Sleep -Seconds 2 + { Get-Process -Id $processId -ErrorAction Stop } | Should -Throw ` + -ErrorId 'NoProcessFoundForGivenId,Microsoft.PowerShell.Commands.GetProcessCommand' + } + } } From a30faa0a24ecf1aa5953b3f70b1c9b6671008488 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Fri, 27 Mar 2020 14:18:02 -0700 Subject: [PATCH 7/8] Use 'Wait-UntilTrue' to avoid 'Start-Sleep' --- test/powershell/engine/Job/Jobs.Tests.ps1 | 28 ++++++++++++++++------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/test/powershell/engine/Job/Jobs.Tests.ps1 b/test/powershell/engine/Job/Jobs.Tests.ps1 index 57dcd437217..756ca04b447 100644 --- a/test/powershell/engine/Job/Jobs.Tests.ps1 +++ b/test/powershell/engine/Job/Jobs.Tests.ps1 @@ -344,8 +344,12 @@ Describe 'Basic Job Tests' -Tags 'Feature' { $job = Start-Job { $pid } $processId = Receive-Job $job -Wait - # The job is done, wait 2 seconds for the cleanup to finish. - Start-Sleep -Seconds 2 + # The job is done, wait for the cleanup to finish. + Wait-UntilTrue { + $null -eq (Get-Process -Id $processId -ErrorAction Ignore) + } -IntervalInMilliseconds 300 | Should -BeTrue + + # Double check that we cannot find the process. { Get-Process -Id $processId -ErrorAction Stop } | Should -Throw ` -ErrorId 'NoProcessFoundForGivenId,Microsoft.PowerShell.Commands.GetProcessCommand' @@ -356,12 +360,16 @@ Describe 'Basic Job Tests' -Tags 'Feature' { $job = Start-Job { $pid; Start-Sleep -Second 10 } # Wait for the pid to be received. - Start-Sleep -Seconds 2 + Wait-UntilTrue { [bool](Receive-Job $job -Keep) } | Should -BeTrue $processId = Receive-Job $job - # Stop the job and wait 2 seconds for the cleanup to finish. + # Stop the job and wait for the cleanup to finish. Stop-Job $job - Start-Sleep -Seconds 2 + Wait-UntilTrue { + $null -eq (Get-Process -Id $processId -ErrorAction Ignore) + } -IntervalInMilliseconds 300 | Should -BeTrue + + # Double check that we cannot find the process. { Get-Process -Id $processId -ErrorAction Stop } | Should -Throw ` -ErrorId 'NoProcessFoundForGivenId,Microsoft.PowerShell.Commands.GetProcessCommand' @@ -372,12 +380,16 @@ Describe 'Basic Job Tests' -Tags 'Feature' { $job = Start-Job { $pid; Start-Sleep -Second 10 } # Wait for the pid to be received. - Start-Sleep -Seconds 2 + Wait-UntilTrue { [bool](Receive-Job $job -Keep) } | Should -BeTrue $processId = Receive-Job $job - # Remove the job and wait 2 seconds for the cleanup to finish. + # Remove the job and wait for the cleanup to finish. Remove-Job $job -Force - Start-Sleep -Seconds 2 + Wait-UntilTrue { + $null -eq (Get-Process -Id $processId -ErrorAction Ignore) + } -IntervalInMilliseconds 300 | Should -BeTrue + + # Double check that we cannot find the process. { Get-Process -Id $processId -ErrorAction Stop } | Should -Throw ` -ErrorId 'NoProcessFoundForGivenId,Microsoft.PowerShell.Commands.GetProcessCommand' } From d13397143c780b0aafd441a91c11a74f37b52c84 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Fri, 27 Mar 2020 16:27:23 -0700 Subject: [PATCH 8/8] Fix test --- test/powershell/engine/Job/Jobs.Tests.ps1 | 38 +++++++++++------------ 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/test/powershell/engine/Job/Jobs.Tests.ps1 b/test/powershell/engine/Job/Jobs.Tests.ps1 index 756ca04b447..7200c6ad45b 100644 --- a/test/powershell/engine/Job/Jobs.Tests.ps1 +++ b/test/powershell/engine/Job/Jobs.Tests.ps1 @@ -344,14 +344,12 @@ Describe 'Basic Job Tests' -Tags 'Feature' { $job = Start-Job { $pid } $processId = Receive-Job $job -Wait - # The job is done, wait for the cleanup to finish. - Wait-UntilTrue { - $null -eq (Get-Process -Id $processId -ErrorAction Ignore) - } -IntervalInMilliseconds 300 | Should -BeTrue - - # Double check that we cannot find the process. - { Get-Process -Id $processId -ErrorAction Stop } | Should -Throw ` - -ErrorId 'NoProcessFoundForGivenId,Microsoft.PowerShell.Commands.GetProcessCommand' + 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 } @@ -365,13 +363,13 @@ Describe 'Basic Job Tests' -Tags 'Feature' { # Stop the job and wait for the cleanup to finish. Stop-Job $job - Wait-UntilTrue { - $null -eq (Get-Process -Id $processId -ErrorAction Ignore) - } -IntervalInMilliseconds 300 | Should -BeTrue - # Double check that we cannot find the process. - { Get-Process -Id $processId -ErrorAction Stop } | Should -Throw ` - -ErrorId 'NoProcessFoundForGivenId,Microsoft.PowerShell.Commands.GetProcessCommand' + 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 } @@ -385,13 +383,13 @@ Describe 'Basic Job Tests' -Tags 'Feature' { # Remove the job and wait for the cleanup to finish. Remove-Job $job -Force - Wait-UntilTrue { - $null -eq (Get-Process -Id $processId -ErrorAction Ignore) - } -IntervalInMilliseconds 300 | Should -BeTrue - # Double check that we cannot find the process. - { Get-Process -Id $processId -ErrorAction Stop } | Should -Throw ` - -ErrorId 'NoProcessFoundForGivenId,Microsoft.PowerShell.Commands.GetProcessCommand' + 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' + } } } }