From 902ab80b590e474bb2077b1fae8aac497b856d66 Mon Sep 17 00:00:00 2001 From: "Nathaniel J. Smith" Date: Sun, 17 Dec 2017 20:10:18 -0800 Subject: [PATCH] bpo-30050: Allow disabling full buffer warnings in signal.set_wakeup_fd (#4792) --- Doc/library/signal.rst | 28 ++++- Lib/test/test_signal.py | 92 ++++++++++++++++ .../2017-12-10-23-44-56.bpo-30050.4SZ3lY.rst | 3 + Modules/signalmodule.c | 102 +++++++++--------- 4 files changed, 169 insertions(+), 56 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2017-12-10-23-44-56.bpo-30050.4SZ3lY.rst diff --git a/Doc/library/signal.rst b/Doc/library/signal.rst index 2bc39d9f133..67eaa2c6381 100644 --- a/Doc/library/signal.rst +++ b/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) diff --git a/Lib/test/test_signal.py b/Lib/test/test_signal.py index dc048e56665..f17123c3bb5 100644 --- a/Lib/test/test_signal.py +++ b/Lib/test/test_signal.py @@ -91,6 +91,15 @@ class WindowsSignalTests(unittest.TestCase): 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 @@ class WakeupSocketSignalTests(unittest.TestCase): """.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): diff --git a/Misc/NEWS.d/next/Library/2017-12-10-23-44-56.bpo-30050.4SZ3lY.rst b/Misc/NEWS.d/next/Library/2017-12-10-23-44-56.bpo-30050.4SZ3lY.rst new file mode 100644 index 00000000000..76de12bbe80 --- /dev/null +++ b/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. diff --git a/Modules/signalmodule.c b/Modules/signalmodule.c index 1023244b626..b553eedc0f2 100644 --- a/Modules/signalmodule.c +++ b/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); + rc = send(fd, &byte, 1, 0); - /* 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); + 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