Skip to content

bpo-30654: Do not reset SIGINT handler to SIG_DFL in finisignal #2162

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

Closed
wants to merge 1 commit into from
Closed

bpo-30654: Do not reset SIGINT handler to SIG_DFL in finisignal #2162

wants to merge 1 commit into from

Conversation

yol
Copy link

@yol yol commented Jun 13, 2017

Instead of saving old_siginthandler which will always be SIG_DFL, just use the normal reset code. This means that SIGINT will not be reset to SIG_DFL if it had a custom C handler during initialization, since func will be Py_None then.

I'm not sure if this is the intended behavior. But the prior behavior was broken, so it's an improvement.

https://bugs.python.org/issue30654

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

@pitrou pitrou added needs backport to 2.7 type-bug An unexpected behavior, bug, or error labels Jun 13, 2017
@pitrou
Copy link
Member

pitrou commented Jun 13, 2017

Thank you @pkerling for submitting this patch. The fix looks sane to me.
You'll have to sign the contributor agreement before this proceeds, though.
Also, you'll have to add a Misc/NEWS entry, or you can let a core developer do it for you.

@yol
Copy link
Author

yol commented Jun 15, 2017

Tried to add a NEWS entry.

@yol
Copy link
Author

yol commented Dec 2, 2017

Hi, what's the status? Do I still need to do something? This is a small fix and it's sitting here for quite some time now.

@yol
Copy link
Author

yol commented May 27, 2018

Friendly ping :-)

@pitrou
Copy link
Member

pitrou commented May 27, 2018

Hi @pkerling

I'm sorry for letting this slip. In turn there are two issues:

  • I hadn't noticed, but you submitted your PR against the 2.7 branch; instead, you should submit it against the default branch and we'll backport it ourselves
  • in the meantime, we switched from editing Misc/NEWS directly to using a dedicated tool that avoids merge conflicts: this means you must not change Misc/NEWS but use the blurb utility instead which will generate a separate self-contained file for this issue.

@yol
Copy link
Author

yol commented May 28, 2018

Superseded by #7146

@yol yol closed this May 28, 2018
@yol yol deleted the fix-sigint branch May 28, 2018 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants