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.

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

how to Kill remote process on channel.close so it can be garbage collected correctly? #785

Closed
rustyscottweber opened this issue Jul 23, 2016 · 8 comments
Labels

Comments

@rustyscottweber
Copy link

I have a problem where getting the PTY is not an option I would/can consider. However, I need to be able to kill a long running process when the process is no longer needed. Previously, this could be accompolished by setting get_pty=True.. But that's not an option for me because of certain new issues with wall spitting out lots of data which will tamper with my long running output.
I have some code similar to this..
pstdin, pstdout, pstderr = connection.pclient.exec_command(command=cmd, timeout=timeout, get_pty=False)

do stuff for a really long time

self.pstdout.channel.close()
I also cannot call kill on the process using the command line or the 'kill -9' or 'pkill' because there are MANY other processes that look exactly like it and I cannot distinguish which process is the one that needs to die and which ones need to keep living.. Strange.. I know....
Is there a way to get the remote ssh server to kill the process when the channel is closed without using get_pty=True, or is it just a better solution to create a new connection for each process and then disconnect the connection to make the ssh server clean up the process that way to ensure that there are no zombie processes created?

@radssh
Copy link
Contributor

radssh commented Jul 23, 2016

There is a signal send message in the SSH protocol RFC https://www.ietf.org/rfc/rfc4254.txt that looks like it would be exactly what you seek.

Even though I don't see direct support in paramiko for this category of messages, you should be able to construct it in a Message and use transport._send_user_message() to issue SIGTERM and/or SIGKILL to the underlying channel. I may try my hand at putting together a PR for a general Channel.send_signal(), but not sure when I'd be able to dabble with it.

@radssh
Copy link
Contributor

radssh commented Jul 25, 2016

Got something ready to go, except for the fact that during testing, it turns out that OpenSSH server doesn't do anything with the signal request (other than logging that it received it).

Seems like they've been perpetually kicking that feature request out to next release for quite some time.
See: https://bugzilla.mindrot.org/show_bug.cgi?id=1424

@bitprophet
Copy link
Member

bitprophet commented Jul 25, 2016

Thanks for fielding this, @radssh! Agree with your analysis, at a glance.

@rustyscottweber Another option I can think of offhand is for you to wrap your remote processes in screen or tmux or similar, which allow you to label/name their sessions. This would presumably allow you to inject unique identifiers so you can tell them apart via an out-of-band kill (or use the screen-manager's own management mechanisms for terminating things). Food for thought.

Going to close this as "cantfix" for now but as always, more discussion is totally cool. Thanks for the report!

@radssh
Copy link
Contributor

radssh commented Aug 12, 2016

I've got three semi-related feature bits coded:

  • A Channel send_signal method to assemble & send the signal message to the server (that OpenSSH won't currently do anything with). It might be advantageous to have this in place if/when OpenSSH gets updated to actually forward the signal to the subprocess.
  • A ServerInterface check_channel_signal_request method, which seems iffy since the want_reply bool should be False, so I'm not sure if this is meaningful or not
  • A Channel _handle_request addition for receiving exit-signal messages and capturing the terminating signal name, core_dumped flag, and (never populated by OpenSSH Server) message & language fields. Added new Channel attributes to hold the signal name and core_dumped flag, for lack of a convenient way to fold that info into the int currently used for exit_status. Open to suggestions on preferred alternatives on this, as it feels like there ought to be a cleaner way within Channel to determine whether the remote process terminated normally (with exit_status) or via signal (with exit-signal) as they appear to be mutually exclusive.

Let me know if you're okay with all this lumped into one PR, or if you prefer them split up, especially if there's a bunch of tinkering to do on the last part.

@bitprophet
Copy link
Member

I think one PR is probably OK, having any of those things merged without the other doesn't feel crazy useful & I'd feel compelled to track/merge/release them together anyways. Thanks!

@radssh
Copy link
Contributor

radssh commented Oct 2, 2018

Looks like OpenSSH code is prepping this for an upcoming release (at long last)

openssh/openssh-portable@cd98925

@rustyscottweber
Copy link
Author

Thank you for the update @radssh. It will probably be years before every outdated ssh server which just logs a message is phased out, but it's good progress and news. Thank you for putting in features as well.

@bra-fsn
Copy link

bra-fsn commented Mar 4, 2019

Did this actually make its way into paramiko?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants