複数エンジン対応: `DEFAULT_ENGINE_INFOS`に登録されたエンジンのプロセスをすべて起動する（Promise版） #750

aoirint · 2022-03-10T18:31:07Z

内容

#741 の別バージョン（コールバックではなくPromiseを使った）の実装になります。~~こちらでは、エディタがエンジンのプロセスを管理していないとき、エディタが終了しないという #741 の不具合を修正しています。~~ #741 でも修正しました。

可読性向上のための変更です。

~~マージの想定としては、不具合のある #741 の次にこのPR または、このPRのみをマージすることを想定しています。~~

手元での動作確認手順
- VOICEVOXエンジン 0.11.3・COEIROINKエンジン c0.2.0+v0.10.preview.0を.envに追加
- COEIROINK Ver. 1.0.1を起動、同梱されたCOEIROINKエンジンの起動完了を待機
- npm run electron:serveを実行
- 閉じるボタンを押して、VOICEVOXエディタ・VOICEVOXエンジンのプロセスが正常終了することを確認

エンジンプロセスのキルの流れ

#654 や #741 では、エンジンプロセスをキルする処理をコールバックを使って書いていましたが、このPRではPromiseで実装しています。

アプリケーションのウインドウがすべて閉じられ、app.quitが呼び出されたとき、before-quitイベントが発火します。

before-quitイベントのハンドラでは、event.preventDefaultを同期的に呼び出すことでアプリケーションの終了を回避することができます。

実装では、キルしなければならないエンジンプロセスが1つ以上ある場合、before-quitイベントを同期的にキャンセルし、エンジンプロセス終了後に非同期にapp.quitを再呼び出ししています。これにより、エンジンプロセスが生存したままエディタが終了することを防ぎ、エンジンプロセスの終了を待機してからエディタが終了するようにしています。

#654 や #741 では、エンジンプロセスのキルを非同期に実行しつつ、before-quitイベントを同期的にキャンセルする関数（killEngineとその呼び出し元のこと）を実装するため、
可読性が落ちる（と思っている）のを承知でコールバックを使っていましたが、戻り値を工夫すればPromiseで書けることに気づいたので書き直してみました。

スクリーンショット・動画など

その他

…-process-container

src/background.ts

…tiple-engine-process-container-promise

…ne-process-container-promise

aoirint · 2022-03-11T17:38:22Z

#741 マージによるコンフリクトを解消してWindows 10上での動作を確認しました。

Segu-g

少し気になったところを質問しました

とても読みやすくなっていていいと思います！

src/background.ts

Segu-g · 2022-03-14T16:46:10Z

プロセスの未起動やプロセスの終了を同期的に知らせるためにprocessKillStartedを返していますがこれって同期的に返す必要はありますか？「既にresolveされている」と解釈しても良さそうだと思いました

aoirint · 2022-03-14T17:56:52Z

@Segu-g

プロセスの未起動やプロセスの終了を同期的に知らせるためにprocessKillStartedを返していますがこれって同期的に返す必要はありますか？

before-quitイベントのevent.preventDefault()を同期的に呼び出す必要があるため、同期的に返すようにしています。
Electronの仕様として、before-quitイベントのハンドラには非同期関数を使用できないという認識です。

Segu-g · 2022-03-15T06:17:12Z

@aoirint

before-quitイベントのevent.preventDefault()を同期的に呼び出す必要があるため、同期的に返すようにしています。 Electronの仕様として、before-quitイベントのハンドラには非同期関数を使用できないという認識です。

確かにそうでした！他に場所がないので仕方がないとはいえ、before-quitが2回呼ばれるので同期的に分岐しないとしょうがないですね....

Hiroshiba

改めて読んでみてコメントしてみました！
たぶんだいぶすらっとすると思います･･･！

src/background.ts

Hiroshiba · 2022-06-21T15:31:07Z

こちら、マージできればと思います！
一旦マージするか、修正をお待ちするかでちょっと迷っているので、お忙しければ申し訳ないのですがコメント頂けますと幸いです･･･！

（マージできればみんなでリファクタリングとかもできると思うので！）

Hiroshiba · 2022-06-25T13:17:20Z

@aoirint さん
こちらいかがでしょう👀
お忙しいようでしたらいったんマージして、こちらでガッとリファクタリングしちゃいたいなと･･･！

…ne-process-container-promise

… inferencing issue of array.filter

Hiroshiba

LGTM！！！

めちゃくちゃ見通しが良くなった気がします！！

Hiroshiba · 2022-06-28T15:03:46Z

もしよかったら @Segu-g さんも見て頂けると･･･！！

Segu-g

Electronの終了系イベントについて理解していないので見当違いなことを言っているかもしれませんがbefore-quitでevent.preventDefault()を呼んでreturnした際に、window-all-closedが再度発火し、エンジンが終了するまで何度もbefore-quitが呼ばれているように見えます．

何度も呼んでもPromiseがいずれ解決されるなら問題ないのですが、これは想定された動作でしょうか？

src/background.ts

Co-authored-by: Segu <51497552+Segu-g@users.noreply.github.com>

src/background.ts

Hiroshiba · 2022-07-23T13:20:06Z

エンジンが終了するまで何度もbefore-quitが呼ばれているように見えます．

ちゃんとわかってないのですが、まあ少なくとも以前と同じ処理がされそうなので大丈夫･･･なはず･･･！

aoirint · 2022-08-10T08:42:34Z

#750 (comment)

そうですね、複数エンジン化前にも、もともとwindow-all-closedは2回呼ばれていたと思います。

Xボタンを押したときにwindow.destroyとapp.quitとが両方呼ばれているのかもです。。
OSによって挙動が違う可能性もありそうです。

過去のPRで終了操作をしてもVOICEVOXが終了できないより、動作に問題ないなら2回呼ばれていてもいい、という方向にした気がします！
（直せそうなら直したい・・・）

…ne-process-container-promise engineKey to engineId

aoirint · 2022-08-10T09:05:02Z

engineKeyからengineIdへのリネーム #871 でコンフリクトが起きていたのを修正しました！

Hiroshiba · 2022-08-10T17:13:43Z

修正ありがとうございます！
正直起動・終了シーケンスは、何が正しいのか、今VOICEVOXはどうなっているのかわからないので、整理したいですよね･･････。

@Segu-g さん、もしよければまた見て頂けると･･･！

Segu-g

LGTM!

Segu-g · 2022-08-11T14:55:44Z

そうですね、複数エンジン化前にも、もともとwindow-all-closedは2回呼ばれていたと思います。

app.quitが非同期に呼ばれるせいで呼ばれるまでの間にwindow-all-closedが呼ばれているのかなと考えていましたが、元からだったんですね．試しにapp.quitを何度もキャンセルしてもwindow-all-closedは2回以上呼ばれませんでした．

終了イベント辺りはCommand + QとかCLOSE＿WINDOWの複数の経路があるからややこしくなってますね....

Hiroshiba · 2022-08-11T17:46:24Z

起動シーケンス含むライフサイクルについて、とりあえず可視化するissueを作ってみました。

起動シーケンスなどのライフサイクルを可視化する #891

Hiroshiba · 2022-08-11T17:46:50Z

マージします！！

aoirint added 20 commits March 9, 2022 04:25

add runEngineAll

a135eff

fix log

f81a774

fix log

22e0168

add killEngineAll

c5202dd

fix log

6e46ad9

fix log

31bfe9f

add restartEngineAll

eb919bd

call killEngineAll

39b81df

fix log unit

c8a7c22

npm run fmt

9778831

assume to be killed

2102710

add memo comment

c24c7e4

fix comment

5c1cf7c

call onAllKilled after final process kill errored

086bd30

fix log

9399dd1

fix log

db81c2b

rewrite engine process kill logic to use promise

d53c4e5

add comment

84f350d

remove redundant comment

fb97d4a

npm run fmt

f421c36

aoirint mentioned this pull request Mar 11, 2022

複数エンジン対応: DEFAULT_ENGINE_INFOSに登録されたエンジンのプロセスをすべて起動する（Callback版） #741

Merged

aoirint marked this pull request as ready for review March 11, 2022 15:28

Merge remote-tracking branch 'origin/main' into patch-multiple-engine…

26e4b2d

…-process-container

aoirint commented Mar 11, 2022

View reviewed changes

src/background.ts Outdated Show resolved Hide resolved

aoirint added 3 commits March 12, 2022 01:27

fix infinite loop when using external engine

b681ec4

Merge branch 'patch-multiple-engine-process-container' into patch-mul…

2f20215

…tiple-engine-process-container-promise

Merge remote-tracking branch 'upstream/main' into patch-multiple-engi…

45df811

…ne-process-container-promise

Segu-g reviewed Mar 14, 2022

View reviewed changes

src/background.ts Outdated Show resolved Hide resolved

src/background.ts Outdated Show resolved Hide resolved

Hiroshiba reviewed Jun 14, 2022

View reviewed changes

src/background.ts Outdated Show resolved Hide resolved

src/background.ts Outdated Show resolved Hide resolved

aoirint added 9 commits June 27, 2022 01:01

Merge remote-tracking branch 'upstream/main' into patch-multiple-engi…

359d4e4

…ne-process-container-promise

use promise.resolve/reject and undefined to return killing status

acc0b02

return undefined if engine already killed

eec0d02

shrink code

b8704d6

rename var

c6dce69

killEngineAll now returns Record<string, Promise<void>> to avoid type…

1bdf974

… inferencing issue of array.filter

asynchronized each engine process kill

5ecc203

add comment

5ad4780

strip redundant new promise

3fceda1

Hiroshiba approved these changes Jun 28, 2022

View reviewed changes

Segu-g reviewed Jun 30, 2022

View reviewed changes

src/background.ts Outdated Show resolved Hide resolved

Update src/background.ts

0dd3893

Co-authored-by: Segu <51497552+Segu-g@users.noreply.github.com>

Hiroshiba reviewed Jul 23, 2022

View reviewed changes

src/background.ts Outdated Show resolved Hide resolved

Update src/background.ts

2280ae9

Merge remote-tracking branch 'upstream/main' into patch-multiple-engi…

087d473

…ne-process-container-promise engineKey to engineId

Segu-g approved these changes Aug 11, 2022

View reviewed changes

Hiroshiba merged commit a845b5a into VOICEVOX:main Aug 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

複数エンジン対応: `DEFAULT_ENGINE_INFOS`に登録されたエンジンのプロセスをすべて起動する（Promise版） #750

複数エンジン対応: `DEFAULT_ENGINE_INFOS`に登録されたエンジンのプロセスをすべて起動する（Promise版） #750

aoirint commented Mar 10, 2022 •

edited

Loading

aoirint commented Mar 11, 2022 •

edited

Loading

Segu-g left a comment •

edited

Loading

Segu-g commented Mar 14, 2022

aoirint commented Mar 14, 2022

Segu-g commented Mar 15, 2022

Hiroshiba left a comment

Hiroshiba commented Jun 21, 2022

Hiroshiba commented Jun 25, 2022

Hiroshiba left a comment

Hiroshiba commented Jun 28, 2022

Segu-g left a comment

Hiroshiba commented Jul 23, 2022

aoirint commented Aug 10, 2022 •

edited

Loading

aoirint commented Aug 10, 2022

Hiroshiba commented Aug 10, 2022 •

edited

Loading

Segu-g left a comment

Segu-g commented Aug 11, 2022

Hiroshiba commented Aug 11, 2022 •

edited

Loading

Hiroshiba commented Aug 11, 2022

複数エンジン対応: DEFAULT_ENGINE_INFOSに登録されたエンジンのプロセスをすべて起動する（Promise版） #750

複数エンジン対応: DEFAULT_ENGINE_INFOSに登録されたエンジンのプロセスをすべて起動する（Promise版） #750

Conversation

aoirint commented Mar 10, 2022 • edited Loading

内容

エンジンプロセスのキルの流れ

関連 Issue

スクリーンショット・動画など

その他

aoirint commented Mar 11, 2022 • edited Loading

Segu-g left a comment • edited Loading

Choose a reason for hiding this comment

Segu-g commented Mar 14, 2022

aoirint commented Mar 14, 2022

Segu-g commented Mar 15, 2022

Hiroshiba left a comment

Choose a reason for hiding this comment

Hiroshiba commented Jun 21, 2022

Hiroshiba commented Jun 25, 2022

Hiroshiba left a comment

Choose a reason for hiding this comment

Hiroshiba commented Jun 28, 2022

Segu-g left a comment

Choose a reason for hiding this comment

Hiroshiba commented Jul 23, 2022

aoirint commented Aug 10, 2022 • edited Loading

aoirint commented Aug 10, 2022

Hiroshiba commented Aug 10, 2022 • edited Loading

Segu-g left a comment

Choose a reason for hiding this comment

Segu-g commented Aug 11, 2022

Hiroshiba commented Aug 11, 2022 • edited Loading

Hiroshiba commented Aug 11, 2022

複数エンジン対応: `DEFAULT_ENGINE_INFOS`に登録されたエンジンのプロセスをすべて起動する（Promise版） #750

複数エンジン対応: `DEFAULT_ENGINE_INFOS`に登録されたエンジンのプロセスをすべて起動する（Promise版） #750

aoirint commented Mar 10, 2022 •

edited

Loading

aoirint commented Mar 11, 2022 •

edited

Loading

Segu-g left a comment •

edited

Loading

aoirint commented Aug 10, 2022 •

edited

Loading

Hiroshiba commented Aug 10, 2022 •

edited

Loading

Hiroshiba commented Aug 11, 2022 •

edited

Loading