Re: Next 1.8.6 on Sept. 22

Hi,

At Mon, 10 Sep 2007 03:57:32 +0900,
Urabe Shyouhei wrote in [ruby-core:12103]:
> >     [thread] mutex_relock is useless, remove it
> 
> Nobu, can you commit this please?

I agree that function should be removed.  However, it revives
the old bug mentioned in [ruby-dev:30653].

Now I kinda conclude that assert_no_survivors() is
false-positive.

A patch for 1.8 head.


Index: eval.c
===================================================================
--- eval.c	(revision 13423)
+++ eval.c	(working copy)
@@ -6827,5 +6827,4 @@ rb_load(fname, wrap)
     NODE *volatile last_node;
     NODE *saved_cref = ruby_cref;
-    TMP_PROTECT;
 
     if (wrap && ruby_safe_level >= 4) {
@@ -11235,8 +11234,17 @@ rb_thread_wakeup(thread)
     VALUE thread;
 {
+    if (!RTEST(rb_thread_wakeup_alive(thread)))
+	rb_raise(rb_eThreadError, "killed thread");
+    return thread;
+}
+
+VALUE
+rb_thread_wakeup_alive(thread)
+    VALUE thread;
+{
     rb_thread_t th = rb_thread_check(thread);
 
     if (th->status == THREAD_KILLED)
-	rb_raise(rb_eThreadError, "killed thread");
+	return Qnil;
     rb_thread_ready(th);
 
@@ -12174,5 +12182,5 @@ rb_thread_status(thread)
  */
 
-static VALUE
+VALUE
 rb_thread_alive_p(thread)
     VALUE thread;
Index: intern.h
===================================================================
--- intern.h	(revision 13423)
+++ intern.h	(working copy)
@@ -208,6 +208,8 @@ void rb_thread_sleep_forever _((void));
 VALUE rb_thread_stop _((void));
 VALUE rb_thread_wakeup _((VALUE));
+VALUE rb_thread_wakeup_alive _((VALUE));
 VALUE rb_thread_run _((VALUE));
 VALUE rb_thread_kill _((VALUE));
+VALUE rb_thread_alive_p _((VALUE));
 VALUE rb_thread_create _((VALUE (*)(ANYARGS), void*));
 void rb_thread_interrupt _((void));
Index: ext/thread/thread.c
===================================================================
--- ext/thread/thread.c	(revision 13423)
+++ ext/thread/thread.c	(working copy)
@@ -22,9 +22,10 @@ static VALUE set_critical(VALUE value);
 
 static VALUE
-thread_exclusive_do(void)
+thread_exclusive(VALUE (*func)(ANYARGS), VALUE arg)
 {
-    rb_thread_critical = 1;
+    VALUE critical = rb_thread_critical;
 
-    return rb_yield(Qundef);
+    rb_thread_critical = 1;
+    return rb_ensure(func, arg, set_critical, (VALUE)critical);
 }
 
@@ -41,5 +42,5 @@ static VALUE
 rb_thread_exclusive(void)
 {
-    return rb_ensure(thread_exclusive_do, Qundef, set_critical, rb_thread_critical);
+    return thread_exclusive(rb_yield, Qundef);
 }
 
@@ -145,5 +146,5 @@ shift_list(List *list)
 
     entry = list->entries;
-    if (!entry) return Qundef;
+    if (!entry) return Qnil;
 
     list->entries = entry->next;
@@ -208,6 +209,5 @@ static VALUE
 wake_thread(VALUE thread)
 {
-    return rb_rescue2(rb_thread_wakeup, thread,
-      NULL, Qundef, rb_eThreadError, 0);
+    return rb_thread_wakeup_alive(thread);
 }
 
@@ -215,6 +215,8 @@ static VALUE
 run_thread(VALUE thread)
 {
-    return rb_rescue2(rb_thread_run, thread,
-      NULL, Qundef, rb_eThreadError, 0);
+    thread = wake_thread(thread);
+    if (RTEST(thread) && !rb_thread_critical)
+	rb_thread_schedule();
+    return thread;
 }
 
@@ -264,11 +266,10 @@ wait_list(List *list)
 
 static void
-assert_no_survivors(List *waiting, const char *label, void *addr)
+kill_waiting_threads(List *waiting)
 {
     Entry *entry;
+
     for (entry = waiting->entries; entry; entry = entry->next) {
-        if (RTEST(wake_thread(entry->value))) {
-            rb_bug("%s %p freed with live thread(s) waiting", label, addr);
-        }
+	rb_thread_kill(entry->value);
     }
 }
@@ -304,4 +305,6 @@ typedef struct _Mutex {
 } Mutex;
 
+#define MUTEX_LOCKED_P(mutex) (RTEST((mutex)->owner) && rb_thread_alive_p((mutex)->owner))
+
 static void
 mark_mutex(Mutex *mutex)
@@ -320,5 +323,5 @@ static void
 free_mutex(Mutex *mutex)
 {
-    assert_no_survivors(&mutex->waiting, "mutex", mutex);
+    kill_waiting_threads(&mutex->waiting);
     finalize_mutex(mutex);
     xfree(mutex);
@@ -362,5 +365,5 @@ rb_mutex_locked_p(VALUE self)
     Mutex *mutex;
     Data_Get_Struct(self, Mutex, mutex);
-    return RTEST(mutex->owner) ? Qtrue : Qfalse;
+    return MUTEX_LOCKED_P(mutex) ? Qtrue : Qfalse;
 }
 
@@ -381,5 +384,5 @@ rb_mutex_try_lock(VALUE self)
     Data_Get_Struct(self, Mutex, mutex);
 
-    if (RTEST(mutex->owner))
+    if (MUTEX_LOCKED_P(mutex))
         return Qfalse;
 
@@ -404,9 +407,17 @@ lock_mutex(Mutex *mutex)
     rb_thread_critical = 1;
 
-    while (RTEST(mutex->owner)) {
-        wait_list(&mutex->waiting);
-        rb_thread_critical = 1;
+    if (!MUTEX_LOCKED_P(mutex)) {
+	mutex->owner = current;
+    }
+    else {
+	do {
+	    wait_list(&mutex->waiting);
+	    rb_thread_critical = 1;
+	    if (!MUTEX_LOCKED_P(mutex)) {
+		mutex->owner = current;
+		break;
+	    }
+	} while (mutex->owner != current);
     }
-    mutex->owner = current; 
 
     rb_thread_critical = 0;
@@ -435,14 +446,10 @@ unlock_mutex_inner(Mutex *mutex)
     VALUE waking;
 
-    if (!RTEST(mutex->owner)) {
-	rb_raise(rb_eThreadError, "not owner");
-    }
-
     if (mutex->owner != rb_thread_current()) {
 	rb_raise(rb_eThreadError, "not owner");
     }
 
-    mutex->owner = Qnil;
     waking = wake_one(&mutex->waiting);
+    mutex->owner = waking;
 
     return waking;
@@ -459,16 +466,11 @@ static VALUE
 unlock_mutex(Mutex *mutex)
 {
-    VALUE waking;
+    VALUE waking = thread_exclusive(unlock_mutex_inner, (VALUE)mutex);
 
-    rb_thread_critical = 1;
-    waking = rb_ensure(unlock_mutex_inner, (VALUE)mutex, set_critical, 0);
-
-    if (waking == Qundef) {
+    if (!RTEST(waking)) {
         return Qfalse;
     }
 
-    if (RTEST(waking)) {
-        run_thread(waking);
-    }
+    run_thread(waking);
 
     return Qtrue;
@@ -513,14 +515,11 @@ rb_mutex_exclusive_unlock(VALUE self)
     Data_Get_Struct(self, Mutex, mutex);
 
-    rb_thread_critical = 1;
-    waking = rb_ensure(rb_mutex_exclusive_unlock_inner, (VALUE)mutex, set_critical, 0);
+    waking = thread_exclusive(rb_mutex_exclusive_unlock_inner, (VALUE)mutex);
 
-    if (waking == Qundef) {
+    if (!RTEST(waking)) {
         return Qnil;
     }
 
-    if (RTEST(waking)) {
-        run_thread(waking);
-    }
+    run_thread(waking);
 
     return self;
@@ -593,5 +592,5 @@ static void
 free_condvar(ConditionVariable *condvar)
 {
-    assert_no_survivors(&condvar->waiting, "condition variable", condvar);
+    kill_waiting_threads(&condvar->waiting);
     finalize_condvar(condvar);
     xfree(condvar);
@@ -634,4 +633,6 @@ static void
 wait_condvar(ConditionVariable *condvar, Mutex *mutex)
 {
+    VALUE waking;
+
     rb_thread_critical = 1;
     if (rb_thread_current() != mutex->owner) {
@@ -639,5 +640,8 @@ wait_condvar(ConditionVariable *condvar,
         rb_raise(rb_eThreadError, "not owner of the synchronization mutex");
     }
-    unlock_mutex_inner(mutex);
+    waking = unlock_mutex_inner(mutex);
+    if (RTEST(waking)) {
+	wake_thread(waking);
+    }
     rb_ensure(wait_list, (VALUE)&condvar->waiting, lock_mutex, (VALUE)mutex);
 }
@@ -698,6 +702,5 @@ rb_condvar_broadcast(VALUE self)
     Data_Get_Struct(self, ConditionVariable, condvar);
   
-    rb_thread_critical = 1;
-    rb_ensure(wake_all, (VALUE)&condvar->waiting, set_critical, 0);
+    thread_exclusive(wake_all, (VALUE)&condvar->waiting);
     rb_thread_schedule();
 
@@ -716,7 +719,6 @@ static void
 signal_condvar(ConditionVariable *condvar)
 {
-    VALUE waking;
-    rb_thread_critical = 1;
-    waking = rb_ensure(wake_one, (VALUE)&condvar->waiting, set_critical, 0);
+    VALUE waking = thread_exclusive(wake_one, (VALUE)&condvar->waiting);
+
     if (RTEST(waking)) {
         run_thread(waking);
@@ -793,7 +795,7 @@ static void
 free_queue(Queue *queue)
 {
-    assert_no_survivors(&queue->mutex.waiting, "queue", queue);
-    assert_no_survivors(&queue->space_available.waiting, "queue(push)", queue);
-    assert_no_survivors(&queue->value_available.waiting, "queue(pop)", queue);
+    kill_waiting_threads(&queue->mutex.waiting);
+    kill_waiting_threads(&queue->space_available.waiting);
+    kill_waiting_threads(&queue->value_available.waiting);
     finalize_queue(queue);
     xfree(queue);
@@ -836,8 +838,8 @@ rb_queue_marshal_load(VALUE self, VALUE 
     array = rb_marshal_load(data);
     if (TYPE(array) != T_ARRAY) {
-        rb_raise(rb_eRuntimeError, "expected Array of queue data");
+	rb_raise(rb_eTypeError, "expected Array of queue data");
     }
     if (RARRAY(array)->len < 1) {
-        rb_raise(rb_eRuntimeError, "missing capacity value");
+	rb_raise(rb_eArgError, "missing capacity value");
     }
     queue->capacity = NUM2ULONG(rb_ary_shift(array));
Index: test/thread/lbtest.rb
===================================================================
--- test/thread/lbtest.rb	(revision 0)
+++ test/thread/lbtest.rb	(revision 0)
@@ -0,0 +1,51 @@
+#! /usr/bin/ruby
+require 'thread'
+
+class LocalBarrier
+  def initialize(n)
+    @wait = Queue.new
+    @done = Queue.new
+    @keeper = begin_keeper(n)
+  end
+
+  def sync
+    @done.push(true)
+    @wait.pop
+  end
+
+  def join
+    @keeper.join
+  end
+
+  private
+  def begin_keeper(n)
+    Thread.start do
+      n.times do
+        @done.pop
+      end
+      n.times do
+        @wait.push(true)
+      end
+    end
+  end
+end
+
+n = 10
+
+lb = LocalBarrier.new(n)
+
+(n - 1).times do |i|
+  Thread.start do
+    sleep((rand(n) + 1) / 10.0)
+    puts "#{i}: done"
+    lb.sync
+    puts "#{i}: cont"
+  end
+end
+
+lb.sync
+puts "#{n-1}: done"
+
+# lb.join # leaving waiting threads.
+
+puts "exit."

Property changes on: test/thread/lbtest.rb
___________________________________________________________________
Name: svn:executable
   + *
Name: svn:eol-style
   + LF

Index: test/thread/test_thread.rb
===================================================================
--- test/thread/test_thread.rb	(revision 13423)
+++ test/thread/test_thread.rb	(working copy)
@@ -1,2 +1,3 @@
+# -*- ruby-indent-level: 4 -*-
 require 'thread'
 require 'test/unit'
@@ -64,4 +65,17 @@ class TC_Thread < Test::Unit::TestCase
 	assert(locked)
     end
+
+    def test_local_barrier
+        dir = File.dirname(__FILE__)
+        lbtest = File.join(dir, "lbtest.rb")
+        $:.unshift File.join(File.dirname(dir), 'ruby')
+        require 'envutil'
+        $:.shift
+        10.times {
+            result = `#{EnvUtil.rubybin} #{lbtest}`
+            assert(!$?.coredump?, '[ruby-dev:30653]')
+            assert_equal("exit.", result[/.*\Z/], '[ruby-dev:30653]')
+        }
+    end
 end
 


-- 
Nobu Nakada