From 4516253ec3fe85297ffa7129d0671d70d0ae7d85 Mon Sep 17 00:00:00 2001 From: Paul Higinbotham Date: Mon, 23 Mar 2020 16:04:19 -0700 Subject: [PATCH 1/8] Fix IPC default named pipe clean up on exit --- .../commands/EnterPSHostProcessCommand.cs | 28 +++++++++++- .../remoting/common/RemoteSessionNamedPipe.cs | 40 ++++++++--------- .../Get-PSHostProcessInfo.Tests.ps1 | 45 +++++++++++++++++++ 3 files changed, 89 insertions(+), 24 deletions(-) diff --git a/src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs b/src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs index cf06c4cda1a..18276edf655 100644 --- a/src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs +++ b/src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs @@ -707,7 +707,7 @@ internal static IReadOnlyCollection GetAppDomainNamesFromProc else if (process.ProcessName.Equals(pName, StringComparison.Ordinal)) { // only add if the process name matches - procAppDomainInfo.Add(new PSHostProcessInfo(pName, id, appDomainName)); + procAppDomainInfo.Add(new PSHostProcessInfo(pName, id, appDomainName, namedPipe)); } } } @@ -736,6 +736,12 @@ internal static IReadOnlyCollection GetAppDomainNamesFromProc /// public sealed class PSHostProcessInfo { + #region Members + + private readonly string _pipeNameFilePath; + + #endregion + #region Properties /// @@ -786,7 +792,12 @@ private PSHostProcessInfo() { } /// Name of process. /// Id of process. /// Name of process AppDomain. - internal PSHostProcessInfo(string processName, int processId, string appDomainName) + /// File path of pipe name. + internal PSHostProcessInfo( + string processName, + int processId, + string appDomainName, + string pipeNameFilePath) { if (string.IsNullOrEmpty(processName)) { throw new PSArgumentNullException("processName"); } @@ -804,6 +815,19 @@ internal PSHostProcessInfo(string processName, int processId, string appDomainNa this.ProcessName = processName; this.ProcessId = processId; this.AppDomainName = appDomainName; + _pipeNameFilePath = pipeNameFilePath; + } + + #endregion + + #region Methods + + /// + /// Returns the pipe name file path. + /// + public string GetPipeNameFilePath() + { + return _pipeNameFilePath; } #endregion diff --git a/src/System.Management.Automation/engine/remoting/common/RemoteSessionNamedPipe.cs b/src/System.Management.Automation/engine/remoting/common/RemoteSessionNamedPipe.cs index eefe2f51f73..ad292328935 100644 --- a/src/System.Management.Automation/engine/remoting/common/RemoteSessionNamedPipe.cs +++ b/src/System.Management.Automation/engine/remoting/common/RemoteSessionNamedPipe.cs @@ -560,9 +560,7 @@ static RemoteSessionNamedPipeServer() CreateIPCNamedPipeServerSingleton(); -#if !CORECLR // There is only one AppDomain per application in CoreCLR, which would be the default - CreateAppDomainUnloadHandler(); -#endif + CreateProcessExitHandler(); } #endregion @@ -961,30 +959,28 @@ internal static void CreateIPCNamedPipeServerSingleton() } } -#if !CORECLR // There is only one AppDomain per application in CoreCLR, which would be the default - private static void CreateAppDomainUnloadHandler() + private static void CreateProcessExitHandler() { - // Subscribe to the app domain unload event. - AppDomain.CurrentDomain.DomainUnload += (sender, args) => + AppDomain.CurrentDomain.ProcessExit += (sender, args) => + { + IPCNamedPipeServerEnabled = false; + RemoteSessionNamedPipeServer namedPipeServer = IPCNamedPipeServer; + if (namedPipeServer != null) { - IPCNamedPipeServerEnabled = false; - RemoteSessionNamedPipeServer namedPipeServer = IPCNamedPipeServer; - if (namedPipeServer != null) + try { - try - { - // Terminate the IPC thread. - namedPipeServer.Dispose(); - } - catch (ObjectDisposedException) { } - catch (Exception) - { - // Don't throw an exception on the app domain unload event thread. - } + // Terminate the IPC thread. + namedPipeServer.Dispose(); } - }; + catch (ObjectDisposedException) { } + catch (Exception) + { + // Don't throw an exception on the app domain unload event thread. + } + } + }; } -#endif + private static void OnIPCNamedPipeServerEnded(object sender, ListenerEndedEventArgs args) { if (args.RestartListener) diff --git a/test/powershell/Modules/Microsoft.PowerShell.Core/Get-PSHostProcessInfo.Tests.ps1 b/test/powershell/Modules/Microsoft.PowerShell.Core/Get-PSHostProcessInfo.Tests.ps1 index e53e4942ece..84a8f64d8f6 100644 --- a/test/powershell/Modules/Microsoft.PowerShell.Core/Get-PSHostProcessInfo.Tests.ps1 +++ b/test/powershell/Modules/Microsoft.PowerShell.Core/Get-PSHostProcessInfo.Tests.ps1 @@ -53,4 +53,49 @@ Describe "Get-PSHostProcessInfo tests" -Tag CI { $psProcess.Count | Should -BeGreaterOrEqual 1 $psProcess.ProcessId | Should -Contain $powershell.id } + + It "Verifies named pipe filepath get method" { + $pipeFilePath = (Get-PSHostProcessInfo -Id $pid).GetPipeNameFilePath() + Test-Path -Path $pipeFilePath | Should -BeTrue + } + + It "Verifies named pipe filepath is removed on process exit" { + $aliveFile = Join-Path -Path $TestDrive -ChildPath 'AliveFileXXZZ.txt' + "" | Out-File -FilePath $aliveFile + $testfilePath = Join-Path -Path $TestDrive -ChildPath 'TestScriptXXZZ.ps1' + @' + param ( + [string] $LiveFilePath + ) + + $count = 0 + while ((Test-Path -Path $LiveFilePath) -and ($count++ -lt 60)) + { + Start-Sleep -Milliseconds 500 + } + + exit +'@ | Out-File -FilePath $testfilePath + + # Create PowerShell process to monitor. + $psFileName = $IsWindows ? 'pwsh.exe' : 'pwsh' + $psPath = Join-Path -Path $PSHOME -ChildPath $psFileName + $psProc = Start-Process -FilePath $psPath -ArgumentList "-File $testfilePath -LiveFilePath $aliveFile" -PassThru + Wait-UntilTrue -sb { + (Get-PSHostProcessInfo -Id $psProc.Id) -ne $null + } -TimeoutInMilliseconds 5000 -IntervalInMilliseconds 250 + + # Verify named pipe file path. + $psNamedPipePath = (Get-PSHostProcessInfo -Id $psProc.Id).GetPipeNameFilePath() + Test-Path -Path $psNamedPipePath | Should -BeTrue + + # Signal PowerShell test process to exit normally. + Remove-Item -Path $aliveFile -Force -ErrorAction Ignore + Wait-UntilTrue -sb { + (Test-Path -Path $psNamedPipePath) -eq $false + } -TimeoutInMilliseconds 5000 -IntervalInMilliseconds 250 + + # Verify named pipe file path is removed. + Test-Path -Path $psNamedPipePath | Should -BeFalse + } } From 3363a1db07d782c4b4ccfefbf6bb6472d490240e Mon Sep 17 00:00:00 2001 From: Paul Higinbotham Date: Mon, 23 Mar 2020 16:22:22 -0700 Subject: [PATCH 2/8] Fix codfactor issues --- .../commands/EnterPSHostProcessCommand.cs | 25 ++++++++++++++----- .../remoting/common/RemoteSessionNamedPipe.cs | 5 +++- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs b/src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs index 18276edf655..59f0e16a734 100644 --- a/src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs +++ b/src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs @@ -787,7 +787,7 @@ public string MainWindowTitle private PSHostProcessInfo() { } /// - /// Constructor. + /// Initializes a new instance of the PSHostProcessInfo type. /// /// Name of process. /// Id of process. @@ -799,9 +799,15 @@ internal PSHostProcessInfo( string appDomainName, string pipeNameFilePath) { - if (string.IsNullOrEmpty(processName)) { throw new PSArgumentNullException("processName"); } + if (string.IsNullOrEmpty(processName)) + { + throw new PSArgumentNullException("processName"); + } - if (string.IsNullOrEmpty(appDomainName)) { throw new PSArgumentNullException("appDomainName"); } + if (string.IsNullOrEmpty(appDomainName)) + { + throw new PSArgumentNullException("appDomainName"); + } MainWindowTitle = string.Empty; try @@ -809,8 +815,14 @@ internal PSHostProcessInfo( var proc = Process.GetProcessById(processId); MainWindowTitle = proc.MainWindowTitle ?? string.Empty; } - catch (ArgumentException) { } - catch (InvalidOperationException) { } + catch (ArgumentException) + { + // Ignore. + } + catch (InvalidOperationException) + { + // Ignore. + } this.ProcessName = processName; this.ProcessId = processId; @@ -823,8 +835,9 @@ internal PSHostProcessInfo( #region Methods /// - /// Returns the pipe name file path. + /// Retrieves the pipe name file path. /// + /// Pipe name file path. public string GetPipeNameFilePath() { return _pipeNameFilePath; diff --git a/src/System.Management.Automation/engine/remoting/common/RemoteSessionNamedPipe.cs b/src/System.Management.Automation/engine/remoting/common/RemoteSessionNamedPipe.cs index ad292328935..4bd6f3ce6fb 100644 --- a/src/System.Management.Automation/engine/remoting/common/RemoteSessionNamedPipe.cs +++ b/src/System.Management.Automation/engine/remoting/common/RemoteSessionNamedPipe.cs @@ -972,7 +972,10 @@ private static void CreateProcessExitHandler() // Terminate the IPC thread. namedPipeServer.Dispose(); } - catch (ObjectDisposedException) { } + catch (ObjectDisposedException) + { + // Ignore. + } catch (Exception) { // Don't throw an exception on the app domain unload event thread. From 11881440c689524b1afb2eeabda9ad97dd14c0f0 Mon Sep 17 00:00:00 2001 From: Paul Higinbotham Date: Tue, 24 Mar 2020 07:52:35 -0700 Subject: [PATCH 3/8] Update src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs Co-Authored-By: Ilya --- .../engine/remoting/commands/EnterPSHostProcessCommand.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs b/src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs index 59f0e16a734..64bc92af5f4 100644 --- a/src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs +++ b/src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs @@ -801,7 +801,7 @@ internal PSHostProcessInfo( { if (string.IsNullOrEmpty(processName)) { - throw new PSArgumentNullException("processName"); + throw new PSArgumentNullException(nameof(processName)); } if (string.IsNullOrEmpty(appDomainName)) From 65ab218b892ea82d34afef0d3dbf2b0003e59234 Mon Sep 17 00:00:00 2001 From: Paul Higinbotham Date: Tue, 24 Mar 2020 07:57:20 -0700 Subject: [PATCH 4/8] Style update --- .../engine/remoting/commands/EnterPSHostProcessCommand.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs b/src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs index 64bc92af5f4..79c95734db2 100644 --- a/src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs +++ b/src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs @@ -806,7 +806,7 @@ internal PSHostProcessInfo( if (string.IsNullOrEmpty(appDomainName)) { - throw new PSArgumentNullException("appDomainName"); + throw new PSArgumentNullException(nameof(appDomainName)); } MainWindowTitle = string.Empty; From f3fdd2b7c90713c040cc9b6f0d524701e9a1f0de Mon Sep 17 00:00:00 2001 From: Paul Higinbotham Date: Tue, 24 Mar 2020 09:50:00 -0700 Subject: [PATCH 5/8] Update comments --- .../engine/remoting/commands/EnterPSHostProcessCommand.cs | 4 ++-- .../engine/remoting/common/RemoteSessionNamedPipe.cs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs b/src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs index b3ce75bd9fe..a135543d2fa 100644 --- a/src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs +++ b/src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs @@ -817,11 +817,11 @@ internal PSHostProcessInfo( } catch (ArgumentException) { - // Ignore. + // Window title is optional. } catch (InvalidOperationException) { - // Ignore. + // Window title is optional. } this.ProcessName = processName; diff --git a/src/System.Management.Automation/engine/remoting/common/RemoteSessionNamedPipe.cs b/src/System.Management.Automation/engine/remoting/common/RemoteSessionNamedPipe.cs index 4bd6f3ce6fb..8cc0670c642 100644 --- a/src/System.Management.Automation/engine/remoting/common/RemoteSessionNamedPipe.cs +++ b/src/System.Management.Automation/engine/remoting/common/RemoteSessionNamedPipe.cs @@ -974,7 +974,7 @@ private static void CreateProcessExitHandler() } catch (ObjectDisposedException) { - // Ignore. + // Ignore if object already disposed. } catch (Exception) { From c49f3d556d5d5ab966b6f5c75cbb6bfc59c05b12 Mon Sep 17 00:00:00 2001 From: Paul Higinbotham Date: Wed, 25 Mar 2020 10:09:49 -0700 Subject: [PATCH 6/8] Update test/powershell/Modules/Microsoft.PowerShell.Core/Get-PSHostProcessInfo.Tests.ps1 Co-Authored-By: Steve Lee --- .../Microsoft.PowerShell.Core/Get-PSHostProcessInfo.Tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/powershell/Modules/Microsoft.PowerShell.Core/Get-PSHostProcessInfo.Tests.ps1 b/test/powershell/Modules/Microsoft.PowerShell.Core/Get-PSHostProcessInfo.Tests.ps1 index 84a8f64d8f6..4db23db3ef0 100644 --- a/test/powershell/Modules/Microsoft.PowerShell.Core/Get-PSHostProcessInfo.Tests.ps1 +++ b/test/powershell/Modules/Microsoft.PowerShell.Core/Get-PSHostProcessInfo.Tests.ps1 @@ -56,7 +56,7 @@ Describe "Get-PSHostProcessInfo tests" -Tag CI { It "Verifies named pipe filepath get method" { $pipeFilePath = (Get-PSHostProcessInfo -Id $pid).GetPipeNameFilePath() - Test-Path -Path $pipeFilePath | Should -BeTrue + $pipeFilePath | Should -Exist } It "Verifies named pipe filepath is removed on process exit" { From df4eaf42ae9288b95047050121b891215a3945f0 Mon Sep 17 00:00:00 2001 From: Paul Higinbotham Date: Wed, 25 Mar 2020 10:10:08 -0700 Subject: [PATCH 7/8] Update test/powershell/Modules/Microsoft.PowerShell.Core/Get-PSHostProcessInfo.Tests.ps1 Co-Authored-By: Steve Lee --- .../Microsoft.PowerShell.Core/Get-PSHostProcessInfo.Tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/powershell/Modules/Microsoft.PowerShell.Core/Get-PSHostProcessInfo.Tests.ps1 b/test/powershell/Modules/Microsoft.PowerShell.Core/Get-PSHostProcessInfo.Tests.ps1 index 4db23db3ef0..7ab9ebe688e 100644 --- a/test/powershell/Modules/Microsoft.PowerShell.Core/Get-PSHostProcessInfo.Tests.ps1 +++ b/test/powershell/Modules/Microsoft.PowerShell.Core/Get-PSHostProcessInfo.Tests.ps1 @@ -87,7 +87,7 @@ Describe "Get-PSHostProcessInfo tests" -Tag CI { # Verify named pipe file path. $psNamedPipePath = (Get-PSHostProcessInfo -Id $psProc.Id).GetPipeNameFilePath() - Test-Path -Path $psNamedPipePath | Should -BeTrue + $psNamedPipePath | Should -Exist # Signal PowerShell test process to exit normally. Remove-Item -Path $aliveFile -Force -ErrorAction Ignore From 11c361da6be86c612603836adc56029e680e3972 Mon Sep 17 00:00:00 2001 From: Paul Higinbotham Date: Wed, 25 Mar 2020 10:10:19 -0700 Subject: [PATCH 8/8] Update test/powershell/Modules/Microsoft.PowerShell.Core/Get-PSHostProcessInfo.Tests.ps1 Co-Authored-By: Steve Lee --- .../Microsoft.PowerShell.Core/Get-PSHostProcessInfo.Tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/powershell/Modules/Microsoft.PowerShell.Core/Get-PSHostProcessInfo.Tests.ps1 b/test/powershell/Modules/Microsoft.PowerShell.Core/Get-PSHostProcessInfo.Tests.ps1 index 7ab9ebe688e..bc31c446baf 100644 --- a/test/powershell/Modules/Microsoft.PowerShell.Core/Get-PSHostProcessInfo.Tests.ps1 +++ b/test/powershell/Modules/Microsoft.PowerShell.Core/Get-PSHostProcessInfo.Tests.ps1 @@ -96,6 +96,6 @@ Describe "Get-PSHostProcessInfo tests" -Tag CI { } -TimeoutInMilliseconds 5000 -IntervalInMilliseconds 250 # Verify named pipe file path is removed. - Test-Path -Path $psNamedPipePath | Should -BeFalse + $psNamedPipePath | Should -Not -Exist } }