From: Eric Wong <normalperson@...>
Date: 2017-12-16T07:54:21+00:00
Subject: [ruby-core:84300] Re: [Ruby trunk Bug#14181] hangs or deadlocks from waitpid, threads, and trapping SIGCHLD

Eric Wong <normalperson@yhbt.net> wrote:
> That might be correct, but I don't like making the sleep_*
> functions too complex and probably prefer them open-coded like
> you did in r61274.

Something like below (maybe more cleanups possible,
timeval is tedious to use...):

```diff
diff --git a/test/ruby/test_thread.rb b/test/ruby/test_thread.rb
index 5f64a08155..3695f6e4ea 100644
--- a/test/ruby/test_thread.rb
+++ b/test/ruby/test_thread.rb
@@ -1284,6 +1284,20 @@ def test_signal_at_join
             end
           end
         end
+        n.times do
+          w = Thread.start { sleep 30 }
+          begin
+            f.puts
+            f.gets
+          ensure
+            w.kill
+            t0 = Process.clock_gettime(Process::CLOCK_MONOTONIC)
+            w.join(30)
+            t1 = Process.clock_gettime(Process::CLOCK_MONOTONIC)
+            diff = t1 - t0
+            assert_operator diff, :<=, 2
+          end
+        end
       end
     };
   end
diff --git a/thread.c b/thread.c
index cc62ea3905..1cdf119da1 100644
--- a/thread.c
+++ b/thread.c
@@ -91,7 +91,6 @@ static VALUE sym_never;
 static ID id_locals;
 
 static void sleep_timeval(rb_thread_t *th, struct timeval time, int spurious_check);
-static void sleep_wait_for_interrupt(rb_thread_t *th, double sleepsec, int spurious_check);
 static void sleep_forever(rb_thread_t *th, int nodeadlock, int spurious_check);
 static void rb_thread_sleep_deadly_allow_spurious_wakeup(void);
 static double timeofday(void);
@@ -888,18 +887,22 @@ thread_join_sleep(VALUE arg)
 	    rb_check_deadlock(th->vm);
 	    native_sleep(th, 0);
 	    th->vm->sleeper--;
-	    RUBY_VM_CHECK_INTS_BLOCKING(th->ec);
-	    th->status = THREAD_RUNNABLE;
 	}
 	else {
 	    double now = timeofday();
+	    struct timeval tv;
+
 	    if (now > limit) {
 		thread_debug("thread_join: timeout (thid: %"PRI_THREAD_ID")\n",
 			     thread_id_str(target_th));
 		return Qfalse;
 	    }
-	    sleep_wait_for_interrupt(th, limit - now, 0);
+	    tv = double2timeval(limit - now);
+	    th->status = THREAD_STOPPED;
+	    native_sleep(th, &tv);
 	}
+	RUBY_VM_CHECK_INTS_BLOCKING(th->ec);
+	th->status = THREAD_RUNNABLE;
 	thread_debug("thread_join: interrupted (thid: %"PRI_THREAD_ID", status: %s)\n",
 		     thread_id_str(target_th), thread_status_name(target_th, TRUE));
     }
@@ -1135,41 +1138,57 @@ getclockofday(struct timeval *tp)
 }
 
 static void
-sleep_timeval(rb_thread_t *th, struct timeval tv, int spurious_check)
+timeval_add(struct timeval *dst, const struct timeval *tv)
 {
-    struct timeval to, tvn;
-    enum rb_thread_status prev_status = th->status;
-
-    getclockofday(&to);
-    if (TIMEVAL_SEC_MAX - tv.tv_sec < to.tv_sec)
-        to.tv_sec = TIMEVAL_SEC_MAX;
+    if (TIMEVAL_SEC_MAX - tv->tv_sec < dst->tv_sec)
+        dst->tv_sec = TIMEVAL_SEC_MAX;
     else
-        to.tv_sec += tv.tv_sec;
-    if ((to.tv_usec += tv.tv_usec) >= 1000000) {
-        if (to.tv_sec == TIMEVAL_SEC_MAX)
-            to.tv_usec = 999999;
+        dst->tv_sec += tv->tv_sec;
+    if ((dst->tv_usec += tv->tv_usec) >= 1000000) {
+        if (dst->tv_sec == TIMEVAL_SEC_MAX)
+            dst->tv_usec = 999999;
         else {
-            to.tv_sec++;
-            to.tv_usec -= 1000000;
+            dst->tv_sec++;
+            dst->tv_usec -= 1000000;
         }
     }
+}
+
+static int
+timeval_update_expire(struct timeval *tv, const struct timeval *to)
+{
+    struct timeval tvn;
+
+    getclockofday(&tvn);
+    if (to->tv_sec < tvn.tv_sec) return 1;
+    if (to->tv_sec == tvn.tv_sec && to->tv_usec <= tvn.tv_usec) return 1;
+    thread_debug("timeval_update_expire: "
+		 "%"PRI_TIMET_PREFIX"d.%.6ld > %"PRI_TIMET_PREFIX"d.%.6ld\n",
+		 (time_t)to->tv_sec, (long)to->tv_usec,
+		 (time_t)tvn.tv_sec, (long)tvn.tv_usec);
+    tv->tv_sec = to->tv_sec - tvn.tv_sec;
+    if ((tv->tv_usec = to->tv_usec - tvn.tv_usec) < 0) {
+	--tv->tv_sec;
+	tv->tv_usec += 1000000;
+    }
+    return 0;
+}
 
+static void
+sleep_timeval(rb_thread_t *th, struct timeval tv, int spurious_check)
+{
+    struct timeval to;
+    enum rb_thread_status prev_status = th->status;
+
+    getclockofday(&to);
+    timeval_add(&to, &tv);
     th->status = THREAD_STOPPED;
     RUBY_VM_CHECK_INTS_BLOCKING(th->ec);
     while (th->status == THREAD_STOPPED) {
 	native_sleep(th, &tv);
 	RUBY_VM_CHECK_INTS_BLOCKING(th->ec);
-	getclockofday(&tvn);
-	if (to.tv_sec < tvn.tv_sec) break;
-	if (to.tv_sec == tvn.tv_sec && to.tv_usec <= tvn.tv_usec) break;
-	thread_debug("sleep_timeval: %"PRI_TIMET_PREFIX"d.%.6ld > %"PRI_TIMET_PREFIX"d.%.6ld\n",
-		     (time_t)to.tv_sec, (long)to.tv_usec,
-		     (time_t)tvn.tv_sec, (long)tvn.tv_usec);
-	tv.tv_sec = to.tv_sec - tvn.tv_sec;
-	if ((tv.tv_usec = to.tv_usec - tvn.tv_usec) < 0) {
-	    --tv.tv_sec;
-	    tv.tv_usec += 1000000;
-	}
+	if (timeval_update_expire(&tv, &to))
+	    break;
 	if (!spurious_check)
 	    break;
     }
@@ -1215,12 +1234,6 @@ timeofday(void)
     }
 }
 
-static void
-sleep_wait_for_interrupt(rb_thread_t *th, double sleepsec, int spurious_check)
-{
-    sleep_timeval(th, double2timeval(sleepsec), spurious_check);
-}
-
 void
 rb_thread_wait_for(struct timeval time)
 {
```

Unsubscribe: <mailto:ruby-core-request@ruby-lang.org?subject=unsubscribe>
<http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-core>