Bug #13768: SIGCHLD and Thread dead-lock problem

• Status: Closed
• Assignee: -
• Target version: -
• Backport: 2.2: UNKNOWN, 2.3: UNKNOWN, 2.4: UNKNOWN
• Tags: core

[keiju (Keiju Ishitsuka)]

けいじゅ＠いしつかです.

下記のプログラムを実行するとThreadがデッドロックしたと例外が発生します.

コメント部分をはずせば, デッドロックはしません

シグナルのtrapが絡むときのデッドロックの検知に問題があるのではないかと
思いますがいかがでしょう?

% ruby -v
ruby 2.5.0dev (2017-07-25 trunk 59417) [i686-linux]

q = Queue.new
p = Queue.new

trap(:SIGCHLD) do
  puts "SIGCHLD"
  q.push 1
end

Thread.start do
  Process.spawn("/bin/sleep 1")
end

#Thread.start do
#  loop do
#    sleep 100
#  end
#end

th = Thread.start{
  p.push q.pop
}

p.pop

__
---------------------------------------------------->> 石塚 圭樹 <<---
---------------------------------->> e-mail: [e-mail] <<---

[nobu (Nobuyoshi Nakada)]

• Description updated (diff)

Signal.trapを設定していたら常にデッドロックではない、とすべきというところでしょうか。
まぁ設定していなくても実際に受けとればデッドロックを抜けはするのですが。

diff --git c/signal.c i/signal.c
index 2e69cf08ac..3b026724f9 100644
--- c/signal.c
+++ i/signal.c
@@ -1235,6 +1235,7 @@ trap(int sig, sighandler_t func, VALUE command)
     sighandler_t oldfunc;
     VALUE oldcmd;
     rb_vm_t *vm = GET_VM();
+    int handle_count = 0;
 
     /*
      * Be careful. ruby_signal() and trap_list[sig].cmd must be changed
@@ -1261,8 +1262,20 @@ trap(int sig, sighandler_t func, VALUE command)
 	break;
       case Qundef:
 	oldcmd = rb_str_new2("EXIT");
+      default:
+	--handle_count;
 	break;
     }
+    switch (command) {
+      case 0: case Qtrue: case Qnil:
+	break;
+      default:
+	++handle_count;
+	break;
+    }
+    if (vm->trap_count + handle_count < 0) rb_fatal("trap_count underflow");
+
+    vm->trap_count += handle_count;
 
     vm->trap_list[sig].cmd = command;
     vm->trap_list[sig].safe = rb_safe_level();
diff --git c/thread.c i/thread.c
index b7ee1d8d9b..fdb9221fd3 100644
--- c/thread.c
+++ i/thread.c
@@ -4947,6 +4947,7 @@ rb_check_deadlock(rb_vm_t *vm)
     if (vm_living_thread_num(vm) > vm->sleeper) return;
     if (vm_living_thread_num(vm) < vm->sleeper) rb_bug("sleeper must not be more than vm_living_thread_num(vm)");
     if (patrol_thread && patrol_thread != GET_THREAD()) return;
+    if (vm->trap_count > 0) return;
 
     list_for_each(&vm->living_threads, th, vmlt_node) {
 	if (th->status != THREAD_STOPPED_FOREVER || RUBY_VM_INTERRUPTED(th)) {
diff --git c/vm_core.h i/vm_core.h
index c23f50690e..a2fa25ad50 100644
--- c/vm_core.h
+++ i/vm_core.h
@@ -550,6 +550,7 @@ typedef struct rb_vm_struct {
 	VALUE cmd;
 	int safe;
     } trap_list[RUBY_NSIG];
+    int trap_count;
 
     /* hook */
     rb_hook_list_t event_hooks;

[ko1 (Koichi Sasada)]

これ、dead-lock の定義の問題なので、ちょっと議論が要るような気がしています。

Signal.trapを設定していたら常にデッドロックではない、とすべきというところでしょうか。

いいんかな、これで。例えば、webrick 使ってたらデッドロック検出出来ないとか。

[keiju (Keiju Ishitsuka)]

けいじゅ＠いしつかです.

[e-mail] wrote:

Issue #13768 has been updated by ko1 (Koichi Sasada).
これ、dead-lock の定義の問題なので、ちょっと議論が要るような気がして
います。

signalで外部イベントをを待つのも, selectとかで外部入力を待っている(た
めにデッドロックしているように見える)のも外部から何らかの通知を待って
いることには変わらないと思いうのですが?

Signal.trapを設定していたら常にデッドロックではない、とすべきというところでしょうか。

いいんかな、これで。例えば、webrick 使ってたらデッドロック検出出来な
いとか。

たしかに, SIGINTを設定しただけでデッドロックが検出できなくなるのも不便
かも知れません.

シグナルに関しては, そのあとの処理内容にもよるので, trap時に何らかの指
定をすれば, デッドロックの検出をしなくなるととか?

あとは, singanl専用のConditionVariable(SigCV)みたいなのを用意するとい
うのもあるかも.

待つ方は SigCVでwaitして, signalハンドラ側でSigCVにsignalする. デッド
ロック検知側はSigCV.waitで待っている場合はデッドロックとしない.

いかがでしょう? 仕様はもっとつめる必要があるとは思いますが?

__
---------------------------------------------------->> 石塚 圭樹 <<---
---------------------------------->> e-mail: [e-mail] <<---

[matz (Yukihiro Matsumoto)]

This issue told us there's false positive in our deadlock detector. So I accept adding Thread.ignore_deadlock = true.

Matz,

• Status changed from Open to Closed

Applied in changeset git|dfb3605bbee9c3cfbc1c354594c367472f29cb35.

---

Add Thread.ignore_deadlock accessor

Setting this to true disables the deadlock detector. It should
only be used in cases where the deadlock could be broken via some
external means, such as via a signal.

Now that $SAFE is no longer used, replace the safe_level_ VM flag
with ignore_deadlock for storing the setting.

Fixes [Bug #13768]

[ioquatix (Samuel Williams)]

@ko1 (Koichi Sasada) we discussed this briefly. I disagree with introducing Thread.ignore_deadlock.

It should be responsibility of signal handler.