#1267 のリファクタリング + ポートの割り当て可能の判定を修正 #1317

wappon28dev · 2023-05-18T13:36:53Z

内容

#1267 のリファクタリングとポートの割り当てが可能かどうかの判定を修正します.

具体的には以下のことを行います:

メインエンジンの altPort をタイトルバーに表示する
現状, デフォルトエンジンに関係なく, 0 番目のエンジンの代替ポートを表示しているのでそれを改善する.
→ ref: #1310: その他
PortManager を整理 (余計なインスタンスを生成しないようにする)
→ ref: #1267: コメント
getProcessIdFromPort の整理 (os での分岐, stdout の命名をどうにかする)
→ ref: #1267: コメント
ポートの割り当てが可能かどうかの判定を修正 (windows)
エディターを落としたあとすぐに起動すると, TCP の状態が TIME_WAIT になっているので, 他のプロセスが使用していて割り当て不可と判定してしまうのを修正します.
ref: 実際にポートが使用されていなくてもTIME_WAIT状態のポートを使用中と判定してしまう #1307

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

なし

その他

あまりにも diff が大きい場合, メンテナーさんが大変になってしまうので, もしそうなったら分割したいと思います！
(いじるファイル数は少ないはず…！)

- トースト通知のタイミングも調整した - ウィンドウタイトルの値もwatchした

src/background/portManager.ts

wappon28dev · 2023-05-18T17:54:29Z

レビューの準備ができました！なにか気になる箇所があればお教えください…！

sevenc-nanashi

とりあえずレビューしてみました。（携帯からなのでまだ動かせてません）学校終わったら動かしてみます。

src/background/portManager.ts

src/views/EditorHome.vue

Hiroshiba

リファクタリングありがとうございます！！！
PortManagerが分解されてかなり読みやすくなっているように感じました！！

コード範囲外なのでこちらに１つコメントです。

２箇所にawait store.dispatch("GET_ALT_PORT_INFOS");がありますが、どちらもエンジン再起動時に通らなそうです。（アプリ起動時には２回通りますが）

代わりにPOST_ENGINE_STARTの一番最初に実行すれば大丈夫だと思います！

voicevox/src/store/engine.ts

Lines 221 to 222 in 32940ea

    
           POST_ENGINE_START: { 
        
             async action({ state, dispatch }, { engineIds }) {

ちなみにこれは初回エンジン起動とエンジン再起動の処理を共通化するために作った関数です。

あとは実行順序の問題（UI起動しちゃってからrunEngineしちゃう）がありますが、これはここの２行を逆にすると問題が軽くなる･･･はずです･･･！
全EngineInfoと全AltInfoPortsが揃ってからUIを起動することになるはず。
（追加エンジンのInfoが２回作られる問題は解決しませんが）

@sevenc-nanashi @sabonerune
↑の２行の逆転、問題ないはずなのですが、なにか問題ありそうか思い当たるところあったりされませんか･･･ 😇

src/background/portManager.ts

src/store/engine.ts

src/components/MenuBar.vue

sabonerune · 2023-05-20T12:06:52Z

↑の２行の逆転、問題ないはずなのですが、なにか問題ありそうか思い当たるところあったりされませんか･･･ 😇

多分エディタの表示が遅くなる以外の問題はないと思います。

気づいたのですがrunEngineAllはasync functionですが内部の処理に一切の非同期処理が存在しないため全てのエンジンの起動が終わるまでメインプロセスはブロックされます。
その結果GET_ENGINE_INFOS内のEngineInfoの取得はエンジンの起動が終わるまでブロックされるのでどんなにエンジンの起動が遅くても現状のままでも偶然エンジンの起動が終わってから取得できるみたいです。

Hiroshiba · 2023-05-20T12:26:04Z

@sabonerune エディタの表示が遅くなるのはその通りなので、影響を見てみたいですねぇ。。デフォルトエンジン１つしかない場合でも最悪１秒くらい変わる可能性ありそう。ま、まあ最初二音声合成できるまでの待機時間は変わらないはず。

後半に関して、おっとなるほどという感じです！
よくある言語仕様だと、await中に何かしらの処理（GET_ENGINE_INFOSのipc通信とか）が来るとそちらが先に処理される気がします。（runEngineAll内で10秒くらいsleepするとわかりそう）
じゃあなんでEngineInfoが取得できるのかというと、EngineInfoはrunEngineAllが実行される前から作成されているからだと思います！
そもそもrunEngineAllを呼ばなくてもちゃんと取得できるはず。

wappon28dev · 2023-05-20T13:49:04Z

２行の逆転のところ以外は, レビューを反映しました！ (逆転させて良いのかな…？)
また何か気になることがあればお教えください！

src/views/EditorHome.vue

Hiroshiba

２行の逆転のところ以外は, レビューを反映しました！ (逆転させて良いのかな…？)
また何か気になることがあればお教えください！

厳密には逆転させないとダメなはず･･･！
たぶんですが、今のままだとrunEngineAll関数の最初に１０秒sleepしたりすると（エンジン起動に時間がかかった場合を想定）ポート変更が取得できない･･･はず！

- !isVuexReady を上に - ２行のとこ反転

Hiroshiba · 2023-05-26T19:48:25Z

LGTMと書く前に念のためチェックしてみたのですが、自分の環境だと設定→エンジンモードを切り替えるとポートが変更されました･･･。
そもそもここはエンジンが自動再起動するのですが、どうやら一瞬だけWAIT_CLOSEになっているようでした。
（restart関数の中でkillしたあと10msec待ってエンジン起動開始させたらポート変更されませんでした。）

メニューボタンからのエンジン再起動はポート変更なしで実行できました。
コード読んだ感じ再起動処理はほぼ一緒なので、なぜこの違いが出るのかは全く分かりません･･･。

ど、どうしましょう。。律儀にやる場合はWAIT_CLOSEだった場合はリトライとかが考えられそうですが。。。
（結構差分がすでに大きいので、一旦この挙動はissueにして、こちらのプルリクエストはマージできると嬉しいかもです！）

wappon28dev · 2023-05-27T12:55:26Z

Re: #1317 (comment)

なるほど…　エンジンモードの切り替えにもエンジンの再起動があるのか…
気づかなかったです.　モード切り替えのときのみ, 一瞬 CLOSE_WAIT になるんですね.
確かに, CLOSE_WAIT のときは, 少しタイムアウトを待ってみても良いかもしれないですね. (ref: 下図)

¹

具体的なリトライ時間は, 後に議論しましょう！　Issue 作っておきます.

マージについては, こちらの準備はOKです！

信頼性のある通信を実現するTCPプロトコル - @IT より ↩

Hiroshiba · 2023-05-27T13:04:15Z

モード切り替えのときのみ, 一瞬 CLOSE_WAIT になるんですね

理由がほんとに謎で、コード読んだ感じ差としてはawaitがあるかどうかくらいに見えるんですが、それで違いが出るとは思えず…

図もありがとうございます、とても勉強になります！！

Hiroshiba

ほぼLGTMなのですが１つ問題ありそうだったのでコメントしました！

src/background/portManager.ts

- basePortを定義したのに全然使われてなかったので使った

Hiroshiba

LGTM！！！！！

ほんとに他分野に渡る知識が必要で難しいリファクタリングだと思います、取り組んでくださって本当にありがとうございます！！

@sevenc-nanashi さん、 @sabonerune さん、もし気になるところあればコメントいただけると心強いです･･･！！

- ここまで来たらURLを引数に渡しても良いかもと思った

sabonerune · 2023-05-31T22:52:02Z

気になった点というと一度ポートの変更が起こるとエンジンを再起動する度にその再起動でポートが切り替わらなくても通知が出ることですね。
エンジンが再起動する度にデフォルトのポートに戻る挙動にするならこれでもいいような気がしますが維持されるのなら通知が過剰のような気がします。
ただ、エンジンの再起動はそこまで頻繁にすることではないからそこまで気にすることででもない?

もう一つ気になったことというより今更気づいたことなのですがWindowsで時々発生する[Errno 13] error while attempting to bind on address ('127.0.0.1', 50021): アクセス許可で禁じられた方法でソケットにアクセスしようとしました。に対処できない点です。
これはnetstatにも出てこない上にエディタから見るとエンジンは起動自体はするが強制終了してしまうという挙動になるので結構面倒そうですね…

src/background/engineManager.ts

Co-authored-by: sabonerune <102559104+sabonerune@users.noreply.github.com>

resolve: VOICEVOX#1326

Hiroshiba · 2023-06-02T16:12:43Z

@sabonerune ポートが使えない系にも対処するの、issue建ててみました！

「アクセス許可で禁じられた方法でソケットにアクセスしようとしました」エラー #1330

Hiroshiba · 2023-06-02T16:31:00Z

ポートやTCP周りの知識に詳しくなれました！！！
リファクタリング本当にありがとうございます！！
PRレビューもありがとうございます！！！

多分大丈夫だと思うのでマージします！

Hiroshiba · 2023-06-02T16:32:06Z

自動クローズされなかったのでテスト

fix: #1307

Hiroshiba · 2023-06-02T16:32:18Z

fix #1307

Hiroshiba · 2023-06-02T16:33:18Z

間違えていました。。

fix #1317

Hiroshiba · 2023-06-02T16:33:29Z

。。。。

fix #1326

Hiroshiba · 2023-06-02T16:33:56Z

･･･よくわかりませんでした！！！

wappon28dev · 2023-06-04T05:43:12Z

マージありがとうございました！！！　私もポート周りに詳しくなれて嬉しいです.
またご機会あれば contribute したいと思います. よろしくお願いします！

そこまで重要ではないですが, おそらくお読み飛ばされている conversion: #1317 (comment)

sevenc-nanashi · 2023-06-04T06:38:48Z

自動クローズされなかったのでテスト

fix: #1307

自動クローズはPRの最初のメッセージに書かないといけないはず？
後から設定するなら右の方のDevelopmentですね

wappon28dev added 2 commits May 18, 2023 22:33

Fix: PortManager を整理

98f9c95

Add: stateにaltPortInfosを持たせた

5523362

- トースト通知のタイミングも調整した - ウィンドウタイトルの値もwatchした

wappon28dev marked this pull request as ready for review May 18, 2023 17:44

wappon28dev requested a review from a team as a code owner May 18, 2023 17:44

wappon28dev requested review from Hiroshiba and removed request for a team May 18, 2023 17:44

wappon28dev changed the title ~~WIP: #1267 のリファクタリング + ポートの割り当て可能の判定を修正~~ #1267 のリファクタリング + ポートの割り当て可能の判定を修正 May 18, 2023

Fix: isDeep -> isNested

c2b9668

wappon28dev commented May 18, 2023

View reviewed changes

src/background/portManager.ts Show resolved Hide resolved

Fix: コメント修正

d815c83

sevenc-nanashi reviewed May 18, 2023

View reviewed changes

src/background/portManager.ts Outdated Show resolved Hide resolved

src/views/EditorHome.vue Show resolved Hide resolved

Fix: eslintのdisableの行を削除

7988e28

wappon28dev mentioned this pull request May 19, 2023

設定画面に “非表示にしたヒントを全て再表示する” ボタンを追加 #1318

Merged

Hiroshiba reviewed May 19, 2023

View reviewed changes

wappon28dev added 2 commits May 20, 2023 22:04

Add: isVuexReady 追加

1524d7f

Fix: レビュー反映

5794248

Hiroshiba requested changes May 21, 2023

View reviewed changes

src/views/EditorHome.vue Outdated Show resolved Hide resolved

Hiroshiba reviewed May 21, 2023

View reviewed changes

fix: レビューを反映

61914ed

- !isVuexReady を上に - ２行のとこ反転

wappon28dev requested review from Hiroshiba and sevenc-nanashi May 26, 2023 13:27

Hiroshiba reviewed May 27, 2023

View reviewed changes

src/background/portManager.ts Outdated Show resolved Hide resolved

wappon28dev mentioned this pull request May 27, 2023

エンジン再起動時にTCPの状態が CLOSE_WAIT のときに, そのタイムアウトを待つ #1326

Closed

Fix: レビュー反映

10b0917

- basePortを定義したのに全然使われてなかったので使った

Hiroshiba approved these changes May 29, 2023

View reviewed changes

Fix: プロトコルのホルダーを追加

f41106a

- ここまで来たらURLを引数に渡しても良いかもと思った

sabonerune reviewed Jun 1, 2023

View reviewed changes

src/background/engineManager.ts Outdated Show resolved Hide resolved

wappon28dev and others added 2 commits June 2, 2023 00:41

プロトコルの部分修正

ee9c644

Co-authored-by: sabonerune <102559104+sabonerune@users.noreply.github.com>

Fix: 割り当て可能な判定を LISTENING 以外のみに

93bf24d

resolve: VOICEVOX#1326

wappon28dev force-pushed the refactor/pr-1267 branch from ce69ff4 to 93bf24d Compare June 2, 2023 14:43

Hiroshiba mentioned this pull request Jun 2, 2023

「アクセス許可で禁じられた方法でソケットにアクセスしようとしました」エラー #1330

Closed

3 tasks

Hiroshiba merged commit ccf3ff0 into VOICEVOX:main Jun 2, 2023

wappon28dev deleted the refactor/pr-1267 branch June 4, 2023 05:40

Hiroshiba mentioned this pull request Aug 2, 2023

エンジンのエラーが早かった場合にwin存在しない問題を迂回する #1461

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#1267 のリファクタリング + ポートの割り当て可能の判定を修正 #1317

#1267 のリファクタリング + ポートの割り当て可能の判定を修正 #1317

wappon28dev commented May 18, 2023

wappon28dev commented May 18, 2023

sevenc-nanashi left a comment

Hiroshiba left a comment •

edited

Loading

sabonerune commented May 20, 2023 •

edited

Loading

Hiroshiba commented May 20, 2023 •

edited

Loading

wappon28dev commented May 20, 2023 •

edited

Loading

Hiroshiba left a comment •

edited

Loading

Hiroshiba commented May 26, 2023 •

edited

Loading

wappon28dev commented May 27, 2023 •

edited

Loading

Hiroshiba commented May 27, 2023

Hiroshiba left a comment

Hiroshiba left a comment

sabonerune commented May 31, 2023 •

edited

Loading

Hiroshiba commented Jun 2, 2023

Hiroshiba commented Jun 2, 2023

Hiroshiba commented Jun 2, 2023

Hiroshiba commented Jun 2, 2023

Hiroshiba commented Jun 2, 2023

Hiroshiba commented Jun 2, 2023

Hiroshiba commented Jun 2, 2023

wappon28dev commented Jun 4, 2023

sevenc-nanashi commented Jun 4, 2023

	POST_ENGINE_START: {
	async action({ state, dispatch }, { engineIds }) {

#1267 のリファクタリング + ポートの割り当て可能の判定を修正 #1317

#1267 のリファクタリング + ポートの割り当て可能の判定を修正 #1317

Conversation

wappon28dev commented May 18, 2023

内容

関連 Issue

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

その他

wappon28dev commented May 18, 2023

sevenc-nanashi left a comment

Choose a reason for hiding this comment

Hiroshiba left a comment • edited Loading

Choose a reason for hiding this comment

sabonerune commented May 20, 2023 • edited Loading

Hiroshiba commented May 20, 2023 • edited Loading

wappon28dev commented May 20, 2023 • edited Loading

Hiroshiba left a comment • edited Loading

Choose a reason for hiding this comment

Hiroshiba commented May 26, 2023 • edited Loading

wappon28dev commented May 27, 2023 • edited Loading

Footnotes

Hiroshiba commented May 27, 2023

Hiroshiba left a comment

Choose a reason for hiding this comment

Hiroshiba left a comment

Choose a reason for hiding this comment

sabonerune commented May 31, 2023 • edited Loading

Hiroshiba commented Jun 2, 2023

Hiroshiba commented Jun 2, 2023

Hiroshiba commented Jun 2, 2023

Hiroshiba commented Jun 2, 2023

Hiroshiba commented Jun 2, 2023

Hiroshiba commented Jun 2, 2023

Hiroshiba commented Jun 2, 2023

wappon28dev commented Jun 4, 2023

そこまで重要ではないですが, おそらくお読み飛ばされている conversion: #1317 (comment)

sevenc-nanashi commented Jun 4, 2023

Hiroshiba left a comment •

edited

Loading

sabonerune commented May 20, 2023 •

edited

Loading

Hiroshiba commented May 20, 2023 •

edited

Loading

wappon28dev commented May 20, 2023 •

edited

Loading

Hiroshiba left a comment •

edited

Loading

Hiroshiba commented May 26, 2023 •

edited

Loading

wappon28dev commented May 27, 2023 •

edited

Loading

sabonerune commented May 31, 2023 •

edited

Loading