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

SIGINT catching regression on windows in 2.7 #62240

Closed
dgilman mannequin opened this issue May 23, 2013 · 7 comments
Closed

SIGINT catching regression on windows in 2.7 #62240

dgilman mannequin opened this issue May 23, 2013 · 7 comments
Assignees

Comments

@dgilman
Copy link
Mannequin

dgilman mannequin commented May 23, 2013

BPO 18040
Nosy @loewis, @tjguk, @dgilman
Files
  • issue18040.py
  • signalmodule.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 = 'https://github.com/tjguk'
    closed_at = <Date 2013-05-30.17:44:16.280>
    created_at = <Date 2013-05-23.03:01:06.914>
    labels = []
    title = 'SIGINT catching regression on windows in 2.7'
    updated_at = <Date 2013-10-01.09:43:29.832>
    user = 'https://github.com/dgilman'

    bugs.python.org fields:

    activity = <Date 2013-10-01.09:43:29.832>
    actor = 'Gabi.Davar'
    assignee = 'tim.golden'
    closed = True
    closed_date = <Date 2013-05-30.17:44:16.280>
    closer = 'tim.golden'
    components = []
    creation = <Date 2013-05-23.03:01:06.914>
    creator = 'David.Gilman'
    dependencies = []
    files = ['30370', '30382']
    hgrepos = []
    issue_num = 18040
    keywords = ['patch']
    message_count = 7.0
    messages = ['189843', '189996', '190072', '190349', '190353', '190371', '190373']
    nosy_count = 5.0
    nosy_names = ['loewis', 'tim.golden', 'sbt', 'Gabi.Davar', 'David.Gilman']
    pr_nums = []
    priority = 'low'
    resolution = 'wont fix'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue18040'
    versions = ['Python 2.7']

    @dgilman
    Copy link
    Mannequin Author

    dgilman mannequin commented May 23, 2013

    I opened this StackOverflow bug with an example simplified testcase. As you can see in the first comment a user added that this code worked under Python 2.6 on Windows and no longer works on 2.7.

    http://stackoverflow.com/questions/16686510/how-do-i-capture-sigint-in-python-on-windows?noredirect=1#comment24013681_16686510

    Here's a patch that went into the 2.7 release that maybe is related? http://bugs.python.org/issue1677

    @tjguk
    Copy link
    Member

    tjguk commented May 25, 2013

    My initial reaction is that, whether the 2.7 behaviour is faulty or not, I can't reproduce the "correct" behaviour on any version of Windows going back to 2.4. Take the attached Python file bpo-18040.py and run "c:\pythonxx\python.exe -i bpo-18040.py" for any version of Python from 2.4 to 3.4. At the interpreter prompt, pressing Ctrl-C produces "Keyboard Interrupt" consistently (except for the few times it exits the interpreter which is the problem fixed in bpo-1677).

    Note that this applies to pressing Ctrl-C *at the interpreter prompt* (while the running code is the function my_fgets in parser/myreadline.c). Pressing Ctrl-C in other circumstances, eg in the middle of a long-running os.walk or a time.sleep, invokes the signal handler as expected.

    I don't know if the handler *should* be invoked at the interpreter prompt. I recognise that it does so under Linux, but are there any circumstances where that would actually be useful?

    @tjguk tjguk self-assigned this May 25, 2013
    @tjguk
    Copy link
    Member

    tjguk commented May 26, 2013

    Correction: I see the desired behaviour in 3.3/3.4 which is where the
    overhaul to Ctrl-C handling on Windows was applied. I still can't see it
    in 2.6 or in 3.1/3.2 on Windows.

    The problem lies in the fact that PyOS_InterruptOccurred and
    PyErr_CheckSignals from signalmodule.c both check and reset signalled
    events. The former is used (solely within myreadline.c) to determine
    whether a SIGINT has fired; the latter is called in many different
    places to initiate Python's signal-handling but doesn't return any
    information about which signal fired.

    The check in line 70 of Parser/myreadline.c determines that a SIGINT
    signal has fired, but clears that signal at the same time. Any later
    call to PyErr_CheckSignals will not see that the SIGINT had fired.

    The 3.3+ situation is different, as a Windows event is the indication
    that SIGINT was raised, and the check for this doesn't affect the
    internal Handlers table which is examined by PyErr_CheckSignals.

    The attached patch to signalmodule.c appears to produce SIGINT signal
    handling as desired. Tested only on Windows. It's not clear whether any
    unittest could be produced for this kind of functionality.

    @tjguk
    Copy link
    Member

    tjguk commented May 30, 2013

    Personally, I'm +0 at best on this change. It would achieve consistency with Linux but I'm not sure what you'd do with such functionality.

    Adding Richard Oudkerk who did the rework of the interrupt signal for 3.3. Richard, any opinion on this?

    @sbt
    Copy link
    Mannequin

    sbt mannequin commented May 30, 2013

    I am not to familiar with the signal handling machinery. (I only did
    some refactoring to expose the event handle already used by time.sleep().)

    The change looks reasonable, but I am also not sure how necessary it is.

    @dgilman
    Copy link
    Mannequin Author

    dgilman mannequin commented May 30, 2013

    So the original motivation here was to piggyback on SIGINT in order to do something like this on Windows: http://stackoverflow.com/questions/132058/showing-the-stack-trace-from-a-running-python-application

    I've given Tim's patch a shot and I see that I've been barking up the wrong tree. I agree that this one-liner is a kinda invasive change and probably isn't worth putting in a point release. Thanks for the help.

    @tjguk
    Copy link
    Member

    tjguk commented May 30, 2013

    Thanks for the feedback, David. Closing as won't fix.

    @tjguk tjguk closed this as completed May 30, 2013
    @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
    None yet
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant