Backport #5757: main threadがreadやselectで待っていると、^C でなかなか死なない

• Status: Closed
• Assignee: nagachika (Tomoyuki Chikanaga)

[naruse (Yui NARUSE)]

FreeBSD 9 にて、 ./ruby と起動して ^C を投げてもなかなか死にません。
./miniruby でも -e'$stdin.read' でも同じです。

仕組みとしては、main thread が read や select で待つ場合、最近は blocking region で
unblock.func に ubf_select を設定するわけですが、この時にシグナルが来ると、

1. どこかのスレッドの sighandler が呼ばれて、rb_thread_wakeup_timer_thread() が呼ばれる
2. タイマースレッドが起きて、thread_timer() -> timer_thread_function() -> rb_threadptr_check_signal() -> rb_threadptr_interrupt() -> (th->unblock.func)(th->unblock.arg) -> ubf_select() -> rb_thread_wakeup_timer_thread() が呼ばれる
3. タイマースレッドが起きて、thread_timer() -> timer_thread_function() -> rb_threadptr_check_signal() -> rb_threadptr_interrupt() -> (th->unblock.func)(th->unblock.arg) -> ubf_select() -> rb_thread_wakeup_timer_thread() が呼ばれる
4. タイマースレッドが起きて、thread_timer() -> timer_thread_function() -> rb_threadptr_check_signal() -> rb_threadptr_interrupt() -> (th->unblock.func)(th->unblock.arg) -> ubf_select() -> rb_thread_wakeup_timer_thread() が呼ばれる
   ...

対策はいくつかあり得ると思うのですが、例えば、ubf_select() から rb_thread_wakeup_timer_thread() を呼ばないようにするとか

[kosaki (Motohiro KOSAKI)]

[Bug #5343] (r33307) でのregressionですね(193ブランチだと r33310)。単純にそこを削除すると Bug #5343が復活してしまうので、タイマー貼る必要があるんじゃないですかね。タイマースレッドを起こすためのタイマーって泥縄感が半端ないけど。

[nagachika (Tomoyuki Chikanaga)]

Bug #5343 を登録したものです。
ちゃんと試さずに書いてしまいますが、 #5343 は別の Thread からの kill や raise での割り込みの場合の問題だったと記憶していますので、ubf_select() での rb_thread_wakeup_timer_thread() の呼び出しはタイマースレッドで実行する場合は呼ばないようにすることで両方大丈夫にならないでしょうか。

diff --git a/thread_pthread.c b/thread_pthread.c
index afef326..16f7674 100644
--- a/thread_pthread.c
+++ b/thread_pthread.c
@@ -924,6 +924,8 @@ native_sleep(rb_thread_t *th, struct timeval *timeout_tv)
thread_debug("native_sleep done\n");
}

+static pthread_t timer_thread_id;
+
#ifdef USE_SIGNAL_THREAD_LIST
struct signal_thread_list {
rb_thread_t *th;
@@ -1018,7 +1020,8 @@ ubf_select(void *ptr)
{
rb_thread_t *th = (rb_thread_t *)ptr;
add_signal_thread_list(th);

• rb_thread_wakeup_timer_thread(); /* activate timer thread */

• if ( pthread_self() != timer_thread_id )

•   rb_thread_wakeup_timer_thread(); /* activate timer thread */
  

  ubf_select_each(th);
  }

@@ -1047,7 +1050,6 @@ ping_signal_thread_list(void) {
static int ping_signal_thread_list(void) { return 0; }
#endif /* USE_SIGNAL_THREAD_LIST */

-static pthread_t timer_thread_id;
static int timer_thread_pipe[2] = {-1, -1};
static int timer_thread_pipe_owner_process;

[kosaki (Motohiro KOSAKI)]

• ruby -v changed from ruby 2.0.0dev (2011-12-12 trunk 34015) [x86_64-freebsd9.0] to -

2011年12月13日19:45 Tomoyuki Chikanaga [e-mail]:

Issue #5757 has been updated by Tomoyuki Chikanaga.

Bug #5343 を登録したものです。
ちゃんと試さずに書いてしまいますが、 #5343 は別の Thread からの kill や raise での割り込みの場合の問題だったと記憶していますので、ubf_select() での rb_thread_wakeup_timer_thread() の呼び出しはタイマースレッドで実行する場合は呼ばないようにすることで両方大丈夫にならないでしょうか。

いや、スレッドがGVL競合してなくて、ubf_selectを登録したけどまだselectを読んでないとき
ubf_selectのkill(VTALRM) はスカるので、もう一回ubf_select_eachが呼ばれるようにする
トリックが必要だという認識です。

もうコードがうろ覚えなのでなにか見落としてる可能性もありますが。

[naruse (Yui NARUSE)]

• ruby -v changed from - to ruby 2.0.0dev (2011-12-12 trunk 34015) [x86_64-freebsd9.0]

レビューありがとうございます。

いや、スレッドがGVL競合してなくて、ubf_selectを登録したけどまだselectを読んでないとき
ubf_selectのkill(VTALRM) はスカるので、もう一回ubf_select_eachが呼ばれるようにする
トリックが必要だという認識です。

そのもう一回も外しちゃう可能性って無いんでしょうか。
どちらかというと、rb_thread_kill 側で確実に止めを刺す用にするべきな気もします。

rb_thread_kill で rb_thread_wakeup_timer_thread() というのも考えていたんですが、
とりあえず [ruby-dev:44992] で動いてるっぽかったので、各プラットフォームでの確認の利便性を鑑み、
r34038 にてコミットしてます。

[ko1 (Koichi Sasada)]

• ruby -v changed from ruby 2.0.0dev (2011-12-12 trunk 34015) [x86_64-freebsd9.0] to -

(2011/12/14 15:00), Yui NARUSE wrote:

そのもう一回も外しちゃう可能性って無いんでしょうか。
どちらかというと、rb_thread_kill 側で確実に止めを刺す用にするべきな気もします。

　もう一回外す可能性があるため，必要が無くなるまで何度もやります．

--
// SASADA Koichi at atdot dot net

[nagachika (Tomoyuki Chikanaga)]

• File bug5757.patch bug5757.patch added

Motohiro KOSAKI wrote:

いや、スレッドがGVL競合してなくて、ubf_selectを登録したけどまだselectを読んでないとき
ubf_selectのkill(VTALRM) はスカるので、もう一回ubf_select_eachが呼ばれるようにする
トリックが必要だという認識です。
少しゆっくり考えてみましたところ、ubf_select() を呼ぶのがタイマースレッドでなければこれまで通り rb_thread_wakeup_timer_thread() を呼ぶので、 signal_thread_list に入れたものをタイマースレッドが確実に blocking region を抜けるまで面倒をみてくれる(定期的にSIGVTALRM送信)ので安泰です。
しかしタイマースレッドがシグナルを処理させるために rb_threadptr_check_signal() からメインスレッドに割り込みする時の ubf_select() で rb_thread_wakeup_timer_thread() を呼ばなくなるため、シグナル受信処理時に SIGVTALRM の喪失してメインスレッドが止まったままになる可能性があるのではないかと思います。

thread_timer() の ping_signal_thread_list() で signal_thread_list をチェックした(そして空だった)後に timer_thread_function() で新たに signal_thread_list にメインスレッドが追加された場合が問題なわけなので、この後に signal_thread_list のチェックを入れればいいのではないかと思います。というわけで追加のパッチを添付します。

[naruse (Yui NARUSE)]

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

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

---

• thread_pthread.c (ubf_select): call rb_thread_wakeup_timer_thread()
  only when it is not timer_thread. [Bug #5757] [ruby-dev:44985]
  patched by Tomoyuki Chikanaga.

[nagachika (Tomoyuki Chikanaga)]

• Status changed from Closed to Open

[ruby-dev:44999] に書いたように今度はシグナル処理の漏れの懸念があるので再 open させて頂きます。

[kosaki (Motohiro KOSAKI)]

• Status changed from Open to Assigned
• Assignee set to kosaki (Motohiro KOSAKI)

すいません。なるべく早く時間取ってレビューします

[kosaki (Motohiro KOSAKI)]

• Assignee changed from kosaki (Motohiro KOSAKI) to nagachika (Tomoyuki Chikanaga)

しばらく考えた結果、[ruby-dev:44999] のパッチで問題ないという結論に至りました。コミットお願い出来ますでしょうか

[nagachika (Tomoyuki Chikanaga)]

確認ありがとうございます。 r34099 でコミットしました。

[nagachika (Tomoyuki Chikanaga)]

• Status changed from Assigned to Closed

[naruse (Yui NARUSE)]

• Tracker changed from Bug to Backport
• Project changed from Ruby to 13
• Status changed from Closed to Open

[naruse (Yui NARUSE)]

• Status changed from Open to Closed

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

---

merge revision(s) r34038,34099:

* thread_pthread.c (ubf_select): call rb_thread_wakeup_timer_thread()
  only when it is not timer_thread. [Bug #5757] [ruby-dev:44985]
  patched by Tomoyuki Chikanaga.

* thread_pthread.c (ping_signal_thread_list): remove return value.

* thread_pthread.c (check_signal_thread_list): add a new function to
  check if signal thread list is empty.

* thread_pthread.c (thread_timer): check signal thread list after
  timer_thread_function(). main thread might be added into signal thread
  list during timer_thread_function().