diff options
author | eregon <eregon@b2dd03c8-39d4-4d8f-98ff-823fe69b080e> | 2018-08-18 13:52:53 +0000 |
---|---|---|
committer | eregon <eregon@b2dd03c8-39d4-4d8f-98ff-823fe69b080e> | 2018-08-18 13:52:53 +0000 |
commit | c742050ea5fd30108f913383c0fafc4614adb04c (patch) | |
tree | af2da0e27cf305ea29d12ae9636977a4e07d25d6 | |
parent | b5b5b28c650fc51cba7c06b48501de84c0ef9523 (diff) | |
download | ruby-c742050ea5fd30108f913383c0fafc4614adb04c.tar.gz |
Revert r64441
* This reverts commit 647fc1227a4146ecbfeb0d59358abc8d99cd8ae6:
"thread_sync.c (rb_mutex_synchronize): only unlock if we own the mutex"
* Let's try to preserve the semantics of always being locked inside
Mutex#synchronize, even if an exception interrupts ConditionVariable#wait.
* As discussed on [Bug #14999].
git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@64448 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
-rw-r--r-- | spec/ruby/library/conditionvariable/wait_spec.rb | 26 | ||||
-rw-r--r-- | thread_sync.c | 74 |
2 files changed, 44 insertions, 56 deletions
diff --git a/spec/ruby/library/conditionvariable/wait_spec.rb b/spec/ruby/library/conditionvariable/wait_spec.rb index 99e14efe35..d4950a7b27 100644 --- a/spec/ruby/library/conditionvariable/wait_spec.rb +++ b/spec/ruby/library/conditionvariable/wait_spec.rb @@ -23,15 +23,21 @@ describe "ConditionVariable#wait" do th.join end - it "the lock remains usable even if the thread is killed" do + it "reacquires the lock even if the thread is killed" do m = Mutex.new cv = ConditionVariable.new in_synchronize = false + owned = nil th = Thread.new do m.synchronize do in_synchronize = true - cv.wait(m) + begin + cv.wait(m) + ensure + owned = m.owned? + $stderr.puts "\nThe Thread doesn't own the Mutex!" unless owned + end end end @@ -43,19 +49,24 @@ describe "ConditionVariable#wait" do th.kill th.join - m.try_lock.should == true - m.unlock + owned.should == true end - it "lock remains usable even if the thread is killed after being signaled" do + it "reacquires the lock even if the thread is killed after being signaled" do m = Mutex.new cv = ConditionVariable.new in_synchronize = false + owned = nil th = Thread.new do m.synchronize do in_synchronize = true - cv.wait(m) + begin + cv.wait(m) + ensure + owned = m.owned? + $stderr.puts "\nThe Thread doesn't own the Mutex!" unless owned + end end end @@ -73,8 +84,7 @@ describe "ConditionVariable#wait" do } th.join - m.try_lock.should == true - m.unlock + owned.should == true end it "supports multiple Threads waiting on the same ConditionVariable and Mutex" do diff --git a/thread_sync.c b/thread_sync.c index f3d1ccb120..5e511af0db 100644 --- a/thread_sync.c +++ b/thread_sync.c @@ -321,38 +321,6 @@ rb_mutex_owned_p(VALUE self) return owned; } -static void -mutex_do_unlock(rb_mutex_t *mutex, rb_thread_t *th) -{ - struct sync_waiter *cur = 0, *next = 0; - rb_mutex_t **th_mutex = &th->keeping_mutexes; - - VM_ASSERT(mutex->th == th); - - mutex->th = 0; - list_for_each_safe(&mutex->waitq, cur, next, node) { - list_del_init(&cur->node); - switch (cur->th->status) { - case THREAD_RUNNABLE: /* from someone else calling Thread#run */ - case THREAD_STOPPED_FOREVER: /* likely (rb_mutex_lock) */ - rb_threadptr_interrupt(cur->th); - goto found; - case THREAD_STOPPED: /* probably impossible */ - rb_bug("unexpected THREAD_STOPPED"); - case THREAD_KILLED: - /* not sure about this, possible in exit GC? */ - rb_bug("unexpected THREAD_KILLED"); - continue; - } - } - found: - while (*th_mutex != mutex) { - th_mutex = &(*th_mutex)->next_mutex; - } - *th_mutex = mutex->next_mutex; - mutex->next_mutex = NULL; -} - static const char * rb_mutex_unlock_th(rb_mutex_t *mutex, rb_thread_t *th) { @@ -365,7 +333,31 @@ rb_mutex_unlock_th(rb_mutex_t *mutex, rb_thread_t *th) err = "Attempt to unlock a mutex which is locked by another thread"; } else { - mutex_do_unlock(mutex, th); + struct sync_waiter *cur = 0, *next = 0; + rb_mutex_t **th_mutex = &th->keeping_mutexes; + + mutex->th = 0; + list_for_each_safe(&mutex->waitq, cur, next, node) { + list_del_init(&cur->node); + switch (cur->th->status) { + case THREAD_RUNNABLE: /* from someone else calling Thread#run */ + case THREAD_STOPPED_FOREVER: /* likely (rb_mutex_lock) */ + rb_threadptr_interrupt(cur->th); + goto found; + case THREAD_STOPPED: /* probably impossible */ + rb_bug("unexpected THREAD_STOPPED"); + case THREAD_KILLED: + /* not sure about this, possible in exit GC? */ + rb_bug("unexpected THREAD_KILLED"); + continue; + } + } + found: + while (*th_mutex != mutex) { + th_mutex = &(*th_mutex)->next_mutex; + } + *th_mutex = mutex->next_mutex; + mutex->next_mutex = NULL; } return err; @@ -505,20 +497,6 @@ mutex_sleep(int argc, VALUE *argv, VALUE self) return rb_mutex_sleep(self, timeout); } -static VALUE -mutex_unlock_if_owned(VALUE self) -{ - rb_thread_t *th = GET_THREAD(); - rb_mutex_t *mutex; - GetMutexPtr(self, mutex); - - /* we may not own the mutex if an exception occured */ - if (mutex->th == th) { - mutex_do_unlock(mutex, th); - } - return Qfalse; -} - /* * call-seq: * mutex.synchronize { ... } -> result of the block @@ -531,7 +509,7 @@ VALUE rb_mutex_synchronize(VALUE mutex, VALUE (*func)(VALUE arg), VALUE arg) { rb_mutex_lock(mutex); - return rb_ensure(func, arg, mutex_unlock_if_owned, mutex); + return rb_ensure(func, arg, rb_mutex_unlock, mutex); } /* |