Fix a potential busy-loop in the thread scheduler (esp. on FreeBSD)

This patch fixes a potential busy-loop in the thread scheduler. If there
are two threads, the main thread (where Ruby signal handlers must run)
and a sleeping thread, it is possible for the following sequence of
events to occur:

* The sleeping thread is in native_sleep -> sigwait_sleep A signal
* arives, kicking this thread out of rb_sigwait_sleep The sleeping
* thread calls THREAD_BLOCKING_END and eventually
  thread_sched_to_running_common
* the sleeping thread writes into the sigwait_fd pipe by calling
  rb_thread_wakeup_timer_thread
* the sleeping thread re-loops around in native_sleep() because
  the desired sleep time has not actually yet expired
* that calls rb_sigwait_sleep again the ppoll() in rb_sigwait_sleep
* immediately returns because
  of the byte written into the sigwait_fd by
rb_thread_wakeup_timer_thread
* that wakes the thread up again and kicks the whole cycle off again.

Such a loop can only be broken by the main thread waking up and handling
the signal, such that ubf_threads_empty() below becomes true again;
however this loop can actually keep things so busy (and cause so much
contention on the main thread's interrupt_lock) that the main thread
doesn't deal with the signal for many seconds. This seems particuarly
likely on FreeBSD 13.

(the cycle can also be broken by the sleeping thread finally elapsing
its desired sleep time).

The fix for _this_ loop is to only wakeup the timer thrad in
thread_sched_to_running_common if the current thread is not itself the
sigwait thread.

An almost identical loop also happens in the same circumstances because
the call to check_signals_nogvl (through sigwait_timeout) in
rb_sigwait_sleep returns true if there is any pending signal for the
main thread to handle. That then causes rb_sigwait_sleep to skip over
sleeping entirely.

This is unnescessary and counterproductive, I believe; if the main
thread needs to be woken up that is done inline in check_signals_nogvl
anyway.

See https://bugs.ruby-lang.org/issues/19680

1 files changed, 20 insertions, 1 deletions

diff --git a/thread_pthread.c b/thread_pthread.c
index 3bb6b3d980..1f8bf7a1d5 100644
--- a/thread_pthread.c
+++ b/thread_pthread.c
@@ -506,7 +506,26 @@ thread_sched_to_running_common(struct rb_thread_sched *sched, rb_thread_t *th)
     RB_INTERNAL_THREAD_HOOK(RUBY_INTERNAL_THREAD_EVENT_RESUMED);
 
     if (!sched->timer) {
-        if (!designate_timer_thread(sched) && !ubf_threads_empty()) {
+        /* Make sure that this thread is not currently the sigwait_thread before we
+           decide to wake it up. Otherwise, we can end up in a loop of the following
+           operations:
+             * We are in native_sleep -> sigwait_sleep
+             * A signal arives, kicking this thread out of rb_sigwait_sleep
+             * We get here because of the call to THREAD_BLOCKING_END() in native_sleep
+             * write into the sigwait_fd pipe here
+             * re-loop around in native_sleep() because the desired sleep time has not
+               actually yet expired
+             * that calls rb_sigwait_sleep again
+             * the ppoll() in rb_sigwait_sleep immediately returns because of the byte we
+               wrote to the sigwait_fd here
+             * that wakes the thread up again and we end up here again.
+           Such a loop can only be broken by the main thread waking up and handling the
+           signal, such that ubf_threads_empty() below becomes true again; however this
+           loop can actually keep things so busy (and cause so much contention on the
+           main thread's interrupt_lock) that the main thread doesn't deal with the
+           signal for many seconds. This seems particuarly likely on FreeBSD 13.
+        */
+        if (!designate_timer_thread(sched) && !ubf_threads_empty() && th != sigwait_th) {
             rb_thread_wakeup_timer_thread(-1);
         }
     }