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

Expose the C raise() function in the signal module, for use on Windows #79749

Closed
njsmith opened this issue Dec 23, 2018 · 7 comments
Closed

Expose the C raise() function in the signal module, for use on Windows #79749

njsmith opened this issue Dec 23, 2018 · 7 comments
Labels
3.8 only security fixes easy stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@njsmith
Copy link
Contributor

njsmith commented Dec 23, 2018

BPO 35568
Nosy @pfmoore, @tjguk, @njsmith, @zware, @zooba, @miss-islington, @vladima
PRs
  • bpo-35568: add 'raise_signal' function #11335
  • bpo-35568: add 'raise_signal' function #11335
  • bpo-35568: add 'raise_signal' function #11335
  • 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-01-08.09:59:37.107>
    created_at = <Date 2018-12-23.03:55:56.695>
    labels = ['easy', '3.8', 'type-feature', 'library']
    title = 'Expose the C raise() function in the signal module, for use on Windows'
    updated_at = <Date 2019-01-08.09:59:37.104>
    user = 'https://github.com/njsmith'

    bugs.python.org fields:

    activity = <Date 2019-01-08.09:59:37.104>
    actor = 'asvetlov'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-01-08.09:59:37.107>
    closer = 'asvetlov'
    components = ['Library (Lib)']
    creation = <Date 2018-12-23.03:55:56.695>
    creator = 'njs'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 35568
    keywords = ['patch', 'patch', 'patch', 'easy (C)']
    message_count = 7.0
    messages = ['332382', '332383', '332384', '332385', '332468', '332577', '333216']
    nosy_count = 7.0
    nosy_names = ['paul.moore', 'tim.golden', 'njs', 'zach.ware', 'steve.dower', 'miss-islington', 'v2m']
    pr_nums = ['11335', '11335', '11335']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue35568'
    versions = ['Python 3.8']

    @njsmith
    Copy link
    Contributor Author

    njsmith commented Dec 23, 2018

    Suppose we want to test how a program responds to control-C. We'll want to write a test that delivers a SIGINT to our process at a time of our choosing, and then checks what happens. For example, asyncio and Trio both have tests like this, and Trio even does a similar thing at run-time to avoid dropping signals in an edge case [1].

    So, how can we deliver a signal to our process?

    On POSIX platforms, you can use os.kill(os.getpid(), signal.SIGINT), and that works fine. But on Windows, things are much more complicated: #11274 (comment)

    The simplest solution is to use the raise() function. On POSIX, raise(signum) is just a shorthand for kill(getpid(), signum). But, that's only on POSIX. On Windows, kill() doesn't even exist... but raise() does. In fact raise() is specified in C89, so *every* C runtime has to provide raise(), no matter what OS it runs on.

    So, you might think, that's ok, if we need to generate synthetic signals on Windows then we'll just use ctypes/cffi to access raise(). *But*, Windows has multiple C runtime libraries (for example: regular and debug), and you have to load raise() from the same library that Python is linked against. And I don't know of any way for a Python script to figure out which runtime it's linked against. (I know how to detect whether the interpreter is configured in debug mode, but that's not necessarily the same as being linked against the debug CRT.) So on the one platform where you really need to use raise(), there's AFAICT no reliable way to get access to it.

    This would all be much simpler if the signal module wrapped the raise() function, so that we could just do 'signal.raise_(signal.SIGINT)'. We should do that.

    -------

    [1] Specifically, consider the following case (I'll use asyncio terminology for simplicity): (1) the user calls loop.add_signal_handler(...) to register a custom signal handler. (2) a signal arrives, and is written to the wakeup pipe. (3) but, before the loop reads the wakeup pipe, the user calls loop.remove_signal_handler(...) to remove the custom handler and restore the original signal settings. (4) now the loop reads the wakeup pipe, and discovers that a signal has arrived, that it no longer knows how to handle. Now what? In this case trio uses raise() to redeliver the signal, so that the new signal handler has a chance to run.

    @njsmith njsmith added 3.8 only security fixes type-feature A feature request or enhancement labels Dec 23, 2018
    @zooba
    Copy link
    Member

    zooba commented Dec 23, 2018

    Sounds fine to me.

    Any particular reason to put it in signal rather than os/posixmodule? If it's just to avoid treating os/posixmodule like a dumping ground for C89/POSIX APIs... well... too late :) I say keep dumping them there. But I don't have a strong opinion about this, and would approve a PR to either location.

    @njsmith
    Copy link
    Contributor Author

    njsmith commented Dec 23, 2018

    It probably doesn't matter too much either way, but almost all the signal-related wrappers are in signal. os.kill is the only exception AFAIK.

    @njsmith
    Copy link
    Contributor Author

    njsmith commented Dec 23, 2018

    Vladimir Matveev pointed out that there's already a wrapper in _testcapimodule.c:

    static PyObject*
    test_raise_signal(PyObject* self, PyObject *args)
    {
    int signum, err;
    if (!PyArg_ParseTuple(args, "i:raise_signal", &signum)) {
    return NULL;
    }
    err = raise(signum);
    if (err)
    return PyErr_SetFromErrno(PyExc_OSError);
    if (PyErr_CheckSignals() < 0)
    return NULL;
    Py_RETURN_NONE;
    }

    So if we do add this to signalmodule.c, we can drop that.

    @zooba
    Copy link
    Member

    zooba commented Dec 24, 2018

    That implementation will require some more of our usual macros around the call itself (for GIL and invalid values), and maybe clinicisation, as well as more through tests, which are probably going to be the hardest part.

    Nathaniel - were you planning to do this? Or should we put the "easy (C)" keyword on it and wait for some sprints?

    @njsmith
    Copy link
    Contributor Author

    njsmith commented Dec 27, 2018

    I'd like to see it in 3.8, but don't know if I'll get to it, and it'd be a good onramp issue for someone who wants to get into cpython development. So, let's put the keyword on it for now, and see what happens...

    @njsmith njsmith added the easy label Dec 27, 2018
    @miss-islington
    Copy link
    Contributor

    New changeset c24c6c2 by Miss Islington (bot) (Vladimir Matveev) in branch 'master':
    bpo-35568: add 'raise_signal' function (GH-11335)
    c24c6c2

    @asvetlov asvetlov added the stdlib Python modules in the Lib dir label Jan 8, 2019
    @asvetlov asvetlov closed this as completed Jan 8, 2019
    @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.8 only security fixes easy stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants