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

エンジンが使うポートが割り当てできなければ、他の空いているポートで起動してユーザーに通知する #1267

Merged
merged 24 commits into from
Apr 25, 2023

Conversation

wappon28dev
Copy link
Contributor

@wappon28dev wappon28dev commented Mar 30, 2023

内容

エンジンが使うポートが割り当てできなければ、他の空いているポートで起動してユーザーに通知します

方針

おそらくポートが割り当てできなかった場合は, ↓ の部分が exit 1 で終了して

const engineProcess = spawn(enginePath, args, {
cwd: path.dirname(enginePath),
});

↓ にエラーダイアログが出てくる仕様になってると思います

engineProcess.on("close", (code, signal) => {
log.info(
`ENGINE ${engineId}: Process terminated due to receipt of signal ${signal}`
);
log.info(`ENGINE ${engineId}: Process exited with code ${code}`);
if (!engineProcessContainer.willQuitEngine) {
ipcMainSend(win, "DETECTED_ENGINE_ERROR", { engineId });
const dialogMessage =
engineInfos.length === 1
? "音声合成エンジンが異常終了しました。エンジンを再起動してください。"
: `${engineInfo.name}が異常終了しました。エンジンを再起動してください。`;
dialog.showErrorBox("音声合成エンジンエラー", dialogMessage);
}
});

なので, spawn の前にポートが割り当てられるかを確認して, もし割り当てられなかったら...

  • 使っているプロセスid (と プロセス名) をログに出す
  • engineInfohost に他の空いているポートを書き換える
  • ユーザーにポートを変更したことを通知する
    をしたいと思います

関連 Issue

close #1252
ref VOICEVOX/voicevox_engine#634 (エディターがログを出すけど一応)
ref #1189

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

なし

その他

なし

net の createServer はなぜかエラーを検知してくれない...
いったん外部ライブラリに頼る
@wappon28dev wappon28dev requested a review from a team as a code owner March 30, 2023 07:03
@wappon28dev wappon28dev requested review from y-chan and removed request for a team March 30, 2023 07:03
@wappon28dev wappon28dev changed the title エンジンが使うポートが割り当てできなければ、他の空いているポートで起動してユーザーに通知する WIP: エンジンが使うポートが割り当てできなければ、他の空いているポートで起動してユーザーに通知する Mar 30, 2023
@wappon28dev wappon28dev marked this pull request as draft March 30, 2023 07:04
@wappon28dev
Copy link
Contributor Author

ユーザーにはどうやって, 何を通知するべきでしょうか

how:

  1. Electron の dialog
    ・エンジンの異常終了と同じ形式のダイアログ

  2. エディター内のトースト通知
    ・ユーザーが普通にエディターを使うにあたって, ポートが変わって困ることは少ないかも?
    ・ユーザーが特定のアクションを必要としない場合, 下のような通知で十分かも?
    (もしくは, エディターに通知機構があればそれを使う)
    image

what:

  1. ポートが変わったこと
  2. どのプロセスがポートを使っていたか (プロセスID, プロセス名)
  3. ポートを変更するかしないかの選択 (→ トースト通知には向いていない)

ユーザーにどこまで伝えるかは難しいですね... ご意見よろしくお願いします!

- 外部ライブラリに頼る必要がなくなった
- bash 要検証
@y-chan
Copy link
Member

y-chan commented Mar 30, 2023

PRありがとうございます!

ご質問に対して、まずVOICEVOXのUX・UIデザインの方針を基準に考えてみました。
ご覧いただければ分かる通り、VOICEVOXが最も大事にしているのは「ユーザーが作ろうとしたコンテンツが完成までたどり着くこと」です。
エンジンが起動しないことは、「コンテンツが完成までたどり着く」どころか、まずそれを始めることを妨げています。
また、VOICEVOXエンジンが原因不明で終了してしまい、使えないことはユーザーにとって不利益であり、使用離脱に繋がってしまいます。その妨げを取り除くことがVOICEVOXにとっては最も解決したい問題であり、目標になりますね!

