Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

system() を呼んだ際に、終了した全ての子プロセスが回収されてしまう #187

Closed
eagletmt opened this issue Apr 3, 2012 · 32 comments

Comments

@eagletmt
Copy link
Member

eagletmt commented Apr 3, 2012

https://gist.github.com/2290573
この proc.vim を source して call Test() とすると、os.waitpid の時点で ECHILD により例外が投げられます。

これは Vim の system() の実装において、wait によって全ての子プロセスを待っていることが原因だと考えました。
実際、以下のように修正すると上記の例外は発生しなくなりました。修正後のコードはすぐ上の #ifdef _THREAD_SAFE の場合とほぼ同じです。

--- src/os_unix.c.orig  2012-04-03 00:57:55.810703746 +0900
+++ src/os_unix.c   2012-04-03 18:01:35.980770182 +0900
@@ -3750,7 +3750,11 @@
        continue;
    }
 # else
-   wait_pid = wait(status);
+#  ifdef __NeXT__
+   wait_pid = wait4(child, status, 0, (struct rusage *)0);
+#  else
+   wait_pid = waitpid(child, status, 0);
+#  endif
 # endif
    if (wait_pid <= 0
 # ifdef ECHILD

ゾンビを防いだり終了を待つ意味ではユーザ側で ECHILD を無視すればよいわけですが、終了ステータスを取得したい場合もあると思います。

@koron
Copy link
Member

koron commented Apr 3, 2012

_THREAD_SAFEが定義される時とされない時で同じ動作にしたいなら、そこのifdef消しちゃうのがいいんじゃないですか?

どうやら_THREAD_SAFEはBSD系でだけで定義されるみたいですね。ここに関して言えば分ける意味はあまりなさそうです。

@eagletmt
Copy link
Member Author

eagletmt commented Apr 3, 2012

WNOHANG が渡されるかどうかが異なっています。

私も分ける必要は無いと思っていて、通常ここで WNOHANG を渡してループする必要なんて無いと思うんですが、コメントとして

/* Ugly hack: when compiled with Python threads are probably

  • used, in which case wait() sometimes hangs for no obvious
  • reason. Use waitpid() instead and loop (like the GUI). */

とあったので、よくわからないですけど分けたままのほうがいいのかなと考えました。

@koron
Copy link
Member

koron commented Apr 3, 2012

そのコメント、つまりWNOHANGが要る、って言ってますよね。特にPython(のスレッド)絡みで問題になるからそっち使えと。

逆に @eagletmt さんのパッチでWNOHANG使って問題になることはありますかね?

@eagletmt
Copy link
Member Author

eagletmt commented Apr 3, 2012

ポーリングになってしまうくらいで、大きな問題は無いと思います。

では ifdef を消したほうがいいですかね。

--- src/os_unix.c.orig  2012-04-03 00:57:55.810703746 +0900
+++ src/os_unix.c   2012-04-03 20:37:19.322730906 +0900
@@ -3734,7 +3734,6 @@

     while (wait_pid != child)
     {
-# ifdef _THREAD_SAFE
    /* Ugly hack: when compiled with Python threads are probably
     * used, in which case wait() sometimes hangs for no obvious
     * reason.  Use waitpid() instead and loop (like the GUI). */
@@ -3749,9 +3748,6 @@
        mch_delay(10L, TRUE);
        continue;
    }
-# else
-   wait_pid = wait(status);
-# endif
    if (wait_pid <= 0
 # ifdef ECHILD
        && errno == ECHILD

@koron
Copy link
Member

koron commented Apr 3, 2012

修正内容としては筋が通ったものになったように思います。

仮にポーリングを嫌うなら、そもそもwhileループ自体が無効になるように直した方が好ましいですが、それは wait() sometimes hangs for no obvious reason この問題があるから無理っていうところでしょうか。それとも簡単に場合分けできるのかな?

@Shougo
Copy link
Member

Shougo commented Apr 4, 2012

すみません。気づきませんでした……。ということは、バグということになったんですね。

@mattn
Copy link
Member

mattn commented Apr 5, 2012

バグじゃないです。
本来vimは自分の子供を1つだけ起動する機能しか持ってません。なのでwaitでok。そこを型破りしたことでwaitpidじゃないといけなくなった。なのでこれは機能追加です。

@Shougo
Copy link
Member

Shougo commented Apr 5, 2012

ふむふむ。了解しました。

@koron
Copy link
Member

koron commented Apr 5, 2012

一般的なプログラムとして、子供を1つしか持たない場合にその終了を待つのに wait() を使うのはどうなのかなぁ? 子供のIDわかってんだからそいつだけ待てば良いじゃん、というのが素直な感想。

逆に wait() から waitpid() に変えることで孫以下全部待たなくなるわけだけど、これって何か問題をひき起こさないかしら?

@Shougo
Copy link
Member

Shougo commented Apr 5, 2012

孫以下がゾンビになったら、initの配下になるので大丈夫なはずです。
あと、子供が一つしかないなら、waitpid()でも問題がないと思います。
wait()は複数の子供プロセスを一気に終了させるためのシステムコールなので。

@mattn
Copy link
Member

mattn commented Apr 5, 2012

wait()は複数の子供プロセスを一気に終了させるためのシステムコールなので。

いやちがうw
wait()は複数の子プロセス(プロセスグループ)の内、どれかが終了するまで待つシステムコール。
親プロセスで複数回fork()した内、どれかが死ねば待機が終了する。上でも言った通り、いままでは一つのプロセスしか相手にしてなかったからok。まぁ今回もvimが扱うのは一つで且つ、目的のpidになるまでループを回してるのでこのケースだとwait()でもwaitpid()でもok。
まぁ、挙動の違いがあるとすればこれまでwait()で待っていたのがwaitpid()のWNOHANGになった事でGUIのイベントループが回る事になるので、もし画面のロックをかけていない様であれば、そこで別のバグがでる可能性はある。

@Shougo
Copy link
Member

Shougo commented Apr 5, 2012

wait()は複数の子プロセス(プロセスグループ)の内、どれかが終了するまで待つシステムコール。

OH……。認識ミス。調べてみると、確かにそのとおりでした。

まぁ、挙動の違いがあるとすればこれまでwait()で待っていたのがwaitpid()のWNOHANGになった事でGUIのイベントループが回る事になるので、もし画面のロックをかけていない様であれば、そこで別のバグがでる可能性はある。

バグになったら、そこも修正するしかないですね。あとはこの変更により、その修正が必要になることが妥当かどうか、でしょうか。

@koron
Copy link
Member

koron commented Apr 5, 2012

wait()で待っていたのがwaitpid()のWNOHANGになった事でGUIのイベントループが回る事になるので、もし画面のロックをかけていない様であれば、そこで別のバグがでる可能性はある

Thanks. なるほど。Vim側で問題が出る(わずかな?)可能性は把握しました。

ところで次は子プロセス側なんだけど waitpid() 使う時って本質的には例外としてECHILDをトラップしなきゃいけないんじゃないの? だって子プロセス起動してから waitpid() 呼ぶまでの間に終わっちゃう可能性は0ではないでしょ? 仮に終わってたらやっぱしECHILDがでるような気がするんだけど…どうかな?

つまり元々の スクリプト の書き方が本質的に間違っているのでは?という問。まぁ、Vimが勝手に孫の終了補足しちゃうのは変な動作だけど、waitpid() 使うのにそもそもECHILDを想定してないコードはどうなのよ、ということ。

@Shougo
Copy link
Member

Shougo commented Apr 5, 2012

そのスクリプトはもともとvimprocにあったもので、vimproc側で例外を補足している場合と補足していない場合があったのです。今は修正してあります。

@koron
Copy link
Member

koron commented Apr 5, 2012

つまり waitpid() でECHILDを補足しないのは正しくないってことね。だとすればVim側を直す根拠としては弱くなるんじゃないかなぁ。もっと明快に「正しいプログラム書いたのにVimの挙動のせいでちゃんと動かない」っていう例を作ったほうが説得力は増すと思われます。

@mattn
Copy link
Member

mattn commented Apr 5, 2012

ところで次は子プロセス側なんだけど waitpid() 使う時って本質的には例外としてECHILDをトラップしなきゃいけないんじゃないの? だって子プロセス起動してから waitpid() 呼ぶまでの間に終わっちゃう可能性は0ではないでしょ? 仮に終わってたらやっぱしECHILDがでるような気がするんだけど…どうかな?

それはない。一度は取れる。Ⅰプロセスに対して1度waitされたかをカーネルが持ってる。
ちゅうか、でないと終了コードなんて取れない。

@mattn
Copy link
Member

mattn commented Apr 5, 2012

ちなみに同じプロセスIDに対して2度waitpidするとECHILDが出ます。

@koron
Copy link
Member

koron commented Apr 5, 2012

BSDのmanページより

If there are no children not previously awaited, -1 is returned with errno set to ECHILD.

ってところか。二重否定でわかりにくいけどw

となるとコレを根拠に waitpid() 推し行けそう、ってことで良いのかな?

@Shougo
Copy link
Member

Shougo commented Apr 5, 2012

大分話がややこしくなっていますが、そもそも「vimprocで起動したプロセスがECHILDを発生させることがあるのでトラップしておいた」のが正解なのです。それを@eagletmtさんが原因を調べて、「Vimがwait()しているから」というのが明らかになりました。つまり、Vimがwait()をしなければ、vimproc(とvimprocを使ったプラグイン)にバグがない限り、ECHILDは発生しないはずなのです。

@mattn
Copy link
Member

mattn commented Apr 5, 2012

ちょっとGUIロックかけてるか見てみる。

@mattn
Copy link
Member

mattn commented Apr 5, 2012

かけてた。(一瞬だった)

@mattn
Copy link
Member

mattn commented Apr 5, 2012

あ、嘘だ。

@mattn
Copy link
Member

mattn commented Apr 5, 2012

あー、すんません。wait()してるのメインループ側だったw
ってことでwaitpid()でも弊害無しです。

@mattn
Copy link
Member

mattn commented Apr 5, 2012

あとは理由付けと、こう変える事でも弊害が無いんだよっていう説明がいる。

@koron
Copy link
Member

koron commented Apr 5, 2012

w

理由としては「waitpidに変えないと、if_*側で作ったプロセスに対するwaitpidが補足できないケースができちゃうよ」というあたりか。できれば「終了コードが取れなくなっちゃうYO!」とか言えるともっと良さそう。

弊害が無いことはここで検証したプロセスを書き下すだけかな。

@mattn
Copy link
Member

mattn commented Apr 17, 2012

この件、今は誰がボール握ってるんでしょう

@koron
Copy link
Member

koron commented Apr 17, 2012

@eagletmt さんのパッチで私はOKだと思いますが、状況を伝えるのにちょっとだけ苦労するかも、くらい。

@mattn
Copy link
Member

mattn commented Apr 17, 2012

下手な英語で良ければ私がやります。要点だけ。

  • 言語拡張でプロセスを起動したい
  • 現状プロセスIDの指定無しにwait()しているのでSHGCHLDが発生する
  • それwaitpidで出来るよ

でok?

@koron
Copy link
Member

koron commented Apr 17, 2012

OK

@ghost ghost assigned mattn Apr 17, 2012
@mattn
Copy link
Member

mattn commented Apr 18, 2012

@Shougo
Copy link
Member

Shougo commented Apr 19, 2012

mattn++;

@h-east
Copy link
Member

h-east commented Apr 23, 2012

Patch 7.3.499 で入りました。イェイ!
https://groups.google.com/d/topic/vim_dev/M3EE1OaiOd4/discussion

@h-east h-east closed this as completed Apr 23, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants