From ec62be21cf109dac436f9a68a84fbd674c1faebb Mon Sep 17 00:00:00 2001 From: Sebastian Berg Date: Mon, 8 Jun 2020 08:56:54 -0500 Subject: [PATCH] BUG: Remove non-threadsafe sigint handling from fft calculation The fft calculation is the only point in our code where this function is used. Allowing Ctrl+C, in FFT specifically used have more reasons, since before pocketfft, some array-sizes could lead to very large run-times. Pocketfft fixed that issue, and now FFT is not really any slower, faster, or memory hungry than any other NumPy operation so it feels it does not need this handling. Rather, if we can find a better solution, it should also be added to more functions. The reason for removal is that it is not only unsafe while the FFT is running (in theory). Multiple, threaded FFT run can easily leave the signal handler in a bad state, causing crashes if Ctrl+C (sigint) is given at any point after the call. It would be possible to patch that over, by only resetting the signal handler if we actually changed it (or even more complex tricks), or possibly only using this technique when on the main thread. But, all of these solutions seem to complicate things, when the main reason for why allowing sigint seems useful is gone with pocketfft. --- numpy/core/include/numpy/npy_interrupt.h | 91 ++++---------------- numpy/core/multiarray.py | 2 +- numpy/core/src/multiarray/multiarraymodule.c | 33 ------- numpy/fft/_pocketfft.c | 6 -- 4 files changed, 16 insertions(+), 116 deletions(-) diff --git a/numpy/core/include/numpy/npy_interrupt.h b/numpy/core/include/numpy/npy_interrupt.h index 40cb7ac5efdb..bcb539326e88 100644 --- a/numpy/core/include/numpy/npy_interrupt.h +++ b/numpy/core/include/numpy/npy_interrupt.h @@ -1,79 +1,18 @@ - -/* Signal handling: - -This header file defines macros that allow your code to handle -interrupts received during processing. Interrupts that -could reasonably be handled: - -SIGINT, SIGABRT, SIGALRM, SIGSEGV - -****Warning*************** - -Do not allow code that creates temporary memory or increases reference -counts of Python objects to be interrupted unless you handle it -differently. - -************************** - -The mechanism for handling interrupts is conceptually simple: - - - replace the signal handler with our own home-grown version - and store the old one. - - run the code to be interrupted -- if an interrupt occurs - the handler should basically just cause a return to the - calling function for finish work. - - restore the old signal handler - -Of course, every code that allows interrupts must account for -returning via the interrupt and handle clean-up correctly. But, -even still, the simple paradigm is complicated by at least three -factors. - - 1) platform portability (i.e. Microsoft says not to use longjmp - to return from signal handling. They have a __try and __except - extension to C instead but what about mingw?). - - 2) how to handle threads: apparently whether signals are delivered to - every thread of the process or the "invoking" thread is platform - dependent. --- we don't handle threads for now. - - 3) do we need to worry about re-entrance. For now, assume the - code will not call-back into itself. - -Ideas: - - 1) Start by implementing an approach that works on platforms that - can use setjmp and longjmp functionality and does nothing - on other platforms. - - 2) Ignore threads --- i.e. do not mix interrupt handling and threads - - 3) Add a default signal_handler function to the C-API but have the rest - use macros. - - -Simple Interface: - - -In your C-extension: around a block of code you want to be interruptible -with a SIGINT - -NPY_SIGINT_ON -[code] -NPY_SIGINT_OFF - -In order for this to work correctly, the -[code] block must not allocate any memory or alter the reference count of any -Python objects. In other words [code] must be interruptible so that continuation -after NPY_SIGINT_OFF will only be "missing some computations" - -Interrupt handling does not work well with threads. - -*/ - -/* Add signal handling macros - Make the global variable and signal handler part of the C-API -*/ +/* + * This API is only provided because it is part of publicly exported + * headers. Its use is considered DEPRECATED, and it will be removed + * eventually. + * (This includes the _PyArray_SigintHandler and _PyArray_GetSigintBuf + * functions which are however, public API, and not headers.) + * + * Instead of using these non-threadsafe macros consider periodically + * querying `PyErr_CheckSignals()` or `PyOS_InterruptOccurred()` will work. + * Both of these require holding the GIL, although cpython could add a + * version of `PyOS_InterruptOccurred()` which does not. Such a version + * actually exists as private API in Python 3.10, and backported to 3.9 and 3.8, + * see also https://bugs.python.org/issue41037 and + * https://github.com/python/cpython/pull/20599). + */ #ifndef NPY_INTERRUPT_H #define NPY_INTERRUPT_H diff --git a/numpy/core/multiarray.py b/numpy/core/multiarray.py index 2cc2a8e710c1..b196687afc6b 100644 --- a/numpy/core/multiarray.py +++ b/numpy/core/multiarray.py @@ -38,7 +38,7 @@ 'nested_iters', 'normalize_axis_index', 'packbits', 'promote_types', 'putmask', 'ravel_multi_index', 'result_type', 'scalar', 'set_datetimeparse_function', 'set_legacy_print_mode', 'set_numeric_ops', - 'set_string_function', 'set_typeDict', 'shares_memory', 'test_interrupt', + 'set_string_function', 'set_typeDict', 'shares_memory', 'tracemalloc_domain', 'typeinfo', 'unpackbits', 'unravel_index', 'vdot', 'where', 'zeros'] diff --git a/numpy/core/src/multiarray/multiarraymodule.c b/numpy/core/src/multiarray/multiarraymodule.c index 84c22ba65390..614930bdcd3c 100644 --- a/numpy/core/src/multiarray/multiarraymodule.c +++ b/numpy/core/src/multiarray/multiarraymodule.c @@ -3795,36 +3795,6 @@ _PyArray_GetSigintBuf(void) #endif -static PyObject * -test_interrupt(PyObject *NPY_UNUSED(self), PyObject *args) -{ - int kind = 0; - int a = 0; - - if (!PyArg_ParseTuple(args, "|i:test_interrupt", &kind)) { - return NULL; - } - if (kind) { - Py_BEGIN_ALLOW_THREADS; - while (a >= 0) { - if ((a % 1000 == 0) && PyOS_InterruptOccurred()) { - break; - } - a += 1; - } - Py_END_ALLOW_THREADS; - } - else { - NPY_SIGINT_ON - while(a >= 0) { - a += 1; - } - NPY_SIGINT_OFF - } - return PyInt_FromLong(a); -} - - static PyObject * array_shares_memory_impl(PyObject *args, PyObject *kwds, Py_ssize_t default_max_work, int raise_exceptions) @@ -4119,9 +4089,6 @@ static struct PyMethodDef array_module_methods[] = { {"_vec_string", (PyCFunction)_vec_string, METH_VARARGS | METH_KEYWORDS, NULL}, - {"test_interrupt", - (PyCFunction)test_interrupt, - METH_VARARGS, NULL}, {"_insert", (PyCFunction)arr_insert, METH_VARARGS | METH_KEYWORDS, "Insert vals sequentially into equivalent 1-d positions " diff --git a/numpy/fft/_pocketfft.c b/numpy/fft/_pocketfft.c index 764116a840ab..ba9995f97254 100644 --- a/numpy/fft/_pocketfft.c +++ b/numpy/fft/_pocketfft.c @@ -2206,7 +2206,6 @@ execute_complex(PyObject *a1, int is_forward, double fct) double *dptr = (double *)PyArray_DATA(data); int fail=0; Py_BEGIN_ALLOW_THREADS; - NPY_SIGINT_ON; plan = make_cfft_plan(npts); if (!plan) fail=1; if (!fail) @@ -2217,7 +2216,6 @@ execute_complex(PyObject *a1, int is_forward, double fct) dptr += npts*2; } if (plan) destroy_cfft_plan(plan); - NPY_SIGINT_OFF; Py_END_ALLOW_THREADS; if (fail) { Py_XDECREF(data); @@ -2258,7 +2256,6 @@ execute_real_forward(PyObject *a1, double fct) *dptr = (double *)PyArray_DATA(data); Py_BEGIN_ALLOW_THREADS; - NPY_SIGINT_ON; plan = make_rfft_plan(npts); if (!plan) fail=1; if (!fail) @@ -2272,7 +2269,6 @@ execute_real_forward(PyObject *a1, double fct) dptr += npts; } if (plan) destroy_rfft_plan(plan); - NPY_SIGINT_OFF; Py_END_ALLOW_THREADS; } if (fail) { @@ -2303,7 +2299,6 @@ execute_real_backward(PyObject *a1, double fct) *dptr = (double *)PyArray_DATA(data); Py_BEGIN_ALLOW_THREADS; - NPY_SIGINT_ON; plan = make_rfft_plan(npts); if (!plan) fail=1; if (!fail) { @@ -2316,7 +2311,6 @@ execute_real_backward(PyObject *a1, double fct) } } if (plan) destroy_rfft_plan(plan); - NPY_SIGINT_OFF; Py_END_ALLOW_THREADS; } if (fail) {