となると、原因(すでにポートが使われていること)や、原因の原因(どのプロセスがポートを占有しているか)をユーザーに提示したところで、開発者でもない限り対処することは難しいでしょうから、解決したい問題に対するアプローチにはならないといえます。また、この対処を行ったところで、ある意味でデザイン方針の「操作のために考える必要がないこと」を侵害しています。

であれば、デフォルトポート以外で起動するのが、ユーザーにとって求めている挙動(問題なくVOCIEVOXエンジンが起動して使える状態になり、ユーザーがコンテンツを作り始められる)になり、ユーザーにとっての不快感排除、そして現状のVOICEVOXが抱えている問題の解決策になるといえます。

次に、ユーザーに何を通知するか(もしくは通知しないか)を考えます。
単にエディタを使いたい人にとっては、ポートの変更情報は不要ですが、例えばVOICEVOX APIを利用したアプリケーションを利用したい場合は、APIのポート番号変更情報は必要になりますね。大多数の人にとっては不要な情報である可能性が高いです。つまり、必要以上に目立たせるのはユーザーに不安を煽るだけで、効果が薄いと考えられます。
よって、「XXXXX番ポートが使用中であるため、〇〇はYYYYY番ポートで起動しました」とトースト通知のような、小さい通知をするのが望ましいと思いました!

@wappon28dev
Copy link
Contributor Author

wappon28dev commented Mar 30, 2023

コメントありがとうございます

UX・UIのデザインの方針ももう一度読んでみました
確かにユーザーにとって必要のないことを UI に表示する (選択させる) ことは良くないですね
ご提案通り, デフォルトポート以外でVOICEVOXは起動し, ユーザーへの通知はエディター内のトースト通知で,

XXXXX番ポートが使用中であるため、〇〇はYYYYY番ポートで起動しました

と表示します (バツボタンより, 5秒ほどで消えるのが望ましいかな)
エディター内のトースト通知の表示については, また実装していきます

ところで, ログには ↓ のような形式で十分でしょうか?

すでに割り当てられていたとき

[17:14:23.437] [info]  ENGINE 074fc39e-678b-4c13-8916-ffca8d505d1d: Start launching
[17:14:23.438] [info]  ENGINE 074fc39e-678b-4c13-8916-ffca8d505d1d: Checking whether port 50021 assignable...
[17:14:23.444] [info]  PORT 50021: Getting process id ...
[17:14:23.444] [info]  PORT 50021: Running command: "netstat -ano"
[17:14:23.524] [warn]  PORT 50021: Nonassignable; pid=6780 uses this port!
[17:14:23.526] [info]  PORT 50021: Getting process name from pid=6780...
[17:14:24.135] [info]  PORT 50021: Found process name: node.exe
[17:14:24.136] [warn]  ENGINE 074fc39e-678b-4c13-8916-ffca8d505d1d: Port 50021 has already been assigned by node.exe (pid=6780)

割り当てられてないとき

[17:19:37.519] [info]  ENGINE 074fc39e-678b-4c13-8916-ffca8d505d1d: Start launching
[17:19:37.520] [info]  ENGINE 074fc39e-678b-4c13-8916-ffca8d505d1d: Checking whether port 50021 assignable...
[17:19:37.520] [info]  PORT 50021: Getting process id ...
[17:19:37.520] [info]  PORT 50021: Running command: "netstat -ano"
[17:19:37.600] [info]  PORT 50021: Assignable; Nobody uses this port!
[17:19:37.600] [info]  ENGINE 074fc39e-678b-4c13-8916-ffca8d505d1d: Starting process

@sabonerune
Copy link
Contributor

あまり詳しくないのですが気になることがあります。

まずWindowsにポートを使われないように予約する機能があるらしくその場合は使用しているプロセスがなくても起動できない可能性があるみたいです。
netsh int ipv4 show excludedportrange tcpコマンドで予約されているTCPポートが確認できます。
この場合ポートを使用しているプロセスが存在しない可能性もあります。

あと自分は持っていないので試せないのですがmacOSではlsofを実行するためにroot権限が必要らしいです。

@wappon28dev
Copy link
Contributor Author

Re: #1267 (comment)

貴重な情報ありがとうございます! 再起動後のPCで試しに該当のコマンドを叩いてみました.

> netsh int ipv4 show excludedportrange tcp

