[Python-checkins] bpo-35423: Stop using the "pending calls" machinery for signals. (gh-10972)

Eric Snow webhook-mailer at python.org
Fri Jan 11 16:27:08 EST 2019


https://github.com/python/cpython/commit/fdf282d609fd172d52b59a6f1f062eb701494528
commit: fdf282d609fd172d52b59a6f1f062eb701494528
branch: master
author: Eric Snow <ericsnowcurrently at gmail.com>
committer: GitHub <noreply at github.com>
date: 2019-01-11T14:26:55-07:00
summary:

bpo-35423: Stop using the "pending calls" machinery for signals. (gh-10972)

This change separates 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.

The change makes both components less confusing. It also eliminates the risk of changes to the pending calls affecting signal handling. This is particularly relevant for some upcoming pending calls changes I have in the works.

files:
A Misc/NEWS.d/next/Core and Builtins/2018-12-05-16-24-05.bpo-35423.UIie_O.rst
M Include/internal/pycore_ceval.h
M Modules/signalmodule.c
M Python/ceval.c

diff --git a/Include/internal/pycore_ceval.h b/Include/internal/pycore_ceval.h
index c8c63b1c7fda..b9f2d7d17585 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/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 000000000000..271ec48b3475
--- /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.
diff --git a/Modules/signalmodule.c b/Modules/signalmodule.c
index 4f8f71a0a1df..9d49cbd14400 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 */
diff --git a/Python/ceval.c b/Python/ceval.c
index de6ff2994550..3e82ceb95251 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; \
@@ -306,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
@@ -356,21 +369,28 @@ Py_AddPendingCall(int (*func)(void *), void *arg)
     return result;
 }
 
-int
-Py_MakePendingCalls(void)
+static int
+handle_signals(void)
 {
-    static int busy = 0;
-    int i;
-    int r = 0;
-
-    assert(PyGILState_Check());
+    /* Only handle signals on main thread. */
+    if (_PyRuntime.ceval.pending.main_thread &&
+        PyThread_get_thread_ident() != _PyRuntime.ceval.pending.main_thread)
+    {
+        return 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)
+{
+    static int busy = 0;
 
     /* only service pending calls on main thread */
     if (_PyRuntime.ceval.pending.main_thread &&
@@ -378,22 +398,28 @@ Py_MakePendingCalls(void)
     {
         return 0;
     }
+
     /* don't perform recursive pending calls */
-    if (busy)
+    if (busy) {
         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;
 
-    /* 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) {
+            res = -1;
+            goto error;
+        }
     }
 
     /* perform a bounded number of calls, in case of recursion */
-    for (i=0; i<NPENDINGCALLS; i++) {
+    for (int i=0; i<NPENDINGCALLS; i++) {
         int j;
         int (*func)(void *);
         void *arg = NULL;
@@ -412,19 +438,41 @@ Py_MakePendingCalls(void)
         /* having released the lock, perform the callback */
         if (func == NULL)
             break;
-        r = func(arg);
-        if (r) {
+        res = func(arg);
+        if (res) {
             goto error;
         }
     }
 
     busy = 0;
-    return r;
+    return res;
 
 error:
     busy = 0;
-    SIGNAL_PENDING_CALLS(); /* We're not done yet */
-    return -1;
+    SIGNAL_PENDING_CALLS();
+    return res;
+}
+
+/* Py_MakePendingCalls() is a simple wrapper for the sake
+   of backward-compatibility. */
+int
+Py_MakePendingCalls(void)
+{
+    assert(PyGILState_Check());
+
+    /* Python signal handler doesn't really queue a callback: it only signals
+       that a signal was received, see _PyEval_SignalReceived(). */
+    int res = handle_signals();
+    if (res != 0) {
+        return res;
+    }
+
+    res = make_pending_calls();
+    if (res != 0) {
+        return res;
+    }
+
+    return 0;
 }
 
 /* The interpreter's recursion limit */
@@ -957,12 +1005,22 @@ _PyEval_EvalFrameDefault(PyFrameObject *f, int throwflag)
                 */
                 goto fast_next_opcode;
             }
+
+            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 (Py_MakePendingCalls() < 0)
+                if (make_pending_calls() != 0) {
                     goto error;
+                }
             }
+
             if (_Py_atomic_load_relaxed(
                         &_PyRuntime.ceval.gil_drop_request))
             {



More information about the Python-checkins mailing list