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

signal module always overwrites SIGINT on interpreter shutdown #74839

Closed
yol mannequin opened this issue Jun 13, 2017 · 21 comments
Closed

signal module always overwrites SIGINT on interpreter shutdown #74839

yol mannequin opened this issue Jun 13, 2017 · 21 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@yol
Copy link
Mannequin

yol mannequin commented Jun 13, 2017

BPO 30654
Nosy @pitrou, @vstinner, @benjaminp, @ned-deily, @yol
PRs
  • bpo-30654: Do not reset SIGINT handler to SIG_DFL in finisignal #2162
  • bpo-30654: Do not reset SIGINT handler to SIG_DFL in finisignal #7146
  • [3.7] bpo-30654: Do not reset SIGINT handler to SIG_DFL in finisignal (GH-7146) #7306
  • [3.6] bpo-30654: Do not reset SIGINT handler to SIG_DFL in finisignal (GH-7146) #7307
  • [2.7] bpo-30654: Do not reset SIGINT handler to SIG_DFL in finisignal (GH-7146) #7347
  • 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 = None
    closed_at = <Date 2018-06-03.19:11:15.189>
    created_at = <Date 2017-06-13.13:06:04.818>
    labels = ['interpreter-core', '3.8', 'type-bug', '3.7']
    title = 'signal module always overwrites SIGINT on interpreter shutdown'
    updated_at = <Date 2020-03-09.23:24:11.602>
    user = 'https://github.com/yol'

    bugs.python.org fields:

    activity = <Date 2020-03-09.23:24:11.602>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-06-03.19:11:15.189>
    closer = 'pitrou'
    components = ['Interpreter Core']
    creation = <Date 2017-06-13.13:06:04.818>
    creator = 'pkerling'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 30654
    keywords = ['patch']
    message_count = 21.0
    messages = ['295915', '318377', '318378', '318379', '318380', '318381', '318382', '318393', '318394', '318395', '318398', '318402', '318414', '318415', '318484', '318490', '318496', '318513', '318577', '318580', '363788']
    nosy_count = 5.0
    nosy_names = ['pitrou', 'vstinner', 'benjamin.peterson', 'ned.deily', 'pkerling']
    pr_nums = ['2162', '7146', '7306', '7307', '7347']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue30654'
    versions = ['Python 2.7', 'Python 3.6', 'Python 3.7', 'Python 3.8']

    @yol
    Copy link
    Mannequin Author

    yol mannequin commented Jun 13, 2017

    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.

    @yol yol mannequin added 3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Jun 13, 2017
    @pitrou
    Copy link
    Member

    pitrou commented Jun 1, 2018

    New changeset e905c84 by Antoine Pitrou (pkerling) in branch 'master':
    bpo-30654: Do not reset SIGINT handler to SIG_DFL in finisignal (GH-7146)
    e905c84

    @pitrou pitrou added the 3.8 only security fixes label Jun 1, 2018
    @pitrou
    Copy link
    Member

    pitrou commented Jun 1, 2018

    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.

    @vstinner
    Copy link
    Member

    vstinner commented Jun 1, 2018

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

    @pitrou
    Copy link
    Member

    pitrou commented Jun 1, 2018

    So if an application has its own signal handler, Python replaces it...

    You have it backwards. Please read the bug report.

    @pitrou
    Copy link
    Member

    pitrou commented Jun 1, 2018

    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

    @vstinner
    Copy link
    Member

    vstinner commented Jun 1, 2018

    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.

    @ned-deily
    Copy link
    Member

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

    @pitrou
    Copy link
    Member

    pitrou commented Jun 1, 2018

    Ok, I think it go into 3.7.0 after all. 3.7.1 wouldn't get more testing before it's released anyway.

    @pitrou
    Copy link
    Member

    pitrou commented Jun 1, 2018

    New changeset 623b439 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)
    623b439

    @pitrou
    Copy link
    Member

    pitrou commented Jun 1, 2018

    New changeset 1d5198f 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)
    1d5198f

    @pitrou pitrou closed this as completed Jun 1, 2018
    @vstinner
    Copy link
    Member

    vstinner commented Jun 1, 2018

    Thanks pkerling for your bug report and your fix!

    @yol
    Copy link
    Mannequin Author

    yol mannequin commented Jun 1, 2018

    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.

    @pitrou
    Copy link
    Member

    pitrou commented Jun 1, 2018

    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.

    @benjaminp
    Copy link
    Contributor

    It seems like this only affects embeddings and for embeddings, it's strictly an improvement?

    @pitrou
    Copy link
    Member

    pitrou commented Jun 2, 2018

    Short of any bug in the patch, yes, it's stricly an improvement.

    @vstinner
    Copy link
    Member

    vstinner commented Jun 2, 2018

    IMHO it's a bugfix.

    @pitrou
    Copy link
    Member

    pitrou commented Jun 2, 2018

    Reopening for the 2.7 backport discussion.

    @pitrou pitrou reopened this Jun 2, 2018
    @pitrou
    Copy link
    Member

    pitrou commented Jun 3, 2018

    New changeset ded666f 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)
    ded666f

    @pitrou
    Copy link
    Member

    pitrou commented Jun 3, 2018

    So this is in 2.7 finally. Closing again.

    @pitrou pitrou closed this as completed Jun 3, 2018
    @vstinner
    Copy link
    Member

    vstinner commented Mar 9, 2020

    I marked bpo-24415 as duplicate of this issue.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life 3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants