improve handling of timer thread shutdown

Shutting down the timer thread now always closes pipes to free FDs.
In fact, we close the write ends of the pipes is done in the main
RubyVM to signal the timer thread shutdown.

To effectively close pipes, we implement userspace locks via
atomics to force the pipe closing thread to wait on any signal
handlers which may be waking up.

While we're at it, improve robustness during resource exhaustion and
allow it to limp along non-fatally if restarting a timer thread
fails.

This reverts r51268

Note: this change is tested with VM_CHECK_MODE 1 in vm_core.h

* process.c (close_unless_reserved): add extra check
  (dup2_with_divert): remove
  (redirect_dup2): use dup2 without divert
  (before_exec_non_async_signal_safe): adjust call + comment
  (rb_f_exec): stop timer thread for all OSes
  (rb_exec_without_timer_thread): remove
* eval.c (ruby_cleanup): adjust call
* thread.c (rb_thread_stop_timer_thread): always close pipes
* thread_pthread.c (struct timer_thread_pipe): add writing field,
    mark owner_process volatile for signal handlers
  (rb_thread_wakeup_timer_thread_fd): check valid FD
  (rb_thread_wakeup_timer_thread): set writing flag to prevent close
  (rb_thread_wakeup_timer_thread_low): ditto
  (CLOSE_INVALIDATE): new macro
  (close_invalidate): new function
  (close_communication_pipe): removed
  (setup_communication_pipe_internal): make errors non-fatal
  (setup_communication_pipe): ditto
  (thread_timer): close reading ends inside timer thread
  (rb_thread_create_timer_thread): make errors non-fatal
  (native_stop_timer_thread): close write ends only, always,
   wait for signal handlers to finish
  (rb_divert_reserved_fd): remove
* thread_win32.c (native_stop_timer_thread): adjust (untested)
  (rb_divert_reserved_fd): remove
* vm_core.h: adjust prototype

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@51576 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

1 files changed, 11 insertions, 37 deletions

diff --git a/process.c b/process.c
index cdc9692988..206a44546c 100644
--- a/process.c
+++ b/process.c
@@ -297,24 +297,14 @@ extern ID ruby_static_id_status;
 static inline int
 close_unless_reserved(int fd)
 {
-    /* Do nothing to the reserved fd because it should be closed in exec(2)
-       due to the O_CLOEXEC or FD_CLOEXEC flag. */
+    /* We should not have reserved FDs at this point */
     if (rb_reserved_fd_p(fd)) { /* async-signal-safe */
+        rb_async_bug_errno("BUG timer thread still running", 0 /* EDOOFUS */);
         return 0;
     }
     return close(fd); /* async-signal-safe */
 }
 
-static inline int
-dup2_with_divert(int oldfd, int newfd)
-{
-    if (rb_divert_reserved_fd(newfd) == -1) { /* async-signal-safe if no error occurred */
-        return -1;
-    } else {
-        return dup2(oldfd, newfd); /* async-signal-safe */
-    }
-}
-
 /*#define DEBUG_REDIRECT*/
 #if defined(DEBUG_REDIRECT)
 
@@ -354,7 +344,7 @@ static int
 redirect_dup2(int oldfd, int newfd)
 {
     int ret;
-    ret = dup2_with_divert(oldfd, newfd);
+    ret = dup2(oldfd, newfd);
     ttyprintf("dup2(%d, %d) => %d\n", oldfd, newfd, ret);
     return ret;
 }
@@ -388,7 +378,7 @@ parent_redirect_close(int fd)
 
 #else
 #define redirect_dup(oldfd) dup(oldfd)
-#define redirect_dup2(oldfd, newfd) dup2_with_divert((oldfd), (newfd))
+#define redirect_dup2(oldfd, newfd) dup2((oldfd), (newfd))
 #define redirect_close(fd) close_unless_reserved(fd)
 #define parent_redirect_open(pathname, flags, perm) rb_cloexec_open((pathname), (flags), (perm))
 #define parent_redirect_close(fd) close_unless_reserved(fd)
@@ -1151,8 +1141,10 @@ before_exec_non_async_signal_safe(void)
      * internal threads temporary. [ruby-core:10583]
      * This is also true on Haiku. It returns Errno::EPERM against exec()
      * in multiple threads.
+     *
+     * Nowadays, we always stop the timer thread completely to allow redirects.
      */
-    rb_thread_stop_timer_thread(0);
+    rb_thread_stop_timer_thread();
 }
 
 static void
@@ -2472,10 +2464,6 @@ rb_execarg_parent_end(VALUE execarg_obj)
     RB_GC_GUARD(execarg_obj);
 }
 
-#if defined(__APPLE__) || defined(__HAIKU__)
-static int rb_exec_without_timer_thread(const struct rb_execarg *eargp, char *errmsg, size_t errmsg_buflen);
-#endif
-
 /*
  *  call-seq:
  *     exec([env,] command... [,options])
@@ -2559,16 +2547,14 @@ rb_f_exec(int argc, const VALUE *argv)
 
     execarg_obj = rb_execarg_new(argc, argv, TRUE);
     eargp = rb_execarg_get(execarg_obj);
+    before_exec(); /* stop timer thread before redirects */
     rb_execarg_parent_start(execarg_obj);
     fail_str = eargp->use_shell ? eargp->invoke.sh.shell_script : eargp->invoke.cmd.command_name;
 
-#if defined(__APPLE__) || defined(__HAIKU__)
-    rb_exec_without_timer_thread(eargp, errmsg, sizeof(errmsg));
-#else
-    before_exec_async_signal_safe(); /* async-signal-safe */
     rb_exec_async_signal_safe(eargp, errmsg, sizeof(errmsg));
-    preserving_errno(after_exec_async_signal_safe()); /* async-signal-safe */
-#endif
+
+    preserving_errno(after_exec()); /* restart timer thread */
+
     RB_GC_GUARD(execarg_obj);
     if (errmsg[0])
         rb_sys_fail(errmsg);
@@ -3076,18 +3062,6 @@ failure:
     return -1;
 }
 
-#if defined(__APPLE__) || defined(__HAIKU__)
-static int
-rb_exec_without_timer_thread(const struct rb_execarg *eargp, char *errmsg, size_t errmsg_buflen)
-{
-    int ret;
-    before_exec();
-    ret = rb_exec_async_signal_safe(eargp, errmsg, errmsg_buflen); /* hopefully async-signal-safe */
-    preserving_errno(after_exec()); /* not async-signal-safe because it calls rb_thread_start_timer_thread.  */
-    return ret;
-}
-#endif
-
 #ifdef HAVE_WORKING_FORK
 /* This function should be async-signal-safe.  Hopefully it is. */
 static int