Skip to content

Wait for SSH exit when closing remote connection#14635

Merged
adityapatwardhan merged 2 commits into
PowerShell:masterfrom
dinhngtu:ssh-waitfix
Jun 3, 2021
Merged

Wait for SSH exit when closing remote connection#14635
adityapatwardhan merged 2 commits into
PowerShell:masterfrom
dinhngtu:ssh-waitfix

Conversation

@dinhngtu

@dinhngtu dinhngtu commented Jan 19, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

@dinhngtu dinhngtu changed the title Wait for SSH exit when closing remote connection WIP: Wait for SSH exit when closing remote connection Jan 19, 2021
iSazonov
iSazonov reviewed Jan 20, 2021
View reviewed changes
Comment thread src/System.Management.Automation/engine/remoting/common/RunspaceConnectionInfo.cs Outdated
Comment on lines 2480 to 2518

@iSazonov iSazonov Jan 20, 2021 •
edited
Loading

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.

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)].

@dinhngtu dinhngtu Jan 20, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@iSazonov iSazonov Jan 20, 2021

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.

I don't see we use these constants - I see only 2 parameters and return value.

@dinhngtu dinhngtu Jan 20, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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).

@iSazonov iSazonov Jan 20, 2021

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.

Aren't both POSIX?

@dinhngtu dinhngtu Jan 20, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

POSIX doesn't define the specific values of either constants (though SIGKILL should equal 9 most of the time).

@iSazonov iSazonov Jan 21, 2021

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.

Do they really different on supported platforms?

@dinhngtu dinhngtu Jan 21, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@ghost ghost added the Review - Needed The PR is being reviewed label Jan 28, 2021
@ghost

ghost commented Jan 28, 2021

Copy link
Copy Markdown

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

iSazonov
iSazonov reviewed Apr 16, 2021
View reviewed changes
Comment thread src/System.Management.Automation/engine/remoting/common/RunspaceConnectionInfo.cs Outdated
@adityapatwardhan

adityapatwardhan commented May 17, 2021

Copy link
Copy Markdown
Member

@dinhngtu Just merged the PR in PowerShell Native.

@ghost ghost removed the Review - Needed The PR is being reviewed label May 17, 2021
@dinhngtu dinhngtu changed the title WIP: Wait for SSH exit when closing remote connection Wait for SSH exit when closing remote connection May 18, 2021
iSazonov
iSazonov reviewed May 18, 2021
View reviewed changes
Comment thread src/System.Management.Automation/engine/remoting/common/RunspaceConnectionInfo.cs Outdated
Comment thread src/System.Management.Automation/engine/remoting/common/RunspaceConnectionInfo.cs Outdated
@dinhngtu

dinhngtu commented May 18, 2021

Copy link
Copy Markdown
Contributor Author

@dinhngtu Just merged the PR in PowerShell Native.

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.

@iSazonov

iSazonov commented May 19, 2021 •
edited by unfurl-links Bot
Loading

Copy link
Copy Markdown
Collaborator

@dinhngtu It is still unpublished https://www.nuget.org/packages/Microsoft.PowerShell.Native/

Native binaries for PowerShell Core

@iSazonov

iSazonov commented May 20, 2021

Copy link
Copy Markdown
Collaborator

@dinhngtu 7.2.0-preview.2 was published - please update the reference in csproj file.

@iSazonov

iSazonov commented May 21, 2021

Copy link
Copy Markdown
Collaborator

@daxian-dbw @TravisEz13 Could you please clarify what is "1 workflow awaiting approval"? Is it CodeQL? Can I approve?

@ghost ghost added the Review - Needed The PR is being reviewed label May 28, 2021
@ghost

ghost commented May 28, 2021

Copy link
Copy Markdown

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@adityapatwardhan adityapatwardhan merged commit ea19cea into PowerShell:master Jun 3, 2021
@ghost ghost removed the Review - Needed The PR is being reviewed label Jun 3, 2021
@adityapatwardhan adityapatwardhan added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Jun 3, 2021
@adityapatwardhan adityapatwardhan added this to the 7.2.0-preview.7 milestone Jun 3, 2021
@dinhngtu dinhngtu deleted the ssh-waitfix branch June 3, 2021 20:21
@iSazonov

iSazonov commented Jun 4, 2021

Copy link
Copy Markdown
Collaborator

@dinhngtu Thanks for your contribution!

@ghost

ghost commented Jun 17, 2021

Copy link
Copy Markdown

🎉v7.2.0-preview.7 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Jun 17, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adityapatwardhan adityapatwardhan adityapatwardhan approved these changes
@iSazonov iSazonov iSazonov approved these changes
@anmenaga anmenaga Awaiting requested review from anmenaga
@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw daxian-dbw is a code owner
@TravisEz13 TravisEz13 Awaiting requested review from TravisEz13 TravisEz13 is a code owner

Assignees

@adityapatwardhan adityapatwardhan

Labels

CL-Engine Indicates that a PR should be marked as an engine change in the Change Log

Projects

None yet

Milestone

7.2.0-preview.7

Development

Successfully merging this pull request may close these issues.

Defunct ssh processes and CPU load are kept after executing powershell through ssh on linux

3 participants

@dinhngtu @adityapatwardhan @iSazonov