From: Sylvain Joyeux <sylvain.joyeux@...4x.org>
Date: 2007-03-15T03:11:41+09:00
Subject: Re: [PATCH] grave bug in 1.8.6's thread implementation

--Boundary-00=_arD+FGsPJCbVt7Z
Content-Type: text/plain;
  charset="utf-8"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

Here the next one (hopefully the last)

#wait should re-lock even in exception context

It replaces the last patch

I attached a test suite which checks for the things I fixed today

Sylvain

--Boundary-00=_arD+FGsPJCbVt7Z
Content-Type: text/x-diff;
  charset="utf-8";
  name="thread-condvar_wait.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
	filename="thread-condvar_wait.diff"

--- /home/sjoyeux/system/ruby-1.8.6/ext/thread/thread.c	2007-03-03 11:08:06.000000000 +0100
+++ thread.c	2007-03-14 17:39:31.000000000 +0100
@@ -391,7 +410,7 @@ rb_mutex_try_lock(VALUE self)
  *
  */
 
-static void
+static VALUE
 lock_mutex(Mutex *mutex)
 {
     VALUE current;
@@ -406,6 +425,7 @@ lock_mutex(Mutex *mutex)
     mutex->owner = current; 
 
     rb_thread_critical = 0;
+    return Qnil;
 }
 
 static VALUE
@@ -624,18 +645,12 @@ static void
 wait_condvar(ConditionVariable *condvar, Mutex *mutex)
 {
     rb_thread_critical = 1;
-    if (!RTEST(mutex->owner)) {
+    if (rb_thread_current() != mutex->owner) {
         rb_thread_critical = 0;
-        return;
+        rb_raise(rb_eThreadError, "not owner of the synchronization mutex");
     }
-    if (mutex->owner != rb_thread_current()) {
-        rb_thread_critical = 0;
-        rb_raise(rb_eThreadError, "Not owner");
-    }
-    mutex->owner = Qnil;
-    wait_list(&condvar->waiting);
-
-    lock_mutex(mutex);
+    unlock_mutex_inner(mutex);
+    rb_ensure(wait_list, (VALUE)&condvar->waiting, lock_mutex, (VALUE)mutex);
 }
 
 static VALUE

--Boundary-00=_arD+FGsPJCbVt7Z
Content-Type: application/x-ruby;
  name="test_thread.rb"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
	filename="test_thread.rb"

require 'thread'
require 'test/unit'

class TC_Thread < Test::Unit::TestCase
    def setup
	Thread.abort_on_exception = true
    end
    def teardown
	Thread.abort_on_exception = false
    end
    def test_condvar
	mutex = Mutex.new
	condvar = ConditionVariable.new
	result = []
	mutex.synchronize do
	    t = Thread.new do
		mutex.synchronize do
		    result << 1
		    condvar.signal
		end
	    end
	
	    result << 0
	    condvar.wait(mutex)
	    result << 2
	    t.join
	end
	assert_equal([0, 1, 2], result)
    end

    def test_condvar_wait_not_owner
	mutex = Mutex.new
	condvar = ConditionVariable.new

	assert_raises(ThreadError) { condvar.wait(mutex) }
    end

    def test_condvar_wait_exception_handling
	# Calling wait in the only thread running should raise a ThreadError of
	# 'stopping only thread'
	mutex = Mutex.new
	condvar = ConditionVariable.new

	Thread.abort_on_exception = false

	thread = Thread.new do
	    mutex.synchronize do
		condvar.wait(mutex)
	    end
	end

	while !thread.stop?
	    sleep(0.1)
	end

	thread.raise Interrupt, "interrupt a dead condition variable"
	assert_raises(Interrupt) { thread.value }
    end
end


--Boundary-00=_arD+FGsPJCbVt7Z--