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

Python doesn't exit with proper resultcode on SIGINT #41078

Closed
foom mannequin opened this issue Oct 25, 2004 · 23 comments
Closed

Python doesn't exit with proper resultcode on SIGINT #41078

foom mannequin opened this issue Oct 25, 2004 · 23 comments
Assignees
Labels
3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@foom
Copy link
Mannequin

foom mannequin commented Oct 25, 2004

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
    Copy link
    Mannequin Author

    foom mannequin commented Oct 25, 2004

    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.

    @foom foom mannequin added interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Oct 25, 2004
    @mwhudson
    Copy link

    mwhudson commented Apr 7, 2005

    Logged In: YES
    user_id=6656

    Feel like writing a patch?

    @devdanzin devdanzin mannequin added type-feature A feature request or enhancement labels Feb 14, 2009
    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Aug 19, 2010

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

    @BreamoreBoy BreamoreBoy mannequin added type-bug An unexpected behavior, bug, or error and removed type-feature A feature request or enhancement labels Aug 19, 2010
    @pitrou pitrou added type-feature A feature request or enhancement and removed type-bug An unexpected behavior, bug, or error labels Jan 4, 2011
    @gpshead
    Copy link
    Member

    gpshead commented Jan 4, 2011

    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
    Copy link
    Mannequin

    rnk mannequin commented Jan 6, 2011

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

    @pitrou
    Copy link
    Member

    pitrou commented Jan 6, 2011

    • 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
    Copy link
    Mannequin

    loewis mannequin commented Jan 6, 2011

    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
    Copy link
    Member

    gpshead commented Jan 7, 2011

    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
    Copy link
    Member

    + 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
    Copy link
    Member

    vadmium commented Oct 13, 2015

    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 gpshead added 3.8 only security fixes labels Feb 13, 2019
    @gpshead
    Copy link
    Member

    gpshead commented Feb 13, 2019

    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
    Copy link
    Member

    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
    Copy link
    Member

    gpshead commented Feb 14, 2019

    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
    Copy link
    Contributor

    eryksun commented Feb 14, 2019

    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
    Copy link
    Mannequin

    jwilk mannequin commented Feb 14, 2019

    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
    Copy link
    Member

    pitrou commented Feb 14, 2019

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

    @gpshead
    Copy link
    Member

    gpshead commented Feb 14, 2019

    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
    Copy link
    Contributor

    eryksun commented Feb 15, 2019

    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
    Copy link
    Member

    gpshead commented Feb 16, 2019

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

    @gpshead
    Copy link
    Member

    gpshead commented Feb 16, 2019

    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).

    @gpshead gpshead closed this as completed Feb 16, 2019
    @gpshead gpshead self-assigned this Feb 16, 2019
    @gpshead gpshead closed this as completed Feb 16, 2019
    @gpshead gpshead self-assigned this Feb 16, 2019
    @vstinner
    Copy link
    Member

    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
    Copy link
    Member

    gpshead commented Feb 19, 2019

    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
    Copy link
    Member

    gpshead commented Feb 23, 2019

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

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 9, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants