Wait for SSH exit when closing remote connection #14635
Conversation
There was a problem hiding this comment.
Do we really need this in libpsl? Why not call from libc directly? Search by DllImport("libc" for examples.
Also we should add [return: MarshalAs(UnmanagedType.Bool)].
There was a problem hiding this comment.
Signals and other constants don't have a defined, standardized value, so I prefer to wrap it in libpsl and use the header constants there rather than coding it into S.M.A.
There was a problem hiding this comment.
I don't see we use these constants - I see only 2 parameters and return value.
There was a problem hiding this comment.
These are referenced in libpsl wrappers: PowerShell/PowerShell-Native#63
If you called directly into libc from S.M.A then you'd need to call kill(pid, SIGKILL) and waitpid(pid, NULL, WNOHANG).
There was a problem hiding this comment.
POSIX doesn't define the specific values of either constants (though SIGKILL should equal 9 most of the time).
There was a problem hiding this comment.
Do they really different on supported platforms?
There was a problem hiding this comment.
I haven't looked into all platforms but following POSIX should be more portable than hardcoding the constants in S.M.A. While it might work with SIGKILL I doubt it'll be very reliable to hardcode WNOHANG. In addition, if we directly imported kill()/waitpid() in S.M.A it would then be possible to use them directly with nonportable constants, so I prefer to restrict the call interface in this fashion.
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
src/System.Management.Automation/engine/remoting/common/RunspaceConnectionInfo.cs
Outdated
Show resolved
Hide resolved
|
@dinhngtu Just merged the PR in PowerShell Native. |
src/System.Management.Automation/engine/remoting/common/RunspaceConnectionInfo.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/remoting/common/RunspaceConnectionInfo.cs
Outdated
Show resolved
Hide resolved
Should I update something in powershell to use the new libpsl? I had to manually replace libpsl-native.so to test the new native library. |
|
@dinhngtu It is still unpublished https://www.nuget.org/packages/Microsoft.PowerShell.Native/
|
|
@dinhngtu 7.2.0-preview.2 was published - please update the reference in csproj file. |
|
@daxian-dbw @TravisEz13 Could you please clarify what is "1 workflow awaiting approval"? Is it CodeQL? Can I approve? |
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
|
@dinhngtu Thanks for your contribution! |
|
🎉 Handy links: |
PR Summary
Fixes #13356 by adding custom functions for killing and waiting for SSH processes. Requires PowerShell/PowerShell-Native#63.
PR Context
On Unix, after a SSH session finishes, PowerShell uses System.Diagnostics.Process.Kill() to stop the process, but doesn't reap the process with waitpid(). This causes SSH to become a defunct (zombie) process.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.