Backport #5757

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

Added by Yui NARUSE over 3 years ago. Updated about 3 years ago.

[ruby-dev:44985]
Status:Closed
Priority:Normal
Assignee:Tomoyuki Chikanaga

Description

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() を呼ばないようにするとか

bug5757.patch Magnifier (1.26 KB) Tomoyuki Chikanaga, 12/14/2011 09:45 PM


Related issues

Related to Ruby trunk - Bug #5343: Unexpected blocking behavior when interrupt Socket#accept Closed 09/20/2011

Associated revisions

Revision 34038
Added by Yui NARUSE over 3 years ago

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

Revision 34425
Added by Yui NARUSE about 3 years ago

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] 
  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().

History

#1 Updated by Motohiro KOSAKI over 3 years ago

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

#2 Updated by Tomoyuki Chikanaga over 3 years ago

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;

#3 Updated by Motohiro KOSAKI over 3 years ago

  • 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 nagachika00@gmail.com:

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が呼ばれるようにする
トリックが必要だという認識です。

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

#4 Updated by Yui NARUSE over 3 years ago

  • 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() というのも考えていたんですが、
とりあえず で動いてるっぽかったので、各プラットフォームでの確認の利便性を鑑み、
r34038 にてコミットしてます。

#5 Updated by Koichi Sasada over 3 years ago

  • 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

#6 Updated by Tomoyuki Chikanaga over 3 years ago

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 のチェックを入れればいいのではないかと思います。というわけで追加のパッチを添付します。

#7 Updated by Yui NARUSE over 3 years ago

  • 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] patched by Tomoyuki Chikanaga.

#8 Updated by Tomoyuki Chikanaga over 3 years ago

  • Status changed from Closed to Open

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

#9 Updated by Motohiro KOSAKI over 3 years ago

  • Status changed from Open to Assigned
  • Assignee set to Motohiro KOSAKI

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

#10 Updated by Motohiro KOSAKI over 3 years ago

  • Assignee changed from Motohiro KOSAKI to Tomoyuki Chikanaga

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

#11 Updated by Tomoyuki Chikanaga over 3 years ago

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

#12 Updated by Tomoyuki Chikanaga over 3 years ago

  • Status changed from Assigned to Closed

#13 Updated by Yui NARUSE about 3 years ago

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

#14 Updated by Yui NARUSE about 3 years ago

  • 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] 
  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().

Also available in: Atom PDF