-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
fix:Before killing a process under high concurrency, it is necessary … #2261
base: main
Are you sure you want to change the base?
Conversation
…to check if the process exists.
paramiko/proxy.py
Outdated
self.process.stdin.close() | ||
self.process.stdout.close() | ||
self.process.stderr.close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there other errors that could be raised during these stream closure actions? If anything other than a ProcessLookupError
occurs, then the except
won't catch correctly.
If I read the docs right, close()
is idempotent and won't raise on an already-closed stream... so, there may not actually be any additional errors to worry about?
If so, augmenting your comment here to note that close()
won't raise even if the streams are already closed would be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
close()
is redundant, I have deleted it.
But it is necessary to check whether a process is still present before killing it.
Because when I use distributed task queues Celery, this process is usually killed by the system.
It is possible to report an error ProcessLookupError
.
In short, it is necessary to check whether a process is still alive before killing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry, I think I expressed myself poorly -- I wasn't requesting that you change your implementation, only that you add some comments &c. to document it more thoroughly.
Although, if that if self.closed:
check will do just as well as the full set of stream .close()
calls and the try
/except
, that's certainly much cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to close IO as it will kill directly
paramiko/proxy.py
Outdated
if not self.closed: | ||
os.kill(self.process.pid, signal.SIGTERM) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use Popen.terminate()
simply, in order to terminate the process if alive.
See
- posix - https://github.com/python/cpython/blob/v3.6.15/Lib/subprocess.py#L1596-L1605
- nt - https://github.com/python/cpython/blob/v3.6.15/Lib/subprocess.py#L1153-L1166
self.process.terminate()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your guidance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nuonuonn Thanks for the explain.
The issue has been fixed in Python 3.10.0, python/cpython@01a202a
However, the issue occurs for the version or early. We could change like the following:
diff --git a/paramiko/proxy.py b/paramiko/proxy.py
index f7609c984..111e91627 100644
--- a/paramiko/proxy.py
+++ b/paramiko/proxy.py
@@ -119,7 +119,13 @@ class ProxyCommand(ClosingContextManager):
raise ProxyCommandFailure(" ".join(self.cmd), e.strerror)
def close(self):
- os.kill(self.process.pid, signal.SIGTERM)
+ try:
+ self.process.terminate()
+ except ProcessLookupError:
+ pass # Suppress the error if the process is not existent
+ # Close pipes and avoid zombie process
+ with self.process:
+ pass
@property
def closed(self):
The
I think that we should invoke See https://docs.python.org/3/library/subprocess.html?highlight=Added%20context%20manager%20support |
I have questions. In this situation, what have you encountered issues? It is able to send signals to the process until calling >>> import os, time
>>> from subprocess import Popen, PIPE
>>> proc = Popen(['sleep', '1'], stdin=PIPE, stdout=PIPE, stderr=PIPE) # the process terminates in 1 second
>>> proc.returncode is None
True
>>> proc.pid
3657305
>>> time.sleep(3)
>>> os.kill(proc.pid, 0)
>>> proc.terminate()
>>> proc.returncode is None
False
>>> proc.wait()
0
>>> proc.returncode is None
False
>>> proc.returncode
0
>>> os.kill(proc.pid, 0)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ProcessLookupError: [Errno 3] No such process
>>> proc.terminate() # do nothing because proc.returncode is already set
>>> Also, See https://docs.python.org/3/library/subprocess.html#subprocess.Popen.returncode |
@jun66j5
The subprocess has been killed by the operating system. But in paramiko (proxy.py)
So there was an error |
…to check if the process exists.
In high concurrency or asynchronous situations, the ProxyCommand process may have been killed by other means.