-
-
Notifications
You must be signed in to change notification settings - Fork 30.7k
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
Comments
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 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. |
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. |
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. |
Vladimir Matveev pointed out that there's already a wrapper in _testcapimodule.c: cpython/Modules/_testcapimodule.c Lines 3862 to 3879 in f06fba5
So if we do add this to signalmodule.c, we can drop that. |
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? |
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... |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: