10000 BUG: Remove non-threadsafe sigint handling from fft calculation · numpy/numpy@ec62be2 · GitHub
[go: up one dir, main page]
Skip to content

Commit ec62be2

Browse files
sebergseberg
committed
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.
1 parent 267621f commit ec62be2

File tree

4 files changed

+16
-116
lines changed

4 files changed

+16
-116
lines changed

‎numpy/core/include/numpy/npy_interrupt.h

Lines changed: 15 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -1,79 +1,18 @@
1-
2-
/* Signal handling:
3-
4-
This header file defines macros that allow your code to handle
5-
interrupts received during processing. Interrupts that
6-
could reasonably be handled:
7-
8-
SIGINT, SIGABRT, SIGALRM, SIGSEGV
9-
10-
****Warning***************
11-
12-
Do not allow code that creates temporary memory or increases reference
13-
counts of Python objects to be interrupted unless you handle it
14-
differently.
15-
16-
**************************
17-
18-
The mechanism for handling interrupts is conceptually simple:
19-
20-
- replace the signal handler with our own home-grown version
21-
and store the old one.
22-
- run the code to be interrupted -- if an interrupt occurs
23-
the handler should basically just cause a return to the
24-
calling function for finish work.
25-
- restore the old signal handler
26-
27-
Of course, every code that allows interrupts must account for
28-
returning via the interrupt and handle clean-up correctly. But,
29-
even still, the simple paradigm is complicated by at least three
30-
factors.
31-
32-
1) platform portability (i.e. Microsoft says not to use longjmp
33-
to return from signal handling. They have a __try and __except
34-
extension to C instead but what about mingw?).
35-
36-
2) how to handle threads: apparently whether signals are delivered to
37-
every thread of the process or the "invoking" thread is platform
38-
dependent. --- we don't handle threads for now.
39-
40-
3) do we need to worry about re-entrance. For now, assume the
41-
code will not call-back into itself.
42-
43-
Ideas:
44-
45-
1) Start by implementing an approach that works on platforms that
46-
can use setjmp and longjmp functionality and does nothing
47-
on other platforms.
48-
49-
2) Ignore threads --- i.e. do not mix interrupt handling and threads
50-
51-
3) Add a default signal_handler function to the C-API but have the rest
52-
use macros.
53-
54-
55-
Simple Interface:
56-
57-
58-
In your C-extension: around a block of code you want to be interruptible
59-
with a SIGINT
60-
61-
NPY_SIGINT_ON
62-
[code]
63-
NPY_SIGINT_OFF
64-
65-
In order for this to work correctly, the
66-
[code] block must not allocate any memory or alter the reference count of any
67-
Python objects. In other words [code] must be interruptible so that continuation
68-
after NPY_SIGINT_OFF will only be "missing some computations"
69-
70-
Interrupt handling does not work well with threads.
71-
72-
*/
73-
74-
/* Add signal handling macros
75-
Make the global variable and signal handler part of the C-API
76-
*/
1+
/*
2+
* This API is only provided because it is part of publicly exported
3+
* headers. Its use is considered DEPRECATED, and it will be removed
4+
* eventually.
5+
* (This includes the _PyArray_SigintHandler and _PyArray_GetSigintBuf
6+
* functions which are however, public API, and not headers.)
7+
*
8+
* Instead of using these non-threadsafe macros consider periodically
9+
* querying `PyErr_CheckSignals()` or `PyOS_InterruptOccurred()` will work.
10+
* Both of these require holding the GIL, although cpython could add a
11+
* version of `PyOS_InterruptOccurred()` which does not. Such a version
12+
* actually exists as private API in Python 3.10, and backported to 3.9 and 3.8,
13+
* see also https://bugs.python.org/issue41037 and
14+
* https://github.com/python/cpython/pull/20599).
15+
*/
7716

7817
#ifndef NPY_INTERRUPT_H
7918
#define NPY_INTERRUPT_H

‎numpy/core/multiarray.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@
3838
'nested_iters', 'normalize_axis_index', 'packbits',
3939
'promote_types', 'putmask', 'ravel_multi_index', 'result_type', 'scalar',
4040
'set_datetimeparse_function', 'set_legacy_print_mode', 'set_numeric_ops',
41-
'set_string_function', 'set_typeDict', 'shares_memory', 'test_interrupt',
41+
'set_string_function', 'set_typeDict', 'shares_memory',
4242
'tracemalloc_domain', 'typeinfo', 'unpackbits', 'unravel_index', 'vdot',
4343
'where', 'zeros']
4444

