eregon	2018-08-18 22:52:53 +0900 (Sat, 18 Aug 2018)

  New Revision: 64448

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

  Log:
    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].

  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 64447)
+++ spec/ruby/library/conditionvariable/wait_spec.rb	(revision 64448)
@@ -23,15 +23,21 @@ describe "ConditionVariable#wait" do https://github.com/ruby/ruby/blob/trunk/spec/ruby/library/conditionvariable/wait_spec.rb#L23
     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 https://github.com/ruby/ruby/blob/trunk/spec/ruby/library/conditionvariable/wait_spec.rb#L49
     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 https://github.com/ruby/ruby/blob/trunk/spec/ruby/library/conditionvariable/wait_spec.rb#L84
     }
 
     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
Index: thread_sync.c
===================================================================
--- thread_sync.c	(revision 64447)
+++ thread_sync.c	(revision 64448)
@@ -321,38 +321,6 @@ 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)
 {
@@ -365,7 +333,31 @@ rb_mutex_unlock_th(rb_mutex_t *mutex, rb https://github.com/ruby/ruby/blob/trunk/thread_sync.c#L333
 	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 https://github.com/ruby/ruby/blob/trunk/thread_sync.c#L497
     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 https://github.com/ruby/ruby/blob/trunk/thread_sync.c#L509
 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);
 }
 
 /*

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