Bug #4474: 複数のスレッドからトランザクションに入ろうとした場合のPStoreの挙動

• Status: Closed
• Assignee: -
• Target version: -
• ruby -v: -

[Glass_saga (Masaki Matsushita)]

=begin
PStoreは、initializeの第2引数thread_safeが真であればデータベースの読み書きをMutexで同期するようになっています。
しかし、次のコード

require 'pstore'
require 'thread'

pstore = PStore.new("foo", true)
q = Queue.new

Thread.start do
pstore.transaction do
pstore[:hoge] = "fuga"
q.push(nil)
sleep
end
end

q.pop
pstore.transaction do
p pstore[:hoge]
end

を実行すると例外が発生します。

/usr/local/lib/ruby/1.9.1/pstore.rb:321:in transaction': nested transaction (PStore::Error) from pstore.rb:16:in '

以下のコードはpstore.rbの319行目以降から抜粋したものです。

319 def transaction(read_only = false, &block) # :yields: pstore
320 value = nil
321 raise PStore::Error, "nested transaction" if @transaction
322 @lock.synchronize do
　　　　 (中略)
348 end
(中略)
352 end

Mutexで保護されたセクションに入る前に、別のトランザクションが実行されていないかどうか調べています。
あるスレッドがトランザクションを実行中に別のスレッドがトランザクションに入ろうとすると、ここで例外が発生します。
thread-safeの定義にも依りますが、これはバグではないでしょうか？
=end

[sorah (Sorah Fukumori)]

• ruby -v changed from ruby 1.9.2p180 (2011-02-18 revision 30909) [x86_64-linux] to -

=begin
sora_hです。

rdocはこうなっていますが...

PStore objects are always reentrant. But if thread_safe is set to true,

then it will become thread-safe at the cost of a minor performance hit.

スレッドセーフになるとどうなるのかという仕様が良くわからない感じです。

で、ちょっとしらべてみたところr15948でこの引数が入ったみたいですね
コード書いたのは外部の人か。どういう意図なんでしょうかねえ。
(http://svn.ruby-lang.org/cgi-bin/viewvc.cgi?revision=15948&view=revision)

個人的にnested transaction例外は発生すべきでは無いと思うのですが…

P.S.: スレッドセーフ引数周りのテストが無いみたいです (test/test_pstore.rb)

2011/3/6 Masaki Matsushita [e-mail]:

PStoreは、initializeの第2引数thread_safeが真であればデータベースの読み書きをMutexで同期するようになっています。

--
Shota Fukumori a.k.a. @sora_h - http://codnote.net/
=end

[sorah (Sorah Fukumori)]

=begin
sora_hです。

rdocはこうなっていますが...

PStore objects are always reentrant. But if thread_safe is set to true,

then it will become thread-safe at the cost of a minor performance hit.

スレッドセーフになるとどうなるのかという仕様が良くわからない感じです。

で、ちょっとしらべてみたところr15948でこの引数が入ったみたいですね
コード書いたのは外部の人か。どういう意図なんでしょうかねえ。
(http://svn.ruby-lang.org/cgi-bin/viewvc.cgi?revision=15948&view=revision)

個人的にnested transaction例外は発生すべきでは無いと思うのですが…

P.S.: スレッドセーフ引数周りのテストが無いみたいです (test/test_pstore.rb)

2011/3/6 Masaki Matsushita [e-mail]:

PStoreは、initializeの第2引数thread_safeが真であればデータベースの読み書きをMutexで同期するようになっています。

--
Shota Fukumori a.k.a. @sora_h - http://codnote.net/
=end

[sorah (Sorah Fukumori)]

• File patch.diff patch.diff added

=begin
sora_hです。

一応これをバグを見て、nested transaction例外を発生しないようにする変更とテストを追加するパッチを
書いてみました。どうでしょうか。 (添付します)

また、サンプルコードは例外が発生しないときいつまでも終わらないため、すこし手を加えました。
一応パッチをあてると !thread_safe なときには例外が発生し、test_pstoreもpassします。

require 'pstore'

pstore = PStore.new("/tmp/foo", true)
a = false

Thread.start do
pstore.transaction do
pstore[:foo] = :foo
a = true
sleep 1
end
end

until a;end
pstore.transaction do
p pstore[:foo]
end

=end

[Glass_saga (Masaki Matsushita)]

• File patch.diff patch.diff added

=begin
僕もパッチを書いてみましたので添付します。
sora_hさんが書いたパッチと異なる点は、トランザクションに入っているスレッドがあるかどうかを格納しているインスタンス変数@transactionを廃して、
Mutexがロックされているかどうかでトランザクションに入っているスレッドがあるかどうかを判定している点です。
また、第2引数thread_safeの真偽に関わらずトランザクション内の処理をMutexで同期しますが、互換性の為、thread_safeが偽である場合にトランザクションを入ろうとすると例外nested transactionが発生するようにしてあります。
test_pstoreもpassします。
いかがでしょうか。
=end

[sorah (Sorah Fukumori)]

=begin
ふむ。

異議がなければ@transactionを消したほうのパッチをマージしたいと思いますが、どうでしょう。
=end

[sorah (Sorah Fukumori)]

• Status changed from Open to Closed

=begin
r31050にてlib/pstore.rbはGlass_sagaさんのパッチ、test/test_pstore.rbは自分のパッチを適用しコミットしました。

Closeします。報告ありがとうございました。
=end

[mame (Yusuke Endoh)]

=begin
遠藤です。

2011年3月7日9:47 Masaki Matsushita [e-mail]:

sora_hさんが書いたパッチと異なる点は、トランザクションに入っているスレッドがあるかどうかを格納しているインスタンス変数@transactionを廃して、
Mutexがロックされているかどうかでトランザクションに入っているスレッドがあるかどうかを判定している点です。
また、第2引数thread_safeの真偽に関わらずトランザクション内の処理をMutexで同期しますが、互換性の為、thread_safeが偽である場合にトランザクションを入ろうとすると例外nested transactionが発生するようにしてあります。

ちょっとだけ考古学してみました。Hongli Lai の元記事がこちら。

http://izumi.plan99.net/blog/index.php/2008/03/26/making-pstore-reaaaally-fast/

とにかく高速化したかったみたいですね。最後に "I’ll send a patch to
ruby-core so that this can be merged back upstream" とあるけれど、
そのメールは見つかりませんでした。matz が勝手に (または ruby-core
以外で話をして) 取り込んだんですかね。

r26673 で thread_safe が全然動いてなかったお粗末なバグが修正されて
いることから、Hongli Lai は thread_safe のテストを全くやらなかった
と思われます。なので、thread_safe のときにも nested transation 例外
が投げられるのは特に意図はしていなさそうな気がします。

PStore#initialize の rdoc を見ると、

PStore objects are always reentrant. But if thread_safe is set to true,

then it will become thread-safe at the cost of a minor performance hit.

とあり、元の目的が超高速化なだけあって、Mutex のロックのコストを気に
しているようです。ケチな話ですね。そういう意味で、

インスタンス変数@transactionを廃して、
Mutexがロックされているかどうか

の変更は、少なくとも Hongli Lai の望むところではなさそうです。
少なくとも、上の rdoc は嘘になっているので修正する必要があります。

一応、Mutex#synchronize は DummyMutex より 2 倍以上遅いみたいです。
1 回あたりを考えるともともと無視できそうなコストですが、PStore のよ
うな用途だと問題になりうるんですかね。

$ time ./ruby -e '
class DummyMutex
def synchronize
yield
end
end
m = DummyMutex.new
1000000.times { m.synchronize { } }
'

real 0m0.219s
user 0m0.200s
sys 0m0.012s

$ time ./ruby -e '
m = Mutex.new
1000000.times { m.synchronize { } }
'

real 0m0.527s
user 0m0.492s
sys 0m0.004s

Hongli Lai は ruby-core でちょくちょく見かける人なので、聞けば返事を
してくれるんじゃないでしょうか。この話は ruby-core で議論した方がいい
のでは。

--
Yusuke Endoh [e-mail]
=end

[mame (Yusuke Endoh)]

=begin
遠藤です。

2011年3月7日9:47 Masaki Matsushita [e-mail]:

sora_hさんが書いたパッチと異なる点は、トランザクションに入っているスレッドがあるかどうかを格納しているインスタンス変数@transactionを廃して、
Mutexがロックされているかどうかでトランザクションに入っているスレッドがあるかどうかを判定している点です。
また、第2引数thread_safeの真偽に関わらずトランザクション内の処理をMutexで同期しますが、互換性の為、thread_safeが偽である場合にトランザクションを入ろうとすると例外nested transactionが発生するようにしてあります。

ちょっとだけ考古学してみました。Hongli Lai の元記事がこちら。

http://izumi.plan99.net/blog/index.php/2008/03/26/making-pstore-reaaaally-fast/

とにかく高速化したかったみたいですね。最後に "I’ll send a patch to
ruby-core so that this can be merged back upstream" とあるけれど、
そのメールは見つかりませんでした。matz が勝手に (または ruby-core
以外で話をして) 取り込んだんですかね。

r26673 で thread_safe が全然動いてなかったお粗末なバグが修正されて
いることから、Hongli Lai は thread_safe のテストを全くやらなかった
と思われます。なので、thread_safe のときにも nested transation 例外
が投げられるのは特に意図はしていなさそうな気がします。

PStore#initialize の rdoc を見ると、

PStore objects are always reentrant. But if thread_safe is set to true,

then it will become thread-safe at the cost of a minor performance hit.

とあり、元の目的が超高速化なだけあって、Mutex のロックのコストを気に
しているようです。ケチな話ですね。そういう意味で、

インスタンス変数@transactionを廃して、
Mutexがロックされているかどうか

の変更は、少なくとも Hongli Lai の望むところではなさそうです。
少なくとも、上の rdoc は嘘になっているので修正する必要があります。

一応、Mutex#synchronize は DummyMutex より 2 倍以上遅いみたいです。
1 回あたりを考えるともともと無視できそうなコストですが、PStore のよ
うな用途だと問題になりうるんですかね。

$ time ./ruby -e '
class DummyMutex
def synchronize
yield
end
end
m = DummyMutex.new
1000000.times { m.synchronize { } }
'

real 0m0.219s
user 0m0.200s
sys 0m0.012s

$ time ./ruby -e '
m = Mutex.new
1000000.times { m.synchronize { } }
'

real 0m0.527s
user 0m0.492s
sys 0m0.004s

Hongli Lai は ruby-core でちょくちょく見かける人なので、聞けば返事を
してくれるんじゃないでしょうか。この話は ruby-core で議論した方がいい
のでは。

--
Yusuke Endoh [e-mail]
=end

[Glass_saga (Masaki Matsushita)]

=begin
私も試してみましたが、確かにMutexはDummyMutexと比べて2倍程度遅いようです。

しかし、次のコード

require 'benchmark'
require 'pstore'

pd = PStore.new("foo") #DummyMutex
pm = PStore.new("bar", true) #Mutex

pd.transaction { pd["hoge"] = 0 }
pm.transaction { pm["hoge"] = 0 }

N = 1000

Benchmark.bmbm do |x|
x.report("DummyMutex") do
N.times do
pd.transaction { pd["hoge"] += 1 }
end
end
x.report("Mutex") do
N.times do
pm.transaction { pm["hoge"] += 1 }
end
end
end

をruby 1.9.2p180 (2011-02-18 revision 30909) [x86_64-linux]で実行したところ

Rehearsal ----------------------------------------------
DummyMutex 0.020000 0.270000 0.290000 ( 0.424386)
Mutex 0.080000 0.250000 0.330000 ( 0.468132)
------------------------------------- total: 0.620000sec

             user     system      total        real

DummyMutex 0.100000 0.240000 0.340000 ( 0.476209)
Mutex 0.110000 0.210000 0.320000 ( 0.469675)

という結果となりました。
トランザクションの出入りに関わる処理全体として見るとあまり大きな差とはなっていないように思えます。

次のコード

require 'pstore'

p = PStore.new("foo", true)
p.transaction { p["hoge"] = 0 }

1000.times do
p.transaction { p["hoge"] += 1 }
end

をプロファイルしたところ、

% cumulative self self total
time seconds seconds calls ms/call ms/call name
64.25 1.42 1.42 1001 1.42 1.42 File#truncate
3.62 1.50 0.08 2003 0.04 0.05 Digest::Instance.digest
3.17 1.57 0.07 2003 0.03 0.09 Digest::Class#digest
3.17 1.64 0.07 1001 0.07 1.67 PStore#save_data
3.17 1.71 0.07 1001 0.07 0.20 PStore#load_data
2.26 1.76 0.05 1001 0.05 2.03 Mutex#synchronize
(以下略)

となっており、Mutexによるロックは時間的に多くを占めるものではないようです。
以上のような結果も併せて、ruby-coreの方にも投稿してみようと思います。
=end

[Glass_saga (Masaki Matsushita)]

• File patch2.diff patch2.diff added

=begin
r31050にて#4のパッチが取り込まれましたが、

require 'pstore'

p = PStore.new("foo", true)

p.transaction { p.transaction { } }

上記のコードで例外nested transaction (PStore::Error)ではなく例外deadlock; recursive locking (ThreadError)が発生するというバグがありましたのでこれを修正するパッチを書きました。
つきましては、こちらのパッチも取り込んで頂けないでしょうか？
よろしくお願いします。
=end