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

_thread.interrupt_main() errors if SIGINT handler in SIG_DFL, SIG_IGN #67584

Closed
takluyver mannequin opened this issue Feb 4, 2015 · 20 comments
Closed

_thread.interrupt_main() errors if SIGINT handler in SIG_DFL, SIG_IGN #67584

takluyver mannequin opened this issue Feb 4, 2015 · 20 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@takluyver
Copy link
Mannequin

takluyver mannequin commented Feb 4, 2015

BPO 23395
Nosy @pitrou, @vstinner, @mcepl, @takluyver, @minrk, @miss-islington, @gozdal
PRs
  • WIP: bpo-23395: Raise an exception if the SIGINT signal is ignored or not handled #7777
  • bpo-23395: Raise an exception if the SIGINT signal is ignored or not handled #7778
  • [3.7] bpo-23395: Fix PyErr_SetInterrupt if the SIGINT signal is ignored or not handled (GH-7778) #13541
  • Files
  • issue23395.patch
  • issue23395.2.patch
  • interrupt_main.patch
  • 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 2019-05-24.09:28:54.994>
    created_at = <Date 2015-02-04.23:50:50.367>
    labels = ['3.7', '3.8', 'type-bug', 'library']
    title = '_thread.interrupt_main() errors if SIGINT handler in SIG_DFL, SIG_IGN'
    updated_at = <Date 2021-03-04.23:22:55.101>
    user = 'https://github.com/takluyver'

    bugs.python.org fields:

    activity = <Date 2021-03-04.23:22:55.101>
    actor = 'pitrou'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-05-24.09:28:54.994>
    closer = 'pitrou'
    components = ['Library (Lib)']
    creation = <Date 2015-02-04.23:50:50.367>
    creator = 'takluyver'
    dependencies = []
    files = ['43544', '43546', '43560']
    hgrepos = []
    issue_num = 23395
    keywords = ['patch']
    message_count = 20.0
    messages = ['235412', '269287', '269289', '269298', '269300', '269301', '269303', '269304', '269307', '269309', '269362', '269414', '269418', '282756', '343323', '343354', '343357', '343359', '353291', '388136']
    nosy_count = 10.0
    nosy_names = ['pitrou', 'vstinner', 'mcepl', 'neologix', 'takluyver', 'minrk', 'Andre Merzky', 'sxsns243', 'miss-islington', 'gozdal']
    pr_nums = ['7777', '7778', '13541']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue23395'
    versions = ['Python 3.7', 'Python 3.8']

    @takluyver
    Copy link
    Mannequin Author

    takluyver mannequin commented Feb 4, 2015

    In tracking down an obscure error we were seeing, we boiled it down to this test case for thread.interrupt_main():

    import signal, threading, _thread, time
    signal.signal(signal.SIGINT, signal.SIG_DFL) # or SIG_IGN
    
    def thread_run():
        _thread.interrupt_main()
    
    t = threading.Thread(target=thread_run)
    t.start()
    time.sleep(10)

    This fails with an error message "TypeError: 'int' object is not callable", and a traceback completely disconnected from the cause of the error, presumably because it's not coming from the usual Python stack.

    The problem appears to be that interrupt_main sets (in the C code) Handlers[SIGINT].tripped, which is only expected to occur when the handler is a Python function. When PyErr_CheckSignals() runs, it tries to call Handlers[SIGINT].func as a Python function, but it's a Python integer, causing the error.

    I think the fix for this is to check what Handlers[sig_num].func is, either in trip_signal() before setting Handlers[sig_num].tripped, or in PyErr_CheckSignals before calling it. I can work on a patch if desired, but I'm not brilliant at C.

    @takluyver takluyver mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Feb 4, 2015
    @AndreMerzky
    Copy link
    Mannequin

    AndreMerzky mannequin commented Jun 26, 2016

    Did anything ever come of this? I also frequently stumble over that error -- but alas not in a debug setting, but on actual running code... :/

    @takluyver
    Copy link
    Mannequin Author

    takluyver mannequin commented Jun 26, 2016

    As far as I know it has not been fixed.

    @AndreMerzky
    Copy link
    Mannequin

    AndreMerzky mannequin commented Jun 26, 2016

    Thanks for the PingBack, Thomas.

    I am not very familiar with the Python community approach to bug reports, so can someone comment if that is worth waiting for to get fixed, or is it that a rather futile hope without providing a patch myself? I don't think I currently have the resources to dig into the details...

    Thanks, Andre.

    @takluyver
    Copy link
    Mannequin Author

    takluyver mannequin commented Jun 26, 2016

    It's waiting for someone to make a patch - I'm no longer running into it, so it's not high on my priority list. Given that it's been over a year since I created this issue, it's probably not about to get fixed unless you've got some time to work on it.

    @AndreMerzky
    Copy link
    Mannequin

    AndreMerzky mannequin commented Jun 26, 2016

    Thanks Thomas.

    @AndreMerzky
    Copy link
    Mannequin

    AndreMerzky mannequin commented Jun 26, 2016

    Attached is a patch which seems to resolve the issue for me -- this now triggers the expected KeyboardInterrupt in the main thread. For verification one can run the unaltered code as provided by Thomas.

    I would very much appreciate feedback, to make sure that the semantics is actually what one would expect. The patch is against the 2.7 branch from https://github.com/python/cpython.git, and I did not test it against any other branch.

    I also opened a pull request (#39).

    @takluyver
    Copy link
    Mannequin Author

    takluyver mannequin commented Jun 26, 2016

    Does Py_DECREF(arglist); need to be called in all cases? I'm genuinely unsure, as I don't usually work on C code, but my guess would be that it does.

    I'm not sure whether review is now done on Github.

    @AndreMerzky
    Copy link
    Mannequin

    AndreMerzky mannequin commented Jun 26, 2016

    thanks for looking into this! And also, thanks for the details in the original bug report -- I found this by chance, after unsuccessfully banging my head against this very problem for quite a while!

    I am not sure if the DecRef needs to be called really if the arglist is not stored or passed on. But thanks for pointing that out, I'll check if I can find where the corresponding IncRef is called (assuming that happens somewhere).

    @AndreMerzky
    Copy link
    Mannequin

    AndreMerzky mannequin commented Jun 26, 2016

    It seems you were right, that needed a DecRef indeed: the IncRef is already called on construction. The DecRef for the result also needed fixing - an updated patch is attached.

    @vstinner
    Copy link
    Member

    Attached interrupt_main.patch fixes for _thread.interrupt_main(): raise an exception if the SIGINT signal is ignored (SIG_IGN) or not handled by Python (SIG_DFL).

    My patch updates the documentation and addds unit tests.

    bpo-23395.2.patch looks wrong: it's not the right place to handle the error.

    @AndreMerzky
    Copy link
    Mannequin

    AndreMerzky mannequin commented Jun 28, 2016

    I can confirm that the patch provided by Victor addresses the problem. It seems to be against the current HEAD -- is there a chance that this will be backported to 2.7 (which is what I need to use)?

    Thanks! Andre.

    @vstinner
    Copy link
    Member

    It seems to be against the current HEAD -- is there a chance that this will be backported to 2.7 (which is what I need to use)?

    Yes, I will fix Python 2.7 and 3.5, I will just make PyErr_SetInterruptWithErr() private (rename to _PyErr_SetInterruptWithErr()).

    @sxsns243
    Copy link
    Mannequin

    sxsns243 mannequin commented Dec 9, 2016

    Recently I've encountered the similar problem.
    I found the case that SIGINT signal is ignored(set as SIG_IGN) due to the external reason.
    If the python program starts background in bash script, bash set SIGINT as SIG_IGN.

    test_interrupt.py
    import _thread
    _thread.interrupt_main()

    run.sh
    #!/bin/bash
    python test_interrupt.py &
    sleep 1s

    ./run.sh 
    Traceback (most recent call last):
      File "test_interrupt.py", line 2, in <module>
        _thread.interrupt_main()
    RuntimeError: the SIGINT signal is ignored
    (it was TypeError: 'int' object is not callable before the patch)

    Python mapped default_int_handler to SIG_INT on SIG_DFL in PyInit__signal.
    As the python developer can't control how the program is started, it may be better to setup default_int_handler regardless the handler type.

    And initially SIG_INT is mapped to default_int_handler but once signal.signal is called, the mapping is lost even though the SIG_DFL is specified.
    It may need to handle SIG_INT specially in signal_signal_impl as well to keep the consistent behavior.

    @pitrou
    Copy link
    Member

    pitrou commented May 23, 2019

    New changeset 608876b by Antoine Pitrou (Matěj Cepl) in branch 'master':
    bpo-23395: Fix PyErr_SetInterrupt if the SIGINT signal is ignored or not handled (GH-7778)
    608876b

    @vstinner
    Copy link
    Member

    The interrupt_main() documentation needs a "versionchanged:: 3.8" to document that the behavior changed in Python 3.8, no? (do nothing if the signal is not handled by Python).

    I'm not comfortable to backport this change. I suggest to only change Python 3.8 (master branch).

    While reviewing this change, I found code which should be refactored: see bpo-37031.

    @pitrou
    Copy link
    Member

    pitrou commented May 24, 2019

    It's just a bugfix to avoid a rogue exception. It sounds gratuitous to add markers for bug fixes in the documentation.

    @pitrou pitrou added 3.7 (EOL) end of life 3.8 only security fixes labels May 24, 2019
    @miss-islington
    Copy link
    Contributor

    New changeset 310f414 by Miss Islington (bot) in branch '3.7':
    bpo-23395: Fix PyErr_SetInterrupt if the SIGINT signal is ignored or not handled (GH-7778)
    310f414

    @pitrou pitrou closed this as completed May 24, 2019
    @gozdal
    Copy link
    Mannequin

    gozdal mannequin commented Sep 26, 2019

    A program which failed under Python 3.6 with

    TypeError: 'int' object is not callable

    still fails under Python 3.7.4 with same exception:

    import signal
    import os
    import sys
    import time
    
    import multiprocessing
    
    def fail():
        def handler(*args):
            pass
    
        while True:
            signal.signal(signal.SIGUSR1, signal.SIG_IGN)
            signal.signal(signal.SIGUSR1, handler)
    
    proc = multiprocessing.Process(target=fail)
    proc.start()
    time.sleep(1)
    pid = proc.pid
    
    i = 0
    try:
        while proc.is_alive():
            print("\r", end='')
            print(i, end='')
            i += 1
            os.kill(pid, signal.SIGUSR1)
            time.sleep(0.001)
    finally:
        proc.join()

    @pitrou
    Copy link
    Member

    pitrou commented Mar 4, 2021

    Turns out this didn't fix all possible situations. See bpo-43406 for an updated patch.

    @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 stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants