Python doesn't exit with proper resultcode on SIGINT

[foom]

BPO	1054041
Nosy	@mwhudson, @loewis, @gpshead, @pitrou, @vstinner, @jwilk, @rnk, @akheron, @vadmium, @eryksun
PRs	bpo-1054041: Exit properly after an uncaught ^C. #11862 bpo-1054041: Re-init _Py_UnhandledKeyboardInterrupt before run. #11963 bpo-1054041: Add What's New docs. #11999
Files	issue1054041-sigint-exit-gps01.diff sig.py winsig.bat winsig.py

^{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 2019-02-16.21:08:03.486>
created_at = <Date 2004-10-25.20:27:22.000>
labels = ['interpreter-core', 'type-feature', '3.8']
title = "Python doesn't exit with proper resultcode on SIGINT"
updated_at = <Date 2019-02-23.18:43:51.714>
user = 'https://bugs.python.org/foom'

bugs.python.org fields:

activity = <Date 2019-02-23.18:43:51.714>
actor = 'gregory.p.smith'
assignee = 'gregory.p.smith'
closed = True
closed_date = <Date 2019-02-16.21:08:03.486>
closer = 'gregory.p.smith'
components = ['Interpreter Core']
creation = <Date 2004-10-25.20:27:22.000>
creator = 'foom'
dependencies = []
files = ['20251', '48138', '48141', '48142']
hgrepos = []
issue_num = 1054041
keywords = ['patch', 'needs review']
message_count = 23.0
messages = ['60590', '60591', '114385', '125287', '125593', '125596', '125605', '125624', '136416', '252910', '335481', '335482', '335485', '335488', '335516', '335517', '335556', '335577', '335721', '335723', '335927', '336024', '336399']
nosy_count = 12.0
nosy_names = ['mwh', 'loewis', 'gregory.p.smith', 'foom', 'pitrou', 'vstinner', 'jwilk', 'rnk', 'petri.lehtinen', 'martin.panter', 'SamB', 'eryksun']
pr_nums = ['11862', '11963', '11999']
priority = 'normal'
resolution = 'fixed'
stage = 'commit review'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue1054041'
versions = ['Python 3.8']

[foom]

If you kill a python process with SIGINT (e.g. control-c), it catches
the signal, and raises a KeyboardInterrupt. If the KeyboardInterrupt
propagates to the top level, python exits. However, it exits with a
result of 1, not a result of SIGINT. This means that if you run
python in a shell script, the script will not properly exit on C-c.

When exiting because of a KeyboardInterrupt, python ought to
reraise the SIGINT, as follows, so that the exit code is correct for
the caller:
signal(SIGINT, SIG_DFL);
kill(getpid(), SIGINT);

See also http://www.cons.org/cracauer/sigint.html for a more
detailed discussion on the topic.

[mwhudson]

Logged In: YES
user_id=6656

Feel like writing a patch?

[BreamoreBoy]

I'll close this in a couple of weeks unless someone wants it kept open.

[gpshead]

Here's a patch that implements this behavior. It is too late in the 3.2 beta/rc cycle to get this into 3.2. Consider it for 3.3. I'd like a review.

[rnk]

Looks good to me. Do you need the TODO(gps)'s in there after implementing the behavior described?

[pitrou]

• should KeyboardInterrupt always exit with SIGINT, or only if it was actually raised by a signal handler?
• if _Py_UnhandledKeyboardInterrupt is defined in Modules/main.c but used in Python/pythonrun.c, can't it cause linker errors when embedding Python?
• please use PEP-8 (testKeyboardInterruptSignalExit -> test_some_long_name)

[loewis]

I wonder whether there is a precedent of some system mapping SIGINT to
an exception. We could probably learn something from them.

• should KeyboardInterrupt always exit with SIGINT, or only if it was
  actually raised by a signal handler?

IMO, if we give the illusion that the interpreter was actually killed,
we should equate KeyboardInterrupt with SIGINT; any uncaught
KeyboardInterrupt should consequently always lead to raising SIGINT.

[gpshead]

IMO, if we give the illusion that the interpreter was actually killed,
we should equate KeyboardInterrupt with SIGINT; any uncaught
KeyboardInterrupt should consequently always lead to raising SIGINT.

Agreed. Plus that is easier to implement and what I did.

I'll remove the left over TODO(gps) comments (oops) before this is
ever committed. I'm waiting until after 3.2 is released unless the
release manager jumps in and says otherwise. remaining items:

1. I need to add a second test case that writes the code to a file
   and launches a subprocess executing that file instead of using -c
   given that they are different code paths that each need testing. For
   variety I'll probably make that one send an actual SIGINT to the child
   process rather than having it raise KeyboardInterrupt.

2. The tests probably needs a decorator to limit their execution to posix.

3. Do the signal and kill calls also need to be conditional based on
   platform or is the function I put them in already posix-only? If
   necessary I'll protect them with #ifdefs so they don't break a windows
   build.

[vstinner]

+ kill(getpid(), SIGINT);

kill() doesn't exist on Windows: use raise() which is more portable and doesn't require a PID argument.

We may need to do something on Windows for console applications: see SetConsoleCtrlHandler(),
http://msdn.microsoft.com/en-us/library/ms686016(v=vs.85).aspx

+ self.assertEqual(returncode, -signal.SIGINT,
+ "not a SIGINT exit code. process stderr:\n%s" % stderr)

I don't think that such test can pass on Windows.

[vadmium]

This covers the same ground as bpo-14229, which was rejected with the argument that re-raising the signal is “ugly” and bypasses some things that are done by the standard OS exit() function.

[gpshead]

Taking a renewed look at this 8 years later... I agree, re-triggering the signal with the SIG_DFL handler would prevent us from doing the existing interpreter shutdown cleanup if we did it too early which would be a behavior change other than the exit value correction.

So we should delay the re-signaling kill(getpid(), SIGINT) call until we've completed that and are about to exit anyways.

The code has moved around a lot since i generated this patch on a 3.2-ish tree so it'll take me a while to untangle what would need to go where to create a PR.

Instead of kill(getpid(), SIGINT) or raise(SIGINT), we could just checking the _UnhandledKeyboardInterrupt flag we return our exit valye adjusting it to be the one a calling shell may be expecting to indicate a SIGINT. That goes against the advice of https://www.cons.org/cracauer/sigint.html but is likely to work.

A question that leads to is what _is_ the correct value. On Linux the magic 130 happens to be (SIGINT + 128). Triggering the libc or kernel supplied SIG_DFL handler as a final act avoids us ever needing to know what possible mechanisms to indicate this to the parent process are preferred on a platform. (if we know it is merely an exit value we could capture that in to a #define with a configure.ac check if the + 128 trick were deemed too magical despite being what everyone likely implements, standardized or not)

[vstinner]

A question that leads to is what _is_ the correct value.

What do you think of using a short test program (ex: uses raise(SIGINT)) in ./configure to get the "default exit code" to define a constant, and then use the constant for exit() in Python?

I dislike the idea of raising signals in general. The exact behavior of signals depends too much on the OS, it's hard to get it right. But having a configurable exit code looks safe and simple enough.

[gpshead]

I expect that'll work as desired and avoids the re-raising of the signal.

Unless told otherwise I assume this should be a POSIX specific platform behavior. ie: no return value alteration due to an uncaught KeyboardInterrupt on the Windows API build.

[eryksun]

For Windows, the default console control handler calls ExitProcess(STATUS_CONTROL_C_EXIT). If CMD is waiting on an application that exits with STATUS_CONTROL_C_EXIT, it prints "^C" to indicate the process was killed by Ctrl+C. For example:

    >>> STATUS_CONTROL_C_EXIT = 0xC000013A - 2**32
    >>> STATUS_CONTROL_C_EXIT
    -1073741510
    >>> sys.exit(STATUS_CONTROL_C_EXIT)
    ^C

C:\>echo %errorlevel%
-1073741510

Note that switching to SIG_DFL with raise(SIGINT) does not invoke the default console control handler in Windows. It just invokes the default raise() behavior, which is to call _exit(3). This exit status value of 3 is arbitrary and meaningless.

[jwilk]

This issue was reported because with the current behavior, "the script will not properly exit on C-c". Exiting with code 128+SIGINT will not fix this.

Please re-raise the signal.

[pitrou]

I'm assuming the calling shell uses waitpid() and then WIFSIGNALED()?

[gpshead]

jwilk: Confirmed. The exit code is not enough, we must trigger the SIG_DFL handler.

to reproduce:

while true; do ./sig.py ; done

with the attached sig.py.

pass a parameter to sig.py to have it exit 130 instead of triggering SIG_DFL...

[eryksun]

Gregory's last example reminded me that CMD checks for STATUS_CONTROL_C_EXIT for more than simply printing "^C". It also breaks out of a FOR loop when interactive and prompts to continue when executing a batch script.

Normally CMD also gets a console control event when the user presses Ctrl+C, so it knows about the Ctrl+C regardless of the child's exit status. One exception is when we start a process with a new console via CMD's start command. In this case CMD doesn't get a Ctrl+C event, since it's attached to a different console. Another exception is a simulated keyboard interrupt (e.g. from C raise SIGINT or Python _thread.interrupt_main). In these cases, CMD depends on the exit status value to determine whether the process was terminated by the default Ctrl+C handler. I've demonstrated this in the files winsig.bat and winsig.py. Put both in the same directory and run winsig.bat.

[gpshead]

New changeset 38f11cc by Gregory P. Smith in branch 'master':
bpo-1054041: Exit properly after an uncaught ^C. (bpo-11862)
38f11cc

[gpshead]

This is in for 3.8.

On earlier or unpatched Python versions: application owners have a workaround f they do not mind skipping a clean application shutdown (destructors) on posix platforms:
catch KeyboardInterrupt, reset SIGINT to SIG_DFL, kill(getpid(), SIGINT). If you somehow need the python destructor cleanup (never guaranteed, so unwise to _depend_ on it) you could do that in a C atexit handler.

On Windows the workaround is easier without altering clean shutdown, catch KeyboardInterrupt and sys.exit(0xC000013A - 2**32).

[vstinner]

if (PyOS_setsig(SIGINT, SIG_DFL) == SIG_ERR) {
perror("signal"); /* Impossible in normal environments. */

In my experience, Python is were impossible things happen. Would it be possible to write a better error message to explain what happened?

I expect that if PyOS_setsig() fails, the user cannot do much for that. It may be a bug in the libc and so cannot be fixed easily. Maybe we should simply ignore the error?

[gpshead]

i'm curious if this _ever_ comes up so i'd like to leave it in at least through some betas. I don't believe it should be possible. If it is, it'd probably be a restricted execution environment that fails all signal() calls. all perror will do is emit an error message to that effect before continuing so we still exit with an error code as the final fallback.

[gpshead]

New changeset 06babb2 by Gregory P. Smith in branch 'master':
bpo-1054041: Add What's New docs. (GH-11999)
06babb2