‎numpy/core/src/multiarray/multiarraymodule.c

Lines changed: 0 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -3795,36 +3795,6 @@ _PyArray_GetSigintBuf(void)
37953795
#endif
37963796

37973797

3798-
static PyObject *
3799-
test_interrupt(PyObject *NPY_UNUSED(self), PyObject *args)
3800-
{
3801-
int kind = 0;
3802-
int a = 0;
3803-
3804-
if (!PyArg_ParseTuple(args, "|i:test_interrupt", &kind)) {
3805-
return NULL;
3806-
}
3807-
if (kind) {
3808-
Py_BEGIN_ALLOW_THREADS;
3809-
while (a >= 0) {
3810-
if ((a % 1000 == 0) && PyOS_InterruptOccurred()) {
3811-
break;
3812-
}
3813-
a += 1;
3814-
}
3815-
Py_END_ALLOW_THREADS;
3816-
}
3817-
else {
3818-
NPY_SIGINT_ON
3819-
while(a >= 0) {
3820-
a += 1;
3821-
}
3822-
NPY_SIGINT_OFF
3823-
}
3824-
return PyInt_FromLong(a);
3825-
}
3826-
3827-
38283798
static PyObject *
38293799
array_shares_memory_impl(PyObject *args, PyObject *kwds, Py_ssize_t default_max_work,
38303800
int raise_exceptions)
@@ -4119,9 +4089,6 @@ static struct PyMethodDef array_module_methods[] = {
41194089
{"_vec_string",
41204090
(PyCFunction)_vec_string,
41214091
METH_VARARGS | METH_KEYWORDS, NULL},
4122-
{"test_interrupt",
4123-
(PyCFunction)test_interrupt,
4124-
METH_VARARGS, NULL},
41254092
{"_insert", (PyCFunction)arr_insert,
41264093
METH_VARARGS | METH_KEYWORDS,
41274094
"Insert vals sequentially into equivalent 1-d positions "

‎numpy/fft/_pocketfft.c

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2206,7 +2206,6 @@ execute_complex(PyObject *a1, int is_forward, double fct)
22062206
double *dptr = (double *)PyArray_DATA(data);
22072207
int fail=0;
22082208
Py_BEGIN_ALLOW_THREADS;
2209-
NPY_SIGINT_ON;
22102209
plan = make_cfft_plan(npts);
22112210
if (!plan) fail=1;
22122211
if (!fail)
@@ -2217,7 +2216,6 @@ execute_complex(PyObject *a1, int is_forward, double fct)
22172216
dptr += npts*2;
22182217
}
22192218
if (plan) destroy_cfft_plan(plan);
2220-
NPY_SIGINT_OFF;
22212219
Py_END_ALLOW_THREADS;
22222220
if (fail) {
22232221
Py_XDECREF(data);
@@ -2258,7 +2256,6 @@ execute_real_forward(PyObject *a1, double fct)
22582256
*dptr = (double *)PyArray_DATA(data);
22592257

22602258
Py_BEGIN_ALLOW_THREADS;
2261-
NPY_SIGINT_ON;
22622259
plan = make_rfft_plan(npts);
22632260
if (!plan) fail=1;
22642261
if (!fail)
@@ -2272,7 +2269,6 @@ execute_real_forward(PyObject *a1, double fct)
22722269
dptr += npts;
22732270
}
22742271
if (plan) destroy_rfft_plan(plan);
2275-
NPY_SIGINT_OFF;
22762272
Py_END_ALLOW_THREADS;
22772273
}
22782274
if (fail) {
@@ -2303,7 +2299,6 @@ execute_real_backward(PyObject *a1, double fct)
23032299
*dptr = (double *)PyArray_DATA(data);
23042300

23052301
Py_BEGIN_ALLOW_THREADS;
2306-
NPY_SIGINT_ON;
23072302
plan = make_rfft_plan(npts);
23082303
if (!plan) fail=1;
23092304
if (!fail) {
@@ -2316,7 +2311,6 @@ execute_real_backward(PyObject *a1, double fct)
23162311
}
23172312
}
23182313
if (plan) destroy_rfft_plan(plan);
2319-
NPY_SIGINT_OFF;
23202314
Py_END_ALLOW_THREADS;
23212315
}
23222316
if (fail) {

0 commit comments

Comments
 (0)
0