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

Terminate Windows processes with SIGTERM code rather than 0 #1150

Merged
merged 1 commit into from
Oct 20, 2017

Conversation

akosthekiss
Copy link
Contributor

If the TerminateProcess WinAPI function is called with 0, then the
exit code of the terminated process (e.g., observed by its parent)
will be 0. However, this is usually associated with successful
execution. Any other exit code could be used to signal forced
termination, but perhaps the value of SIGTERM is the most
meaningful.

If the TerminateProcess WinAPI function is called with 0, then the
exit code of the terminated process (e.g., observed by its parent)
will be 0. However, this is usually associated with successful
execution. Any other exit code could be used to signal forced
termination, but perhaps the value of SIGTERM is the most
meaningful.
@akosthekiss
Copy link
Contributor Author

A little background to this PR (it's a bit long, sorry). Based on (c)python 3 and psutil 5.4.0, I've conducted a little experiment. There are various ways of terminating a process in Python, and I wanted to see what can be observed by the parent afterwards. First, here is the script I used:

import os
import psutil
import shlex
import signal
import subprocess
import sys
import time

def kill_with_os(proc):
    print('kill with os.kill(pid,SIGTERM)')
    os.kill(proc.pid, signal.SIGTERM)

def kill_with_subprocess(proc):
    print('kill child with subprocess.Popen.terminate()')
    proc.terminate()

def kill_with_psutil(proc):
    print('kill child with psutil.Process(pid).terminate()')
    psutil.Process(proc.pid).terminate()

def do_not_kill(proc):
    pass

def run_and_kill_child(killfn, procarg):
    print('run child with %s' % procarg)
    with subprocess.Popen([sys.executable, __file__, procarg]) as proc:
        time.sleep(1)
        killfn(proc)
        proc.wait()
    print('child terminated with %s' % proc.returncode)

if __name__ == '__main__':
    if len(sys.argv) < 2:
        print('parent process started')
        run_and_kill_child(kill_with_os, 'os')
        run_and_kill_child(kill_with_subprocess, 'subprocess')
        run_and_kill_child(kill_with_psutil, 'psutil')
        run_and_kill_child(do_not_kill, 'return')
    else:
        print('child process started with %s' % sys.argv[1])
        while sys.argv[1] != 'return':
            pass

So, it uses kill from the os module, terminate from subprocess, and terminate from psutil. The next listing is the output I got on macOS 10.12 (and I think I would get the same on Linux, too):

$ python3 killtest.py 
parent process started
run child with os
child process started with os
kill with os.kill(pid,SIGTERM)
child terminated with -15
run child with subprocess
child process started with subprocess
kill child with subprocess.Popen.terminate()
child terminated with -15
run child with psutil
child process started with psutil
kill child with psutil.Process(pid).terminate()
child terminated with -15
run child with return
child process started with return
child terminated with 0

And this is what I got on Windows 10:

>py -3 killtest.py
parent process started
run child with os
child process started with os
kill with os.kill(pid,SIGTERM)
child terminated with 15
run child with subprocess
child process started with subprocess
kill child with subprocess.Popen.terminate()
child terminated with 1
run child with psutil
child process started with psutil
kill child with psutil.Process(pid).terminate()
child terminated with 0
run child with return
child process started with return
child terminated with 0

It can be seen that on a Posix platform, terminate calls are in sync with kill(,SIGTERM), i.e., exit code of the process is always -SIGTERM, and most importantly non-0. However, on Windows, the observable exit code after psutils's terminate differs both from kill(,SIGTERM) and from subprocess's terminate, and quite confusingly, yields 0, which normally signals the end of successful process execution.

Note: of course, it cannot be expected on Windows to get -SIGTERM as exit code, the platform is simply not capable of a negative exit code. But then, +SIGTERM seems to be an acceptable alternative.

Note 2: the approach of the standard subprocess module - i.e., it terminates the process with exit code 1 - is a bit questionable to me, but at least it does not yield 0.

@giampaolo
Copy link
Owner

This seems reasonable. What happens if you os.kill(proc.pid, signal.SIGKILL)? Does the exit code change in accordance? If so then I think it's better for psutil to do the same. That means psutil_proc_kill function should accept an extra sig parameter.

@akosthekiss
Copy link
Contributor Author

On Windows, even at C level, only a handful of signal constants are available: SIGABRT, SIGFPE, SIGILL, SIGINT, SIGSEGV, and SIGTERM; SIGKILL is not among them. (https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/signal#remarks)

Thus, the signal module on Windows does not contain SIGKILL either, only SIGTERM.

>py -3
Python 3.6.3 (v3.6.3:2c5fed8, Oct  3 2017, 18:11:49) [MSC v.1900 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import signal
>>> signal.SIGTERM
<Signals.SIGTERM: 15>
>>> signal.SIGKILL
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: module 'signal' has no attribute 'SIGKILL'

And the doc of subprocess.Popen.kill also declares that "[o]n Windows kill() is an alias for terminate()". (https://docs.python.org/3/library/subprocess.html#subprocess.Popen.kill)

@giampaolo
Copy link
Owner

OK, thanks for looking into this. Merging.

@giampaolo giampaolo merged commit 548656a into giampaolo:master Oct 20, 2017
@akosthekiss akosthekiss deleted the windows-sigterm branch October 20, 2017 07:53
giampaolo added a commit that referenced this pull request Oct 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants