Bug #5368: ensure節でsleepするようなThreadがあるとインタプリタが終了しない

• Status: Assigned
• Priority: Normal
• Assignee: kosaki (Motohiro KOSAKI)
• Target version: 2.2.0
• ruby -v: -

[Glass_saga (Masaki Matsushita)]

=begin
次のコードを実行するとCPU使用率が跳ね上がった状態になりインタプリタが終了しません。

Thread.new do
begin
sleep
ensure
sleep
end
end

現在のrb_thread_terminate_allでは最初に1回だけ生きているスレッドに対してterminate_iを実行していますが、ensure節でsleepするようなThreadがあると、そのThreadは寝たままになってしまいwhile(!rb_thread_alone())が無限ループになってしまいます。

while(!rb_thread_alone())の毎回のループでカレントスレッドがメインスレッドであった場合に、生きているスレッドに対してterminate_iを実行するようなpatchを書いたところ、このバグは再現しなくなりました。
patchを添付します。patchの適用後もtest-allをパスします。
=end

[ko1 (Koichi Sasada)]

• ruby -v changed from ruby 1.9.4dev (2011-09-26 trunk 33338) [x86_64-linux] to -

(2011/09/25 21:51), Masaki Matsushita wrote:

次のコードを実行するとCPU使用率が跳ね上がった状態になりインタプリタが終了しません。

Thread.new do
begin
sleep
ensure
sleep
end
end

現在のrb_thread_terminate_allでは最初に1回だけ生きているスレッドに対してterminate_iを実行していますが、ensure節でsleepするようなThreadがあると、そのThreadは寝たままになってしまいwhile(!rb_thread_alone())が無限ループになってしまいます。

　ここで，バグとは何でしょうか．

(1) CPU 使用率がはねあがる
(2) プロセスが死なない

　(1) が問題というのは理解できます．あまり文句がないんで放置している部分
です．直そうと思えば，正しく他のスレッドを待ち合わせをすればできると思っ
ています．

　(2) については仕様です．ご提案の修正方法では，とにかく生きていたら殺
す，ということをしていますが，例えば他のスレッドで何か時間のかかる終了処
理をしていた場合（例えば，終了時にネットワークで外部に情報を送る，という
ようなことをしている場合），それを強制的に止めてしまうため，まずいことに
なります．

　ということで，これに関しては，現状だと仕様かな，と思いますが，もっと良
い仕様があれば，ご提案頂けと助かります．

--
// SASADA Koichi at atdot dot net

高尾と申します。

　(2) については仕様です．ご提案の修正方法では，とにかく生きていたら殺
す，ということをしていますが，例えば他のスレッドで何か時間のかかる終了処
理をしていた場合（例えば，終了時にネットワークで外部に情報を送る，という
ようなことをしている場合），それを強制的に止めてしまうため，まずいことに
なります．

これは「ensure節の実行中に止めるのはNG」ということですよね?
いまの実装 (rev.33339) では、ensure節の実行中であっても、terminateされたら
(外側にさらなるensure節がなければ) ふつうに終了するので、辻褄が合わない気がします。

[ko1 (Koichi Sasada)]

(2011/09/26 7:03), m_takao wrote:

これは「ensure節の実行中に止めるのはNG」ということですよね?
いまの実装 (rev.33339) では、ensure節の実行中であっても、terminateされたら
(外側にさらなるensure節がなければ) ふつうに終了するので、辻褄が合わない気がします。

　ご指摘の通り，「最初の1回だけは必ず ensure 中でも強制的に止めてしま
う」という仕様になっていますね．現状では，ensure 中（後処理中）ではない
ことを期待しています．ファイナライザ実行中でも，強制的にキャンセルされる
ので，何か後処理をしている最中に止められるのは不可避です．

# 今まで文句が無かったというのは，確率的にこういうケースが起きづらい，
# ということでしょうかね．

　真面目にやるなら，すでに何度か提案がある，こういう割り込みを制御するた
めの仕組みを統一的に整備するってことになると思います．うーん，せめてファ
イナライザの処理は main thread のみで行うようにしたほうがいいんだろうか．

　もしくは，こういう処理を行うことがあるのなら，ちゃんと join してから死
にましょう，と開き直って，最初のご提案の通り，確実に殺す，というふうに振
るのかなぁ．それはそれで不便そうに思うのだけど．

　結論も出てませんが，現状はなんとなくうまく動くことが多い，改善するには
しんどそう，ということで，なんとなく現状の仕様になっている，ということか
と思います．

　もしうまい手があれば教えてください．

--
// SASADA Koichi at atdot dot net

[nobu (Nobuyoshi Nakada)]

=begin
なかだです。

At Mon, 26 Sep 2011 23:52:19 +0900,
SASADA Koichi wrote in :

(2011/09/26 7:03), m_takao wrote:

これは「ensure節の実行中に止めるのはNG」ということですよね?
いまの実装 (rev.33339) では、ensure節の実行中であっても、terminateされたら
(外側にさらなるensure節がなければ) ふつうに終了するので、辻褄が合わない気がします。

　ご指摘の通り，「最初の1回だけは必ず ensure 中でも強制的に止めてしま
う」という仕様になっていますね．現状では，ensure 中（後処理中）ではない
ことを期待しています．ファイナライザ実行中でも，強制的にキャンセルされる
ので，何か後処理をしている最中に止められるのは不可避です．

ensureでsleepされたら止まるのは仕方がないにせよ、そこで割り込みを受けた
らやはり抜けるほうがいいのではないでしょうか。

diff --git i/thread.c w/thread.c
index d9d497a..0e3d096 100644
--- i/thread.c
+++ w/thread.c
@@ -333,6 +333,7 @@ typedef struct rb_mutex_struct

static void rb_mutex_abandon_all(rb_mutex_t mutexes);
static const char rb_mutex_unlock_th(rb_mutex_t *mutex, rb_thread_t volatile *th);
+static int vm_living_thread_num(rb_vm_t *vm);

void
rb_threadptr_unlock_all_locking_mutexes(rb_thread_t *th)
@@ -369,14 +370,20 @@ rb_thread_terminate_all(void)
st_foreach(vm->living_threads, terminate_i, (st_data_t)th);
vm->inhibit_thread_creation = 1;

• while (!rb_thread_alone()) { +
• if (th->vm->living_threads && vm_living_thread_num(th->vm) > 1) {
• int state;
• const double sleep_time = 3600.0; /* should not be too long to
• * get rid of overflow */ PUSH_TAG();
• if (EXEC_TAG() == 0) {
• rb_thread_schedule();
• }
• else {
• /* ignore exception */
• state = EXEC_TAG();
• while (state == 0 || !rb_thread_alone()) {
• if (state) {
• st_foreach(vm->living_threads, terminate_i, (st_data_t)th);
• }
• sleep_wait_for_interrupt(th, sleep_time); }

• /* ignore exception */
  POP_TAG();
  }
  }
  @@ -445,6 +452,8 @@ thread_start_func_2(rb_thread_t *th, VALUE *stack_start, VALUE *register_stack_s

  gvl_acquire(th->vm, th);
  {

• int terminating = 0;
  +
  thread_debug("thread start (get lock): %p\n", (void *)th);
  rb_thread_set_current(th);

@@ -469,6 +478,7 @@ thread_start_func_2(rb_thread_t th, VALUE *stack_start, VALUE *register_stack_s
if (NIL_P(errinfo)) errinfo = rb_errinfo();
if (state == TAG_FATAL) {
/ fatal error within this thread, need to stop whole script /
+ terminating = (errinfo == eTerminateSignal);
}
else if (rb_obj_is_kind_of(errinfo, rb_eSystemExit)) {
if (th->safe_level >= 4) {
@@ -509,13 +519,23 @@ thread_start_func_2(rb_thread_t *th, VALUE *stack_start, VALUE *register_stack_s
/ delete self other than main thread from living_threads */
if (th != main_th) {
st_delete_wrap(th->vm->living_threads, th->self);
+ if (terminating) {
+ if (vm_living_thread_num(th->vm) == 1) {
+ rb_threadptr_interrupt(main_th);
+ }
+ else {
+ terminating = 0;
+ }
+ }
}

/* wake up joining threads */
join_th = th->join_list_head;
while (join_th) {

• if (join_th == main_th) errinfo = Qnil;
• rb_threadptr_interrupt(join_th);
• if ((join_th != main_th) || (errinfo = Qnil, !terminating)) {
• /* main thread is already interrupted when terminating */
• rb_threadptr_interrupt(join_th);
• } switch (join_th->status) { case THREAD_STOPPED: case THREAD_STOPPED_FOREVER: join_th->status = THREAD_RUNNABLE; 

--
--- 僕の前にBugはない。
--- 僕の後ろにBugはできる。
中田 伸悦
=end

[kosaki (Motohiro KOSAKI)]

2011年9月29日20:20 Nobuyoshi Nakada [e-mail]:

なかだです。

At Mon, 26 Sep 2011 23:52:19 +0900,
SASADA Koichi wrote in :

(2011/09/26 7:03), m_takao wrote:

これは「ensure節の実行中に止めるのはNG」ということですよね?
いまの実装 (rev.33339) では、ensure節の実行中であっても、terminateされたら
(外側にさらなるensure節がなければ) ふつうに終了するので、辻褄が合わない気がします。

　 ご指摘の通り，「最初の1回だけは必ず ensure 中でも強制的に止めてしま
う」という仕様になっていますね．現状では，ensure 中（後処理中）ではない
ことを期待しています．ファイナライザ実行中でも，強制的にキャンセルされる
ので，何か後処理をしている最中に止められるのは不可避です．

ensureでsleepされたら止まるのは仕方がないにせよ、そこで割り込みを受けた
らやはり抜けるほうがいいのではないでしょうか。

現在のensure中の非同期例外の扱いは仕様バグであるという認識なので、こういう
ワークアラウンドにはあまりいい印象を持ちません。
実用的にも問題があると思っていて、C-c を日常的に２−３回連打する癖のあるひとは
いるので、そういう人からするとプロセス終了処理がちゃんと行われなくなるregression
に見えるのではないでしょうか。

[shyouhei (Shyouhei Urabe)]

提案されている workaround はいまいち、という点には同意します。

ただ、じゃあどうなるべきなの? というところのコンセンサスは足りていないでしょう。

というわけで結論を急がずに少し議論したほうがいいんじゃないでしょうか。

[ko1 (Koichi Sasada)]

• Assignee set to ko1 (Koichi Sasada)

今日，非同期例外の話が出たので，それと交えて考えます．
多分．

[shyouhei (Shyouhei Urabe)]

• Status changed from Open to Assigned

[ko1 (Koichi Sasada)]

• Assignee changed from ko1 (Koichi Sasada) to kosaki (Motohiro KOSAKI)
• Priority changed from Normal to 5

ticket の詳細が思い出せないので小崎先生に振ってみます．
control_interrupt じゃ解決しないんだよな，多分．

[kosaki (Motohiro KOSAKI)]

読み返しました。[Feature #1952] とちょっと似た話で終了途中で例外食われてしまったら、という話のようですね。論点をまとめると

• ensure節の中でsleep等、無限待ちが記述されているとハングしてしまう。これはバグではないか (Glass_saga)
• この場合止まっているのはサブスレッドなので、Ctrl-cは意味が無い。メインスレッドが食って無視してしまう
• しかし、タイムアウト等をつくると終了処理に時間のかかるスクリプトが壊れてしまう (ko1)
• ensure節実行中にterminateされたらensure抜けてしまうので言語仕様としてつじつまがあってない (高尾さん）
• sleepで寝ていても、もう一度例外が上がってきたら抜けるべきでは？ （← これよくわからないが Ctrl-Cうけたらメインスレッドがthread terminateを再送しろということ？）
• Ctrl-c 二連打で、終了処理がスッポ抜けるのって本当にうれしい？（こさき）
• 現在、main threadがsub threadの終了を待つロジックがビジーループなのでCPU100%になってしまう。スレッドが終了するまでちゃんと寝るべきではないのか （やや脱線）

ぐらいですかね。

[kosaki (Motohiro KOSAKI)]

なお、#1952 ですでに指摘されているように Ctrl-c が押された時にサブスレッドを待たずに終了してしまうという案はSEGVを引き起こすのでNG。

[kosaki (Motohiro KOSAKI)]

• Status changed from Assigned to Closed
• % Done changed from 0 to 100

This issue was solved with changeset r37865.
Masaki, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.

---

• thread.c (rb_thread_terminate_all): use native_sleep() instead of rb_thread_schedule(). Otherwise, it consume 100% cpu meaninglessly. [Bug #5368]
• thread.c (thread_start_func_2): last sub-thread wakes up main thread.

[kosaki (Motohiro KOSAKI)]

• Status changed from Closed to Assigned
• Assignee changed from kosaki (Motohiro KOSAKI) to matz (Yukihiro Matsumoto)
• Priority changed from 5 to Normal

現状、ささださんが #1にてCPU使用率が跳ね上がるのだけがバグで、終わらない、かつCtrl-Cも効かなくなるのは仕様という見解を出しているのでそれにそって、r37865でCPU使用率問題を直しました。
さて、このまま閉じてしまっていいのかまったく分からないので、まつもとさん意見をください。

元の起票バグだとensureで無限sleepというちょっとありえなさそうなスクリプトですが、IO.read などでも同様の「終了しない＋Ctrl-C効かない」が起こります。これはCtrl-Cが絶対メインスレッドに飛ぶのでサブスレッドが起きれないから。それでも構わないということであれば closeしてください。

だめだと思っている場合は、どのような動作がいいと思っているか教えてください

[Feature #1952] の #12でmameさんが選択肢をいくつか列挙してくれていて

どうしても直したいならば、

• サブスレッドの終了待ち状態で SIGINT を受け取ったら、
  eTerminateSignal を再送する

  • しつこく Ctrl+C を押していればいつか終了できる、かも

• eTerminateSignal を捕捉できない例外とする

  • サブスレッドの ensure が実行されない

• eTerminateSignal を投げて数秒しても終わってくれない場合、
  捕捉できない例外を投げる

  • サブスレッドの ensure が実行されない危険が緩和されるが 本質的に解決はしない。あとダサい

くらいを思いつきましたが、どれも問題がある or 面倒ですね。

とかいうコメントがついています。[Feature #1952]全体を一度読みなおしてからコメントいただけるとなおありがたい

[kosaki (Motohiro KOSAKI)]

1.8 だと、Ctrl-C でsleepを抜けてくれるようです。うーん、この挙動のほうがいいんかなあ。1.8がどういう理屈でこう動いているのかよくわからないんだけど、ようするにInterruptがmain threadではなく、たまたまその時動いていたThreadに飛ぶようなシロモノだったということだろうか（推測）

[kosaki (Motohiro KOSAKI)]

• % Done changed from 100 to 50

[tarui (Masaya Tarui)]

1.8の挙動の方がバグっぽいですけれどねぇ
Ctrl-Cでensureの実行が保証されず、終わってしまうのは大きな問題だと思います。
現にTimeoutでensureが飛ばされるので困ってcontrol_interruptをなどを導入しようと
しているのだと理解しています。kill -KILLしないと死なないプロセスは良くある事で、
これはensureを必ず実行しようとする事とのトレードオフだと思います。

デフォルトでの挙動ではCtrl-CでIntrrupt例外を再送するようにして、
ensure節中ではIntrrupt例外に対して:on_blockingにしておいてやると、
ユーザーが望めば:neverにする事が出来て実行が保証でき、かつ普通にensureを書いてる分には
Ctrl-C連打で上記sleepやio待ちもキャンセル出来て良いんじゃないでしょうか?
(:immediateだと設定を変えようとしてる間に例外が飛んで来て飛ばされる可能性があるので)

[kosaki (Motohiro KOSAKI)]

[Feature #1952]の対策として、r37875でCtrl-Cが押されるたびに eTerminateSignalを最送出するようにしたので、現状1.8と同等の動きをするようになってます。

[kosaki (Motohiro KOSAKI)]

1.8と同等と書いたのは

Thread.new do
begin
sleep
ensure
sleep
end
end

というプログラムが一度ensure節のsleepで寝てしまうが、Ctrl-Cにより終わることは出来る。という挙動を指しており、
このスクリプト以外の細かい挙動について、不整合はあると思います。

[kosaki (Motohiro KOSAKI)]

なお、#1952 ですでに指摘されているように Ctrl-c が押された時にサブスレッドを待たずに終了してしまうという案はSEGVを引き起こすのでNG。

もうちょっとまじめに書くと、SEGVを引き起こすのは終了処理でまじめに１つ１つリソース解放処理をしてるからで、いきなりexit()するという選択肢はあるだろう。しかしそれは当然MVMにしたときに問題を引き起こす。
SEGVしないように各所にNULLチェックを入れるという案については、pthread_cond_destruct()が別スレッドがリソース使用中に呼び出されるとEBUSYで失敗するためうまくいかない

[ko1 (Koichi Sasada)]

• Assignee changed from matz (Yukihiro Matsumoto) to kosaki (Motohiro KOSAKI)
• Target version changed from 2.0.0 to 2.1.0

2.1 で結論を付けたいところ。
多分、まつもとさんはこの辺気にしないと思うので、小崎さん、たるいさん（と私かなぁ）で決めると良いと思います。

[hsbt (Hiroshi SHIBATA)]

• Target version changed from 2.1.0 to 2.2.0