Remove SIGCHLD `waidpid`. (#7527)

* Remove `waitpid_lock` and related code.

* Remove un-necessary test.

* Remove `rb_thread_sleep_interruptible` dead code.

8 files changed, 6 insertions, 328 deletions

diff --git a/process.c b/process.c
index cc02d4dc5a..ca09357d17 100644
--- a/process.c
+++ b/process.c
@@ -1061,8 +1061,6 @@ do_waitpid(rb_pid_t pid, int *st, int flags)
 #endif
 }
 
-#define WAITPID_LOCK_ONLY ((struct waitpid_state *)-1)
-
 struct waitpid_state {
     struct ccan_list_node wnode;
     rb_execution_context_t *ec;
@@ -1074,103 +1072,6 @@ struct waitpid_state {
     int errnum;
 };
 
-int rb_sigwait_fd_get(const rb_thread_t *);
-void rb_sigwait_sleep(const rb_thread_t *, int fd, const rb_hrtime_t *);
-void rb_sigwait_fd_put(const rb_thread_t *, int fd);
-void rb_thread_sleep_interruptible(void);
-
-static int
-waitpid_signal(struct waitpid_state *w)
-{
-    if (w->ec) { /* rb_waitpid */
-        rb_threadptr_interrupt(rb_ec_thread_ptr(w->ec));
-        return TRUE;
-    }
-    return FALSE;
-}
-
-// Used for VM memsize reporting. Returns the size of a list of waitpid_state
-// structs. Defined here because the struct definition lives here as well.
-size_t
-rb_vm_memsize_waiting_list(struct ccan_list_head *waiting_list)
-{
-    struct waitpid_state *waitpid = 0;
-    size_t size = 0;
-
-    ccan_list_for_each(waiting_list, waitpid, wnode) {
-        size += sizeof(struct waitpid_state);
-    }
-
-    return size;
-}
-
-/*
- * When a thread is done using sigwait_fd and there are other threads
- * sleeping on waitpid, we must kick one of the threads out of
- * rb_native_cond_wait so it can switch to rb_sigwait_sleep
- */
-static void
-sigwait_fd_migrate_sleeper(rb_vm_t *vm)
-{
-    struct waitpid_state *w = 0;
-
-    ccan_list_for_each(&vm->waiting_pids, w, wnode) {
-        if (waitpid_signal(w)) return;
-    }
-    ccan_list_for_each(&vm->waiting_grps, w, wnode) {
-        if (waitpid_signal(w)) return;
-    }
-}
-
-void
-rb_sigwait_fd_migrate(rb_vm_t *vm)
-{
-    rb_native_mutex_lock(&vm->waitpid_lock);
-    sigwait_fd_migrate_sleeper(vm);
-    rb_native_mutex_unlock(&vm->waitpid_lock);
-}
-
-#if RUBY_SIGCHLD
-extern volatile unsigned int ruby_nocldwait; /* signal.c */
-/* called by timer thread or thread which acquired sigwait_fd */
-static void
-waitpid_each(struct ccan_list_head *head)
-{
-    struct waitpid_state *w = 0, *next;
-
-    ccan_list_for_each_safe(head, w, next, wnode) {
-        rb_pid_t ret = do_waitpid(w->pid, &w->status, w->options | WNOHANG);
-
-        if (!ret) continue;
-        if (ret == -1) w->errnum = errno;
-
-        w->ret = ret;
-        ccan_list_del_init(&w->wnode);
-        waitpid_signal(w);
-    }
-}
-#else
-# define ruby_nocldwait 0
-#endif
-
-void
-ruby_waitpid_all(rb_vm_t *vm)
-{
-#if RUBY_SIGCHLD
-    rb_native_mutex_lock(&vm->waitpid_lock);
-    waitpid_each(&vm->waiting_pids);
-    if (ccan_list_empty(&vm->waiting_pids)) {
-        waitpid_each(&vm->waiting_grps);
-    }
-    /* emulate SA_NOCLDWAIT */
-    if (ccan_list_empty(&vm->waiting_pids) && ccan_list_empty(&vm->waiting_grps)) {
-        while (ruby_nocldwait && do_waitpid(-1, 0, WNOHANG) > 0)
-            ; /* keep looping */
-    }
-    rb_native_mutex_unlock(&vm->waitpid_lock);
-#endif
-}
-
 static void
 waitpid_state_init(struct waitpid_state *w, rb_pid_t pid, int options)
 {
@@ -1181,77 +1082,6 @@ waitpid_state_init(struct waitpid_state *w, rb_pid_t pid, int options)
     w->status = 0;
 }
 
-static VALUE
-waitpid_sleep(VALUE x)
-{
-    struct waitpid_state *w = (struct waitpid_state *)x;
-
-    while (!w->ret) {
-        rb_thread_sleep_interruptible();
-    }
-
-    return Qfalse;
-}
-
-static VALUE
-waitpid_cleanup(VALUE x)
-{
-    struct waitpid_state *w = (struct waitpid_state *)x;
-
-    /*
-     * XXX w->ret is sometimes set but ccan_list_del is still needed, here,
-     * Not sure why, so we unconditionally do ccan_list_del here:
-     */
-    if (TRUE || w->ret == 0) {
-        rb_vm_t *vm = rb_ec_vm_ptr(w->ec);
-
-        rb_native_mutex_lock(&vm->waitpid_lock);
-        ccan_list_del(&w->wnode);
-        rb_native_mutex_unlock(&vm->waitpid_lock);
-    }
-
-    return Qfalse;
-}
-
-static void
-waitpid_wait(struct waitpid_state *w)
-{
-    rb_vm_t *vm = rb_ec_vm_ptr(w->ec);
-    int need_sleep = FALSE;
-
-    /*
-     * Lock here to prevent do_waitpid from stealing work from the
-     * ruby_waitpid_locked done by rjit workers since rjit works
-     * outside of GVL
-     */
-    rb_native_mutex_lock(&vm->waitpid_lock);
-
-    if (w->pid > 0 || ccan_list_empty(&vm->waiting_pids)) {
-        w->ret = do_waitpid(w->pid, &w->status, w->options | WNOHANG);
-    }
-
-    if (w->ret) {
-        if (w->ret == -1) w->errnum = errno;
-    }
-    else if (w->options & WNOHANG) {
-    }
-    else {
-        need_sleep = TRUE;
-    }
-
-    if (need_sleep) {
-        w->cond = 0;
-        /* order matters, favor specified PIDs rather than -1 or 0 */
-        ccan_list_add(w->pid > 0 ? &vm->waiting_pids : &vm->waiting_grps, &w->wnode);
-    }
-
-    rb_native_mutex_unlock(&vm->waitpid_lock);
-
-    if (need_sleep) {
-        rb_ensure(waitpid_sleep, (VALUE)w, waitpid_cleanup, (VALUE)w);
-    }
-}
-
 static void *
 waitpid_blocking_no_SIGCHLD(void *x)
 {
@@ -1270,8 +1100,7 @@ waitpid_no_SIGCHLD(struct waitpid_state *w)
     }
     else {
         do {
-            rb_thread_call_without_gvl(waitpid_blocking_no_SIGCHLD, w,
-                                       RUBY_UBF_PROCESS, 0);
+            rb_thread_call_without_gvl(waitpid_blocking_no_SIGCHLD, w, RUBY_UBF_PROCESS, 0);
         } while (w->ret < 0 && errno == EINTR && (RUBY_VM_CHECK_INTS(w->ec),1));
     }
     if (w->ret == -1)
@@ -1293,20 +1122,10 @@ rb_process_status_wait(rb_pid_t pid, int flags)
     waitpid_state_init(&waitpid_state, pid, flags);
     waitpid_state.ec = GET_EC();
 
-    if (WAITPID_USE_SIGCHLD) {
-        waitpid_wait(&waitpid_state);
-    }
-    else {
-        waitpid_no_SIGCHLD(&waitpid_state);
-    }
+    waitpid_no_SIGCHLD(&waitpid_state);
 
     if (waitpid_state.ret == 0) return Qnil;
 
-    if (waitpid_state.ret > 0 && ruby_nocldwait) {
-        waitpid_state.ret = -1;
-        waitpid_state.errnum = ECHILD;
-    }
-
     return rb_process_status_new(waitpid_state.ret, waitpid_state.status, waitpid_state.errnum);
 }
 
@@ -4106,16 +3925,10 @@ retry_fork_async_signal_safe(struct rb_process_status *status, int *ep,
     volatile int try_gc = 1;
     struct child_handler_disabler_state old;
     int err;
-    rb_nativethread_lock_t *const volatile waitpid_lock_init =
-        (w && WAITPID_USE_SIGCHLD) ? &GET_VM()->waitpid_lock : 0;
 
     while (1) {
-        rb_nativethread_lock_t *waitpid_lock = waitpid_lock_init;
         prefork();
         disable_child_handler_before_fork(&old);
-        if (waitpid_lock) {
-            rb_native_mutex_lock(waitpid_lock);
-        }
 #ifdef HAVE_WORKING_VFORK
         if (!has_privilege())
             pid = vfork();
@@ -4140,14 +3953,6 @@ retry_fork_async_signal_safe(struct rb_process_status *status, int *ep,
 #endif
         }
         err = errno;
-        waitpid_lock = waitpid_lock_init;
-        if (waitpid_lock) {
-            if (pid > 0 && w != WAITPID_LOCK_ONLY) {
-                w->pid = pid;
-                ccan_list_add(&GET_VM()->waiting_pids, &w->wnode);
-            }
-            rb_native_mutex_unlock(waitpid_lock);
-        }
         disable_child_handler_fork_parent(&old);
         if (0 < pid) /* fork succeed, parent process */
             return pid;
@@ -4192,13 +3997,12 @@ fork_check_err(struct rb_process_status *status, int (*chfunc)(void*, char *, si
             int state = 0;
             status->error = err;
 
-            VM_ASSERT((w == 0 || w == WAITPID_LOCK_ONLY) &&
-                      "only used by extensions");
+            VM_ASSERT((w == 0) && "only used by extensions");
             rb_protect(proc_syswait, (VALUE)pid, &state);
 
             status->status = state;
         }
-        else if (!w || w == WAITPID_LOCK_ONLY) {
+        else if (!w) {
             rb_syswait(pid);
         }
 
@@ -4628,7 +4432,7 @@ rb_spawn_process(struct rb_execarg *eargp, char *errmsg, size_t errmsg_buflen)
     rb_last_status_set((status & 0xff) << 8, pid);
 # endif
 
-    if (eargp->waitpid_state && eargp->waitpid_state != WAITPID_LOCK_ONLY) {
+    if (eargp->waitpid_state) {
         eargp->waitpid_state->pid = pid;
     }
 
@@ -4659,15 +4463,6 @@ static rb_pid_t
 rb_execarg_spawn(VALUE execarg_obj, char *errmsg, size_t errmsg_buflen)
 {
     struct spawn_args args;
-    struct rb_execarg *eargp = rb_execarg_get(execarg_obj);
-
-    /*
-     * Prevent a race with RJIT where the compiler process where
-     * can hold an FD of ours in between vfork + execve
-     */
-    if (!eargp->waitpid_state && rb_rjit_enabled) {
-        eargp->waitpid_state = WAITPID_LOCK_ONLY;
-    }
 
     args.execarg = execarg_obj;
     args.errmsg.ptr = errmsg;
diff --git a/signal.c b/signal.c
index 544854e014..b7a8dbe4dd 100644
--- a/signal.c
+++ b/signal.c
@@ -1085,16 +1085,6 @@ rb_vm_trap_exit(rb_vm_t *vm)
     }
 }
 
-void ruby_waitpid_all(rb_vm_t *); /* process.c */
-
-void
-ruby_sigchld_handler(rb_vm_t *vm)
-{
-    if (SIGCHLD_LOSSY || GET_SIGCHLD_HIT()) {
-        ruby_waitpid_all(vm);
-    }
-}
-
 /* returns true if a trap handler was run, false otherwise */
 int
 rb_signal_exec(rb_thread_t *th, int sig)
@@ -1623,33 +1613,5 @@ fake_grantfd(int masterfd)
 int
 rb_grantpt(int masterfd)
 {
-    if (RUBY_SIGCHLD) {
-        rb_vm_t *vm = GET_VM();
-        int ret, e;
-
-        /*
-         * Prevent waitpid calls from Ruby by taking waitpid_lock.
-         * Pedantically, grantpt(3) is undefined if a non-default
-         * SIGCHLD handler is defined, but preventing conflicting
-         * waitpid calls ought to be sufficient.
-         *
-         * We could install the default sighandler temporarily, but that
-         * could cause SIGCHLD to be missed by other threads.  Blocking
-         * SIGCHLD won't work here, either, unless we stop and restart
-         * timer-thread (as only timer-thread sees SIGCHLD), but that
-         * seems like overkill.
-         */
-        rb_nativethread_lock_lock(&vm->waitpid_lock);
-        {
-            ret = grantpt(masterfd); /* may spawn `pt_chown' and wait on it */
-            if (ret < 0) e = errno;
-        }
-        rb_nativethread_lock_unlock(&vm->waitpid_lock);
-
-        if (ret < 0) errno = e;
-        return ret;
-    }
-    else {
-        return grantpt(masterfd);
-    }
+    return grantpt(masterfd);
 }
diff --git a/test/ruby/test_signal.rb b/test/ruby/test_signal.rb
index c5043eea59..7877a35129 100644
--- a/test/ruby/test_signal.rb
+++ b/test/ruby/test_signal.rb
@@ -323,49 +323,6 @@ class TestSignal < Test::Unit::TestCase
     end;
   end
 
-  def test_sigchld_ignore
-    omit 'no SIGCHLD' unless Signal.list['CHLD']
-    old = trap(:CHLD, 'IGNORE')
-    cmd = [ EnvUtil.rubybin, '--disable=gems', '-e' ]
-    assert(system(*cmd, 'exit!(0)'), 'no ECHILD')
-    IO.pipe do |r, w|
-      pid = spawn(*cmd, "STDIN.read", in: r)
-      nb = Process.wait(pid, Process::WNOHANG)
-      th = Thread.new(Thread.current) do |parent|
-        Thread.pass until parent.stop? # wait for parent to Process.wait
-        w.close
-      end
-      assert_raise(Errno::ECHILD) { Process.wait(pid) }
-      th.join
-      assert_nil nb
-    end
-
-    IO.pipe do |r, w|
-      pids = 3.times.map { spawn(*cmd, 'exit!', out: w) }
-      w.close
-      zombies = pids.dup
-      assert_nil r.read(1), 'children dead'
-
-      Timeout.timeout(10) do
-        zombies.delete_if do |pid|
-          begin
-            Process.kill(0, pid)
-            false
-          rescue Errno::ESRCH
-            true
-          end
-        end while zombies[0]
-      end
-      assert_predicate zombies, :empty?, 'zombies leftover'
-
-      pids.each do |pid|
-        assert_raise(Errno::ECHILD) { Process.waitpid(pid) }
-      end
-    end
-  ensure
-    trap(:CHLD, old) if Signal.list['CHLD']
-  end
-
   def test_sigwait_fd_unused
     t = EnvUtil.apply_timeout_scale(0.1)
     assert_separately([], <<-End)
diff --git a/thread.c b/thread.c
index b82e9a6578..d4348e8df2 100644
--- a/thread.c
+++ b/thread.c
@@ -145,7 +145,6 @@ static int hrtime_update_expire(rb_hrtime_t *, const rb_hrtime_t);
 NORETURN(static void async_bug_fd(const char *mesg, int errno_arg, int fd));
 static int consume_communication_pipe(int fd);
 static int check_signals_nogvl(rb_thread_t *, int sigwait_fd);
-void rb_sigwait_fd_migrate(rb_vm_t *); /* process.c */
 
 #define eKillSignal INT2FIX(0)
 #define eTerminateSignal INT2FIX(1)
@@ -258,7 +257,6 @@ timeout_prepare(rb_hrtime_t **to, rb_hrtime_t *rel, rb_hrtime_t *end,
 }
 
 MAYBE_UNUSED(NOINLINE(static int thread_start_func_2(rb_thread_t *th, VALUE *stack_start)));
-void ruby_sigchld_handler(rb_vm_t *); /* signal.c */
 
 static void
 ubf_sigwait(void *ignore)
@@ -1369,18 +1367,6 @@ rb_thread_sleep_deadly(void)
     sleep_forever(GET_THREAD(), SLEEP_DEADLOCKABLE|SLEEP_SPURIOUS_CHECK);
 }
 
-void
-rb_thread_sleep_interruptible(void)
-{
-    rb_thread_t *th = GET_THREAD();
-    enum rb_thread_status prev_status = th->status;
-
-    th->status = THREAD_STOPPED;
-    native_sleep(th, 0);
-    RUBY_VM_CHECK_INTS_BLOCKING(th->ec);
-    th->status = prev_status;
-}
-
 static void
 rb_thread_sleep_deadly_allow_spurious_wakeup(VALUE blocker, VALUE timeout, rb_hrtime_t end)
 {
@@ -2318,9 +2304,7 @@ rb_threadptr_execute_interrupts(rb_thread_t *th, int blocking_timing)
 
             if (sigwait_fd >= 0) {
                 (void)consume_communication_pipe(sigwait_fd);
-                ruby_sigchld_handler(th->vm);
                 rb_sigwait_fd_put(th, sigwait_fd);
-                rb_sigwait_fd_migrate(th->vm);
             }
             th->status = THREAD_RUNNABLE;
             while ((sig = rb_get_next_signal()) != 0) {
@@ -4070,7 +4054,6 @@ select_set_free(VALUE p)
 
     if (set->sigwait_fd >= 0) {
         rb_sigwait_fd_put(set->th, set->sigwait_fd);
-        rb_sigwait_fd_migrate(set->th->vm);
     }
 
     rb_fd_term(&set->orig_rset);
@@ -4286,7 +4269,6 @@ rb_thread_wait_for_single_fd(int fd, int events, struct timeval *timeout)
                 int fd1 = sigwait_signals_fd(result, fds[1].revents, fds[1].fd);
                 (void)check_signals_nogvl(wfd.th, fd1);
                 rb_sigwait_fd_put(wfd.th, fds[1].fd);
-                rb_sigwait_fd_migrate(wfd.th->vm);
             }
             RUBY_VM_CHECK_INTS_BLOCKING(wfd.th->ec);
         } while (wait_retryable(&result, lerrno, to, end));
@@ -4506,7 +4488,6 @@ check_signals_nogvl(rb_thread_t *th, int sigwait_fd)
     rb_vm_t *vm = GET_VM(); /* th may be 0 */
     int ret = sigwait_fd >= 0 ? consume_communication_pipe(sigwait_fd) : FALSE;
     ubf_wakeup_all_threads();
-    ruby_sigchld_handler(vm);
     if (rb_signal_buff_size()) {
         if (th == vm->ractor.main_thread) {
             /* no need to lock + wakeup if already in main thread */
@@ -4607,7 +4588,6 @@ rb_thread_atfork_internal(rb_thread_t *th, void (*atfork)(rb_thread_t *, const r
     rb_ractor_atfork(vm, th);
 
     /* may be held by RJIT threads in parent */
-    rb_native_mutex_initialize(&vm->waitpid_lock);
     rb_native_mutex_initialize(&vm->workqueue_lock);
 
     /* may be held by any thread in parent */
@@ -5274,7 +5254,6 @@ Init_Thread_Mutex(void)
 {
     rb_thread_t *th = GET_THREAD();
 
-    rb_native_mutex_initialize(&th->vm->waitpid_lock);
     rb_native_mutex_initialize(&th->vm->workqueue_lock);
     rb_native_mutex_initialize(&th->interrupt_lock);
 }
diff --git a/thread_pthread.c b/thread_pthread.c
index 9723a5f6b4..c523509b9f 100644
--- a/thread_pthread.c
+++ b/thread_pthread.c
@@ -348,7 +348,6 @@ do_gvl_timer(struct rb_thread_sched *sched, rb_thread_t *th)
     sched->timer_err = native_cond_timedwait(&th->nt->cond.readyq, &sched->lock, &abs);
 
     ubf_wakeup_all_threads();
-    ruby_sigchld_handler(vm);
 
     if (UNLIKELY(rb_signal_buff_size())) {
         if (th == vm->ractor.main_thread) {
@@ -2359,7 +2358,6 @@ native_sleep(rb_thread_t *th, rb_hrtime_t *rel)
         THREAD_BLOCKING_END(th);
 
         rb_sigwait_fd_put(th, sigwait_fd);
-        rb_sigwait_fd_migrate(th->vm);
     }
     else if (th == th->vm->ractor.main_thread) { /* always able to handle signals */
         native_ppoll_sleep(th, rel);
diff --git a/thread_win32.c b/thread_win32.c
index b5d10e2200..77953d3245 100644
--- a/thread_win32.c
+++ b/thread_win32.c
@@ -753,7 +753,6 @@ timer_thread_func(void *dummy)
     while (WaitForSingleObject(timer_thread.lock,
                                TIME_QUANTUM_USEC/1000) == WAIT_TIMEOUT) {
         vm->clock++;
-        ruby_sigchld_handler(vm); /* probably no-op */
         rb_threadptr_check_signal(vm->ractor.main_thread);
     }
     RUBY_DEBUG_LOG("end");
diff --git a/vm.c b/vm.c
index 3c18c9c5a5..b4aa2a04a9 100644
--- a/vm.c
+++ b/vm.c
@@ -2877,7 +2877,6 @@ ruby_vm_destruct(rb_vm_t *vm)
         if (objspace) {
             rb_objspace_free(objspace);
         }
-        rb_native_mutex_destroy(&vm->waitpid_lock);
         rb_native_mutex_destroy(&vm->workqueue_lock);
         /* after freeing objspace, you *can't* use ruby_xfree() */
         ruby_mimfree(vm);
@@ -2887,7 +2886,6 @@ ruby_vm_destruct(rb_vm_t *vm)
     return 0;
 }
 
-size_t rb_vm_memsize_waiting_list(struct ccan_list_head *waiting_list); // process.c
 size_t rb_vm_memsize_waiting_fds(struct ccan_list_head *waiting_fds); // thread.c
 size_t rb_vm_memsize_postponed_job_buffer(void); // vm_trace.c
 size_t rb_vm_memsize_workqueue(struct ccan_list_head *workqueue); // vm_trace.c
@@ -2944,8 +2942,6 @@ vm_memsize(const void *ptr)
 
     return (
         sizeof(rb_vm_t) +
-        rb_vm_memsize_waiting_list(&vm->waiting_pids) +
-        rb_vm_memsize_waiting_list(&vm->waiting_grps) +
         rb_vm_memsize_waiting_fds(&vm->waiting_fds) +
         rb_st_memsize(vm->loaded_features_index) +
         rb_st_memsize(vm->loading_table) +
diff --git a/vm_core.h b/vm_core.h
index 66602efea7..724c8cd739 100644
--- a/vm_core.h
+++ b/vm_core.h
@@ -145,9 +145,6 @@ extern int ruby_assert_critical_section_entered;
 #  define SIGCHLD_LOSSY (0)
 #endif
 
-/* define to 0 to test old code path */
-#define WAITPID_USE_SIGCHLD (RUBY_SIGCHLD || SIGCHLD_LOSSY)
-
 #if defined(SIGSEGV) && defined(HAVE_SIGALTSTACK) && defined(SA_SIGINFO) && !defined(__NetBSD__)
 #  define USE_SIGALTSTACK
 void *rb_allocate_sigaltstack(void);
@@ -654,9 +651,6 @@ typedef struct rb_vm_struct {
 #endif
 
     rb_serial_t fork_gen;
-    rb_nativethread_lock_t waitpid_lock;
-    struct ccan_list_head waiting_pids; /* PID > 0: <=> struct waitpid_state */
-    struct ccan_list_head waiting_grps; /* PID <= 0: <=> struct waitpid_state */
     struct ccan_list_head waiting_fds; /* <=> struct waiting_fd */
 
     /* set in single-threaded processes only: */
@@ -1764,9 +1758,7 @@ static inline void
 rb_vm_living_threads_init(rb_vm_t *vm)
 {
     ccan_list_head_init(&vm->waiting_fds);
-    ccan_list_head_init(&vm->waiting_pids);
     ccan_list_head_init(&vm->workqueue);
-    ccan_list_head_init(&vm->waiting_grps);
     ccan_list_head_init(&vm->ractor.set);
 }