Skip to content

Fix IPC default named pipe clean up on exit - #12187

Merged
Dongbo Wang (daxian-dbw) merged 9 commits into
PowerShell:masterfrom
PaulHigin:fix-namedpiperesource-onexit
Mar 26, 2020
Merged

Fix IPC default named pipe clean up on exit#12187
Dongbo Wang (daxian-dbw) merged 9 commits into
PowerShell:masterfrom
PaulHigin:fix-namedpiperesource-onexit

Conversation

@PaulHigin

@PaulHigin Paul Higinbotham (PaulHigin) commented Mar 23, 2020 •
edited by iSazonov
Loading

Copy link
Copy Markdown
Contributor

PR Summary

This PR addresses #11599. On Linux platforms, the default named pipe file was not getting cleaned up on PowerShell exit.

PR Context

Windows PowerShell uses the CLR Appdomain unload event to dispose the NamedPipeServerStream object on exit, but that does not work with .NET core runtime. This was not a problem on Windows platforms because the Windows OS releases all process resources on process exit, including named pipes.

Fix is to change this to use the CLR 'ProcessExit' event.

Note that this only works when PowerShell exits normally, i.e., via the 'exit' command. If the console or terminal window is closed then the CLR cannot fire the event.

PR Checklist

iSazonov
Ilya (iSazonov) reviewed Mar 24, 2020
View reviewed changes

@iSazonov Ilya (iSazonov) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

For Windows PowerShell we were using the CLR Appdomain unload event to dispose of the server pipe, but that is not implemented in dotNet core.

Is this a fundamental Unix behavior/problem or we should open an issue in .Net repo to request a fix?

Comment thread src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs Outdated
Comment thread src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs Outdated
Comment thread src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs Outdated
Comment thread src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs Outdated
Comment thread src/System.Management.Automation/engine/remoting/common/RemoteSessionNamedPipe.cs Outdated
Comment thread test/powershell/Modules/Microsoft.PowerShell.Core/Get-PSHostProcessInfo.Tests.ps1
@PaulHigin @iSazonov
Update src/System.Management.Automation/engine/remoting/commands/Ente…
1188144 
…rPSHostProcessCommand.cs

Co-Authored-By: Ilya <darpa@yandex.ru>
@PaulHigin
Style update
65ab218
@PaulHigin

Paul Higinbotham (PaulHigin) commented Mar 24, 2020

Copy link
Copy Markdown
Contributor Author

PoshChan-Bot (@PoshChan) Please rerun tests

@PoshChan

PoshChan-Bot (PoshChan) commented Mar 24, 2020

Copy link
Copy Markdown
Collaborator

Paul Higinbotham (@PaulHigin), I do not understand the build target(s) tests; I only allow static, windows, macos, linux, all

@PaulHigin

Paul Higinbotham (PaulHigin) commented Mar 24, 2020

Copy link
Copy Markdown
Contributor Author

PoshChan-Bot (@PoshChan) Please rerun all

@PoshChan

PoshChan-Bot (PoshChan) commented Mar 24, 2020

Copy link
Copy Markdown
Collaborator

Paul Higinbotham (@PaulHigin), successfully started rebuild of PowerShell-CI-static-analysis, PowerShell-CI-Windows, PowerShell-CI-macOS, PowerShell-CI-Linux

@iSazonov Ilya (iSazonov) added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Mar 24, 2020
@iSazonov Ilya (iSazonov) added this to the 7.1.0-preview.1 milestone Mar 24, 2020
SteveL-MSFT
Steve Lee (SteveL-MSFT) requested changes Mar 25, 2020
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Core/Get-PSHostProcessInfo.Tests.ps1 Outdated
Comment thread test/powershell/Modules/Microsoft.PowerShell.Core/Get-PSHostProcessInfo.Tests.ps1 Outdated
Comment thread test/powershell/Modules/Microsoft.PowerShell.Core/Get-PSHostProcessInfo.Tests.ps1 Outdated
@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Mar 25, 2020
@PaulHigin @SteveL-MSFT
Update test/powershell/Modules/Microsoft.PowerShell.Core/Get-PSHostPr…
c49f3d5 
…ocessInfo.Tests.ps1

Co-Authored-By: Steve Lee <slee@microsoft.com>
@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Mar 25, 2020
@PaulHigin @SteveL-MSFT
Update test/powershell/Modules/Microsoft.PowerShell.Core/Get-PSHostPr…
df4eaf4 
…ocessInfo.Tests.ps1

Co-Authored-By: Steve Lee <slee@microsoft.com>
@PaulHigin @SteveL-MSFT
Update test/powershell/Modules/Microsoft.PowerShell.Core/Get-PSHostPr…
11c361d 
…ocessInfo.Tests.ps1

