From: Yui NARUSE Date: 2011-12-20T13:43:30+09:00 Subject: [ruby-dev:45011] [ruby-trunk - Bug #5768] TestRequire#test_race_exceptionで競合するケースがまだある Issue #5768 has been updated by Yui NARUSE. 追いかけた結果、どうも lock に用いている rb_barrier_release/rb_barrier_destroy や rb_mutex_lock のマルチスレッド対応がいまいちだったようです。 前者は rb_mutex_unlock を外した直後から、待っていた他のスレッドが動き始めるのでもはや mutex->cond_waiting は古い情報となってしまう点、 後者は GVL_UNLOCK_BEGIN() した直後から、メインのスレッドが動いてしまう可能性があるのがダメですね。 diff --git a/test/ruby/test_require.rb b/test/ruby/test_require.rb index 9186a6f..f1d8d12 100644 --- a/test/ruby/test_require.rb +++ b/test/ruby/test_require.rb @@ -350,9 +350,18 @@ class TestRequire < Test::Unit::TestCase path = tmp.path tmp.print <<-EOS TestRequire.scratch << :pre -Thread.pass until t2 = TestRequire.scratch[1] +TestRequire.scratch << Thread.current +Thread.pass until t2 = TestRequire.scratch[2] Thread.pass until t2.stop? -open(__FILE__, "w") {|f| f.puts "TestRequire.scratch << :post"} +open(__FILE__, "w") do |f| + f.puts "t1, t2 = TestRequire.scratch[1, 2]" + f.puts "if Thread.current == t2" + f.puts " TestRequire.scratch << :post" + f.puts " until t1.stop?" + f.puts " Thread.pass" + f.puts " end" + f.puts "end" +end raise "con1" EOS tmp.close @@ -368,7 +377,6 @@ raise "con1" require(path) rescue RuntimeError end - t1_res = require(path) Thread.pass until fin @@ -376,7 +384,7 @@ raise "con1" end t2 = Thread.new do - Thread.pass until scratch[0] + Thread.pass until scratch[1] begin scratch << t2 t2_res = require(path) @@ -389,8 +397,8 @@ raise "con1" assert_nothing_raised(ThreadError, bug5754) {t1.join} assert_nothing_raised(ThreadError, bug5754) {t2.join} - assert_equal(true, (t1_res ^ t2_res), bug5754) - assert_equal([:pre, t2, :post, :t2, :t1], scratch, bug5754) + assert_equal(true, (t1_res ^ t2_res), bug5754 + " t1:#{t1_res} t2:#{t2_res}") + assert_equal([:pre, t1, t2, :post, :t2, :t1], scratch, bug5754) ensure tmp.close(true) if tmp end diff --git a/thread.c b/thread.c index ced10c2..c46653b 100644 --- a/thread.c +++ b/thread.c @@ -3409,7 +3409,6 @@ lock_func(rb_thread_t *th, rb_mutex_t *mutex, int timeout_ms) int interrupted = 0; int err = 0; - mutex->cond_waiting++; for (;;) { if (!mutex->th) { mutex->th = th; @@ -3438,7 +3437,6 @@ lock_func(rb_thread_t *th, rb_mutex_t *mutex, int timeout_ms) err = 0; } } - mutex->cond_waiting--; return interrupted; } @@ -3493,10 +3491,12 @@ rb_mutex_lock(VALUE self) if (vm_living_thread_num(th->vm) == th->vm->sleeper) { timeout_ms = 100; } + mutex->cond_waiting++; GVL_UNLOCK_BEGIN(); interrupted = lock_func(th, mutex, timeout_ms); native_mutex_unlock(&mutex->lock); GVL_UNLOCK_END(); + mutex->cond_waiting--; reset_unblock_function(th, &oldubf); @@ -3727,9 +3727,11 @@ rb_barrier_release(VALUE self) { VALUE mutex = GetBarrierPtr(self); rb_mutex_t *m; - rb_mutex_unlock(mutex); + int waiting; GetMutexPtr(mutex, m); - return m->cond_waiting > 0 ? Qtrue : Qfalse; + waiting = m->cond_waiting; + rb_mutex_unlock(mutex); + return waiting > 0 ? Qtrue : Qfalse; } /* @@ -3740,10 +3742,12 @@ rb_barrier_destroy(VALUE self) { VALUE mutex = GetBarrierPtr(self); rb_mutex_t *m; + int waiting; DATA_PTR(self) = 0; - rb_mutex_unlock(mutex); GetMutexPtr(mutex, m); - return m->cond_waiting > 0 ? Qtrue : Qfalse; + waiting = m->cond_waiting; + rb_mutex_unlock(mutex); + return waiting > 0 ? Qtrue : Qfalse; } int ---------------------------------------- Bug #5768: TestRequire#test_race_exceptionで競合するケースがまだある https://bugs.ruby-lang.org/issues/5768 Author: Yui NARUSE Status: Assigned Priority: Normal Assignee: Nobuyoshi Nakada Category: Target version: ruby -v: ruby 2.0.0dev (2011-12-16 trunk 34058) [x86_64-freebsd9.0] まだrequireで競合するケースが残っています。 現在のテストだと確率的にしか起きませんが、以下の通り変更すると確実に起きるようになります。 diff --git a/test/ruby/test_require.rb b/test/ruby/test_require.rb index 9186a6f..262a5ef 100644 --- a/test/ruby/test_require.rb +++ b/test/ruby/test_require.rb @@ -352,7 +352,7 @@ class TestRequire < Test::Unit::TestCase TestRequire.scratch << :pre Thread.pass until t2 = TestRequire.scratch[1] Thread.pass until t2.stop? -open(__FILE__, "w") {|f| f.puts "TestRequire.scratch << :post"} +open(__FILE__, "w") {|f| f.puts "TestRequire.scratch << :post"; f.puts "t1,t2=TestRequire.scratch[1, 2];if Thread.current == t2; Thread.pass until t1.stopped?; end"} raise "con1" EOS tmp.close @@ -364,6 +364,7 @@ raise "con1" t2_res = nil t1 = Thread.new do + scratch << t1 begin require(path) rescue RuntimeError @@ -389,8 +390,8 @@ raise "con1" assert_nothing_raised(ThreadError, bug5754) {t1.join} assert_nothing_raised(ThreadError, bug5754) {t2.join} - assert_equal(true, (t1_res ^ t2_res), bug5754) - assert_equal([:pre, t2, :post, :t2, :t1], scratch, bug5754) + assert_equal(true, (t1_res ^ t2_res), bug5754 + " t1:#{t1_res} t2:#{t2_res}") + assert_equal([:pre, t1, t2, :post, :t2, :t1], scratch, bug5754) ensure tmp.close(true) if tmp end -- http://redmine.ruby-lang.org