classification
Title: subprocess.Popen.send_signal doesn't check whether the process has terminated
Type: behavior Stage: needs patch
Components: Library (Lib) Versions: Python 3.1, Python 2.6
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: astrand, brian.curtin, ezio.melotti, giampaolo.rodola, markon, milko.krachounov
Priority: normal Keywords: patch

Created on 2009-09-22 22:47 by milko.krachounov, last changed 2010-06-09 19:48 by giampaolo.rodola.

Files
File name Uploaded Description Edit
test_subprocess.patch markon, 2009-10-15 22:09 Provided patch for test_subprocess.py in trunk/
subprocess.patch markon, 2009-10-15 22:10 Provided patch for subprocess.py in trunk/
Messages (3)
msg93022 - (view) Author: Milko Krachounov (milko.krachounov) Date: 2009-09-22 22:47
When subprocess.Popen.send_signal is called, it simply calls
os.kill(self.pid, ...) without checking whether the child has already
terminated. If the child has been terminated, and Popen.wait() or
Popen.poll() have been called, a process with PID self.pid no longer
exists, and what's worse, could belong to a totally unrelated process.

A better behavior would be to raise an exception when self.returncode is
not None. Alternatively, self.pid might be set to None after the process
has been terminated, as it is no longer meaningful. Or to another value
that would be invalid pid and invalid argument to os.kill and os.wait,
but unlike None would still evaluate to True.
msg94091 - (view) Author: Marco Buccini (markon) Date: 2009-10-15 15:04
I agree with Milko. 

However, I think Popen.send_signal should poll() before sending any
signals, without resetting any variables to zero (or None).
In this way, if you poll() before sending a signal, if the return code
is None, the child is still running. If the poll return code is
different than None, the child has been terminated, so you must not send
anything to the child process.

In this way, instead of polling before sending signals to check if the
child has been terminated, we can make Python do the work for us.

I've provided a patch. All the tests pass.
A new test has been added: test_terminate_already_terminated_child.
This method asserts `terminate()` (through `send_signal`) raises an
OSError exception. 

However, even though you try the old version of subprocess.py all the
tests pass. This happens because the method `send_signal` calls
os.kill() (or Terminate for Windows systems) that can raise an OSError
exception if the process to which it send the signal does not exist. 
`send_signal` has been fixed to be a safer method, so that it raises an
OSError exception if the child has been terminated.
msg107411 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) (Python committer) Date: 2010-06-09 19:48
I totally agree this should be fixed (there was a similar issue for psutil: http://code.google.com/p/psutil/issues/detail?id=58) but communicate() and wait() should be affected in the same manner.

Probably it would make sense to add an is_running() method and decorate send_signal(), kill(), communicate() and wait() methods with a function which makes sure that the process is still running, otherwise raises an exception instead.

Maybe it would also makes sense to provide a brand new NoSuchProcess exception class.
History
Date User Action Args
2010-06-09 19:48:47giampaolo.rodolasetnosy: + brian.curtin, giampaolo.rodola, astrand
messages: + msg107411
2009-10-15 22:10:20markonsetfiles: + subprocess.patch
2009-10-15 22:09:58markonsetfiles: + test_subprocess.patch
2009-10-15 22:09:24markonsetfiles: - test_subprocess.patch
2009-10-15 22:09:21markonsetfiles: - subprocess.patch
2009-10-15 21:31:40markonsetfiles: + subprocess.patch
2009-10-15 21:31:18markonsetfiles: - subprocess.patch
2009-10-15 15:08:13markonsetfiles: + test_subprocess.patch
2009-10-15 15:07:29markonsetfiles: - test_subprocess.patch
2009-10-15 15:05:16markonsetfiles: + test_subprocess.patch
2009-10-15 15:04:35markonsetfiles: + subprocess.patch

nosy: + markon
messages: + msg94091

keywords: + patch
2009-10-15 11:28:18ezio.melottisetpriority: normal
nosy: + ezio.melotti

stage: needs patch
2009-09-22 22:47:47milko.krachounovcreate