Co-Authored-By: Steve Lee <slee@microsoft.com>
@PaulHigin

Paul Higinbotham (PaulHigin) commented Mar 25, 2020

Copy link
Copy Markdown
Contributor Author

PoshChan-Bot (@PoshChan) Please rerun all

@PoshChan

PoshChan-Bot (PoshChan) commented Mar 25, 2020

Copy link
Copy Markdown
Collaborator

Paul Higinbotham (@PaulHigin), successfully started rebuild of PowerShell-CI-static-analysis, PowerShell-CI-Windows, PowerShell-CI-macOS, PowerShell-CI-Linux

@daxian-dbw
Dongbo Wang (daxian-dbw) merged commit 668d72c into PowerShell:master Mar 26, 2020
@PaulHigin
Paul Higinbotham (PaulHigin) deleted the fix-namedpiperesource-onexit branch March 26, 2020 16:49
@MaFreiberger

MaFreiberger commented Apr 21, 2020 •
edited
Loading

Copy link
Copy Markdown

PowerShell 7.1.0-preview.1 on debian 9.12

Problem still exists. Every time i power up a console (pwsh-preview) it creates something like
srwxr-xr-x 1 xxx xxx 0 Apr 21 15:17 CoreFxPipe_PSHost.D617DF2A.12797.None.ConsoleHost mai
srwxr-xr-x 1 xxx xxx 0 Apr 21 15:17 CoreFxPipe_PSHost.D617DF3B.12894.None.ConsoleHost mai
srwxr-xr-x 1 xxx xxx 0 Apr 21 15:17 CoreFxPipe_PSHost.D617DF40.12937.None.ConsoleHost mai

@iSazonov

Ilya (iSazonov) commented Apr 21, 2020

Copy link
Copy Markdown
Collaborator

MaFreiberger It is not in Preview1. You can load nightly build or wait next Preview.

@ghost

Deleted user (ghost) commented Apr 23, 2020

Copy link
Copy Markdown

🎉v7.1.0-preview.2 has been released which incorporates this pull request.:tada:

Handy links:

@iSazonov Ilya (iSazonov) mentioned this pull request Sep 18, 2020
Closed
@rjmholt

Rob Holt (rjmholt) commented Sep 30, 2020

Copy link
Copy Markdown
Collaborator

Should this be backported to PowerShell 7.0?

@rjmholt Rob Holt (rjmholt) added Review - Committee The PR/Issue needs a review from the PowerShell Committee Review - Maintainer The PR/issue needs a review from the PowerShell repo Maintainers labels Sep 30, 2020
@iSazonov

Ilya (iSazonov) commented Oct 1, 2020 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Have we requests for this? User can already migrate to 7.1.

@rjmholt

Rob Holt (rjmholt) commented Feb 2, 2021

Copy link
Copy Markdown
Collaborator

Just labelled this as backport consider so that we can discuss whether this should be backported (since otherwise the LTS will just pollute named pipes onto the system). Just a suggestion though -- we can skip it if it's not thought to be severe.

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) added BackPort-7.0.x-Approved and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee BackPort-7.0.x-Consider Review - Maintainer The PR/issue needs a review from the PowerShell repo Maintainers labels Mar 2, 2021
Dongbo Wang (daxian-dbw) pushed a commit to daxian-dbw/PowerShell that referenced this pull request Mar 11, 2021
@PaulHigin @adityapatwardhan
Clean up the IPC named pipe on PowerShell exit (PowerShell#12187)
43fba38
@ghost

Deleted user (ghost) commented Mar 11, 2021

Copy link
Copy Markdown

🎉v7.0.6 has been released which incorporates this pull request.:tada:

Handy links:

@TravisEz13 Travis Plunk (TravisEz13) mentioned this pull request Oct 12, 2022
Merged
22 tasks
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj/PowerShell that referenced this pull request Aug 6, 2026
@PaulHigin
Clean up the IPC named pipe on PowerShell exit (PowerShell#12187)
2ad3088
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SteveL-MSFT Steve Lee (SteveL-MSFT) SteveL-MSFT approved these changes
@iSazonov Ilya (iSazonov) iSazonov approved these changes
+1 more reviewer
@PoshChan PoshChan-Bot (PoshChan) PoshChan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@daxian-dbw Dongbo Wang (daxian-dbw)

Labels

Backport-7.0.x-Done CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Milestone

7.1.0-preview.2

Development

Successfully merging this pull request may close these issues.

8 participants

@PaulHigin @PoshChan @MaFreiberger @iSazonov @rjmholt @SteveL-MSFT @daxian-dbw @adityapatwardhan