classification
Title: signal module always overwrites SIGINT on interpreter shutdown
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.8, Python 3.7, Python 3.6, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: benjamin.peterson, ned.deily, pitrou, pkerling, vstinner
Priority: normal Keywords: patch

Created on 2017-06-13 13:06 by pkerling, last changed 2018-06-03 19:11 by pitrou. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 2162 closed python-dev, 2017-06-13 13:12
PR 7146 merged pkerling, 2018-05-28 10:34
PR 7306 merged miss-islington, 2018-06-01 09:48
PR 7307 merged miss-islington, 2018-06-01 09:49
PR 7347 merged pitrou, 2018-06-02 20:11
Messages (20)
msg295915 - (view) Author: (pkerling) * Date: 2017-06-13 13:06
The signal module checks the SIGINT handler on startup. It only registers a new custom handler if the default OS handler is still installed, so that when embedding python in an application the SIGINT handler of that application is not overwritten. 

But on shutdown in finisignal, it *always* sets SIGINT to SIG_DFL. The reason is that it saves the old handler in old_siginthandler, but *only* if the signal handler is overwritten in init, which only happens when it was SIG_DFL in the first place! If there was already a handler in place in init (-> no overwriting), old_siginthandler will default to the initialization value, which is also SIG_DFL.

This means that when an application embeds Python and needs a custom SIGINT handler, it will stop to work as soon as it shuts down Python since it will always be reset to SIG_DFL.
msg318377 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-06-01 09:47
New changeset e905c84494526363086f66a979e317e155bf9536 by Antoine Pitrou (pkerling) in branch 'master':
bpo-30654: Do not reset SIGINT handler to SIG_DFL in finisignal (GH-7146)
https://github.com/python/cpython/commit/e905c84494526363086f66a979e317e155bf9536
msg318378 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-06-01 09:50
I'd rather not backport this to 2.7 as it's quite late in the maintenance cycle and I'd like to avoid any regressions there.
msg318379 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-06-01 09:53
> I'd rather not backport this to 2.7 as it's quite late in the maintenance cycle and I'd like to avoid any regressions there.

Since it's a subtle behavior change and the PR doesn't add a flag to opt-in for the old behaviour, I'm not sure about backporting the change (to 3.6 or 3.7).

For 3.7, we are very close to the final release. It doesn't give much time to users to test the new behavior :-( I would suggest to keep the old behavior in Python 3.7 as well.

To be honest, I don't understand well the change. So I'm not confortable with it.

I understand that Py_Initialize() + Py_Finalize() restored the SIGINT handler, but now Python *always* sets SIGINT to SIG_DFL in Py_Finalize(). So if an application has its own signal handler, Python replaces it...

But embedded Python also gives the choice of not setting Python signal handlers in Py_Initialize().
msg318380 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-06-01 09:55
> So if an application has its own signal handler, Python replaces it...

You have it backwards.  Please read the bug report.
msg318381 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-06-01 09:56
> For 3.7, we are very close to the final release. It doesn't give much time to users to test the new behavior 

That's a fair point.  What's the procedure here?  If I backport the fix to the 3.7 branch, will it go straight into the 3.7.0 release or will it be deferred to 3.7.1?  @Ned
msg318382 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-06-01 10:08
> You have it backwards.  Please read the bug report.

I'm confused by the NEWS entry:

+Fixed reset of the SIGINT handler to SIG_DFL on interpreter shutdown even
+when there was a custom handler set previously. Patch by Philipp Kerling.

I read it as Python now always reset SIGINT to SIG_DFL. The commit title is more explicit: "Do not reset SIGINT (...)".

I propose to rephrase the NEWS entry as:

"""
Fix signal handlers when Python is embedded. On Python shutdown, do not reset the SIGINT handler to SIG_DFL, when a custom handler has been set before Python initialization. Patch by Philipp Kerling.
"""

--

Ok, now I understood the change. In this case, I'm ok to backport it to 3.6 and 3.7.

Python 2.7 has this behavior since 10 years, it seems like people learnt to live with it. I also dislike touching Python 2.7, to prevent any risk of regression if someone really rely on the current behavior.
msg318393 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2018-06-01 10:45
> What's the procedure here?  If I backport the fix to the 3.7 branch, will it go straight into the 3.7.0 release or will it be deferred to 3.7.1??

