Skip to content

Fix IPC default named pipe clean up on exit#12187

Merged
daxian-dbw merged 9 commits intoPowerShell:masterfrom
PaulHigin:fix-namedpiperesource-onexit
Mar 26, 2020
Merged

Fix IPC default named pipe clean up on exit#12187
daxian-dbw merged 9 commits intoPowerShell:masterfrom
PaulHigin:fix-namedpiperesource-onexit

Conversation

@PaulHigin
Copy link
Copy Markdown
Contributor

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

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
@PaulHigin PaulHigin requested a review from SteveL-MSFT March 23, 2020 23:14
iSazonov
iSazonov reviewed Mar 24, 2020
View reviewed changes
Copy link
Copy Markdown
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?

@PaulHigin
Copy link
Copy Markdown
Contributor Author

PaulHigin commented Mar 24, 2020

@PoshChan Please rerun tests

@PoshChan
Copy link
Copy Markdown
Collaborator

PoshChan commented Mar 24, 2020

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

@PaulHigin
Copy link
Copy Markdown
Contributor Author

PaulHigin commented Mar 24, 2020

@PoshChan Please rerun all

@PoshChan
Copy link
Copy Markdown
Collaborator

PoshChan commented Mar 24, 2020

@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
SteveL-MSFT
SteveL-MSFT requested changes Mar 25, 2020
View reviewed changes
@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
@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
Copy link
Copy Markdown
Contributor Author

PaulHigin commented Mar 25, 2020

@PoshChan Please rerun all

@PoshChan
Copy link
Copy Markdown
Collaborator

PoshChan commented Mar 25, 2020

@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
Copy Markdown

MaFreiberger commented Apr 21, 2020 •
edited
Loading

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

iSazonov commented Apr 21, 2020

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

@ghost
Copy link
Copy Markdown

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

iSazonov commented Oct 1, 2020 •
edited
Loading

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

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

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

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

@iSazonov iSazonov iSazonov approved these changes

+1 more reviewer

@PoshChan PoshChan PoshChan left review comments

Reviewers whose approvals may not affect merge requirements

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.

8 participants

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