From 5ec3e749029086b9359e4fd1ffe0c85b1c40a129 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Fri, 21 Sep 2018 16:07:39 -0600 Subject: [PATCH 01/11] Do not clear an existing error when handling signal failures. --- Modules/signalmodule.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Modules/signalmodule.c b/Modules/signalmodule.c index 52ab4e998a9778..91c781a3606643 100644 --- a/Modules/signalmodule.c +++ b/Modules/signalmodule.c @@ -202,12 +202,15 @@ It raises KeyboardInterrupt."); static int report_wakeup_write_error(void *data) { + PyObject *exc, *val, *tb; int save_errno = errno; errno = (int) (intptr_t) data; + PyErr_Fetch(&exc, &val, &tb); PyErr_SetFromErrno(PyExc_OSError); PySys_WriteStderr("Exception ignored when trying to write to the " "signal wakeup fd:\n"); PyErr_WriteUnraisable(NULL); + PyErr_Restore(exc, val, tb); errno = save_errno; return 0; } @@ -216,6 +219,8 @@ report_wakeup_write_error(void *data) static int report_wakeup_send_error(void* data) { + PyObject *exc, *val, *tb; + PyErr_Fetch(&exc, &val, &tb); /* PyErr_SetExcFromWindowsErr() invokes FormatMessage() which recognizes the error codes used by both GetLastError() and WSAGetLastError */ @@ -223,6 +228,7 @@ report_wakeup_send_error(void* data) PySys_WriteStderr("Exception ignored when trying to send to the " "signal wakeup fd:\n"); PyErr_WriteUnraisable(NULL); + PyErr_Restore(exc, val, tb); return 0; } #endif /* MS_WINDOWS */ From e99c5dfae83ce0f175b307a66b5120ddafbb5182 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Fri, 28 Sep 2018 11:10:53 -0600 Subject: [PATCH 02/11] Factor out make_pending_calls(). --- Python/ceval.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/Python/ceval.c b/Python/ceval.c index a4273adee48d81..e2b80308186c77 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -356,15 +356,13 @@ Py_AddPendingCall(int (*func)(void *), void *arg) return result; } -int -Py_MakePendingCalls(void) +static int +make_pending_calls(void) { static int busy = 0; int i; int r = 0; - assert(PyGILState_Check()); - if (!_PyRuntime.ceval.pending.lock) { /* initial allocation of the lock */ _PyRuntime.ceval.pending.lock = PyThread_allocate_lock(); @@ -427,6 +425,13 @@ Py_MakePendingCalls(void) return -1; } +int +Py_MakePendingCalls(void) +{ + assert(PyGILState_Check()); + return make_pending_calls(); +} + /* The interpreter's recursion limit */ #ifndef Py_DEFAULT_RECURSION_LIMIT From 308f579cc05274f2898829c375dcc6bc8ef7308d Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Fri, 28 Sep 2018 11:12:08 -0600 Subject: [PATCH 03/11] Add _PyRuntimeState.ceval.signals_pending. --- Include/internal/pycore_ceval.h | 2 ++ Python/ceval.c | 13 +++++++++++++ 2 files changed, 15 insertions(+) diff --git a/Include/internal/pycore_ceval.h b/Include/internal/pycore_ceval.h index c8c63b1c7fdafe..b9f2d7d1758537 100644 --- a/Include/internal/pycore_ceval.h +++ b/Include/internal/pycore_ceval.h @@ -45,6 +45,8 @@ struct _ceval_runtime_state { /* Request for dropping the GIL */ _Py_atomic_int gil_drop_request; struct _pending_calls pending; + /* Request for checking signals. */ + _Py_atomic_int signals_pending; struct _gil_runtime_state gil; }; diff --git a/Python/ceval.c b/Python/ceval.c index e2b80308186c77..5f5fb7bbe71c23 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -100,6 +100,7 @@ static long dxp[256]; _Py_atomic_store_relaxed( \ &_PyRuntime.ceval.eval_breaker, \ GIL_REQUEST | \ + _Py_atomic_load_relaxed(&_PyRuntime.ceval.signals_pending) | \ _Py_atomic_load_relaxed(&_PyRuntime.ceval.pending.calls_to_do) | \ _PyRuntime.ceval.pending.async_exc) @@ -128,6 +129,18 @@ static long dxp[256]; COMPUTE_EVAL_BREAKER(); \ } while (0) +#define SIGNAL_PENDING_SIGNALS() \ + do { \ + _Py_atomic_store_relaxed(&_PyRuntime.ceval.signals_pending, 1); \ + _Py_atomic_store_relaxed(&_PyRuntime.ceval.eval_breaker, 1); \ + } while (0) + +#define UNSIGNAL_PENDING_SIGNALS() \ + do { \ + _Py_atomic_store_relaxed(&_PyRuntime.ceval.signals_pending, 0); \ + COMPUTE_EVAL_BREAKER(); \ + } while (0) + #define SIGNAL_ASYNC_EXC() \ do { \ _PyRuntime.ceval.pending.async_exc = 1; \ From c6c6416a1ceb82bc60058d36878488cb673811da Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Fri, 28 Sep 2018 11:47:07 -0600 Subject: [PATCH 04/11] Add _PyRuntimeState.ceval.pending.busy. --- Include/internal/pycore_ceval.h | 1 + Python/ceval.c | 9 ++++----- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Include/internal/pycore_ceval.h b/Include/internal/pycore_ceval.h index b9f2d7d1758537..52fda4b468f8f8 100644 --- a/Include/internal/pycore_ceval.h +++ b/Include/internal/pycore_ceval.h @@ -13,6 +13,7 @@ extern "C" { struct _pending_calls { unsigned long main_thread; + int busy; PyThread_type_lock lock; /* Request for running pending calls. */ _Py_atomic_int calls_to_do; diff --git a/Python/ceval.c b/Python/ceval.c index 5f5fb7bbe71c23..90104807d4ca83 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -372,7 +372,6 @@ Py_AddPendingCall(int (*func)(void *), void *arg) static int make_pending_calls(void) { - static int busy = 0; int i; int r = 0; @@ -390,9 +389,9 @@ make_pending_calls(void) return 0; } /* don't perform recursive pending calls */ - if (busy) + if (_PyRuntime.ceval.pending.busy) return 0; - busy = 1; + _PyRuntime.ceval.pending.busy = 1; /* unsignal before starting to call callbacks, so that any callback added in-between re-signals */ UNSIGNAL_PENDING_CALLS(); @@ -429,11 +428,11 @@ make_pending_calls(void) } } - busy = 0; + _PyRuntime.ceval.pending.busy = 0; return r; error: - busy = 0; + _PyRuntime.ceval.pending.busy = 0; SIGNAL_PENDING_CALLS(); /* We're not done yet */ return -1; } From 7255c0b716cc3c0b70c201a996194d9d99d6d6b9 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Fri, 28 Sep 2018 12:16:36 -0600 Subject: [PATCH 05/11] Handle signals (mostly) separately. --- Python/ceval.c | 89 +++++++++++++++++++++++++++++++------------------- 1 file changed, 56 insertions(+), 33 deletions(-) diff --git a/Python/ceval.c b/Python/ceval.c index 90104807d4ca83..f87d9e2c537477 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -319,7 +319,7 @@ _PyEval_SignalReceived(void) /* bpo-30703: Function called when the C signal handler of Python gets a signal. We cannot queue a callback using Py_AddPendingCall() since that function is not async-signal-safe. */ - SIGNAL_PENDING_CALLS(); + SIGNAL_PENDING_SIGNALS(); } /* This implementation is thread-safe. It allows @@ -370,40 +370,35 @@ Py_AddPendingCall(int (*func)(void *), void *arg) } static int -make_pending_calls(void) +handle_signals(void) { - int i; - int r = 0; - - if (!_PyRuntime.ceval.pending.lock) { - /* initial allocation of the lock */ - _PyRuntime.ceval.pending.lock = PyThread_allocate_lock(); - if (_PyRuntime.ceval.pending.lock == NULL) - return -1; + UNSIGNAL_PENDING_SIGNALS(); + if (PyErr_CheckSignals() < 0) { + SIGNAL_PENDING_SIGNALS(); /* We're not done yet */ + return -1; } + return 0; +} +static int +make_pending_calls(void) +{ /* only service pending calls on main thread */ if (_PyRuntime.ceval.pending.main_thread && PyThread_get_thread_ident() != _PyRuntime.ceval.pending.main_thread) { return 0; } - /* don't perform recursive pending calls */ - if (_PyRuntime.ceval.pending.busy) - return 0; - _PyRuntime.ceval.pending.busy = 1; - /* unsignal before starting to call callbacks, so that any callback - added in-between re-signals */ - UNSIGNAL_PENDING_CALLS(); - /* Python signal handler doesn't really queue a callback: it only signals - that a signal was received, see _PyEval_SignalReceived(). */ - if (PyErr_CheckSignals() < 0) { - goto error; + if (!_PyRuntime.ceval.pending.lock) { + /* initial allocation of the lock */ + _PyRuntime.ceval.pending.lock = PyThread_allocate_lock(); + if (_PyRuntime.ceval.pending.lock == NULL) + return -1; } /* perform a bounded number of calls, in case of recursion */ - for (i=0; i Date: Fri, 28 Sep 2018 13:20:55 -0600 Subject: [PATCH 06/11] Explicitly handle signals and pending calls separately in the eval loop. --- Include/internal/pycore_ceval.h | 5 +++++ Python/ceval.c | 33 +++++++++++++++++++++------------ 2 files changed, 26 insertions(+), 12 deletions(-) diff --git a/Include/internal/pycore_ceval.h b/Include/internal/pycore_ceval.h index 52fda4b468f8f8..4c03a16e222e96 100644 --- a/Include/internal/pycore_ceval.h +++ b/Include/internal/pycore_ceval.h @@ -13,6 +13,11 @@ extern "C" { struct _pending_calls { unsigned long main_thread; + /* "busy" is used to avoid reentrant triggering of pending call + handling, including signals. + coordinate between the eval loop and + Py_MakePendingCalls(). Consequently, it may be removed if the + latter is removed. */ int busy; PyThread_type_lock lock; /* Request for running pending calls. */ diff --git a/Python/ceval.c b/Python/ceval.c index f87d9e2c537477..89247cfbcce78f 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -433,8 +433,9 @@ Py_MakePendingCalls(void) assert(PyGILState_Check()); /* don't perform recursive pending calls */ - if (_PyRuntime.ceval.pending.busy) + if (_PyRuntime.ceval.pending.busy) { return 0; + } _PyRuntime.ceval.pending.busy = 1; /* unsignal before starting to call callbacks, so that any callback added in-between re-signals */ @@ -991,17 +992,25 @@ _PyEval_EvalFrameDefault(PyFrameObject *f, int throwflag) */ goto fast_next_opcode; } - if (_Py_atomic_load_relaxed( - &_PyRuntime.ceval.signals_pending)) - { - if (Py_MakePendingCalls() < 0) - goto error; - } - if (_Py_atomic_load_relaxed( - &_PyRuntime.ceval.pending.calls_to_do)) - { - if (Py_MakePendingCalls() < 0) - goto error; + if (!_PyRuntime.ceval.pending.busy) { + _PyRuntime.ceval.pending.busy = 1; + if (_Py_atomic_load_relaxed( + &_PyRuntime.ceval.signals_pending)) + { + if (handle_signals() != 0) { + _PyRuntime.ceval.pending.busy = 0; + goto error; + } + } + if (_Py_atomic_load_relaxed( + &_PyRuntime.ceval.pending.calls_to_do)) + { + if (make_pending_calls() < 0) { + _PyRuntime.ceval.pending.busy = 0; + goto error; + } + } + _PyRuntime.ceval.pending.busy = 1; } if (_Py_atomic_load_relaxed( &_PyRuntime.ceval.gil_drop_request)) From 4830562fe01c98b90dd9e7d3415b3d2c58de9b6d Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Fri, 28 Sep 2018 14:49:28 -0600 Subject: [PATCH 07/11] Move "busy" back into make_pending_calls(). --- Include/internal/pycore_ceval.h | 6 ---- Python/ceval.c | 62 ++++++++++++++++++--------------- 2 files changed, 33 insertions(+), 35 deletions(-) diff --git a/Include/internal/pycore_ceval.h b/Include/internal/pycore_ceval.h index 4c03a16e222e96..b9f2d7d1758537 100644 --- a/Include/internal/pycore_ceval.h +++ b/Include/internal/pycore_ceval.h @@ -13,12 +13,6 @@ extern "C" { struct _pending_calls { unsigned long main_thread; - /* "busy" is used to avoid reentrant triggering of pending call - handling, including signals. - coordinate between the eval loop and - Py_MakePendingCalls(). Consequently, it may be removed if the - latter is removed. */ - int busy; PyThread_type_lock lock; /* Request for running pending calls. */ _Py_atomic_int calls_to_do; diff --git a/Python/ceval.c b/Python/ceval.c index 89247cfbcce78f..e04fd853166f7b 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -383,6 +383,8 @@ handle_signals(void) static int make_pending_calls(void) { + static int busy = 0; + /* only service pending calls on main thread */ if (_PyRuntime.ceval.pending.main_thread && PyThread_get_thread_ident() != _PyRuntime.ceval.pending.main_thread) @@ -390,11 +392,20 @@ make_pending_calls(void) return 0; } + /* don't perform recursive pending calls */ + if (busy) { + return 0; + } + busy = 1; + int res = 0; + if (!_PyRuntime.ceval.pending.lock) { /* initial allocation of the lock */ _PyRuntime.ceval.pending.lock = PyThread_allocate_lock(); - if (_PyRuntime.ceval.pending.lock == NULL) + if (_PyRuntime.ceval.pending.lock == NULL) { + busy = 0; return -1; + } } /* perform a bounded number of calls, in case of recursion */ @@ -417,12 +428,14 @@ make_pending_calls(void) /* having released the lock, perform the callback */ if (func == NULL) break; - int r = func(arg); - if (r) { - return r; + res = func(arg); + if (res) { + break; } } - return 0; + + busy = 0; + return res; } int @@ -432,11 +445,6 @@ Py_MakePendingCalls(void) assert(PyGILState_Check()); - /* don't perform recursive pending calls */ - if (_PyRuntime.ceval.pending.busy) { - return 0; - } - _PyRuntime.ceval.pending.busy = 1; /* unsignal before starting to call callbacks, so that any callback added in-between re-signals */ UNSIGNAL_PENDING_CALLS(); @@ -453,11 +461,9 @@ Py_MakePendingCalls(void) goto error; } - _PyRuntime.ceval.pending.busy = 0; return 0; error: - _PyRuntime.ceval.pending.busy = 0; SIGNAL_PENDING_CALLS(); /* We're not done yet */ return res; } @@ -992,26 +998,24 @@ _PyEval_EvalFrameDefault(PyFrameObject *f, int throwflag) */ goto fast_next_opcode; } - if (!_PyRuntime.ceval.pending.busy) { - _PyRuntime.ceval.pending.busy = 1; - if (_Py_atomic_load_relaxed( - &_PyRuntime.ceval.signals_pending)) - { - if (handle_signals() != 0) { - _PyRuntime.ceval.pending.busy = 0; - goto error; - } + + if (_Py_atomic_load_relaxed( + &_PyRuntime.ceval.signals_pending)) + { + if (handle_signals() != 0) { + goto error; } - if (_Py_atomic_load_relaxed( - &_PyRuntime.ceval.pending.calls_to_do)) - { - if (make_pending_calls() < 0) { - _PyRuntime.ceval.pending.busy = 0; - goto error; - } + } + if (_Py_atomic_load_relaxed( + &_PyRuntime.ceval.pending.calls_to_do)) + { + UNSIGNAL_PENDING_CALLS(); + if (make_pending_calls() != 0) { + SIGNAL_PENDING_CALLS(); + goto error; } - _PyRuntime.ceval.pending.busy = 1; } + if (_Py_atomic_load_relaxed( &_PyRuntime.ceval.gil_drop_request)) { From a4a87c53ea3fed3770bf9502f5f18bcdc3309c48 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Fri, 28 Sep 2018 15:00:43 -0600 Subject: [PATCH 08/11] Handle the pending calls flag exclusively in make_pending_calls(). --- Python/ceval.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/Python/ceval.c b/Python/ceval.c index e04fd853166f7b..d24559a68f9fdc 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -397,14 +397,17 @@ make_pending_calls(void) return 0; } busy = 1; + /* unsignal before starting to call callbacks, so that any callback + added in-between re-signals */ + UNSIGNAL_PENDING_CALLS(); int res = 0; if (!_PyRuntime.ceval.pending.lock) { /* initial allocation of the lock */ _PyRuntime.ceval.pending.lock = PyThread_allocate_lock(); if (_PyRuntime.ceval.pending.lock == NULL) { - busy = 0; - return -1; + res = -1; + goto error; } } @@ -430,14 +433,21 @@ make_pending_calls(void) break; res = func(arg); if (res) { - break; + goto error; } } busy = 0; return res; + +error: + busy = 0; + SIGNAL_PENDING_CALLS(); + return res; } +/* Py_MakePendingCalls() is a simple wrapper for the sake + of backward-compatibility. */ int Py_MakePendingCalls(void) { @@ -445,27 +455,19 @@ Py_MakePendingCalls(void) assert(PyGILState_Check()); - /* unsignal before starting to call callbacks, so that any callback - added in-between re-signals */ - UNSIGNAL_PENDING_CALLS(); - /* Python signal handler doesn't really queue a callback: it only signals that a signal was received, see _PyEval_SignalReceived(). */ res = handle_signals(); if (res != 0) { - goto error; + return res; } res = make_pending_calls(); if (res != 0) { - goto error; + return res; } return 0; - -error: - SIGNAL_PENDING_CALLS(); /* We're not done yet */ - return res; } /* The interpreter's recursion limit */ @@ -1009,9 +1011,7 @@ _PyEval_EvalFrameDefault(PyFrameObject *f, int throwflag) if (_Py_atomic_load_relaxed( &_PyRuntime.ceval.pending.calls_to_do)) { - UNSIGNAL_PENDING_CALLS(); if (make_pending_calls() != 0) { - SIGNAL_PENDING_CALLS(); goto error; } } From 6c3e86fa09ff454513351d35b10529f0fb9f5c77 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 5 Dec 2018 15:48:14 -0800 Subject: [PATCH 09/11] Simplify. --- Python/ceval.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Python/ceval.c b/Python/ceval.c index d24559a68f9fdc..3a214a043ae6cb 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -451,13 +451,11 @@ make_pending_calls(void) int Py_MakePendingCalls(void) { - int res = 0; - assert(PyGILState_Check()); /* Python signal handler doesn't really queue a callback: it only signals that a signal was received, see _PyEval_SignalReceived(). */ - res = handle_signals(); + int res = handle_signals(); if (res != 0) { return res; } From f1ea87f3cb467963a8ed2339c719e10f1d763b10 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 5 Dec 2018 16:24:23 -0800 Subject: [PATCH 10/11] Add a NEWS entry. --- .../Core and Builtins/2018-12-05-16-24-05.bpo-35423.UIie_O.rst | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2018-12-05-16-24-05.bpo-35423.UIie_O.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2018-12-05-16-24-05.bpo-35423.UIie_O.rst b/Misc/NEWS.d/next/Core and Builtins/2018-12-05-16-24-05.bpo-35423.UIie_O.rst new file mode 100644 index 00000000000000..271ec48b34752d --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2018-12-05-16-24-05.bpo-35423.UIie_O.rst @@ -0,0 +1,3 @@ +Separate the signal handling trigger in the eval loop from the "pending +calls" machinery. There is no semantic change and the difference in +performance is insignificant. From c4f4c1d4e79fab2713157815711bac739d2102a1 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Fri, 11 Jan 2019 13:00:30 -0700 Subject: [PATCH 11/11] Ensure signals only get checked in the main thread. --- Python/ceval.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Python/ceval.c b/Python/ceval.c index 3a214a043ae6cb..d18b8a62daed82 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -372,6 +372,13 @@ Py_AddPendingCall(int (*func)(void *), void *arg) static int handle_signals(void) { + /* Only handle signals on main thread. */ + if (_PyRuntime.ceval.pending.main_thread && + PyThread_get_thread_ident() != _PyRuntime.ceval.pending.main_thread) + { + return 0; + } + UNSIGNAL_PENDING_SIGNALS(); if (PyErr_CheckSignals() < 0) { SIGNAL_PENDING_SIGNALS(); /* We're not done yet */