プロトコル tcp ポート除外範囲

開始ポート    終了ポート
----------    --------
      5357        5357
      8501        8501
     50000       50059     *

* - 管理されている除外ポート。

50000 - 50059 番ポートは, 管理されている(?) 除外ポートで, 予約されていることになっていましたが, 50021番も50022番も普通に割り当て (エンジンを動かすこと) ができました.
私は初めて除外ポートについて知ったのですが, VOICEVOX/voicevox_engine#631 の通り, ポート範囲におそらく問題が無いのと除外範囲に入っていたけれど挙動に問題が無かったため, 一旦保留にしたいと思います.
ただ, 他のPCでは試していないので, 問題があればこちらで共有したいと思います.

MacOS の挙動も了解しました. 他のコマンドを探してみます.

- Fix: トーストの表示の仕方もちょっと変更
- Fix: Toastをプレーンからquasarへ
- Fix: Toast系統のステート削除
- Fix:
- Add: タイトルバーに代替ポートを表示
- Add: トーストにアクション追加
- Enh: トーストの色変更
- Fix: getter -> action
@wappon28dev
Copy link
Contributor Author

wappon28dev commented Apr 8, 2023

Re: #1267 (comment)
MacOSが動くものが無かったので, 学校の iMac で確かめてみました

どうやら, MacOS でも rootなしで lsof は使えるっぽいですね Macユーザーの方, また情報があればお教えください

@wappon28dev
Copy link
Contributor Author

wappon28dev commented Apr 8, 2023

UI差分について

  • 今はトースト通知で実装 → 50021番で繋がらないとユーザーが気づかない報告が多かったらダイアログ表示へ 1
  • 使っているポートがわかるようにタイトルバーに Port: XXXXX のように追加 (とりあえず0番目のエンジンのみ) 2

image
image

代替ポートを使ってない場合は通常と変わりません. 色や配置についてお気づきの点があればお教えくださいー!

Footnotes

  1. Discord コミュニティー より

  2. Discord コミュニティー より

@wappon28dev
Copy link
Contributor Author

wappon28dev commented Apr 8, 2023

ログ形式の差分について

通常時

  [info]  Starting 1 engine/s...
  [info]  ENGINE 074fc39e-678b-4c13-8916-ffca8d505d1d: Start launching
+ [info]  ENGINE 074fc39e-678b-4c13-8916-ffca8d505d1d: Checking whether port 50021 is assignable...
+ [info]  PORT 50021: Getting process id...
+ [info]  PORT 50021: Running command: "netstat -ano"
+ [info]  PORT 50021: Assignable; Nobody uses this port!
  [info]  ENGINE 074fc39e-678b-4c13-8916-ffca8d505d1d: Starting process
  [info]  ENGINE 074fc39e-678b-4c13-8916-ffca8d505d1d mode: CPU

50021番が割り当てられないとき (代替ポート: 50022番)

  [info]  Starting 1 engine/s...
  [info]  ENGINE 074fc39e-678b-4c13-8916-ffca8d505d1d: Start launching
+ [info]  ENGINE 074fc39e-678b-4c13-8916-ffca8d505d1d: Checking whether port 50021 is assignable...
+ [info]  PORT 50021: Getting process id...
+ [info]  PORT 50021: Running command: "netstat -ano"
+ [warn]  PORT 50021: Nonassignable; pid=23792 uses this port!
+ [info]  PORT 50021: Getting process name from pid=23792...
+ [info]  PORT 50021: Found process name: node.exe
+ [warn]  ENGINE 074fc39e-678b-4c13-8916-ffca8d505d1d: Port 50021 has already been assigned by node.exe (pid=23792)
+ [info]  PORT 50021: Find another assignable port from 50021...
+ [info]  PORT 50021: Trying whether port 50022 is assignable...
+ [info]  PORT 50022: Getting process id...
+ [info]  PORT 50022: Running command: "netstat -ano"
+ [info]  PORT 50022: Assignable; Nobody uses this port!
+ [warn]  ENGINE 074fc39e-678b-4c13-8916-ffca8d505d1d: Applied Alternative Port: 50021 -> 50022
  [info]  ENGINE 074fc39e-678b-4c13-8916-ffca8d505d1d: Starting process
  [info]  ENGINE 074fc39e-678b-4c13-8916-ffca8d505d1d mode: CPU

代替ポートが見つからない場合

PCの再起動を促すダイアログ表示をします
image

  [info]  Starting 1 engine/s...
  [info]  ENGINE 074fc39e-678b-4c13-8916-ffca8d505d1d: Start launching
+ [info]  ENGINE 074fc39e-678b-4c13-8916-ffca8d505d1d: Checking whether port 50021 is assignable...
+ [info]  PORT 50021: Getting process id...
+ [info]  PORT 50021: Running command: "netstat -ano"
+ [warn]  PORT 50021: Nonassignable; pid=33488 uses this port!
+ [info]  PORT 50021: Getting process name from pid=33488...
+ [info]  PORT 50021: Found process name: node.exe
+ [warn]  ENGINE 074fc39e-678b-4c13-8916-ffca8d505d1d: Port 50021 has already been assigned by node.exe (pid=33488)
+ [info]  PORT 50021: Find another assignable port from 50021...
+ [info]  PORT 50021: Trying whether port 50022 is assignable...
+ [info]  PORT 50022: Getting process id...
+ [info]  PORT 50022: Running command: "netstat -ano"
+ [warn]  PORT 50022: Nonassignable; pid=30324 uses this port!

              ... 代替ポートの探索を続ける ...

+ [warn]  PORT 50021: No alternative port found! 50021...50121
+ [error] ENGINE 074fc39e-678b-4c13-8916-ffca8d505d1d: No Alternative Port Found

                     exit(1) します

どのくらい探索するかはここに書いてあります.

/**
* 割り当て可能な他のポートを探します
*
* @returns 割り当て可能なポート番号 or `undefined` (割り当て可能なポートが見つからなかったとき)
*/
async findAltPort(): Promise<number | undefined> {
this.portLog(`Find another assignable port from ${this.port}...`);
// エンジン指定のポート + 100番までを探索 エフェメラルポートの範囲の最大は超えないようにする
const altPortMax = Math.min(this.port + 100, 65535);
for (let altPort = this.port + 1; altPort <= altPortMax; altPort++) {
this.portLog(`Trying whether port ${altPort} is assignable...`);
const altPid = await new PortManager(
this.hostname,
altPort
).getProcessIdFromPort();
// ポートを既に割り当てられているプロセスidの取得: undefined → ポートが空いている
if (altPid === undefined) return altPort;
}
this.portWarn(`No alternative port found! ${this.port}...${altPortMax}`);
return undefined;
}

ダイアログも含めて, お気づきの点があればお教えくださいー!

@wappon28dev wappon28dev changed the title WIP: エンジンが使うポートが割り当てできなければ、他の空いているポートで起動してユーザーに通知する エンジンが使うポートが割り当てできなければ、他の空いているポートで起動してユーザーに通知する Apr 8, 2023
@wappon28dev wappon28dev marked this pull request as ready for review April 8, 2023 19:39
Copy link
Member

@y-chan y-chan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTMです!

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

コマンド実行から設定、バックグラウンドプロセス、エンジンの理解にVue+Vuexと、完全に総合格闘技な中かなり確度の高いプルリクエストだと思いました!!

細かい点についてとやかくコメントしてしまったのですが、優先度高くなおしてほしい点はほとんどなかった感じです。
もしVueに興味があれば、くらいのコメントが多いです・・・!!

src/background/engineManager.ts Outdated Show resolved Hide resolved
src/background/engineManager.ts Show resolved Hide resolved
src/background/portManager.ts Show resolved Hide resolved
src/background/portManager.ts Outdated Show resolved Hide resolved
src/store/type.ts Outdated Show resolved Hide resolved
src/views/EditorHome.vue Outdated Show resolved Hide resolved
src/type/preload.ts Outdated Show resolved Hide resolved
src/views/EditorHome.vue Outdated Show resolved Hide resolved
src/store/setting.ts Outdated Show resolved Hide resolved
@wappon28dev
Copy link
Contributor Author

レビュー反映しましたー 確認よろしくお願いします!

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!!

