Forum: Ruby-core [PATCH] remove timer signal after last ruby thread has died

Hi -

While playing around with ruby's green threads in ruby 1.8.7 (with
pthreads disabled) I noticed that after all green threads spawned for a
ruby process have died, the timer setup to pre-empt the threads is not
turned off. There is a function for doing this, rb_thread_stop_timer()
in eval.c, but it is never called after the last ruby green thread has
died. As a result, even after all threads have joined, the timer
interrupts continue to fire. The included patch fixes this, and I've
included some examples to help clarify the behavior I am seeing:

with ruby 1.8.7:

[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:27.785959 setitimer(ITIMER_VIRTUAL, {it_interval={0, 10000},
it_value={0, 10000}}, NULL) = 0 <0.000006>
14:29:37.832131 --- SIGVTALRM (Virtual timer expired) @ 0 (0) ---
14:29:37.832217 rt_sigreturn(0x1a)      = 10127344 <0.000008>
14:29:37.878799 --- SIGVTALRM (Virtual timer expired) @ 0 (0) ---
14:29:37.878867 rt_sigreturn(0x1a)      = 14271352 <0.000009>
14:29:37.945461 --- SIGVTALRM (Virtual timer expired) @ 0 (0) ---
14:29:37.945508 rt_sigreturn(0x1a)      = 7158032 <0.000006>
14:29:37.988797 --- SIGVTALRM (Virtual timer expired) @ 0 (0) ---
14:29:37.988859 rt_sigreturn(0x1a)      = 9634280 <0.000007>
14:29:38.028799 --- SIGVTALRM (Virtual timer expired) @ 0 (0) ---
14:29:38.028855 rt_sigreturn(0x1a)      = 9986760 <0.000006>
14:29:38.075464 --- SIGVTALRM (Virtual timer expired) @ 0 (0) ---
14:29:38.075523 rt_sigreturn(0x1a)      = 11 <0.000005>
14:29:38.148798 --- SIGVTALRM (Virtual timer expired) @ 0 (0) ---
14:29:38.148856 rt_sigreturn(0x1a)      = 11364504 <0.000007>
14:29:38.180574 exit_group(0)           = ?

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.

Thanks,
Joe Damato
Aman Gupta

Hi,

At Wed, 3 Sep 2008 07:10:24 +0900,
Joe Damato wrote in [ruby-core:18444]:
> 
> 
> 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
     }

Hi -

On Tue, Sep 2, 2008 at 6:59 PM, Nobuyoshi Nakada <nobu@ruby-lang.org> 
wrote:
> Your patch ignores pthread.

Thanks for handling the pthread case.

How can I get this patch merged to the 1.8.7 branch?

Thanks,
Joe Damato

Hi,

In message "Re: [ruby-core:21558] Re: [PATCH] remove timer signal after 
last ruby   thread has died"
    on Mon, 26 Jan 2009 12:40:04 +0900, Joe Damato <ice799@gmail.com> 
writes:

|On Tue, Sep 2, 2008 at 6:59 PM, Nobuyoshi Nakada <nobu@ruby-lang.org> wrote:
|> Your patch ignores pthread.
|
|Thanks for handling the pthread case.

Nobu, could you check the patch in?

|How can I get this patch merged to the 1.8.7 branch?

The 1.8.7 maintainer will decide, but the fix will be in 1.8.8,
definitely.

              matz.

>
> On Tue, Sep 2, 2008 at 6:59 PM, Nobuyoshi Nakada <nobu@ruby-lang.org>
> wrote:
> > Your patch ignores pthread.
>
> Thanks for handling the pthread case.


Am I right in assuming this problem doesn't affect 1.9 at all?
Thanks for your help.
-=r

Is this patch in 1.8 HEAD?

  Aman

Aman Gupta wrote:
> Is this patch in 1.8 HEAD?

For those who might be interested: this patch will be included in the
next version of Ruby Enterprise Edition.

I hope it will make its way into the upstream version soon.