FIX: node-tree-killのエラー処理のミスを修正

[sabonerune]

内容

node-tree-killのエラー処理が間違っていると思います。
現在、エンジンのキルはtry-chatchを使用しています。

voicevox/src/backend/electron/manager/engineManager.ts

Lines 437 to 442 in 0fb559d

	try {
	engineProcess.pid != undefined && treeKill(engineProcess.pid);
	} catch (error: unknown) {
	log.error(`ENGINE ${engineId}: Error during killing process`);
	reject(error);
	}

しかしnode-tree-killのWindowsでの処理はchild_process.exec()でtaskkillコマンドを呼び出しているためchild_process.exec()には成功したがtaskkillのコマンドに失敗した場合resolveもrejectも呼ばれないため処理が停止してしまいます。
https://github.com/pkrumins/node-tree-kill/blob/cb478381547107f5c53362668533f634beff7e6e/index.js#L29

その他

node-tree-killで処理が停止することはなくなりますが何かしらの理由でnode-tree-killでエンジンのキルが不可能になっている場合無限ループに陥るリスクが残っています。

[sabonerune]

エンジンが起動した場合pidがundefinedになることはありえませんが万が一そうなっていたら既にキルされているとみなすことにします。
engineProcess.pid != undefined && treeKill(engineProcess.pid);で処理するとその万が一が起きたとき処理が止まります。
また、例外を投げると結局ここに戻ってきて無限ループになるはずです。

[Hiroshiba]

なるほどです！！

エラーメッセージの方をもうちょっと分かりやすくさせてください！
ちょっと別で提案してみます！

[Hiroshiba]

LGTM！！

コメントの部分だけ適用してマージさせていただきます！！

ちなみにこれってちゃんと理解しておきたいのですが、エラーハンドリングは本来コールバックじゃないとダメなのにtry catchしていた、というところが問題認識であってそうでしょうか？

この辺りの終了シーケンスはかなり微妙な部分が残っていると思うので助かります。。
結構抜本的な変更も必要だと感じています。ちょっと大掛かりになってしまうかもですが、もしご興味あったらissueなどで問題指摘していただけると助かります 🙇
（もし可能でしたら、「こうしたらいいかも」と終了シーケンスを考えてくださるとめちゃくちゃ嬉しいです！！）

何かしらの理由でnode-tree-killでエンジンのキルが不可能になっている場合無限ループに陥るリスクが残っています。

これってtreeKillが、プロセスのキルに失敗しているのに正常終了した場合でしょうか？
これを考慮するのであれば、例えば5秒間ぐらいプロセスがキルされたかどうかを確認して、されてなかったらrejectするみたいな処理を書けばもしかしたら大丈夫かも･･･？

[sabonerune]

@Hiroshiba

ちなみにこれってちゃんと理解しておきたいのですが、エラーハンドリングは本来コールバックじゃないとダメなのにtry catchしていた、というところが問題認識であってそうでしょうか？

その通りです。
Windows環境でTree-killに失敗した場合例外が発生しないためrejectが呼ばれず処理が止まります。

これってtreeKillが、プロセスのキルに失敗しているのに正常終了した場合でしょうか？

自分が不安視しているのはtree-killに失敗してエンジンが終了しなかった場合もう一度tree-killを実行しても成功する可能性は高くないのではという点です。
現在のエディタ終了のシーケンスはエンジンがキルされているか確認してキルされていなければエディタの終了をキャンセルしてエンジンの終了を行います。
そしてエンジンのキルに失敗しても一度エディタを終了するため、エラーが起きてもエンジンプロセスをキルできたとみなす とあります。

voicevox/src/backend/electron/main.ts

Lines 1023 to 1065 in 595d108

	app.on("before-quit", async (event) => {
	if (!appState.willQuit) {
	event.preventDefault();
	ipcMainSend(win, "CHECK_EDITED_AND_NOT_SAVE", { closeOrReload: "close" });
	return;
	}
	log.info("Checking ENGINE status before app quit");
	const engineCleanupResult = cleanupEngines();
	const configSavedResult = configManager.ensureSaved();
	// - エンジンの停止
	// - エンジン終了後処理
	// - 設定ファイルの保存
	// が完了している
	if (
	engineCleanupResult === "alreadyCompleted" &&
	configSavedResult === "alreadySaved"
	) {
	log.info("Post engine kill process and config save done. Quitting app");
	return;
	}
	// すべてのエンジンプロセスのキルを開始
	// 同期的にbefore-quitイベントをキャンセル
	log.info("Interrupt app quit");
	event.preventDefault();
	if (engineCleanupResult !== "alreadyCompleted") {
	log.info("Waiting for post engine kill process");
	await engineCleanupResult;
	}
	if (configSavedResult !== "alreadySaved") {
	log.info("Waiting for config save");
	await configSavedResult;
	}
	// アプリケーションの終了を再試行する
	log.info("Attempting to quit app again");
	app.quit();
	return;
	});

