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

bpo-1054041: Exit properly after an uncaught ^C. #11862

Merged
merged 19 commits into from
Feb 16, 2019

Conversation

gpshead
Copy link
Member

@gpshead gpshead commented Feb 15, 2019

An uncaught KeyboardInterrupt exception means the user pressed ^C and
our code did not handle it. Programs that install SIGINT handlers are
supposed to reraise the SIGINT signal to the SIG_DFL handler in order
to exit in a manner that their calling process can detect that they
died due to a Ctrl-C. https://www.cons.org/cracauer/sigint.html

After this change on POSIX systems

while true; do python -c 'import time; time.sleep(23)'; done

can be stopped via a simple Ctrl-C instead of the shell infinitely
restarting a new python process.

https://bugs.python.org/issue1054041

An uncaught KeyboardInterrupt exception means the user pressed ^C and
our code did not handle it.  Programs that install SIGINT handlers are
supposed to reraise the SIGINT signal to the SIG_DFL handler in order
to exit in a manner that their calling process can detect that they
died due to a Ctrl-C.  https://www.cons.org/cracauer/sigint.html

After this change on POSIX systems

 while true; do python -c 'import time; time.sleep(23)'; done

can be stopped via a simple Ctrl-C instead of the shell infinitely
restarting a new python process.

What to do on Windows, or if anything needs to be done there has not
yet been determined.  That belongs in its own PR.

TODO(gpshead): A unittest for this behavior is still needed.
@gpshead gpshead added type-bug An unexpected behavior, bug, or error type-feature A feature request or enhancement awaiting changes labels Feb 15, 2019
@gpshead
Copy link
Member Author

gpshead commented Feb 15, 2019

A proper unittest is needed, along with the news entry.

Windows specific code should go in a followup PR (if better behavior is meaningful there).

@jdemeyer
Copy link
Contributor

I like this idea in principle. If you replace kill(getpid(), SIGINT) by raise(SIGINT) it might actually work on Windows too.

@eryksun
Copy link
Contributor

eryksun commented Feb 15, 2019

I like this idea in principle. If you replace kill(getpid(), SIGINT) by raise(SIGINT) it might actually work on Windows too.

In Windows the default behavior for C raise is simply _exit(3), which is a meaningless exit status value. The correct course of action in Windows is to exit with STATUS_CONTROL_C_EXIT (0xC000013A), which is what the default console control handler does. The CMD shell has special-cased behavior for this status value. I covered this in more detail on the tracker.

@gpshead
Copy link
Member Author

gpshead commented Feb 16, 2019

Trivial to implement that status return for windows, added to this PR. waiting for buildbots to double check compilation. I still need to add unittests and news.

@gpshead gpshead self-assigned this Feb 16, 2019
@gpshead gpshead changed the title bpo-1054041: Exit properly by a signal after a ^C. bpo-1054041: Exit properly after an uncaught ^C. Feb 16, 2019
@gpshead
Copy link
Member Author

gpshead commented Feb 16, 2019

Things appear to be working, but the unittest checking the behavior of bash fails on macOS. This is probably due to macOS's very old version of bash not enabling the SIGINT detecting behavior under "bash -i".

TODO: A bash version check and skip could be added.

@gpshead gpshead merged commit 38f11cc into python:master Feb 16, 2019
@bedevere-bot
Copy link

@gpshead: Please replace # with GH- in the commit message next time. Thanks!

@gpshead
Copy link
Member Author

gpshead commented Feb 16, 2019

something very strange happened on Github here. it merged my commit with the unedited subject and commit message. I was manually fixing those up and putting in a nice clean message, clicked the button and github rejected it and refreshed to show me it was merged with the mess of messages you see. :(

@gpshead gpshead deleted the sigint_issue1054041 branch February 16, 2019 20:59
@pablogsal
Copy link
Member

There is a buildbot failure that seem related to this PR:

https://bugs.python.org/issue36013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants