[ruby-core:32734] Re: [Ruby 1.9-Feature#3436] Spawn the timer thread lazily

< ^ > P N |<>|^_>< --- | ~ ...Help
--jho1yZJdad60DJr+
Content-Type: multipart/mixed; boundary="OgqxwSJOaUobr8KG"
Content-Disposition: inline


--OgqxwSJOaUobr8KG
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, Oct 08, 2010 at 11:12:47PM +0900, Nobuyoshi Nakada wrote:
> Updated patch with new test.

The new test is much better, thanks a lot.

On Fri, Oct 08, 2010 at 11:36:13PM +0900, SASADA Koichi wrote:
> (2010/10/08 15:12), Nobuyoshi Nakada wrote:
> > At Tue, 5 Oct 2010 03:09:42 +0900,
> > Mark Somerville wrote in [ruby-core:32686]:
> >> The thread is now only used when it is required to schedule
> >> Ruby threads. When there is only the main thread, signals are
> >> handled immediately in sighandler().
> > 
> > Seems almost fine.  Since aded missing prototypes, no needs to
> > move some functions now.
> 
> Timer thread also handles signals.
> I'm not sure that your patch work fine.

The signal handling has not required too many changes - signals are
still added to signal_buff when they are received and in multi-threaded
programs the timer thread is still used to tell the main thread to
process them. In single-threaded programs however, the signals in
signal_buff are processed within sighandler() immediately, since the
timer thread doesn't exist.

I've attached a slightly improved patch which fixes a rare case when the
timer thread may not be stopped correctly after the last thread has
exited.

--OgqxwSJOaUobr8KG
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="bug_3436_fix_with_improvements.patch"
Content-Transfer-Encoding: quoted-printable

Index: thread_win32.c
===================================================================
--- thread_win32.c	(revision 29431)
+++ thread_win32.c	(working copy)
@@ -590,4 +590,10 @@
     }
 }
 
+static int
+native_timer_thread_is_running(void)
+{
+    return timer_thread_id != 0;
+}
+
 #endif /* THREAD_SYSTEM_DEPENDENT_IMPLEMENTATION */
Index: thread_pthread.c
===================================================================
--- thread_pthread.c	(revision 29431)
+++ thread_pthread.c	(working copy)
@@ -840,6 +840,12 @@
     timer_thread_id = 0;
 }
 
+static int
+native_timer_thread_is_running(void)
+{
+    return timer_thread_id != 0;
+}
+
 #ifdef HAVE_SIGALTSTACK
 int
 ruby_stack_overflowed_p(const rb_thread_t *th, const void *addr)
Index: vm_core.h
===================================================================
--- vm_core.h	(revision 29431)
+++ vm_core.h	(working copy)
@@ -635,6 +635,7 @@
 VALUE rb_iseq_eval_main(VALUE iseqval);
 void rb_enable_interrupt(void);
 void rb_disable_interrupt(void);
+int rb_thread_timer_thread_is_running(void);
 #if defined __GNUC__ && __GNUC__ >= 4
 #pragma GCC visibility pop
 #endif
Index: thread.c
===================================================================
--- thread.c	(revision 29431)
+++ thread.c	(working copy)
@@ -72,7 +72,9 @@
 int rb_signal_buff_size(void);
 void rb_signal_exec(rb_thread_t *th, int sig);
 void rb_disable_interrupt(void);
+void rb_threadptr_check_signal(rb_thread_t *mth);
 void rb_thread_stop_timer_thread(void);
+void rb_thread_start_timer_thread(void);
 
 static const VALUE eKillSignal = INT2FIX(0);
 static const VALUE eTerminateSignal = INT2FIX(1);
@@ -562,6 +564,7 @@
 	th->status = THREAD_KILLED;
 	rb_raise(rb_eThreadError, "can't create Thread (%d)", err);
     }
+    rb_thread_start_timer_thread();
     return thval;
 }
 
@@ -577,6 +580,7 @@
 	rb_raise(rb_eThreadError, "uninitialized thread - check `%s#initialize'",
 		 rb_class2name(klass));
     }
+    rb_thread_start_timer_thread();
     return thread;
 }
 
@@ -711,6 +715,16 @@
     thread_debug("thread_join: success (thid: %p)\n",
 		 (void *)target_th->thread_id);
 
