From: Yui NARUSE <naruse@...>
Date: 2011-12-20T16:55:40+09:00
Subject: [ruby-dev:45014] [ruby-trunk - Bug #5768] TestRequire#test_race_exceptionで競合するケースがまだある


Issue #5768 has been updated by Yui NARUSE.


Nobuyoshi Nakada wrote:
>  rb_barrier_release/rb_barrier_destroyが呼ばれた時点ではmutexがロックさ
>  れていることが前提ですので、後半のBarrier部分の変更だけで充分な気がしま
>  す。

その前提は成立しません。

このテストにおいて、t1 は t2.stop? == true を待ってから、例外によって require を抜けます。
言い換えると、t2 が rb_barrier_wait 内の rb_mutex_lock 内の native_cond_wait に到達するのを待ってから、
例外によって require を例外を抜けたいと思っています。
最低限のラインとしては、mutex->cond_waiting++ されてから抜けないといけません。

さて、t2 における rb_mutex_lock ですが、
thread.c の 3481行目に th->status = THREAD_STOPPED_FOREVER という行があります。
これ以降、Thread#stop?はtrueを返すようになりますが、まだ GVL にとって他のスレッドは動けないので大丈夫です。
そこから10行ほど下ると、GVL_UNLOCK_BEGIN();という行があります。
ここから他のスレッドが動けるようになります、まだこのスレッドは STOP してないのに。
t1 がこの状況で動くと、例外を投げて require を抜け、load_unlock が呼ばれます。
つまりこの時点で前提は崩れます。
そして rb_barrier_release を呼び、まだ 0 のままのmutex->cond_waiting を見て Qfalse を返すため、
loading_tbl から当該パスは削除されてしまいます。
一方、t1 はそれと平行して lock_funcに入り、やっとmutex->cond_waiting++します。
30行ほど下って、native_cond_waitで待ちますが、上記のケースの場合この時にはもう手遅れになっています。

まとめると、GVL_UNLOCK_BEGINから、mutex->cond_waiting++の間に、他のスレッドが動いて、
mutex->cond_waitingを見ると、0のままであり、待ってる人がいないと思うので、
待ち人用の何かを破壊したりすると、待ってた人が発狂する。
具体的には、load.c の load_unlock の rb_barrier_destroy/rb_barrier_destroy と、st_delete あたり、というわけです。

>  > 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"
>  
>  この変更ではt2でロードされることを想定していますが、最初のrequireが例外
>  で中断したあとにt1,t2のどちらが実際にロードすることになるかは不定のはず
>  です。

あぁ、なるほど。なんで :post が含まれないケースがあるのかなぁと思っていました。
----------------------------------------
Bug #5768: TestRequire#test_race_exceptionで競合するケースがまだある
https://bugs.ruby-lang.org/issues/5768

Author: Yui NARUSE
Status: Assigned
Priority: Normal
Assignee: Motohiro KOSAKI
Category: core
Target version: 2.0.0
ruby -v: -


まだ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