ちょくちょくリファクタリングできそうな点があるかもと思いました!!
もしよかったらリファクタリングにチャレンジしてみてください! 良い経験になるんじゃないかなと・・・!

@Hiroshiba Hiroshiba merged commit 5ce05b6 into VOICEVOX:main Apr 25, 2023
@wappon28dev wappon28dev deleted the fix/handle-port-50021-used branch April 25, 2023 17:21
@wappon28dev wappon28dev restored the fix/handle-port-50021-used branch April 25, 2023 17:22
@wappon28dev
Copy link
Contributor Author

マージありがとうございます!!!
また時間のあるときにリファクタリングのPRを投げたいと思います!

あと1つだけ, おそらくお読み飛ばされている返信があるので, リンクを貼っておきます!
(たくさんのConversationで埋もれてしまいました…!)
ref: #1267 (comment)

良い経験となりました! ありがとうございました.

@Hiroshiba
Copy link
Member

すみませんコメント忘れていたので追加しました・・・!

@wappon28dev
Copy link
Contributor Author

wappon28dev commented Apr 29, 2023

Linux (Ubuntu 22.04 + node v16.17.0 on nvm) でこのPRによって動作しないという報告がされています. 1

const exec = isWindows
? {
cmd: "netstat",
args: ["-ano"],
}
: {
cmd: "lsof",
args: ["-i", `:${this.port}`, "-t", "-sTCP:LISTEN"],
};
this.portLog(`Running command: "${exec.cmd} ${exec.args.join(" ")}"`);
let stdout = execFileSync(exec.cmd, exec.args, {
shell: true,
}).toString();

どうやら, 上のコードの execFileSync まわりに, Node.js のバグ (nodejs/node #29466, nodejs/node #43345) があるようです.

ただ, 18.7.0 で直っているようなので, #1298 がマージされればこの問題は解決しそうです.

情報ありがとうございました!

thx! :

Footnotes

  1. discordコミュニティー より

@Hiroshiba
Copy link
Member

ちょっとポート周りでいくつかissueができたのでメモ&共有です! @y-chan

@sabonerune さん本当にありがとうございます 🙇 )

特に最後がかなりの強敵で、エンジン起動・マルチエンジン・プロセス間通信・ポート周りの複合的な課題なので凄まじいです 😇
こちらのコメントにまとめています。正直パズルみたいで面白いです。)

@wappon-28-dev さん
レビューのときに見逃しまくってしまって申し訳ないです 🙇‍♂️
ちょっとだいぶ強敵かもなのですが、もしよかったら知恵をお貸しいただけると・・・・・!!!

@wappon28dev
Copy link
Contributor Author

wappon28dev commented May 10, 2023

そちらこそ検証ありがとうございます…!

マルチエンジンやエンジン (+engineInfo) のライフサイクルへの私の理解が足りなかったようで, 私のPRによってたくさん Issues ができてしまい, こちらこそ申し訳ないです…🥲

私が行う予定だったリファクタリングのものと, @sabonerune さんが作ってくださった Issues とでいくつか共通点があったので, 以下はそのメモです:


  1. state に AltPortInfos を持つようにする (エンジンの再起動時のトースト通知のため)
    #1310: ヒホさんのコメント の方針へ追従/移動

  2. NEW: メインエンジンの altPort をタイトルバーに表示する
    現状, デフォルトエンジンに関係なく, 0番目のエンジンの代替ポートを表示しているのでそれを改善する.
    → ref: #1310: その他

  3. PortManager を整理 (余計なインスタンスを生成しないようにする)
    → ref: #1267: コメント

  4. getProcessIdFromPort の整理 (osでの分岐, stdoutの命名をどうにかする)
    → ref: #1267: コメント


@sabonerune さん もしお手空きでしたら, #1308, #1310 にお取組みいただけませんか…?
というのも, 私が少し忙しくなる期間でして, また, マルチエンジンやエンジンのライフサイクルなど, 知恵が私よりずっと詳しいと思ったからです. もしよければ…!

上のリストの 2, 3, 4 については, 私の手が空いたら, 今取り組まれている PR の様子を見ながらリファクタリングの PR を送りたいと思います!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants