[Ruby 1.9 - Bug #4765][Assigned] signal が正しくマスクされておらず main thread でシグナルハンドラが動いている

Issue #4765 has been reported by Motohiro KOSAKI.


Bug #4765: signal が正しくマスクされておらず main thread でシグナルハンドラが動いている
http://redmine.ruby-lang.org/issues/4765

Author: Motohiro KOSAKI
Status: Assigned
Priority: Normal
Assignee: Motohiro KOSAKI
Category: core
Target version: 1.9.x
ruby -v: ruby 1.9.3dev (2011-05-21 trunk 31654) [x86_64-linux]

Bug#4027 から派生させます

sigaction によるシグナルハンドラの実行はタイマースレッドにまかせているはずなのでこれは不要だと思います。
また例外発生時に rb_trap_restore_mask を読んでいるのも、trap() で例外が発生しても trap_ensure() で
シグナルマスクを戻す処理は行なわれているので不要なような気がします。元々どういう理由で呼ばれているのか
わからなかったので自信ないですけど。

レビューした結果、rb_trap_restore_mask()がまったく不要だという意見に賛成します。

  1. Failure:
    test_status_kill(TestProcess)
    [/Users/nagachika/opt/ruby-trunk/src/ruby-trunk/test/ruby/test_process.rb:1073]:
    [s.exited?, s.signaled?, s.stopped?].
    <[false, true, false]> expected but was
    <[true, false, false]>.

原因は rb_syswait()にあります。現状 Process.wait()中は なぜか SIGHUP, SIGQUIT, SIGINTを
SIG_IGNに設定してしまうため
この間に別スレッドが SIGQUITを送るようなテストは正しく動きません。
そもそも、signal handlerはプロセスグローバルなので、処理の途中で一時的に SIG_IGNにしてはいけません。他のスレッドが
迷惑します。pthread_sigmask()に差し替えることを検討したのですが、そもそもシグナルがタイマースレッドにしか配送されない
という設計を貫く限り、いかなる処理も必要ないという結論になりました。

まつもとさん、この処理の実装はまつもとさんのように見えます。10年以上前なので無理かもしれませんが、覚えていたら
SIG_IGNを設定していた理由を教えてください。

また、sighandler() がメインスレッドから呼ばれるのは Process.spawn を実行するとまだおきてしまいます。
process.c の before_fork/after_fork で fork の時に一時的にタイマースレッドを止めてシグナルマスクを外すためです。
こちらもどうしていいものかわからないです。
fork してそのまま動き続けるようなものはどうしようもないような気がしますが、spawn するものは
fork 後に sigprogmask で外すようにするなどでなんとかならないものでしょうか。
あーでもタイマースレッドの再起動で結局一時的に外してしまいますね……

これも、一時的にシグナルマスクを一時的に外す処理自体が不要だと思います。

また、現状、chikanagaさんの報告よりも、さらに状況は悪化しており、シグナル処理を直すと
test_signal.rb#test_kill_immediately_before_termination でテストがハングします。
これは、くだんのテストが子プロセスにSIGINT送って送出されてくる例外を確認しているのですが、
test/unit/parallel.rb#run() に Signal.trap(:INT,“IGNORE”) という極悪な文があるため、
テストでSIGINTが動かなくなる仕様変更がこっそり行われているためです。

soraさん、この行の意図はなんですか?代替案を考える必要があると思いますが、このような
子プロセスに伝搬する設定をこっそり行うのは許容できないと思います。テストに支障が出るし
第一 Ctrl-C がなかなか効かなくて test-all するときにイライラします。

ついでに以下の変更を行いました
・SIGPIPEのハンドラを空関数からSIG_IGNに変更。空関数にするぐらいならユーザ空間に
処理を戻すだけ無駄。それに、ざっと見たところrubyの中でEPIPEをハンドリングしてない場所はない
・処理の最後で、ruby_default_signal() でシグナルハンドラをSIG_DFLに戻している箇所は
シグナルマスクも解除するよう変更

添付のパッチで test-all が全て通ることを確認出来ています。

これは当面コミットせずに1.9.4に回そうと思っています。理由は
・これを直さないと致命的な状況になるような bug report はきてない
・長年放置されていたせいで、すでに複数箇所いまの挙動に依存するコードが見つかっており
安定化に時間がかかる可能性がある

と考えているため

Issue #4765 has been updated by Shota F…

Motohiro KOSAKI wrote:

これは、くだんのテストが子プロセスにSIGINT送って送出されてくる例外を確認しているのですが、
test/unit/parallel.rb#run() に Signal.trap(:INT,“IGNORE”) という極悪な文があるため、
テストでSIGINTが動かなくなる仕様変更がこっそり行われているためです。

soraさん、この行の意図はなんですか?代替案を考える必要があると思いますが、このような
子プロセスに伝搬する設定をこっそり行うのは許容できないと思います。テストに支障が出るし
第一 Ctrl-C がなかなか効かなくて test-all するときにイライラします。

実行するテストでのシグナル周りを失念していました.修正します…

Bug #4765: signal が正しくマスクされておらず main thread でシグナルハンドラが動いている
http://redmine.ruby-lang.org/issues/4765

Author: Motohiro KOSAKI
Status: Assigned
Priority: Normal
Assignee: Motohiro KOSAKI
Category: core
Target version: 1.9.x
ruby -v: ruby 1.9.3dev (2011-05-21 trunk 31654) [x86_64-linux]

Bug#4027 から派生させます

sigaction によるシグナルハンドラの実行はタイマースレッドにまかせているはずなのでこれは不要だと思います。
また例外発生時に rb_trap_restore_mask を読んでいるのも、trap() で例外が発生しても trap_ensure() で
シグナルマスクを戻す処理は行なわれているので不要なような気がします。元々どういう理由で呼ばれているのか
わからなかったので自信ないですけど。

レビューした結果、rb_trap_restore_mask()がまったく不要だという意見に賛成します。

  1. Failure:
    test_status_kill(TestProcess)
    [/Users/nagachika/opt/ruby-trunk/src/ruby-trunk/test/ruby/test_process.rb:1073]:
    [s.exited?, s.signaled?, s.stopped?].
    <[false, true, false]> expected but was
    <[true, false, false]>.

原因は rb_syswait()にあります。現状 Process.wait()中は なぜか SIGHUP, SIGQUIT, SIGINTを
SIG_IGNに設定してしまうため
この間に別スレッドが SIGQUITを送るようなテストは正しく動きません。
そもそも、signal handlerはプロセスグローバルなので、処理の途中で一時的に SIG_IGNにしてはいけません。他のスレッドが
迷惑します。pthread_sigmask()に差し替えることを検討したのですが、そもそもシグナルがタイマースレッドにしか配送されない
という設計を貫く限り、いかなる処理も必要ないという結論になりました。

まつもとさん、この処理の実装はまつもとさんのように見えます。10年以上前なので無理かもしれませんが、覚えていたら
SIG_IGNを設定していた理由を教えてください。

また、sighandler() がメインスレッドから呼ばれるのは Process.spawn を実行するとまだおきてしまいます。
process.c の before_fork/after_fork で fork の時に一時的にタイマースレッドを止めてシグナルマスクを外すためです。
こちらもどうしていいものかわからないです。
fork してそのまま動き続けるようなものはどうしようもないような気がしますが、spawn するものは
fork 後に sigprogmask で外すようにするなどでなんとかならないものでしょうか。
あーでもタイマースレッドの再起動で結局一時的に外してしまいますね……

これも、一時的にシグナルマスクを一時的に外す処理自体が不要だと思います。

また、現状、chikanagaさんの報告よりも、さらに状況は悪化しており、シグナル処理を直すと
test_signal.rb#test_kill_immediately_before_termination でテストがハングします。
これは、くだんのテストが子プロセスにSIGINT送って送出されてくる例外を確認しているのですが、
test/unit/parallel.rb#run() に Signal.trap(:INT,“IGNORE”) という極悪な文があるため、
テストでSIGINTが動かなくなる仕様変更がこっそり行われているためです。

soraさん、この行の意図はなんですか?代替案を考える必要があると思いますが、このような
子プロセスに伝搬する設定をこっそり行うのは許容できないと思います。テストに支障が出るし
第一 Ctrl-C がなかなか効かなくて test-all するときにイライラします。

ついでに以下の変更を行いました
・SIGPIPEのハンドラを空関数からSIG_IGNに変更。空関数にするぐらいならユーザ空間に
処理を戻すだけ無駄。それに、ざっと見たところrubyの中でEPIPEをハンドリングしてない場所はない
・処理の最後で、ruby_default_signal() でシグナルハンドラをSIG_DFLに戻している箇所は
シグナルマスクも解除するよう変更

添付のパッチで test-all が全て通ることを確認出来ています。

これは当面コミットせずに1.9.4に回そうと思っています。理由は
・これを直さないと致命的な状況になるような bug report はきてない
・長年放置されていたせいで、すでに複数箇所いまの挙動に依存するコードが見つかっており
安定化に時間がかかる可能性がある

と考えているため

Issue #4765 has been updated by Motohiro KOSAKI.

File remove-signalmask-op.patch added

ささださんと議論した結果、逆にシグナルマスク操作を全部削除してしまって全スレッドでシグナルを受けれるようにしたほうが
よいという結論にしました。改訂版パッチを添付します。
rb_syswait() の件が経緯不明のため、このパッチは1.9.4 送りの方向で考えています。

Bug #4765: signal が正しくマスクされておらず main thread でシグナルハンドラが動いている
http://redmine.ruby-lang.org/issues/4765

Author: Motohiro KOSAKI
Status: Assigned
Priority: Normal
Assignee: Motohiro KOSAKI
Category: core
Target version: 1.9.x
ruby -v: ruby 1.9.3dev (2011-05-21 trunk 31654) [x86_64-linux]

Bug#4027 から派生させます

sigaction によるシグナルハンドラの実行はタイマースレッドにまかせているはずなのでこれは不要だと思います。
また例外発生時に rb_trap_restore_mask を読んでいるのも、trap() で例外が発生しても trap_ensure() で
シグナルマスクを戻す処理は行なわれているので不要なような気がします。元々どういう理由で呼ばれているのか
わからなかったので自信ないですけど。

レビューした結果、rb_trap_restore_mask()がまったく不要だという意見に賛成します。

  1. Failure:
    test_status_kill(TestProcess)
    [/Users/nagachika/opt/ruby-trunk/src/ruby-trunk/test/ruby/test_process.rb:1073]:
    [s.exited?, s.signaled?, s.stopped?].
    <[false, true, false]> expected but was
    <[true, false, false]>.

原因は rb_syswait()にあります。現状 Process.wait()中は なぜか SIGHUP, SIGQUIT, SIGINTを
SIG_IGNに設定してしまうため
この間に別スレッドが SIGQUITを送るようなテストは正しく動きません。
そもそも、signal handlerはプロセスグローバルなので、処理の途中で一時的に SIG_IGNにしてはいけません。他のスレッドが
迷惑します。pthread_sigmask()に差し替えることを検討したのですが、そもそもシグナルがタイマースレッドにしか配送されない
という設計を貫く限り、いかなる処理も必要ないという結論になりました。

まつもとさん、この処理の実装はまつもとさんのように見えます。10年以上前なので無理かもしれませんが、覚えていたら
SIG_IGNを設定していた理由を教えてください。

また、sighandler() がメインスレッドから呼ばれるのは Process.spawn を実行するとまだおきてしまいます。
process.c の before_fork/after_fork で fork の時に一時的にタイマースレッドを止めてシグナルマスクを外すためです。
こちらもどうしていいものかわからないです。
fork してそのまま動き続けるようなものはどうしようもないような気がしますが、spawn するものは
fork 後に sigprogmask で外すようにするなどでなんとかならないものでしょうか。
あーでもタイマースレッドの再起動で結局一時的に外してしまいますね……

これも、一時的にシグナルマスクを一時的に外す処理自体が不要だと思います。

また、現状、chikanagaさんの報告よりも、さらに状況は悪化しており、シグナル処理を直すと
test_signal.rb#test_kill_immediately_before_termination でテストがハングします。
これは、くだんのテストが子プロセスにSIGINT送って送出されてくる例外を確認しているのですが、
test/unit/parallel.rb#run() に Signal.trap(:INT,“IGNORE”) という極悪な文があるため、
テストでSIGINTが動かなくなる仕様変更がこっそり行われているためです。

soraさん、この行の意図はなんですか?代替案を考える必要があると思いますが、このような
子プロセスに伝搬する設定をこっそり行うのは許容できないと思います。テストに支障が出るし
第一 Ctrl-C がなかなか効かなくて test-all するときにイライラします。

ついでに以下の変更を行いました
・SIGPIPEのハンドラを空関数からSIG_IGNに変更。空関数にするぐらいならユーザ空間に
処理を戻すだけ無駄。それに、ざっと見たところrubyの中でEPIPEをハンドリングしてない場所はない
・処理の最後で、ruby_default_signal() でシグナルハンドラをSIG_DFLに戻している箇所は
シグナルマスクも解除するよう変更

添付のパッチで test-all が全て通ることを確認出来ています。

これは当面コミットせずに1.9.4に回そうと思っています。理由は
・これを直さないと致命的な状況になるような bug report はきてない
・長年放置されていたせいで、すでに複数箇所いまの挙動に依存するコードが見つかっており
安定化に時間がかかる可能性がある

と考えているため

This forum is not affiliated to the Ruby language, Ruby on Rails framework, nor any Ruby applications discussed here.

| Privacy Policy | Terms of Service | Remote Ruby Jobs