normal	2018-08-18 15:33:49 +0900 (Sat, 18 Aug 2018)

  New Revision: 64441

  https://svn.ruby-lang.org/cgi-bin/viewvc.cgi?view=revision&revision=64441

  Log:
    thread_sync.c (rb_mutex_synchronize): only unlock if we own the mutex
    
    If an exception is raised inside Mutex#sleep (via ConditionVariable#wait),
    we cannot guarantee we can own the mutex in the ensure callback.
    
    However, who owns the mutex at that point does not matter.  What
    matters is the Mutex is usable after an exception occurs.
    
    * thread_sync.c (rb_mutex_synchronize): only unlock if we own the mutex
    
    * spec/ruby/library/conditionvariable/wait_spec.rb: only test lock
      usability after thread kill.  Who owns the lock at any
      particular moment is an implementation detail which we cannot
      easily guarantee.

  Modified files:
    trunk/spec/ruby/library/conditionvariable/wait_spec.rb
    trunk/thread_sync.c
Index: spec/ruby/library/conditionvariable/wait_spec.rb
===================================================================
--- spec/ruby/library/conditionvariable/wait_spec.rb	(revision 64440)
+++ spec/ruby/library/conditionvariable/wait_spec.rb	(revision 64441)
@@ -23,21 +23,15 @@ describe "ConditionVariable#wait" do https://github.com/ruby/ruby/blob/trunk/spec/ruby/library/conditionvariable/wait_spec.rb#L23
     th.join
   end
 
-  it "reacquires the lock even if the thread is killed" do
+  it "the lock remains usable 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
-        begin
-          cv.wait(m)
-        ensure
-          owned = m.owned?
-          $stderr.puts "\nThe Thread doesn't own the Mutex!" unless owned
-        end
+        cv.wait(m)
       end
     end
 
@@ -49,24 +43,19 @@ describe "ConditionVariable#wait" do https://github.com/ruby/ruby/blob/trunk/spec/ruby/library/conditionvariable/wait_spec.rb#L43
     th.kill
     th.join
 
-    owned.should == true
+    m.try_lock.should == true
+    m.unlock
   end
 
-  it "reacquires the lock even if the thread is killed after being signaled" do
+  it "lock remains usable 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
-        begin
-          cv.wait(m)
-        ensure
-          owned = m.owned?
-          $stderr.puts "\nThe Thread doesn't own the Mutex!" unless owned
-        end
+        cv.wait(m)
       end
     end
 
@@ -84,7 +73,8 @@ describe "ConditionVariable#wait" do https://github.com/ruby/ruby/blob/trunk/spec/ruby/library/conditionvariable/wait_spec.rb#L73
     }
 
     th.join
-    owned.should == true
+    m.try_lock.should == true
+    m.unlock
   end
 
   it "supports multiple Threads waiting on the same ConditionVariable and Mutex" do
Index: thread_sync.c
===================================================================
--- thread_sync.c	(revision 64440)
+++ thread_sync.c	(revision 64441)
@@ -321,6 +321,38 @@ rb_mutex_owned_p(VALUE self) https://github.com/ruby/ruby/blob/trunk/thread_sync.c#L321
     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)
 {
@@ -333,31 +365,7 @@ rb_mutex_unlock_th(rb_mutex_t *mutex, rb https://github.com/ruby/ruby/blob/trunk/thread_sync.c#L365
 	err = "Attempt to unlock a mutex which is locked by another thread";
     }
     else {
-	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;
+        mutex_do_unlock(mutex, th);
     }
 
     return err;
@@ -497,6 +505,20 @@ mutex_sleep(int argc, VALUE *argv, VALUE https://github.com/ruby/ruby/blob/trunk/thread_sync.c#L505
     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
@@ -509,7 +531,7 @@ VALUE https://github.com/ruby/ruby/blob/trunk/thread_sync.c#L531
 rb_mutex_synchronize(VALUE mutex, VALUE (*func)(VALUE arg), VALUE arg)
 {
     rb_mutex_lock(mutex);
-    return rb_ensure(func, arg, rb_mutex_unlock, mutex);
+    return rb_ensure(func, arg, mutex_unlock_if_owned, mutex);
 }
 
 /*

--
ML: ruby-changes@q...
Info: http://www.atdot.net/~ko1/quickml/