+    if (rb_thread_alone()) {
+        rb_disable_interrupt();
+        if (rb_signal_buff_size() == 0) {
+            /* process any buffered signals before stopping the timer thread */
+            rb_threadptr_check_signal(GET_THREAD());
+	}
+        rb_thread_stop_timer_thread();
+        rb_enable_interrupt();
+    }
+
     if (target_th->errinfo != Qnil) {
 	VALUE err = target_th->errinfo;
 
@@ -2723,9 +2737,17 @@
 rb_thread_start_timer_thread(void)
 {
     system_working = 1;
-    rb_thread_create_timer_thread();
+    if (!rb_thread_alone()) {
+        rb_thread_create_timer_thread();
+    }
 }
 
+int
+rb_thread_timer_thread_is_running(void)
+{
+    return native_timer_thread_is_running();
+}
+
 static int
 clear_coverage_i(st_data_t key, st_data_t val, st_data_t dummy)
 {
@@ -4388,7 +4410,7 @@
 	}
     }
 
-    rb_thread_create_timer_thread();
+    rb_thread_start_timer_thread();
 
     (void)native_mutex_trylock;
 }
Index: process.c
===================================================================
--- process.c	(revision 29431)
+++ process.c	(working copy)
@@ -998,7 +998,7 @@
 #define before_exec() \
     (rb_enable_interrupt(), (void)(forked_child ? 0 : (rb_thread_stop_timer_thread(), 1)))
 #define after_exec() \
-  (rb_thread_reset_timer_thread(), rb_thread_start_timer_thread(), forked_child = 0, rb_disable_interrupt())
+  (rb_thread_reset_timer_thread(), rb_thread_start_timer_thread(), forked_child = 0)
 #define before_fork() before_exec()
 #define after_fork() (GET_THREAD()->thrown_errinfo = 0, after_exec())
 
Index: ext/-test-/thread/timer/timer.c
===================================================================
--- ext/-test-/thread/timer/timer.c	(revision 0)
+++ ext/-test-/thread/timer/timer.c	(revision 0)
@@ -0,0 +1,15 @@
+#include <ruby.h>
+#include "vm_core.h"
+
+VALUE
+thread_timer_thread_running_p(VALUE klass)
+{
+    return rb_thread_timer_thread_is_running() ? Qtrue : Qfalse;
+}
+
+void
+Init_timer(void)
+{
+    VALUE timer = rb_define_module_under(rb_cThread, "Timer");
+    rb_define_module_function(timer, "running?", thread_timer_thread_running_p, 0);
+}
Index: ext/-test-/thread/timer/extconf.rb
===================================================================
--- ext/-test-/thread/timer/extconf.rb	(revision 0)
+++ ext/-test-/thread/timer/extconf.rb	(revision 0)
@@ -0,0 +1,3 @@
+require 'mkmf'
+$INCFLAGS << " -I$(top_srcdir)"
+create_makefile('-test-/thread/timer')
Index: test/ruby/test_signal.rb
===================================================================
--- test/ruby/test_signal.rb	(revision 29431)
+++ test/ruby/test_signal.rb	(working copy)
@@ -181,4 +181,20 @@
     w.close
     assert_equal(r.read, "foo")
   end
+
+  def test_signals_before_and_after_timer_thread
+    count = 0
+    Signal.trap(:INT) { count += 1 }
+
+    Process.kill :INT, Process.pid
+    assert_equal 1, count
+
+    th = Thread.new { sleep 0.5 }
+    Process.kill :INT, Process.pid
+    assert_equal 2, count
+
+    th.join
+    Process.kill :INT, Process.pid
+    assert_equal 3, count
+  end
 end
Index: test/thread/test_timer_thread.rb
===================================================================
--- test/thread/test_timer_thread.rb	(revision 0)
+++ test/thread/test_timer_thread.rb	(revision 0)
@@ -0,0 +1,10 @@
+require 'test/unit'
+require '-test-/thread/timer'
+
+class TestTimerThread < Test::Unit::TestCase
+  def test_timer_is_created_and_destroyed
+    assert !Thread::Timer.running?
+    assert Thread.new {Thread::Timer.running?}.value
+    assert !Thread::Timer.running?
+  end
+end
Index: signal.c
===================================================================
--- signal.c	(revision 29431)
+++ signal.c	(working copy)
@@ -519,6 +519,10 @@
 #if !defined(BSD_SIGNAL) && !defined(POSIX_SIGNAL)
     ruby_signal(sig, sighandler);
 #endif
+
+    if (rb_thread_alone()) {
+        rb_threadptr_check_signal(GET_THREAD());
+    }
 }
 
 int

--OgqxwSJOaUobr8KG--

--jho1yZJdad60DJr+
Content-Type: application/pgp-signature
Content-Disposition: inline

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iEYEARECAAYFAkywmBwACgkQFC+QcFj+6g0tywCbBvZQ8TDHE4vja+jEdMMBV3Mg
00gAn395HNe20M29a9iY0sHr1HmwQGAH
oG
-----END PGP SIGNATURE-----

--jho1yZJdad60DJr+--