"All fixes that have been merged into the 3.7 branch as of cutoff tomorrow
will be in 3.7.0b5 and fixes merged afterwards will be in 3.7.0rc1 up to
its cutoff point. After 3.7.0rc1 cutoff, 3.7 merges will appear in 3.7.1.
Please continue to exercise diligence when deciding whether a change is
appropriate for 3.7; as a rule of thumb, treat the 3.7 branch as if it were
already released and in maintenance mode."
msg318394 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-06-01 10:49
Ok, I think it go into 3.7.0 after all.  3.7.1 wouldn't get more testing before it's released anyway.
msg318395 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-06-01 10:50
New changeset 623b439abebc913bc416d92f38fe371e84b0276b by Antoine Pitrou (Miss Islington (bot)) in branch '3.7':
bpo-30654: Do not reset SIGINT handler to SIG_DFL in finisignal (GH-7146) (GH-7306)
https://github.com/python/cpython/commit/623b439abebc913bc416d92f38fe371e84b0276b
msg318398 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-06-01 11:12
New changeset 1d5198fd41ad9185e9e6b3aa595769c3693d57be by Antoine Pitrou (Miss Islington (bot)) in branch '3.6':
bpo-30654: Do not reset SIGINT handler to SIG_DFL in finisignal (GH-7146) (GH-7307)
https://github.com/python/cpython/commit/1d5198fd41ad9185e9e6b3aa595769c3693d57be
msg318402 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-06-01 12:06
Thanks pkerling for your bug report and your fix!
msg318414 - (view) Author: (pkerling) * Date: 2018-06-01 13:31
Thanks! I'm a bit disappointed that it won't make it into 2.7, but I can understand the decision.

To give some context: I came across this while working on Kodi and noticing that it does not shutdown cleanly via Ctrl+C or SIGTERM. After investigating, I came to the conclusion that it is due to this bug. Kodi shuts down the Python interpreter every time no add-on is doing active work, which is almost guaranteed to happen shortly after application startup. Then this bug caused a reset of the SIGTERM handler to the default handler, making the Kodi handler that does a clean shutdown useless.
Now there are plans to switch to Python 3 in Kodi, but it won't happen until the major release after the next, so we're stuck with 2.7 for some time. I guess we'll have to work around this in Kodi for the time being.
msg318415 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-06-01 13:33
I'm nosy'ing Benjamin, the 2.7 release manager, in case he wants to comment on desirability of this bugfix in Python 2.7.
msg318484 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2018-06-02 03:12
It seems like this only affects embeddings and for embeddings, it's strictly an improvement?
msg318490 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-06-02 07:34
Short of any bug in the patch, yes, it's stricly an improvement.
msg318496 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-06-02 10:45
IMHO it's a bugfix.
msg318513 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-06-02 20:04
Reopening for the 2.7 backport discussion.
msg318577 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-06-03 18:46
New changeset ded666ff0ce44785a495ff89b676ca68e4cc08e8 by Antoine Pitrou in branch '2.7':
[2.7] bpo-30654: Do not reset SIGINT handler to SIG_DFL in finisignal (GH-7146) (GH-7347)
https://github.com/python/cpython/commit/ded666ff0ce44785a495ff89b676ca68e4cc08e8
msg318580 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-06-03 19:11
So this is in 2.7 finally.  Closing again.
History
Date User Action Args
2018-06-15 11:27:53martin.panterlinkissue24415 superseder
2018-06-03 19:11:15pitrousetstatus: open -> closed

stage: patch review -> resolved
messages: + msg318580
versions: + Python 2.7
2018-06-03 18:46:45pitrousetmessages: + msg318577
2018-06-02 20:11:54pitrousetstage: resolved -> patch review
pull_requests: + pull_request6973
2018-06-02 20:04:48pitrousetstatus: closed -> open

messages: + msg318513
2018-06-02 10:45:59vstinnersetmessages: + msg318496
2018-06-02 07:34:49pitrousetmessages: + msg318490
2018-06-02 03:12:22benjamin.petersonsetmessages: + msg318484
2018-06-01 13:33:57pitrousetnosy: + benjamin.peterson
messages: + msg318415
2018-06-01 13:31:05pkerlingsetmessages: + msg318414
2018-06-01 12:06:26vstinnersetmessages: + msg318402
2018-06-01 11:12:34pitrousetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2018-06-01 11:12:17pitrousetmessages: + msg318398
2018-06-01 10:50:32pitrousetmessages: + msg318395
2018-06-01 10:49:45pitrousetmessages: + msg318394
2018-06-01 10:45:43ned.deilysetmessages: + msg318393
2018-06-01 10:08:56vstinnersetmessages: + msg318382
2018-06-01 09:56:40pitrousetnosy: + ned.deily
messages: + msg318381
2018-06-01 09:55:14pitrousetmessages: + msg318380
2018-06-01 09:53:46vstinnersetmessages: + msg318379
2018-06-01 09:50:26pitrousetmessages: + msg318378
2018-06-01 09:49:56pitrousetversions: + Python 3.8, - Python 2.7, Python 3.5
2018-06-01 09:49:27miss-islingtonsetpull_requests: + pull_request6939
2018-06-01 09:48:28miss-islingtonsetpull_requests: + pull_request6938
2018-06-01 09:47:26pitrousetmessages: + msg318377
2018-05-28 10:34:08pkerlingsetkeywords: + patch
stage: patch review
pull_requests: + pull_request6780
2017-06-13 13:16:39vstinnersetversions: - Python 3.3, Python 3.4
2017-06-13 13:16:30vstinnersetnosy: + pitrou, vstinner
2017-06-13 13:12:01python-devsetpull_requests: + pull_request2213
2017-06-13 13:06:04pkerlingcreate