subprocess.Popen.send_signal doesn't check whether the process has terminated

[milkokrachounov]

BPO	6973
Nosy	@gpshead, @giampaolo, @ezio-melotti, @briancurtin, @vadmium
Files	test_subprocess.patch: Provided patch for test_subprocess.py in trunk/ subprocess.patch: Provided patch for subprocess.py in trunk/

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/gpshead'
closed_at = <Date 2015-11-16.02:35:11.081>
created_at = <Date 2009-09-22.22:47:47.322>
labels = ['type-bug', 'library']
title = "subprocess.Popen.send_signal doesn't check whether the process has terminated"
updated_at = <Date 2015-11-16.02:35:11.079>
user = 'https://bugs.python.org/milkokrachounov'

bugs.python.org fields:

activity = <Date 2015-11-16.02:35:11.079>
actor = 'gregory.p.smith'
assignee = 'gregory.p.smith'
closed = True
closed_date = <Date 2015-11-16.02:35:11.081>
closer = 'gregory.p.smith'
components = ['Library (Lib)']
creation = <Date 2009-09-22.22:47:47.322>
creator = 'milko.krachounov'
dependencies = []
files = ['15142', '15143']
hgrepos = []
issue_num = 6973
keywords = ['patch']
message_count = 6.0
messages = ['93022', '94091', '107411', '225483', '254704', '254705']
nosy_count = 9.0
nosy_names = ['gregory.p.smith', 'astrand', 'giampaolo.rodola', 'ezio.melotti', 'brian.curtin', 'milko.krachounov', 'tshepang', 'python-dev', 'martin.panter']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'needs patch'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue6973'
versions = ['Python 2.7', 'Python 3.4', 'Python 3.5']

[milkokrachounov]

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.

[markon]

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.

[giampaolo]

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.

[vadmium]

It seems to me that raising OSError isn’t quite right. Either the error is more of an internal programmer error, like how writing to a closed file object raises ValueError, or there should not be an error, like when the process has terminated but has not been reaped, or when closing an already-closed file.

[python-dev]

New changeset 6bd3c8623bb2 by Gregory P. Smith in branch '3.4':
Fix issue bpo-6973: When we know a subprocess.Popen process has died, do
https://hg.python.org/cpython/rev/6bd3c8623bb2

New changeset bc907c76f054 by Gregory P. Smith in branch '3.5':
Fix issue bpo-6973: When we know a subprocess.Popen process has died, do
https://hg.python.org/cpython/rev/bc907c76f054

New changeset e51c46d12ec1 by Gregory P. Smith in branch 'default':
Fix issue bpo-6973: When we know a subprocess.Popen process has died, do
https://hg.python.org/cpython/rev/e51c46d12ec1

[gpshead]

I chose to go with the "there should not be an error" approach similar to how Python deals with multiple file closes. Both are technically programming errors but the point of Python is make life easier.

Reasoning? I don't want someone to ever think they could depend on getting an exception from send_signal/terminate/kill in this "process has already died and someone has called wait or poll to get its return code" situation as that is unreliable and potentially impacts other innocent processes.

[anki-code]

Hello! Sorry for necro but I'm working on PR for xonsh shell and I need your help.

In xonsh we have a call where from Thread we call Popen with shell command. And in some cases command can stop working immediately after running (e.g. interactive tool ask input but there is no tty). During tracing I see that os.kill, os.kill(popen.pid, 0) signal.signal, signal.pthread_kill have no any exceptions in this case and I can't check that pid is alive to avoid block the execution on os.waitpid. Also I have zero motivation to use psutil for checking pid and add this dependency to the project.

How to check that pid is alive? The solution from stackoverflow about os.kill(pid, 0) is not working for my case.

[giampaolo]

On POSIX you may use this (note: that may return True if PID is a TID (thread ID)):
https://github.com/giampaolo/psutil/blob/d51706d06208d30a2995a31f5b41b6cc2ab7a7fe/psutil/_psposix.py#L40-L59

That won't check whether PID has been reused by another process though, because os.kill() is inherently racy, but I guess you probably don't need that level of security anyway. If you do, you can either:

• use psutil (psutil.Process(PID).is_running())
• somehow integrate os.pidfd_open() + os.pidfd_send_signal() into subprocess.Popen, so that internally it uses os.pidfd_send_signal() instead of os.kill()

[anki-code]

@giampaolo thanks for the answer! I will try!