voicevox/src/backend/electron/main.ts

Lines 513 to 550 in 595d108

	function cleanupEngines(): Promise<void> | "alreadyCompleted" {
	const killingProcessPromises = engineManager.killEngineAll();
	const numLivingEngineProcess = Object.entries(killingProcessPromises).length;
	// 前処理が完了している場合
	if (numLivingEngineProcess === 0 && !vvppManager.hasMarkedEngineDirs()) {
	return "alreadyCompleted";
	}
	let numEngineProcessKilled = 0;
	// 非同期的にすべてのエンジンプロセスをキル
	const waitingKilledPromises: Promise<void>[] = Object.entries(
	killingProcessPromises,
	).map(([engineId, promise]) => {
	return promise
	.catch((error) => {
	// TODO: 各エンジンプロセスキルの失敗をUIに通知する
	log.error(`ENGINE ${engineId}: Error during killing process: ${error}`);
	// エディタを終了するため、エラーが起きてもエンジンプロセスをキルできたとみなす
	})
	.finally(() => {
	numEngineProcessKilled++;
	log.info(
	`ENGINE ${engineId}: Process killed. ${numEngineProcessKilled} / ${numLivingEngineProcess} processes killed`,
	);
	});
	});
	// すべてのエンジンプロセスキル処理が完了するまで待機
	return Promise.all(waitingKilledPromises).then(() => {
	// エンジン終了後の処理を実行
	log.info(
	"All ENGINE process kill operations done. Running post engine kill process",
	);
	return vvppManager.handleMarkedEngineDirs();
	});
	}

これを単純に書くと

flowchart TD
A[befor-quit発火] --> B{エンジンの終了の確認}
B -- エンジンが全てキル -->終了
B -- キルされていないエンジンがある -->C["event.preventDefault();"]
C --> D[エンジンキル]
D -- "app.quit()" -->A

Loading

となりループから抜けられなくなるのではと。
ちょっと心配しすぎでしょうか？

[Hiroshiba]

@sabonerune
なるほどです！！！
確かにtreeKillされてrejectされても、再度killEngineメソッドが呼ばれて、まだプロセスが残ってたらループする気がします。

可能性はもしかしたら低いのかもしれませんが、まあプロセスキルはかなり大事なところなので、しっかりしてあげたいかもしれません。

もし実装するなら「キルしたけど言うことを聞かなかった」フラグを管理する形が良さそう。
EngineProcessContainerが持ってるwillQuitEngineのような感じでcannotQuitEngineとかのフラグ持たせると良さそう。

ただそうすると次は死活判定を誤ってしまいがちになりそうなので、EngineProcessContainerを型ではなくクラスにしてあげて、死活判定メソッドを作るのがいい気がしました。
ついでにプロセスをEngineProcessContainer以外から見えないように（privateに）すると良さそう。
多分死活判定メソッドと、キルメソッドと、あとコンストラクタ内でプロセスをspawnすれば良い･･･はず･･･？

で、可能であればテスト書きたいですね。。。
エンジンマネージャーだけで独立しているので、このクラスだけでテストが作れるはず。
ただ実際にエンジンやプロセスを動かすの大変なので、spawn関数を渡せるようにして、好きなタイミングで好きな応答をするダミーオブジェクトを返せば良さそう。

･･･と、結構入り組みますが、もしよかったら @sabonerune さん実装にチャレンジしてみませんか･･･？
石橋を叩きながらシーケンスを整理する力が相当強いので、追加で舗装する力にご興味あればという感じです！
この課題を解決するだけであればフラグを作って関数を作れば終わりですが、ちょっとこだわって言語の力を借りながら舗装するニュアンスです。
↑のタスクのテストはさておき、とりあえずEngineProcessContainerクラスだけでもよかったら･･･！
（あるいはとりあえずリファクタリングissueだけでも！！）

あ！分からなかったら放送内で説明もできます！！