From 70a8a6d3eb20e4b251191d8cae06ee2453458253 Mon Sep 17 00:00:00 2001 From: normal Date: Sat, 25 Aug 2018 21:59:30 +0000 Subject: [PATCH] thread_pthread.c: main thread always gets hit by signals We need to ensure Signal.trap handlers can function if the main thread is sleeping after a subthread has grabbed sigwait_fd, but later exited. Consider the following timeline: main_thread sub-thread ----------------------------------------- Signal.trap() { ... } get sigwait_fd ppoll on sigwait_fd native_cond_sleep (via pthread_cond_wait) ppoll times-out put sigwait_fd sub-thread exits only thread alive SIGNAL HITS The problem is pthread_cond_wait cannot return EINTR, so we can never run the Signal.trap handler. So we will avoid using native_cond_sleep in the main thread and always use ppoll to sleep when in the main thread. This can guarantee the main thread remains aware of signals; even if it cannot safely read off sigwait_fd git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@64538 b2dd03c8-39d4-4d8f-98ff-823fe69b080e --- test/ruby/test_signal.rb | 20 +++++++++++++++++++ thread_pthread.c | 42 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/test/ruby/test_signal.rb b/test/ruby/test_signal.rb index b8a20d945c..425dc26574 100644 --- a/test/ruby/test_signal.rb +++ b/test/ruby/test_signal.rb @@ -369,4 +369,24 @@ class TestSignal < Test::Unit::TestCase ensure trap(:CHLD, old) if Signal.list['CHLD'] end + + def test_sigwait_fd_unused + t = EnvUtil.apply_timeout_scale(0.1) + assert_separately([], <<-End) + tgt = $$ + trap(:TERM) { exit(0) } + e = "Process.daemon; sleep #{t * 2}; Process.kill(:TERM,\#{tgt})" + term = [ '#{EnvUtil.rubybin}', '--disable=gems', '-e', e ] + t2 = Thread.new { sleep } # grab sigwait_fd + Thread.pass until t2.stop? + Thread.new do + sleep #{t} + t2.kill + t2.join + end + Process.spawn(*term) + # last thread remaining, ensure it can react to SIGTERM + loop { sleep } + End + end if Process.respond_to?(:kill) && Process.respond_to?(:daemon) end diff --git a/thread_pthread.c b/thread_pthread.c index 209ed6781c..92ec66af1e 100644 --- a/thread_pthread.c +++ b/thread_pthread.c @@ -1911,7 +1911,7 @@ rb_sigwait_fd_get(const rb_thread_t *th) return signal_self_pipe.normal[0]; } } - return -1; /* avoid thundering herd */ + return -1; /* avoid thundering herd and work stealing/starvation */ } void @@ -1996,6 +1996,43 @@ rb_sigwait_sleep(rb_thread_t *th, int sigwait_fd, const struct timespec *ts) } } +/* + * This function does not exclusively acquire sigwait_fd, so it + * cannot safely read from it. However, it can be woken up in + * 4 ways: + * + * 1) ubf_select (from another thread) + * 2) rb_thread_wakeup_timer_thread (from signal handler) + * 3) any unmasked signal hitting the process + * 4) periodic ubf timer wakeups (after 3) + */ +static void +native_ppoll_sleep(rb_thread_t *th, rb_hrtime_t *rel) +{ + rb_native_mutex_lock(&th->interrupt_lock); + th->unblock.func = ubf_select; + th->unblock.arg = th; + rb_native_mutex_unlock(&th->interrupt_lock); + + GVL_UNLOCK_BEGIN(th); + if (!RUBY_VM_INTERRUPTED(th->ec)) { + struct pollfd pfd; + struct timespec ts; + + pfd.fd = signal_self_pipe.normal[0]; /* sigwait_fd */ + pfd.events = POLLIN; + (void)ppoll(&pfd, 1, rb_hrtime2timespec(&ts, rel), 0); + + /* + * do not read the fd, here, let uplevel callers or other threads + * that, otherwise we may steal and starve other threads + */ + } + unblock_function_clear(th); + unregister_ubf_list(th); + GVL_UNLOCK_END(th); +} + static void native_sleep(rb_thread_t *th, rb_hrtime_t *rel) { @@ -2020,6 +2057,9 @@ native_sleep(rb_thread_t *th, rb_hrtime_t *rel) rb_sigwait_fd_put(th, sigwait_fd); rb_sigwait_fd_migrate(th->vm); } + else if (th == th->vm->main_thread) { /* always able to handle signals */ + native_ppoll_sleep(th, rel); + } else { native_cond_sleep(th, rel); }