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

fix: ctrl-c now runs shutdown routines #1381

Merged
merged 1 commit into from
Dec 20, 2018
Merged

Conversation

Hanicef
Copy link
Contributor

@Hanicef Hanicef commented Sep 20, 2018

I added SIGINT to the signal handler in __init__.py, which is the signal generated by Ctrl-C.
This fixes #1370 .

@dgw dgw added Bug Things to squish; generally used for issues Low Priority labels Sep 20, 2018
@dgw dgw added this to the 6.6.0 milestone Sep 20, 2018
@dgw
Copy link
Member

dgw commented Sep 20, 2018

Thanks for this, works for me on Linux. I'm sure macOS will work fine, too, because it's a Unix-like—but I'll see if I can find an opportunity to test on Windows too for good measure.

I remember toying with this exact approach, and thought I had committed it in a branch somewhere… Can't find mine, maybe I tossed it to work on something else.

@Hanicef
Copy link
Contributor Author

Hanicef commented Sep 20, 2018

No problem. I really wanted to just fix this bug as it was bugging me (no pun intended) that the shutdown routines didn't run.

Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out Ctrl-C doesn't seem to work on Windows at all. Pressing it throws an AttributeError because signal.SIGUSR1 is missing. Tested on Win7, the newest Windows version I have. Test results from anyone running Windows 8 or 10 would be welcome, I guess, but git blame tells me Ctrl-C has been broken on Windows for at least five years (the length of time since the erroring code was last changed).

Basically, I won't lose any sleep over merging this with a pre-existing bug included. 😹 I'll leave it open a bit in case anyone else comes along with test results from newer Windows (or I find an opportunity to test one/both myself), but no matter what this PR is getting approved.

@dgw dgw merged commit ef78d08 into sopel-irc:master Dec 20, 2018
@dgw dgw added Bugfix Generally, PRs that reference (and fix) one or more issue(s) and removed Bug Things to squish; generally used for issues labels Jan 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix Generally, PRs that reference (and fix) one or more issue(s) Low Priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ctrl-C doesn't run shutdown routines
2 participants