8000 BUG: Remove non-threadsafe sigint handling from fft calculation by seberg · Pull Request #16532 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

BUG: Remove non-threadsafe sigint handling from fft calculation #16532

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 17, 2020
Merged

BUG: Remove non-threadsafe sigint handling from fft calculation #16532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 15 additions & 76 deletions numpy/core/include/numpy/npy_interrupt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down
2 changes: 1 addition & 1 deletion numpy/core/multiarray.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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']

Expand Down
33 changes: 0 additions & 33 deletions numpy/core/src/multiarray/multiarraymodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Comment on lines -3798 to -3825
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth extracting the if / else branches into the comment in npy_interrupt, showing how to replace uses of the SIGINT api.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I thought that might require the GIL to be held, which seemed a bit useless for us. But, it seems like that has been fixed reading through all of: https://bugs.python.org/issue40826

Copy link
Member
@pv pv Jun 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't they end up deciding that it does require GIL?
python/cpython#20618
It looks like the above will crash the process with fatal error in the future...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, damn, so my first read was the right one, I guess I read the fact that it was backported, as that they made it not require the GIL (since I did not see why to backport otherwise) :(.

I think I will open a BPO, it would be nice if it was possible to at least be able to query this without the GIL if you pass in the threadstate or so...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened https://bugs.python.org/issue41037 seems there already is a private function for this. Not sure if we should/could petition more to make this function public, but I suppose it may be the best bet if we want interrupt capabilities at some point.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stumbled on it, so updated the comment now to mention PyErr_CheckSignals() and also link to the bpo.



static PyObject *
array_shares_memory_impl(PyObject *args, PyObject *kwds, Py_ssize_t default_max_work,
int raise_exceptions)
Expand Down Expand Up @@ -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 "
Expand Down
6 changes: 0 additions & 6 deletions numpy/fft/_pocketfft.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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);
Expand Down Expand Up @@ -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)
Expand All @@ -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) {
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down
0