Skip to content
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

Jump to bottom

Fix IPC default named pipe clean up on exit #12187

Merged
merged 9 commits into from
Mar 26, 2020
Merged

Fix IPC default named pipe clean up on exit #12187

merged 9 commits into from
Mar 26, 2020

Conversation

PaulHigin
Copy link
Contributor

@PaulHigin PaulHigin commented Mar 23, 2020 •
edited by iSazonov

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

@ghost ghost assigned daxian-dbw Mar 23, 2020
iSazonov
iSazonov reviewed Mar 24, 2020
View reviewed changes
Copy link
Collaborator

@iSazonov iSazonov left a comment

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?

src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs Outdated Show resolved Hide resolved
src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs Outdated Show resolved Hide resolved
src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs Outdated Show resolved Hide resolved
src/System.Management.Automation/engine/remoting/commands/EnterPSHostProcessCommand.cs Outdated Show resolved Hide resolved
src/System.Management.Automation/engine/remoting/common/RemoteSessionNamedPipe.cs Outdated Show resolved Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Core/Get-PSHostProcessInfo.Tests.ps1 Show resolved Hide resolved
PaulHigin and others added 2 commits March 24, 2020 07:52
@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
Copy link
Contributor Author

@PoshChan Please rerun tests

@PoshChan
Copy link
Collaborator

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

@PaulHigin
Copy link
Contributor Author

@PoshChan Please rerun all

@PoshChan
Copy link
Collaborator

@PaulHigin, successfully started rebuild of PowerShell-CI-static-analysis, PowerShell-CI-Windows, PowerShell-CI-macOS, PowerShell-CI-Linux

@iSazonov 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 iSazonov added this to the 7.1.0-preview.1 milestone Mar 24, 2020
@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 and others added 2 commits March 25, 2020 10:10
@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
Copy link
Contributor Author

@PoshChan Please rerun all

@PoshChan
Copy link
Collaborator

@PaulHigin, successfully started rebuild of PowerShell-CI-static-analysis, PowerShell-CI-Windows, PowerShell-CI-macOS, PowerShell-CI-Linux

@daxian-dbw daxian-dbw merged commit 668d72c into PowerShell:master Mar 26, 2020
@PaulHigin PaulHigin deleted the fix-namedpiperesource-onexit branch March 26, 2020 16:49
@MaFreiberger
Copy link

MaFreiberger commented Apr 21, 2020 •
edited

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
Copy link
Collaborator

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

@ghost
Copy link

ghost commented Apr 23, 2020

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

Handy links:

@iSazonov iSazonov mentioned this pull request Sep 18, 2020
Closed
@rjmholt
Copy link
Collaborator

rjmholt commented Sep 30, 2020

Should this be backported to PowerShell 7.0?

@rjmholt 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
Copy link
Collaborator

iSazonov commented Oct 1, 2020 •
edited

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

@rjmholt
Copy link
Collaborator

rjmholt commented Feb 2, 2021

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 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
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
Copy link

ghost commented Mar 11, 2021

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

Handy links:

@TravisEz13 TravisEz13 mentioned this pull request Oct 12, 2022
Merged
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PoshChan PoshChan PoshChan left review comments

@SteveL-MSFT SteveL-MSFT SteveL-MSFT approved these changes

@iSazonov iSazonov iSazonov approved these changes

Assignees

@daxian-dbw 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.

None yet
8 participants
@PaulHigin @PoshChan @MaFreiberger @iSazonov @rjmholt @SteveL-MSFT @daxian-dbw @adityapatwardhan