Interrupted system call when calling print() to stderr.

[ambrop72]

This test program crashes with a high probability when I hit CTRL+C.

from __future__ import print_function
import sys
import traceback
import signal
import pyuv

def signal_cb(handle, signum):
    loop.stop()

def timer_cb (timer):
    try:
        timer.start(timer_cb, 0.1, False)
        for i in range(1000):
            print(''.join('LALALALALALALALALALALALALALALALA' for i in range(10)), file=sys.stderr)
    except:
        traceback.print_exc()
        sys.exit(1)

loop = pyuv.Loop()
clients = []

signal_h = pyuv.Signal(loop)
signal_h.start(signal_cb, signal.SIGINT)

timer = pyuv.Timer(loop)
timer.start(timer_cb, 0, False)

loop.run()

File "/home/ambro/test2.py", line 14, in timer_cb
    print(''.join('LALALALALALALALALALALALALALALALA' for i in range(10)), file=sys.stderr)
IOError: [Errno 4] Interrupted system call
Unhandled exception in callback

[ambrop72]

It seems that printing to stderr is a precondition for the crash, stdout works.

[ambrop72]

Forgot to mention: This is with Python 2.7.8, pyuv 0.11.5, libuv 0.11.29 (setup script patched to use external libuv), NixOS distro.

[ambrop72]

Actually, in my application I also have issues with printing to stdout, not just stderr. The stdout problems are related to using pyuv.Process and the resulting SIGCHLD.

[saghul]

This is not an actual problem, what happens is that the program is interrupted while in the middle of a callback, and the Loop has a exception hook which prints exceptions to stderr. I just noticed it cannot be disabled, so I'll fix that.

[saghul]

Actually, in my application I also have issues with printing to stdout, not just stderr. The stdout problems are related to using pyuv.Process and the resulting SIGCHLD.

What issues do you have?

[ambrop72]

My program logs messages to standard output using print(). Occasionally when a child terminates while the program is inside a print(), the print() will raise an exception (EINTR = Interrupted system call, as seen above). Who knows what else is going to fail randomly just because a child process terminated at an unlucky time.

It should be easy to modify the test case I provided to reproduce the issue for child programs (e.g. spawn a child every once in a while).

I think the general solution is to fix libuv to not use signal handlers and pipes to handle signals, but to use signalfd or kqueue. With these facilities, signals can be blocked using sigprocmask() but still received by the event loop, and there is no chance for syscalls to randomly fail with EINTR.

[ambrop72]

Here's the test case. On my system it dies in a matter of seconds.
Note that this happens on its own due to a child terminating. CTRL+C was never hit.

from __future__ import print_function
import sys
import traceback
import signal
import pyuv

def signal_cb(handle, signum):
    loop.stop()

def timer_cb (redundant):
    try:
        timer.start(timer_cb, 0.1, False)
        for i in range(100):
            print(''.join('LALALALALALA' for i in range(10)), file=sys.stdout)
    except:
        traceback.print_exc()
        sys.exit(1)

def spawn ():
    proc = pyuv.Process(loop)
    proc.spawn(executable='/usr/bin/env', args=['env', 'sleep', '0.01'], exit_callback=proc_handler)

def proc_handler (proc, a, b):
    proc.close()
    spawn()

loop = pyuv.Loop()

signal_h = pyuv.Signal(loop)
signal_h.start(signal_cb, signal.SIGINT)

timer = pyuv.Timer(loop)
timer.start(timer_cb, 0, False)

spawn()

loop.run()

Traceback (most recent call last):
  File "/home/ambro/test.py", line 14, in timer_cb
    print(''.join('LALALALALALA' for i in range(10)), file=sys.stdout)
IOError: [Errno 4] Interrupted system call
Unhandled exception in callback

[ambrop72]

I made a prototype implementation of signalfd in libuv which seems to fix the test case. I create a global signal handling thread, which reads signals using signalfd, and dispatches them by calling the signal handler function (it seems to be doing the right thing...). The signals are blocked in event loops so they can be consumed by signalfd and so they don't cause EINTR (more about that see below).

(Now after I've written this, I've realized that the use of signalfd is not strictly necessary. The signal handling thread could just stand there idle and receive signals via signal handlers, or with sigwaitinfo.)

But there's a problem regarding blocking signals. Currently, when uv__signal_unregister_handler/uv__signal_unregister_handler is called, the signal is blocked/unblocked only in the current thread. So e.g. if you have a second thread and you change signal watchers from the first thread, there will be problems.

I think these blocking problems can be fixed by blocking all signals in each event loop thread. In the single signal handling thread, they can be blocked or unblocked depending on implementation (signalfd or not).

diff --git a/configure.ac b/configure.ac
index 4de66f7..4d25576 100644
--- a/configure.ac
+++ b/configure.ac
@@ -37,6 +37,7 @@ AC_CHECK_LIB([pthread], [pthread_mutex_init])
 AC_CHECK_LIB([rt], [clock_gettime])
 AC_CHECK_LIB([sendfile], [sendfile])
 AC_CHECK_LIB([socket], [socket])
+AC_CHECK_HEADER(sys/signalfd.h, [AC_CHECK_FUNC(signalfd, [AC_DEFINE(HAVE_SIGNALFD)])])
 AC_SYS_LARGEFILE
 AM_CONDITIONAL([AIX],    [AS_CASE([$host_os],[aix*],          [true], [false])])
 AM_CONDITIONAL([ANDROID],[AS_CASE([$host_os],[linux-android*],[true], [false])])
diff --git a/src/unix/signal.c b/src/unix/signal.c
index 0b7a405..be47fba 100644
--- a/src/unix/signal.c
+++ b/src/unix/signal.c
@@ -27,6 +27,9 @@
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
+#ifdef HAVE_SIGNALFD
+#include <sys/signalfd.h>
+#endif


 typedef struct {
@@ -41,12 +44,29 @@ static int uv__signal_unlock(void);
 static void uv__signal_event(uv_loop_t* loop, uv__io_t* w, unsigned int events);
 static int uv__signal_compare(uv_signal_t* w1, uv_signal_t* w2);
 static void uv__signal_stop(uv_signal_t* handle);
+#ifdef HAVE_SIGNALFD
+static void uv__signal_thread_proc (void *arg);
+#endif


 static pthread_once_t uv__signal_global_init_guard = PTHREAD_ONCE_INIT;
 static struct uv__signal_tree_s uv__signal_tree =
     RB_INITIALIZER(uv__signal_tree);
 static int uv__signal_lock_pipefd[2];
+#ifdef HAVE_SIGNALFD
+static sigset_t uv__signal_signalfd_mask;
+static int uv__signal_signalfd;
+static uv_thread_t uv__signal_thread;
+#endif
+
+
+#if HAVE_SIGNALFD
+#define IF_NOT_SIGNALFD if (uv__signal_signalfd < 0) {
+#define ENDIF_NOT_SIGNALFD }
+#else
+#define IF_NOT_SIGNALFD
+#define ENDIF_NOT_SIGNALFD
+#endif


 RB_GENERATE_STATIC(uv__signal_tree_s,
@@ -55,6 +75,33 @@ RB_GENERATE_STATIC(uv__signal_tree_s,


 static void uv__signal_global_init(void) {
+#ifdef HAVE_SIGNALFD
+  sigset_t ss;
+  sigset_t prev_ss;
+
+  // Try to create the signalfd file descriptor.
+  sigemptyset(&uv__signal_signalfd_mask);
+  uv__signal_signalfd = signalfd(-1, &uv__signal_signalfd_mask, SFD_CLOEXEC);
+  if (uv__signal_signalfd < 0) {
+    goto no_signalfd;
+  }
+
+  // Ensure that all signals are blocked in the signald thread.
+  sigfillset(&ss);
+  if (pthread_sigmask(SIG_SETMASK, &ss, &prev_ss))
+    abort();
+
+  // Create signalfd thread.
+  if (uv_thread_create(&uv__signal_thread, uv__signal_thread_proc, NULL))
+    abort();
+
+  // Restore signal mask now that the thread is created.
+  if (pthread_sigmask(SIG_SETMASK, &prev_ss, NULL))
+    abort();
+
+no_signalfd:
+#endif
+
   if (uv__make_pipe(uv__signal_lock_pipefd, 0))
     abort();

@@ -96,12 +143,16 @@ static int uv__signal_unlock(void) {
 static void uv__signal_block_and_lock(sigset_t* saved_sigmask) {
   sigset_t new_mask;

+  IF_NOT_SIGNALFD
+
   if (sigfillset(&new_mask))
     abort();

   if (pthread_sigmask(SIG_SETMASK, &new_mask, saved_sigmask))
     abort();

+  ENDIF_NOT_SIGNALFD
+
   if (uv__signal_lock())
     abort();
 }
@@ -111,8 +162,12 @@ static void uv__signal_unlock_and_unblock(sigset_t* saved_sigmask) {
   if (uv__signal_unlock())
     abort();

+  IF_NOT_SIGNALFD
+
   if (pthread_sigmask(SIG_SETMASK, saved_sigmask, NULL))
     abort();
+
+  ENDIF_NOT_SIGNALFD
 }


@@ -178,6 +233,24 @@ static int uv__signal_register_handler(int signum) {
   /* When this function is called, the signal lock must be held. */
   struct sigaction sa;

+#ifdef HAVE_SIGNALFD
+  if (uv__signal_signalfd >= 0) {
+    sigset_t this_mask;
+
+    // Block the signal.
+    // Unfortunately this only affects the current thread.
+    sigemptyset(&this_mask);
+    sigaddset(&this_mask, signum);
+    if (pthread_sigmask(SIG_BLOCK, &this_mask, NULL))
+      abort();
+
+    // Update the signalfd signal set.
+    sigaddset(&uv__signal_signalfd_mask, signum);
+    if (signalfd(uv__signal_signalfd, &uv__signal_signalfd_mask, SFD_CLOEXEC) < 0)
+      abort();
+  } else {
+#endif
+
   /* XXX use a separate signal stack? */
   memset(&sa, 0, sizeof(sa));
   if (sigfillset(&sa.sa_mask))
@@ -188,6 +261,10 @@ static int uv__signal_register_handler(int signum) {
   if (sigaction(signum, &sa, NULL))
     return -errno;

+#ifdef HAVE_SIGNALFD
+  }
+#endif
+
   return 0;
 }

@@ -196,6 +273,23 @@ static void uv__signal_unregister_handler(int signum) {
   /* When this function is called, the signal lock must be held. */
   struct sigaction sa;

+#ifdef HAVE_SIGNALFD
+  if (uv__signal_signalfd >= 0) {
+    sigset_t this_mask;
+
+    // Update the signalfd signal set.
+    sigdelset(&uv__signal_signalfd_mask, signum);
+    if (signalfd(uv__signal_signalfd, &uv__signal_signalfd_mask, SFD_CLOEXEC) < 0)
+      abort();
+
+    // Unblock the signal.
+    // Unfortunately this only affects the current thread.
+    sigemptyset(&this_mask);
+    sigaddset(&this_mask, signum);
+    if (pthread_sigmask(SIG_UNBLOCK, &this_mask, NULL))
+      abort();
+  } else {
+#endif
   memset(&sa, 0, sizeof(sa));
   sa.sa_handler = SIG_DFL;

@@ -205,6 +299,9 @@ static void uv__signal_unregister_handler(int signum) {
    */
   if (sigaction(signum, &sa, NULL))
     abort();
+#ifdef HAVE_SIGNALFD
+  }
+#endif
 }


@@ -463,3 +560,25 @@ static void uv__signal_stop(uv_signal_t* handle) {
   handle->signum = 0;
   uv__handle_stop(handle);
 }
+
+
+#ifdef HAVE_SIGNALFD
+
+static void uv__signal_thread_proc (void *arg) {
+  sigset_t block_ss;
+  struct signalfd_siginfo siginfo;
+  int res;
+
+  while (1) {
+    do {
+        res = read(uv__signal_signalfd, &siginfo, sizeof(siginfo));
+    } while (res < 0 && errno == EINTR);
+
+    if (res != sizeof(siginfo))
+      abort();
+
+    uv__signal_handler(siginfo.ssi_signo);
+  }
+}
+
+#endif

[saghul]

This is not an actual problem, what happens is that the program is interrupted while in the middle of a callback, and the Loop has a exception hook which prints exceptions to stderr. I just noticed it cannot be disabled, so I'll fix that.

Actually, that's not a good idea since it hides errors.

[saghul]

But there's a problem regarding blocking signals. Currently, when uv__signal_unregister_handler/uv__signal_unregister_handler is called, the signal is blocked/unblocked only in the current thread. So e.g. if you have a second thread and you change signal watchers from the first thread, there will be problems.

I'm not sure if this can happen since when those functions are called uv__signal_lock was called.

[saghul]

Hey @ambrop72 I think it's better if we move this over to libuv itself. Specially this comment with the attached patch: #173 (comment)

I didn't have much time, but here is a quick review of the patch:

• we don't use autoconf for feature detection, do the syscall manually and learn if signalfd is available, check linux-syscalls.c.
• don't use c++ style comments.

Thanks a lot for your comments!

[bnoordhuis]

The reason libuv doesn't use signalfd() is that it requires too much cooperation from the embedding application (which may not be a direct user of libuv; imagine an application that uses a library that uses libuv.)

With signalfd(), all threads in the process need to block the signal with pthread_sigmask() or else it may get delivered to another thread (and get lost.)

Likewise, if multiple threads are listening with a signal fd for the same signal, the receiving thread is selected essentially at random. If that receiver is not libuv, then the signal is, again, lost.

As to kqueue(), signal handlers take precedence over EVFILT_SIGNAL event listeners. If the application installs a signal handler, libuv won't get notified.

Applications can of course override libuv's signal handler but at least signal handlers can be chained, something you cannot do with EVFILT_SIGNAL.

Two secondary issues with EVFILT_SIGNAL are that it only works for process-wide signals, not per-thread signals, and that you only get a count of delivered signals; any auxiliary information is lost. That's not an issue per se but it would be mighty inconvenient if we were interested in, say, the si_pid field from the signal handler's siginfo_t parameter.

[ambrop72]

Yes, signalfd itself is not very helpful for libuv. What matters is that signals can be blocked in event loop (and other) threads so they will not cause random EINTR errors.

I've attempted a different approach where the signal code in libuv is unchanged. A function is provided which can be called by the application early in main. (please see the documentation of the function, so I don't have to repeat myself)

This approach is of course only applicable when the main application is based on libuv, not when libuv is used only in a library. For that scenario, the application/library is left to deal with the problem.

Patch for libuv:

diff --git a/include/uv.h b/include/uv.h
index 730aa15..ba4d5ec 100644
--- a/include/uv.h
+++ b/include/uv.h
@@ -2252,6 +2252,30 @@ UV_EXTERN extern uint64_t uv_hrtime(void);
 UV_EXTERN void uv_disable_stdio_inheritance(void);

 /*
+ * On Unix, this function can be called early from main, before any threads
+ * are started, to set up a safe environment with respect to signal handling.
+ * 
+ * This function does the following when it is called for the first time:
+ * - Starts a dummy thread, which consists of an infinite pause() loop.
+ * - Blocks all signals in the current thread.
+ * 
+ * Remember that signal masks are thread-specific and inherited at thread
+ * creation. Therefore the dummy thread will have the same signals unblocked
+ * as the main thread did when this function was called.
+ * 
+ * This will ensure that signals directed at the process will only be processed
+ * by this dummy thread. Most imporantly, system calls will not randomly fail
+ * with EINTR errors (unless signals are later unmasked by threads...).
+ * 
+ * Calling this function is not necessary for libuv itself to operate
+ * correctly, since EINTR is handled by retrying calls. But code outside
+ * of libuv may not be handling EINTR.
+ * 
+ * This function does nothing on Windows.
+ */
+UV_EXTERN void uv_sanitize_signal_handling(void);
+
+/*
  * Opens a shared library. The filename is in utf-8. Returns 0 on success and
  * -1 on error. Call uv_dlerror(uv_lib_t*) to get the error message.
  */
diff --git a/src/unix/core.c b/src/unix/core.c
index aaced4c..f08f713 100644
--- a/src/unix/core.c
+++ b/src/unix/core.c
@@ -75,6 +75,10 @@
 #endif

 static void uv__run_pending(uv_loop_t* loop);
+static void uv__signal_thread_proc(void *arg);
+
+static int uv__sanitize_signals_done;
+static uv_thread_t uv__signal_thread;

 /* Verify that uv_buf_t is ABI-compatible with struct iovec. */
 STATIC_ASSERT(sizeof(uv_buf_t) == sizeof(struct iovec));
@@ -662,6 +666,29 @@ void uv_disable_stdio_inheritance(void) {
 }


+void uv_sanitize_signal_handling(void) {
+  sigset_t full_ss;
+  sigset_t old_ss;
+
+  /* Don't do it twice.
+   * This looks like a race condition but then it should only be
+   * called from main. */
+  if (uv__sanitize_signals_done) {
+    return;
+  }
+  uv__sanitize_signals_done = 1;
+
+  // Create dummy signal handling thread.
+  if (uv_thread_create(&uv__signal_thread, uv__signal_thread_proc, NULL))
+    abort();
+
+  /* Block all signals in the current (main) thread. */
+  sigfillset(&full_ss);
+  if (pthread_sigmask(SIG_SETMASK, &full_ss, &old_ss))
+    abort();
+}
+
+
 static void uv__run_pending(uv_loop_t* loop) {
   QUEUE* q;
   uv__io_t* w;
@@ -946,3 +973,14 @@ int uv__dup2_cloexec(int oldfd, int newfd) {
     return r;
   }
 }
+
+
+static void uv__signal_thread_proc(void* arg) {
+  sigset_t empty_ss;
+  sigset_t old_ss;
+
+  /* Handle signals by doing nothing. */
+  while (1) {
+      pause();
+  }
+}
diff --git a/src/win/core.c b/src/win/core.c
index 5c3c639..d9affda 100644
--- a/src/win/core.c
+++ b/src/win/core.c
@@ -456,3 +456,6 @@ int uv__socket_sockopt(uv_handle_t* handle, int optname, int* value) {

   return 0;
 }
+
+void uv_sanitize_signal_handling(void) {
+}

Patch for pyuv:

diff --git a/src/signal.c b/src/signal.c
index eee7706..80263fd 100644
--- a/src/signal.c
+++ b/src/signal.c
@@ -140,10 +140,20 @@ Signal_tp_clear(Signal *self)
 }


+static PyObject *
+Signal_func_sanitize_signal_handling(PyObject *cls)
+{
+    UNUSED_ARG(cls);
+    uv_sanitize_signal_handling();
+    Py_RETURN_NONE;
+}
+
+
 static PyMethodDef
 Signal_tp_methods[] = {
     { "start", (PyCFunction)Signal_func_start, METH_VARARGS, "Start the Signal handle." },
     { "stop", (PyCFunction)Signal_func_stop, METH_NOARGS, "Stop the Signal handle." },
+    { "sanitize_signal_handling", (PyCFunction)Signal_func_sanitize_signal_handling, METH_NOARGS|METH_CLASS, "Sets up a safe environment with respect to signal handling." },
     { NULL }
 };

With these patches, and calling this new function, the test program (below) no longer suffers from EINTR errors. The Signal watcher still works.

from __future__ import print_function
import sys
import traceback
import signal
import pyuv

def signal_cb(handle, signum):
    print('Exiting on signal.')
    loop.stop()

def timer_cb (redundant):
    try:
        timer.start(timer_cb, 0.1, False)
        for i in range(100):
            print(''.join('LALALALALALA' for i in range(10)), file=sys.stdout)
    except:
        traceback.print_exc()
        sys.exit(1)

def spawn ():
    proc = pyuv.Process(loop)
    proc.spawn(executable='/usr/bin/env', args=['env', 'sleep', '0.01'], exit_callback=proc_handler)

def proc_handler (proc, a, b):
    proc.close()
    spawn()

pyuv.Signal.sanitize_signal_handling()

loop = pyuv.Loop()

signal_h = pyuv.Signal(loop)
signal_h.start(signal_cb, signal.SIGINT)

timer = pyuv.Timer(loop)
timer.start(timer_cb, 0, False)

spawn()

loop.run()

[bnoordhuis]

That looks like a workable approach but only when the process is single-threaded or all threads block all signals.

Threads in libuv's thread pool, for example, inherit their signal mask from the parent thread. The threads are created on demand so it's kind of unspecified what their signal mask will be. That's something that can (and probably should) be fixed in libuv but it won't help with threads that are outside libuv's control.

If the problem is that parts of python's guts are not equipped to deal with EINTR, then that's arguably a bug in python. The python interpreter can be embedded in another application so I don't think it's at liberty to make many assumptions about signal handling anyway.

Having said that, what happens when libuv installs the signal handler with the SA_RESTART flag set? That won't help for all system calls but it will for the most important ones and it's something that could be added to libuv proper. I don't know if it should be configurable behavior or not but we can cross that bridge when we get to it.