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

Ctrl-C kills nyagos.exe itself when more runs without redirect #342

Closed
hymkor opened this issue Nov 28, 2018 · 14 comments
Closed

Ctrl-C kills nyagos.exe itself when more runs without redirect #342

hymkor opened this issue Nov 28, 2018 · 14 comments
Assignees
Labels

Comments

@hymkor
Copy link
Collaborator

hymkor commented Nov 28, 2018

No description provided.

@hymkor hymkor added the bug label Nov 28, 2018
@hymkor hymkor self-assigned this Nov 28, 2018
@spiegel-im-spiegel
Copy link

日本語ですみません。こちらまだ未解決ですか。

似た案件かどうか分かりませんが、時間がかかる処理を Ctrl+C で強制終了させようとしたら NYAGOS ごと終了してしまうようです。

具体的には Windows Terminal 上の NYAGOS で、拙作の depm の動作確認のため駆動中に意図的に Ctrl+C した時です。

ちなみに depm は内部で go list コマンドを呼んでまして、 SIGINT を受信した際に Go の context.Context を使ってサブプロセスと自分自身を終了する動きになっています。

@hymkor
Copy link
Collaborator Author

hymkor commented Nov 11, 2020

ありがとうございます。他にも

https://twitter.com/tomato3713/status/1323614105702297600

Ctrl-C入力したときに、Nyagosが終了してしまうのをどうにかしたいんだけど、コードのどこを変更したら終了しなくなるのかわからない。

https://twitter.com/tomato3713/status/1325457861955121154

4.4.8_0-windows-amd64 by go1.15.2 を Windows terminal のバージョン: 1.3.2651.0から使用しています。状況としては、コマンドを実行していない状態でCtrl+Cを連続して入力したときと、コマンド実行中にCtrl+Cを入力時に、コード 3221225786で終了する現象が発生しています。

という情報をいただいており、要対応と認識しています。

@tomato3713
Copy link
Contributor

tomato3713 commented Nov 16, 2020

こちらなのですが、scoopを利用してnyagosをインストールしていたので
https://qiita.com/mkizka/items/5d77efb1c801a8d06fa2
を参考にして,Windows Terminalのプロファイルを
"commandline": "%USERPROFILE%\\scoop\\apps\\nyagos\\current\\nyagos.exe", に変更しました.
(以前は,C:\Users\tomato\scoop\shims\nyagos.exeを指定していました.)
その結果,時間のかかる処理をCtrl+Cで強制停止した際にnyagosごと終了する件については少なくとも手元では生じなくなりました.

ただし,コマンドを実行していない状態でCtrl+Cを連続して入力した際には,コード 2で終了する現象が発生するようになりました.

@spiegel-im-spiegel
Copy link

@tomato3713 さんのご指摘、私のほうでも試してみましたが、確かに Ctrl+C で NYAGOS ごと死ぬ現象はなくなりました。

ただ、起動しているプロセスに SIGINT または SIGTERM が届いてないようで、外部(?)から強制的にプロセスが落とされている感じです。この辺は PowerShell から起動した場合と事象が異なっています。

一応、報告まで。

@hymkor
Copy link
Collaborator Author

hymkor commented Dec 2, 2020

せっかくいろいろ情報を寄せていただいているのにレスポンスが遅くなり、申し訳ありません。

~\scoop\shims\nyagos.exe より起動すると Ctrl-C で死にやすいというのは貴重な情報です。ありがとうございます。本体の話ではなくて恐縮ですが、とりいそぎ、自分が書いている scoop / nyagos 関連のドキュメントで ~\scoop\shims\nyagos.exe を使うよう記載しているものは ~\scoop\apps\nyagos\current\nyagos.exe の方を見るよう改訂しました。

調査の方はぼちぼちと開始していますが、まだ確定的な発見はなく、つらつらと zenn.dev のスクラップ に検討課題を綴っているような状況です。

@hymkor
Copy link
Collaborator Author

hymkor commented Dec 14, 2020

今まで、readline とコマンド処理の間で、わずかながらシグナルをキャッチしていない隙間があったので、隙間が発生しないように、キャッチする範囲を広げてみました。

(context.WithCancel によるコマンドを停止させる channel と、signal.Notify による Ctrl-C をキャッチする channel で有効期限が一致しなくなってしまうため、うまい連携のさせ方がなかなか分からず、結構な難産でした)

結果:

  • トップレベル → nyagos → nyagos → more → Ctrl-C 押下 :子プロセス側の nyagos が落ちる(変化なし)
    • 親の nyagos が子の nyagos を killするため、これは仕様上、仕方がない
  • トップレベル → nyagos → more → Ctrl-C :more だけが終了し、nyagosは終了しない(OK)

と件名の問題については改善が見られました。

報告いただいている事象については、軽く試してみて、もしかして直ってる?という感じなのですが、十分な検証時間がとれていないので、もうちょっと試用を続けてみてから、4.4.9_1(予定)として出すかどうか考えます。
(少なくとも改悪にはなってないので、修正をロールバックすることは多分ないとは思いますが…直りきっていない可能性は十分ありえるので)

hymkor added a commit that referenced this issue Dec 21, 2020
@hymkor
Copy link
Collaborator Author

hymkor commented Dec 21, 2020

上で述べた改善を取り込んだバージョンを公開しました。しばらく様子見ですね

@spiegel-im-spiegel
Copy link

以下の検証コードで簡単にチェックしてみました(自作パッケージの検証も兼ねてw)

package main

import (
    "context"
    "fmt"
    "os"
    "time"

    "github.com/spiegel-im-spiegel/gocli/signal"
)

func ticker(ctx context.Context) error {
    t := time.NewTicker(1 * time.Second) // 1 second cycle
    defer t.Stop()

    for {
        select {
        case now := <-t.C: // ticker event
            fmt.Println(now.Format(time.RFC3339))
        case <-ctx.Done(): // cancel event from context
            fmt.Println("Stop ticker")
            return ctx.Err()
        }
    }
}

func main() {
    if err := ticker(signal.Context(context.Background(), os.Interrupt)); err != nil {
        fmt.Fprintf(os.Stderr, "Error Message: %#v\n", err)
        return
    }
}

これを NYAGOS 上で実行すると

$ go run sample.go
2020-12-24T12:22:55+09:00
2020-12-24T12:22:56+09:00
2020-12-24T12:22:57+09:00
2020-12-24T12:22:58+09:00
2020-12-24T12:22:59+09:00
Stop ticker
Error Message: &errors.errorString{s:"context canceled"}
^C
exit status 1

という感じになります。
最後の "exit status 1" を誰がどうやって出してるのかが謎ですが(PowerShell 上で動かした場合には出ません)とりあえず問題はないです。

あと exec.CommandContext() で子プロセスを起動している状態で Ctrl+C を入力すると PowerShell だと実行結果をエラーで返してきますが, NYAGOS だと自プロセスもろとも強制終了します。 NYAGOS 自身には影響ありません。これは仕方ないって感じですかねぇ。

@hymkor
Copy link
Collaborator Author

hymkor commented Dec 24, 2020

検証報告ありがとうございます。

最後の "exit status 1" を誰がどうやって出してるのかが謎ですが(PowerShell 上で動かした場合には出ません)とりあえず問題はないです。

exit status 1 を表示しているのは間違いなく nyagos なのですが(余計なお世話な機能)、エラー値の「1」がどこからきたのか確認しときます(自分で適当に設定したのか、それとも kill されたプロセスの ExitCode は "os/exec" が1にしてしまうのか…)

あと exec.CommandContext() で子プロセスを起動している状態で Ctrl+C を入力すると PowerShell だと実行結果をエラーで返してきますが, NYAGOS だと自プロセスもろとも強制終了します

これは nyagos が子プロセス待機中に context の Cancel を受信すると、子プロセスを kill するように実装しているせいだと思います。この動作は手本にした (*"os/exec".Cmd).Start() あたりのコードを真似たためですが… kill までしなくてもいいかもしれませんねぇ。ターミナルがターミナル中のすべてのプロセスにシグナルを送ってるかもしれないので【要確認】

(なぜ、標準ライブラリをそのまま使わず、真似たコードになったかというと、二重引用符に関する扱いが CMD.EXE と "os/exec".Cmd で違っていて、コマンド引数に二重引用符をありのままに渡すことができないからです。そのまま渡せないと二重引用符の有無で引数の扱いを変える一部のコマンド(findなど)が意図どおり使えなくなってしまうという問題がありまして)

@hymkor
Copy link
Collaborator Author

hymkor commented Dec 25, 2020

kill までしなくてもいいかもしれませんねぇ。ターミナルがターミナル中のすべてのプロセスにシグナルを送ってるかもしれないので【要確認】

kill すべきではないようですね。「kill しないように改造した nyagos」で以下の検証プログラムを走らせてみました。

package main

import (
	"os"
	"os/signal"
	"time"
)

func main() {
	if len(os.Args) >= 2 {
		println("signal off")
		// signal.Ignore(os.Interrupt)
		sigCh := make(chan os.Signal, 100)
		signal.Notify(sigCh, os.Interrupt)
		defer signal.Stop(sigCh)
	}
	println("start")
	time.Sleep(time.Second * 10)
	println("end")
}

プログラムは go build で EXE ファイルを作成して、nyagos の下から実行しました。

↓ テストプログラム \ nyagos→ killしない版 4.4.9_1
signal 有効 止まる 止まる
signal 無視 止まらない 止まる

以下、結果を考察しました(若干、憶測も交じっているので、それは違うというと思われる点ありましたら、ご指摘ください)

  • nyagos がわざわざ kill しなくても、SetConsoleMode で ENABLE_PROCESSED_INPUT が有効であれば、Ctrl-C を押下された場合、コンソールが全プロセスに SIGINT を送ってくるのではないか?
  • 逆に nyagos が kill してしまうと、子プロセス側が signal で Ctrl-C を無効にすることができなくなってしまう
  • nyagos の子プロセスでも vim とかが止まらないのは、おそらく ENABLE_PROCESSED_INPUT を操作しているからではないか?
  • @spiegel-im-spiegel さんのプログラムが nyagos 4.4.9_1 上で Ctrl-C で強制終了させられないのは、nyagos 4.4.9_1 が殺しているのは go run で起動する go.exe の方で、go.exe から呼び出されるテンポラリな実行ファイルまで類が及ばないからではないかと考えられる( nyagos 4.4.9_1 は直接自分が呼び出したプロセスしか殺さず、子々孫々まで根絶やしにはしない)
  • "os/exec".CommandContext がキャンセル時にプロセスを殺しにかかるのは、Ctrl-C を想定したものではなく、タイムアウトとかデッドタイムを想定したもの。だから、Ctrl-C 対応のような拒否権があるようなケースでは、これに倣うべきではない

おそらくは、kill を外すだけで(moreも含め)万事解決しそうです。

(どうも、自分の CommandContext の仕様の解釈の間違いがそもそもの問題だったようですね。たしかに "context" が登場するまで、こんな問題なかったもんなー)

@hymkor
Copy link
Collaborator Author

hymkor commented Dec 27, 2020

にて対応しました。しばらく様子見です。

このコードのカットの仕方だと無駄な goroutine が省けるので負荷は下がるものの、Ctrl-C 連打で os.Interuppt や context.WithCancel の channel のどれかが満タンになってとまったりしないか?とか心配があります。が、今のところ特にそういうハングとかはなく、一応動いているようです。
(kill の部分だけコメントアウトしてもよかったんですが、goroutine がもったいない貧乏性)

  • トップレベル → nyagos → nyagos → more → Ctrl-C 押下 :子プロセス側の nyagos が落ちる(変化なし)

→ 落ちなくなりました。

@spiegel-im-spiegel
Copy link

上のバージョンで Ctrl+C での挙動が PowerShell と同じになりました。ばっちりです!

@hymkor
Copy link
Collaborator Author

hymkor commented Dec 28, 2020

@spiegel-im-spiegel さん・ @tomato3713 さん:

ご確認ありがとうございました。わたくしの思い違いから発生してしまった本件、皆様のおかげで何とか収束できそうです。
(これも2年越しか…今年はそういう issue が多い年でした)

今回は 4.4.9_0→1 の時と違って、別件に致命的な不具合を抱えているというわけでもないので、キリのいい時(例:Go のマイナーバージョンが一つあがった時)にでも、4.4.9_2 としてバイナリを公開したいと思います。

@hymkor
Copy link
Collaborator Author

hymkor commented Jan 8, 2021

Go のマイナーバージョンはしばらくは出そうにないので、年もかわってしまったことですので 4.4.9_2 としてリリースしました。

自分でオープンした issue ですのにで自分でクローズしてしまいますが、何かおかしい動作が見られるようでしたら、本件を再オープンしていただいても、別途新規に issue を立てていただいても、どちらでも結構です。

どうも、ありがとうございました。

@hymkor hymkor closed this as completed Jan 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants