From: Sylvain Joyeux Date: 2007-03-15T21:08:03+09:00 Subject: Re: [PATCH] grave bug in 1.8.6's thread implementation --Boundary-00=_XcT+FsmeN+gR+I/ Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Content-Disposition: inline > Which set of patches do you think should be committed? The former to > ruby_1_8 and the latter to ruby_1_8_6? The latter patches must be applied to 1.8.6, since for now 1.8.6 with the 'new' thread support is unusable for multi-threaded programs. The former patches may not be applied at all. According to mental, it breaks compatibility (well, in my POV and mental's, it breaks poorly written programs ...). If you want to apply these changes, tell me so that I rewrite the patches, the changes I made are quite ugly ... Moreover, you should add fastthread's test suite to ruby's own, and merge in it the tests I wrote for the particular issues fixed by these patches (test_thread.rb) For the sake of simplicity, I join all the files in this mail. I slightly modified the test for ConditionVariable#wait in exception context, so please consider only the files in this mail. Regards Sylvain Joyeux --Boundary-00=_XcT+FsmeN+gR+I/ 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=_XcT+FsmeN+gR+I/ Content-Type: text/x-diff; charset="utf-8"; name="thread-mutex-not_owner.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="thread-mutex-not_owner.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 15:36:58.000000000 +0100 @@ -429,8 +443,13 @@ unlock_mutex_inner(Mutex *mutex) VALUE waking; if (!RTEST(mutex->owner)) { - return Qundef; + 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); --Boundary-00=_XcT+FsmeN+gR+I/ Content-Type: text/x-diff; charset="utf-8"; name="thread-mutex-remove_one.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="thread-mutex-remove_one.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:12:38.000000000 +0100 @@ -165,12 +165,23 @@ remove_one(List *list, VALUE value) Entry **ref; Entry *entry; - for (ref = &list->entries, entry = list->entries; - entry != NULL; - ref = &entry->next, entry = entry->next) { - if (entry->value == value) { - *ref = entry->next; - recycle_entries(list, entry, entry); + entry = list->entries; + if (!entry) return; + if (entry->value == value) + { + shift_list(list); + return; + } + + for (entry = list->entries; entry->next != NULL; entry = entry->next) { + Entry* next_entry = entry->next; + if (next_entry->value == value) { + entry->next = next_entry->next; + if (!entry->next) + list->last_entry = entry; + + --list->size; + recycle_entries(list, next_entry, next_entry); break; } } --Boundary-00=_XcT+FsmeN+gR+I/ Content-Type: text/x-diff; charset="utf-8"; name="thread-mutex-recursive_lock.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="thread-mutex-recursive_lock.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 15:45:24.000000000 +0100 @@ -399,6 +410,9 @@ lock_mutex(Mutex *mutex) rb_thread_critical = 1; while (RTEST(mutex->owner)) { + if (mutex->owner == current) + rb_raise(rb_eThreadError, "recursive mutex lock"); + wait_list(&mutex->waiting); rb_thread_critical = 1; } --Boundary-00=_XcT+FsmeN+gR+I/ 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 locked = false thread = Thread.new do mutex.synchronize do begin condvar.wait(mutex) rescue Exception locked = mutex.locked? raise end end end while !thread.stop? sleep(0.1) end thread.raise Interrupt, "interrupt a dead condition variable" assert_raises(Interrupt) { thread.value } assert(locked) end end --Boundary-00=_XcT+FsmeN+gR+I/--