Forum: Ruby-dev Fwd: kosaki:r37931 (trunk): * vm_core.h (enum rb_thread_status): remove THREAD_TO_KILL

THREAD_TO_KILL $B$r>C$9M}M3$H$+$r5-O?$K;D$7$FD:$/$HNI$+$C$?$+$H!%(B

$B$=$b$=$b(B THREAD_TO_KILL 
$B$,$&$^$/5!G=$7$F$J$$$+$i0lEY@0M}$7$?$<!$$C$F46$8(B
$B$G$7$g$&$+!%(B

-------- Original Message --------
Subject: [ruby-changes:25874] kosaki:r37931 (trunk): * vm_core.h (enum
rb_thread_status): remove THREAD_TO_KILL
Date: Wed, 28 Nov 2012 17:31:12 +0900 (JST)
From: kosaki <ko1@atdot.net>
Reply-To: ruby-changes@quickml.atdot.net
To: ruby-changes@quickml.atdot.net

kosaki  2012-11-28 17:31:03 +0900 (Wed, 28 Nov 2012)

  New Revision: 37931

  http://svn.ruby-lang.org/cgi-bin/viewvc.cgi?view=r...

  Log:
    * vm_core.h (enum rb_thread_status): remove THREAD_TO_KILL
    * vm_core.h (struct rb_thread_struct): add to_kill field
    * thread.c (terminate_i): convert THREAD_TO_KILL to to_kill.
    * thread.c (rb_threadptr_to_kill): ditto.
    * thread.c (rb_thread_kill): ditto.
    * thread.c (rb_thread_wakeup_alive): ditto.
    * thread.c (thread_list_i): ditto.
    * thread.c (static const char): ditto.
    * thread.c (thread_status_name): ditto.
    * thread.c (rb_thread_status): ditto.
    * thread.c (rb_thread_inspect): ditto.
    * vm_backtrace.c (thread_backtrace_to_ary): ditto.

    * thread.c (rb_threadptr_execute_interrupts): fix thread status
      overwritten issue. [Bug #7450] [ruby-core:50249]

    * test/ruby/test_thread.rb (test_hread_status_raise_after_kill):
      test for the above.
    * test/ruby/test_thread.rb (test_thread_status_in_trap): test for
      thread status in trap.
    * test/ruby/test_thread.rb (test_status_and_stop_p): remove
      Thread.control_interrupt unsafe test. Thread#kill no longer
      changes thread status. Instead of, Thread#kill receiver changes
      their own status when receiving kill signal.

  Modified files:
    trunk/ChangeLog
    trunk/test/ruby/test_thread.rb
    trunk/thread.c
    trunk/vm_backtrace.c
    trunk/vm_core.h

Index: ChangeLog
===================================================================
--- ChangeLog  (revision 37930)
+++ ChangeLog  (revision 37931)
@@ -1,3 +1,30 @@
+Wed Nov 28 16:40:14 2012  KOSAKI Motohiro  <kosaki.motohiro@gmail.com>
+
+  * vm_core.h (enum rb_thread_status): remove THREAD_TO_KILL
+  * vm_core.h (struct rb_thread_struct): add to_kill field
+  * thread.c (terminate_i): convert THREAD_TO_KILL to to_kill.
+  * thread.c (rb_threadptr_to_kill): ditto.
+  * thread.c (rb_thread_kill): ditto.
+  * thread.c (rb_thread_wakeup_alive): ditto.
+  * thread.c (thread_list_i): ditto.
+  * thread.c (static const char): ditto.
+  * thread.c (thread_status_name): ditto.
+  * thread.c (rb_thread_status): ditto.
+  * thread.c (rb_thread_inspect): ditto.
+  * vm_backtrace.c (thread_backtrace_to_ary): ditto.
+
+  * thread.c (rb_threadptr_execute_interrupts): fix thread status
+    overwritten issue. [Bug #7450] [ruby-core:50249]
+
+  * test/ruby/test_thread.rb (test_hread_status_raise_after_kill):
+    test for the above.
+  * test/ruby/test_thread.rb (test_thread_status_in_trap): test for
+    thread status in trap.
+  * test/ruby/test_thread.rb (test_status_and_stop_p): remove
+    Thread.control_interrupt unsafe test. Thread#kill no longer
+    changes thread status. Instead of, Thread#kill receiver changes
+    their own status when receiving kill signal.
+
 Wed Nov 28 16:21:46 2012  KOSAKI Motohiro  <kosaki.motohiro@gmail.com>

   * thread.c (struct rb_mutex_struct): add allow_trap field.
Index: vm_core.h
===================================================================
--- vm_core.h  (revision 37930)
+++ vm_core.h  (revision 37931)
@@ -425,7 +425,6 @@
     TypedData_Get_Struct((obj), rb_thread_t, &ruby_threadptr_data_type,
(ptr))

 enum rb_thread_status {
-    THREAD_TO_KILL,
     THREAD_RUNNABLE,
     THREAD_STOPPED,
     THREAD_STOPPED_FOREVER,
@@ -498,6 +497,7 @@
     /* thread control */
     rb_thread_id_t thread_id;
     enum rb_thread_status status;
+    int to_kill;
     int priority;

     native_thread_data_t native_thread_data;
Index: thread.c
===================================================================
--- thread.c  (revision 37930)
+++ thread.c  (revision 37931)
@@ -326,7 +326,6 @@
     if (th != main_thread) {
   thread_debug("terminate_i: %p\n", (void *)th);
   rb_threadptr_async_errinfo_enque(th, eTerminateSignal);
-  th->status = THREAD_TO_KILL;
   rb_threadptr_interrupt(th);
     }
     else {
@@ -1732,7 +1731,8 @@
 rb_threadptr_to_kill(rb_thread_t *th)
 {
     rb_threadptr_async_errinfo_clear(th);
-    th->status = THREAD_TO_KILL;
+    th->status = THREAD_RUNNABLE;
+    th->to_kill = 1;
     th->errinfo = INT2FIX(TAG_FATAL);
     TH_JUMP_TAG(th, TAG_FATAL);
 }
@@ -1743,7 +1743,6 @@
     if (th->raised_flag) return;

     while (1) {
-  enum rb_thread_status status = th->status;
   rb_atomic_t interrupt;
   rb_atomic_t old;
   int sig;
@@ -1766,13 +1765,16 @@
   finalizer_interrupt = interrupt & FINALIZER_INTERRUPT_MASK;
   trap_interrupt = interrupt & TRAP_INTERRUPT_MASK;

-  th->status = THREAD_RUNNABLE;

+
   /* signal handling */
   if (trap_interrupt && (th == th->vm->main_thread)) {
+      enum rb_thread_status prev_status = th->status;
+      th->status = THREAD_RUNNABLE;
       while ((sig = rb_get_next_signal()) != 0) {
     rb_signal_exec(th, sig);
       }
+      th->status = prev_status;
   }

   /* exception from another thread */
@@ -1788,10 +1790,13 @@
     rb_threadptr_to_kill(th);
       }
       else {
+    /* set runnable if th was slept. */
+    if (th->status == THREAD_STOPPED ||
+        th->status == THREAD_STOPPED_FOREVER)
+        th->status = THREAD_RUNNABLE;
     rb_exc_raise(err);
       }
   }
-  th->status = status;

   if (finalizer_interrupt) {
       rb_gc_finalize_deferred();
@@ -1805,7 +1810,7 @@
       else
     limits_us >>= -th->priority;

-      if (status == THREAD_RUNNABLE || status == THREAD_TO_KILL)
+      if (th->status == THREAD_RUNNABLE)
     th->running_time_us += TIME_QUANTUM_USEC;

       EXEC_EVENT_HOOK(th, RUBY_EVENT_SWITCH, th->cfp->self, 0, 0, 
Qundef);
@@ -1985,7 +1990,7 @@
     if (th != GET_THREAD() && th->safe_level < 4) {
   rb_secure(4);
     }
-    if (th->status == THREAD_TO_KILL || th->status == THREAD_KILLED) {
+    if (th->to_kill || th->status == THREAD_KILLED) {
   return thread;
     }
     if (th == th->vm->main_thread) {
@@ -2000,7 +2005,6 @@
     }
     else {
   rb_threadptr_async_errinfo_enque(th, eKillSignal);
-  th->status = THREAD_TO_KILL;
   rb_threadptr_interrupt(th);
     }
     return thread;
@@ -2082,9 +2086,8 @@
   return Qnil;
     }
     rb_threadptr_ready(th);
-    if (th->status != THREAD_TO_KILL) {
+    if (th->status == THREAD_STOPPED || th->status ==
THREAD_STOPPED_FOREVER)
   th->status = THREAD_RUNNABLE;
-    }
     return thread;
 }

@@ -2157,7 +2160,6 @@
       case THREAD_RUNNABLE:
       case THREAD_STOPPED:
       case THREAD_STOPPED_FOREVER:
-      case THREAD_TO_KILL:
   rb_ary_push(ary, th->self);
       default:
   break;
@@ -2352,16 +2354,17 @@
 }

 static const char *
-thread_status_name(enum rb_thread_status status)
+thread_status_name(rb_thread_t *th)
 {
-    switch (status) {
+    switch (th->status) {
       case THREAD_RUNNABLE:
-  return "run";
+  if (th->to_kill)
+      return "aborting";
+  else
+      return "run";
       case THREAD_STOPPED:
       case THREAD_STOPPED_FOREVER:
   return "sleep";
-      case THREAD_TO_KILL:
-  return "aborting";
       case THREAD_KILLED:
   return "dead";
       default:
@@ -2411,7 +2414,7 @@
   }
   return Qfalse;
     }
-    return rb_str_new2(thread_status_name(th->status));
+    return rb_str_new2(thread_status_name(th));
 }


@@ -2500,7 +2503,7 @@
     VALUE str;

     GetThreadPtr(thread, th);
-    status = thread_status_name(th->status);
+    status = thread_status_name(th);
     str = rb_sprintf("#<%s:%p %s>", cname, (void *)thread, status);
     OBJ_INFECT(str, thread);

Index: vm_backtrace.c
===================================================================
--- vm_backtrace.c  (revision 37930)
+++ vm_backtrace.c  (revision 37931)
@@ -738,15 +738,8 @@
     rb_thread_t *th;
     GetThreadPtr(thval, th);

-    switch (th->status) {
-      case THREAD_RUNNABLE:
-      case THREAD_STOPPED:
-      case THREAD_STOPPED_FOREVER:
-  break;
-      case THREAD_TO_KILL:
-      case THREAD_KILLED:
+    if (th->to_kill || th->status == THREAD_KILLED)
   return Qnil;
-    }

     return vm_backtrace_to_ary(th, argc, argv, 0, 0, to_str);
 }
Index: test/ruby/test_thread.rb
===================================================================
--- test/ruby/test_thread.rb  (revision 37930)
+++ test/ruby/test_thread.rb  (revision 37931)
@@ -497,7 +497,6 @@
     a = ::Thread.new { raise("die now") }
     b = Thread.new { Thread.stop }
     c = Thread.new { Thread.exit }
-    d = Thread.new { sleep }
     e = Thread.current
     sleep 0.5

@@ -511,21 +510,14 @@
     assert_match(/^#<TestThread::Thread:.* dead>$/, c.inspect)
     assert(c.stop?)

-    d.kill
-    # to avoid thread switching...
-    ds1 = d.status
-    ds2 = d.stop?
     es1 = e.status
     es2 = e.stop?
-    assert_equal(["aborting", false], [ds1, ds2])
-
     assert_equal(["run", false], [es1, es2])

   ensure
     a.kill if a
     b.kill if b
     c.kill if c
-    d.kill if d
   end

   def test_safe_level
@@ -912,4 +904,47 @@
 }
     INPUT
   end
+
+  def test_thread_status_in_trap
+    # when running trap handler, Thread#status must show "run"
+    # Even though interrupted from sleeping function
+    assert_in_out_err([], <<-INPUT, %w(sleep run), [])
+      Signal.trap(:INT) {
+        puts Thread.current.status
+      }
+
+      Thread.new(Thread.current) {|mth|
+        sleep 0.01
+        puts mth.status
+        Process.kill(:INT, $$)
+      }
+      sleep 0.1
+    INPUT
+  end
+
+  # Bug #7450
+  def test_thread_status_raise_after_kill
+    ary = []
+
+    t = Thread.new {
+      begin
+        ary << Thread.current.status
+        sleep
+      ensure
+        begin
+          ary << Thread.current.status
+          sleep
+        ensure
+          ary << Thread.current.status
+        end
+      end
+    }
+
+    sleep 0.01
+    t.kill
+    sleep 0.01
+    t.raise
+    sleep 0.01
+    assert_equal(ary, ["run", "aborting", "aborting"])
+  end
 end