[ruby-core:18445] Re: [PATCH] remove timer signal after last ruby thread has died

Hi,

At Wed, 3 Sep 2008 07:10:24 +0900,
Joe Damato wrote in [ruby-core:18444]:
> with the patch applied:
> 
> [joe@mawu:/home/joe]% strace -ttT ruby -e 't1 = Thread.new { sleep(10) 
> }; t1.join; 10000.times { "aaaaaaaa" * 1000 };'  2>&1 | egrep 
> '(sigret|setitimer|timer|exit_group)'
> 14:29:17.536727 setitimer(ITIMER_VIRTUAL, {it_interval={0, 10000}, 
> it_value={0, 10000}}, NULL) = 0 <0.000006>
> 14:29:27.544435 setitimer(ITIMER_VIRTUAL, {it_interval={0, 0}, 
> it_value={0, 0}}, NULL) = 0 <0.000006>
> 14:29:27.948804 exit_group(0)           = ?
> 
> 
> Comments, suggestions, and questions are welcome.

Your patch ignores pthread.


Index: eval.c
===================================================================
--- eval.c	(revision 19075)
+++ eval.c	(working copy)
@@ -10811,4 +10811,9 @@ rb_thread_remove(th)
     th->prev->next = th->next;
     th->next->prev = th->prev;
+
+    /* if this is the last ruby thread, stop timer signals */
+    if (th->next == th->prev && th->next == main_thread) {
+	rb_thread_stop_timer();
+    }
 }
 
@@ -12123,5 +12128,27 @@ catch_timer(sig)
 }
 
+#define cleanup_begin(v, t, i, d) \
+    pthread_cleanup_push((void (*)_((void *)))pthread_##t##_##d, v);\
+    pthread_##t##_##i
+#define cleanup_end() pthread_cleanup_pop(1)
+
+#define PER_NANO 1000000000
+
+static struct timespec *
+nsec_future(struct timespec *to, long ns)
+{
+    struct timeval tv;
+    gettimeofday(&tv, NULL);
+    to->tv_sec = tv.tv_sec;
+    to->tv_nsec = tv.tv_usec * 1000;
+    if ((to->tv_nsec += ns) >= PER_NANO) {
+	to->tv_sec += to->tv_nsec / PER_NANO;
+	to->tv_nsec %= PER_NANO;
+    }
+    return to;
+}
+
 static pthread_t time_thread;
+static pthread_cond_t timer_running = PTHREAD_COND_INITIALIZER;
 
 static void*
@@ -12129,4 +12156,9 @@ thread_timer(dummy)
     void *dummy;
 {
+    pthread_mutex_t lock;
+    pthread_cond_t *cond = dummy;
+
+    struct timespec to;
+
     sigset_t all_signals;
 
@@ -12134,17 +12166,8 @@ thread_timer(dummy)
     pthread_sigmask(SIG_BLOCK, &all_signals, 0);
 
-    for (;;) {
-#ifdef HAVE_NANOSLEEP
-	struct timespec req, rem;
-
-	req.tv_sec = 0;
-	req.tv_nsec = 10000000;
-	nanosleep(&req, &rem);
-#else
-	struct timeval tv;
-	tv.tv_sec = 0;
-	tv.tv_usec = 10000;
-	select(0, NULL, NULL, NULL, &tv);
-#endif
+    cleanup_begin(&lock, mutex, init, destroy)(&lock, NULL);
+    cleanup_begin(&lock, mutex, lock, unlock)(&lock);
+
+    while (pthread_cond_timedwait(cond, &lock, nsec_future(&to, PER_NANO / 100))) {
 	if (!rb_thread_critical) {
 	    rb_thread_pending = 1;
@@ -12154,4 +12177,9 @@ thread_timer(dummy)
 	}
     }
+
+    cleanup_end();
+    cleanup_end();
+
+    return NULL;
 }
 
@@ -12159,4 +12187,8 @@ void
 rb_thread_start_timer()
 {
+    if (!thread_init) return;
+    pthread_create(&time_thread, 0, thread_timer, &timer_running);
+    pthread_atfork(0, 0, rb_thread_stop_timer);
+    thread_init = 1;
 }
 
@@ -12164,4 +12196,8 @@ void
 rb_thread_stop_timer()
 {
+    if (!thread_init) return;
+    pthread_cond_signal(&timer_running);
+    pthread_join(time_thread, NULL);
+    thread_init = 0;
 }
 #elif defined(HAVE_SETITIMER)
@@ -12184,9 +12220,10 @@ rb_thread_start_timer()
     struct itimerval tval;
 
-    if (!thread_init) return;
+    if (thread_init) return;
     tval.it_interval.tv_sec = 0;
     tval.it_interval.tv_usec = 10000;
     tval.it_value = tval.it_interval;
     setitimer(ITIMER_VIRTUAL, &tval, NULL);
+    thread_init = 1;
 }
 
@@ -12201,4 +12238,5 @@ rb_thread_stop_timer()
     tval.it_value = tval.it_interval;
     setitimer(ITIMER_VIRTUAL, &tval, NULL);
+    thread_init = 0;
 }
 #else  /* !(_THREAD_SAFE || HAVE_SETITIMER) */
@@ -12224,5 +12262,4 @@ rb_thread_start_0(fn, arg, th)
 
     if (!thread_init) {
-	thread_init = 1;
 #if defined(HAVE_SETITIMER) || defined(_THREAD_SAFE)
 #if defined(POSIX_SIGNAL)
@@ -12232,10 +12269,6 @@ rb_thread_start_0(fn, arg, th)
 #endif
 
-#ifdef _THREAD_SAFE
-	pthread_create(&time_thread, 0, thread_timer, 0);
-#else
 	rb_thread_start_timer();
 #endif
-#endif
     }


-- 
Nobu Nakada