bpo-30050: Allow disabling full buffer warnings in signal.set_wakeup_fd (#4792)

Author: njsmith

Doc/library/signal.rst

@@ -300,7 +300,7 @@ The :mod:`signal` module defines the following functions:
    Availability: Unix.
 
 
-.. function:: set_wakeup_fd(fd)
+.. function:: set_wakeup_fd(fd, *, warn_on_full_buffer=True)
 
    Set the wakeup file descriptor to *fd*.  When a signal is received, the
    signal number is written as a single byte into the fd.  This can be used by
@@ -312,16 +312,36 @@ The :mod:`signal` module defines the following functions:
    If not -1, *fd* must be non-blocking.  It is up to the library to remove
    any bytes from *fd* before calling poll or select again.
 
-   Use for example ``struct.unpack('%uB' % len(data), data)`` to decode the
-   signal numbers list.
-
    When threads are enabled, this function can only be called from the main thread;
    attempting to call it from other threads will cause a :exc:`ValueError`
    exception to be raised.
 
+   There are two common ways to use this function. In both approaches,
+   you use the fd to wake up when a signal arrives, but then they
+   differ in how they determine *which* signal or signals have
+   arrived.
+
+   In the first approach, we read the data out of the fd's buffer, and
+   the byte values give you the signal numbers. This is simple, but in
+   rare cases it can run into a problem: generally the fd will have a
+   limited amount of buffer space, and if too many signals arrive too
+   quickly, then the buffer may become full, and some signals may be
+   lost. If you use this approach, then you should set
+   ``warn_on_full_buffer=True``, which will at least cause a warning
+   to be printed to stderr when signals are lost.
+
+   In the second approach, we use the wakeup fd *only* for wakeups,
+   and ignore the actual byte values. In this case, all we care about
+   is whether the fd's buffer is empty or non-empty; a full buffer
+   doesn't indicate a problem at all. If you use this approach, then
+   you should set ``warn_on_full_buffer=False``, so that your users
+   are not confused by spurious warning messages.
+
    .. versionchanged:: 3.5
       On Windows, the function now also supports socket handles.
 
+   .. versionchanged:: 3.7
+      Added ``warn_on_full_buffer`` parameter.
 
 .. function:: siginterrupt(signalnum, flag)
 

Lib/test/test_signal.py

@@ -91,6 +91,15 @@ def test_issue9324(self):
 
 class WakeupFDTests(unittest.TestCase):
 
+    def test_invalid_call(self):
+        # First parameter is positional-only
+        with self.assertRaises(TypeError):
+            signal.set_wakeup_fd(signum=signal.SIGINT)
+
+        # warn_on_full_buffer is a keyword-only parameter
+        with self.assertRaises(TypeError):
+            signal.set_wakeup_fd(signal.SIGINT, False)
+
     def test_invalid_fd(self):
         fd = support.make_bad_fd()
         self.assertRaises((ValueError, OSError),
@@ -423,6 +432,89 @@ def handler(signum, frame):
         """.format(action=action)
         assert_python_ok('-c', code)
 
+    @unittest.skipIf(_testcapi is None, 'need _testcapi')
+    def test_warn_on_full_buffer(self):
+        # Use a subprocess to have only one thread.
+        if os.name == 'nt':
+            action = 'send'
+        else:
+            action = 'write'
+        code = """if 1:
+        import errno
+        import signal
+        import socket
+        import sys
+        import time
+        import _testcapi
+        from test.support import captured_stderr
+
+        signum = signal.SIGINT
+
+        # This handler will be called, but we intentionally won't read from
+        # the wakeup fd.
+        def handler(signum, frame):
+            pass
+
+        signal.signal(signum, handler)
+
+        read, write = socket.socketpair()
+        read.setblocking(False)
+        write.setblocking(False)
+
+        # Fill the send buffer
+        try:
+            while True:
+                write.send(b"x")
+        except BlockingIOError:
+            pass
+
+        # By default, we get a warning when a signal arrives
+        signal.set_wakeup_fd(write.fileno())
+
+        with captured_stderr() as err:
+            _testcapi.raise_signal(signum)
+
+        err = err.getvalue()
+        if ('Exception ignored when trying to {action} to the signal wakeup fd'
+            not in err):
+            raise AssertionError(err)
+
+        # And also if warn_on_full_buffer=True
+        signal.set_wakeup_fd(write.fileno(), warn_on_full_buffer=True)
+
+        with captured_stderr() as err:
+            _testcapi.raise_signal(signum)
+
+        err = err.getvalue()
+        if ('Exception ignored when trying to {action} to the signal wakeup fd'
+            not in err):
+            raise AssertionError(err)
+
+        # But not if warn_on_full_buffer=False
+        signal.set_wakeup_fd(write.fileno(), warn_on_full_buffer=False)
+
+        with captured_stderr() as err:
+            _testcapi.raise_signal(signum)
+
+        err = err.getvalue()
+        if err != "":
+            raise AssertionError("got unexpected output %r" % (err,))
+
+        # And then check the default again, to make sure warn_on_full_buffer
+        # settings don't leak across calls.
+        signal.set_wakeup_fd(write.fileno())
+
+        with captured_stderr() as err:
+            _testcapi.raise_signal(signum)
+
+        err = err.getvalue()
+        if ('Exception ignored when trying to {action} to the signal wakeup fd'
+            not in err):
+            raise AssertionError(err)
+
+        """.format(action=action)
+        assert_python_ok('-c', code)
+
 
 @unittest.skipIf(sys.platform == "win32", "Not valid on Windows")
 class SiginterruptTest(unittest.TestCase):

Misc/NEWS.d/next/Library/2017-12-10-23-44-56.bpo-30050.4SZ3lY.rst

@@ -0,0 +1,3 @@
+New argument warn_on_full_buffer to signal.set_wakeup_fd lets you control
+whether Python prints a warning on stderr when the wakeup fd buffer
+overflows.

Modules/signalmodule.c

@@ -100,14 +100,15 @@ static volatile struct {
 
 static volatile struct {
     SOCKET_T fd;
+    int warn_on_full_buffer;
     int use_send;
-    int send_err_set;
-    int send_errno;
-    int send_win_error;
-} wakeup = {INVALID_FD, 0, 0};
+} wakeup = {.fd = INVALID_FD, .warn_on_full_buffer = 1, .use_send = 0};
 #else
 #define INVALID_FD (-1)
-static volatile sig_atomic_t wakeup_fd = -1;
+static volatile struct {
+    sig_atomic_t fd;
+    int warn_on_full_buffer;
+} wakeup = {.fd = INVALID_FD, .warn_on_full_buffer = 1};
 #endif
 
 /* Speed up sigcheck() when none tripped */
@@ -210,28 +211,15 @@ report_wakeup_write_error(void *data)
 
 #ifdef MS_WINDOWS
 static int
-report_wakeup_send_error(void* Py_UNUSED(data))
+report_wakeup_send_error(void* data)
 {
-    PyObject *res;
-
-    if (wakeup.send_win_error) {
-        /* PyErr_SetExcFromWindowsErr() invokes FormatMessage() which
-           recognizes the error codes used by both GetLastError() and
-           WSAGetLastError */
-        res = PyErr_SetExcFromWindowsErr(PyExc_OSError, wakeup.send_win_error);
-    }
-    else {
-        errno = wakeup.send_errno;
-        res = PyErr_SetFromErrno(PyExc_OSError);
-    }
-
-    assert(res == NULL);
-    wakeup.send_err_set = 0;
-
+    /* PyErr_SetExcFromWindowsErr() invokes FormatMessage() which
+       recognizes the error codes used by both GetLastError() and
+       WSAGetLastError */
+    PyErr_SetExcFromWindowsErr(PyExc_OSError, (int) (intptr_t) data);
     PySys_WriteStderr("Exception ignored when trying to send to the "
                       "signal wakeup fd:\n");
     PyErr_WriteUnraisable(NULL);
-
     return 0;
 }
 #endif   /* MS_WINDOWS */
@@ -257,7 +245,7 @@ trip_signal(int sig_num)
        and then set the flag, but this allowed the following sequence of events
        (especially on windows, where trip_signal may run in a new thread):
 
-       - main thread blocks on select([wakeup_fd], ...)
+       - main thread blocks on select([wakeup.fd], ...)
        - signal arrives
        - trip_signal writes to the wakeup fd
        - the main thread wakes up
@@ -274,41 +262,43 @@ trip_signal(int sig_num)
 #ifdef MS_WINDOWS
     fd = Py_SAFE_DOWNCAST(wakeup.fd, SOCKET_T, int);
 #else
-    fd = wakeup_fd;
+    fd = wakeup.fd;
 #endif
 
     if (fd != INVALID_FD) {
         byte = (unsigned char)sig_num;
 #ifdef MS_WINDOWS
         if (wakeup.use_send) {
-            do {
-                rc = send(fd, &byte, 1, 0);
-            } while (rc < 0 && errno == EINTR);
-
-            /* we only have a storage for one error in the wakeup structure */
-            if (rc < 0 && !wakeup.send_err_set) {
-                wakeup.send_err_set = 1;
-                wakeup.send_errno = errno;
-                wakeup.send_win_error = GetLastError();
-                /* Py_AddPendingCall() isn't signal-safe, but we
-                   still use it for this exceptional case. */
-                Py_AddPendingCall(report_wakeup_send_error, NULL);
+            rc = send(fd, &byte, 1, 0);
+
+            if (rc < 0) {
+                int last_error = GetLastError();
+                if (wakeup.warn_on_full_buffer ||
+                    last_error != WSAEWOULDBLOCK)
+                {
+                    /* Py_AddPendingCall() isn't signal-safe, but we
+                       still use it for this exceptional case. */
+                    Py_AddPendingCall(report_wakeup_send_error,
+                                      (void *)(intptr_t) last_error);
+                }
             }
         }
         else
 #endif
         {
-            byte = (unsigned char)sig_num;
-
             /* _Py_write_noraise() retries write() if write() is interrupted by
                a signal (fails with EINTR). */
             rc = _Py_write_noraise(fd, &byte, 1);
 
             if (rc < 0) {
-                /* Py_AddPendingCall() isn't signal-safe, but we
-                   still use it for this exceptional case. */
-                Py_AddPendingCall(report_wakeup_write_error,
-                                  (void *)(intptr_t)errno);
+                if (wakeup.warn_on_full_buffer ||
+                    (errno != EWOULDBLOCK && errno != EAGAIN))
+                {
+                    /* Py_AddPendingCall() isn't signal-safe, but we
+                       still use it for this exceptional case. */
+                    Py_AddPendingCall(report_wakeup_write_error,
+                                      (void *)(intptr_t)errno);
+                }
             }
         }
     }
@@ -549,9 +539,13 @@ signal_siginterrupt_impl(PyObject *module, int signalnum, int flag)
 
 
 static PyObject*
-signal_set_wakeup_fd(PyObject *self, PyObject *args)
+signal_set_wakeup_fd(PyObject *self, PyObject *args, PyObject *kwds)
 {
     struct _Py_stat_struct status;
+    static char *kwlist[] = {
+        "", "warn_on_full_buffer", NULL,
+    };
+    int warn_on_full_buffer = 1;
 #ifdef MS_WINDOWS
     PyObject *fdobj;
     SOCKET_T sockfd, old_sockfd;
@@ -560,7 +554,8 @@ signal_set_wakeup_fd(PyObject *self, PyObject *args)
     PyObject *mod;
     int is_socket;
 
-    if (!PyArg_ParseTuple(args, "O:set_wakeup_fd", &fdobj))
+    if (!PyArg_ParseTupleAndKeywords(args, kwds, "O|$p:set_wakeup_fd", kwlist,
+                                     &fdobj, &warn_on_full_buffer))
         return NULL;
 
     sockfd = PyLong_AsSocket_t(fdobj);
@@ -569,7 +564,8 @@ signal_set_wakeup_fd(PyObject *self, PyObject *args)
 #else
     int fd, old_fd;
 
-    if (!PyArg_ParseTuple(args, "i:set_wakeup_fd", &fd))
+    if (!PyArg_ParseTupleAndKeywords(args, kwds, "i|$p:set_wakeup_fd", kwlist,
+                                     &fd, &warn_on_full_buffer))
         return NULL;
 #endif
 
@@ -620,6 +616,7 @@ signal_set_wakeup_fd(PyObject *self, PyObject *args)
 
     old_sockfd = wakeup.fd;
     wakeup.fd = sockfd;
+    wakeup.warn_on_full_buffer = warn_on_full_buffer;
     wakeup.use_send = is_socket;
 
     if (old_sockfd != INVALID_FD)
@@ -644,15 +641,16 @@ signal_set_wakeup_fd(PyObject *self, PyObject *args)
         }
     }
 
-    old_fd = wakeup_fd;
-    wakeup_fd = fd;
+    old_fd = wakeup.fd;
+    wakeup.fd = fd;
+    wakeup.warn_on_full_buffer = warn_on_full_buffer;
 
     return PyLong_FromLong(old_fd);
 #endif
 }
 
 PyDoc_STRVAR(set_wakeup_fd_doc,
-"set_wakeup_fd(fd) -> fd\n\
+"set_wakeup_fd(fd, *, warn_on_full_buffer=True) -> fd\n\
 \n\
 Sets the fd to be written to (with the signal number) when a signal\n\
 comes in.  A library can use this to wakeup select or poll.\n\
@@ -670,11 +668,11 @@ PySignal_SetWakeupFd(int fd)
 
 #ifdef MS_WINDOWS
     old_fd = Py_SAFE_DOWNCAST(wakeup.fd, SOCKET_T, int);
-    wakeup.fd = fd;
 #else
-    old_fd = wakeup_fd;
-    wakeup_fd = fd;
+    old_fd = wakeup.fd;
 #endif
+    wakeup.fd = fd;
+    wakeup.warn_on_full_buffer = 1;
     return old_fd;
 }
 
@@ -1155,7 +1153,7 @@ static PyMethodDef signal_methods[] = {
     SIGNAL_GETITIMER_METHODDEF
     SIGNAL_SIGNAL_METHODDEF
     SIGNAL_GETSIGNAL_METHODDEF
-    {"set_wakeup_fd",           signal_set_wakeup_fd, METH_VARARGS, set_wakeup_fd_doc},
+    {"set_wakeup_fd", (PyCFunction)signal_set_wakeup_fd, METH_VARARGS | METH_KEYWORDS, set_wakeup_fd_doc},
     SIGNAL_SIGINTERRUPT_METHODDEF
     SIGNAL_PAUSE_METHODDEF
     SIGNAL_PTHREAD_KILL_METHODDEF