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

ポートが使われていると別のポートを使うように #685

Closed
wants to merge 8 commits into from

Conversation

misogihagi
Copy link
Contributor

内容

ポートが使われていると警告を表示して別のポートを使うようにしました。

関連 Issue

#634

@misogihagi misogihagi requested a review from a team as a code owner May 23, 2023 13:12
@misogihagi misogihagi requested review from Hiroshiba and removed request for a team May 23, 2023 13:13
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.

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

この機能が役に立つのってどういう場合でしょうか?
そこがちょっとイメージできなかったのでお聞きしたいです。
(後述していますが、ライブラリを追加したりせず需要を満たす方法もありそうなので、そちらの手を使うのもありかもと思っています)


ちょっと仕様について考えたのですが、指定したポートが使われていたとき、ユーザー的にはエラーになるのが自然かなと思いました。
別のポートが自動的に割り当たる挙動は珍しく、例えばデバッグ時に前回起動したエンジンを停止し忘れていた場合に、新しいエンジンのportが自動的に変わったことに気づけず、古いエンジンでデバッグして問題が起こるといったことが結構な確率で起こり得そうに思いました。

また、この仕様変更は破壊的変更になり、エンジンを利用するすべてのサードパーティアプリに影響が出る可能性があります。

これらのことを考えて、やるとしたら「ポートスキャンモード」を用意し、指定された場合はスキャンしていく、とかかなーと思いました。
まあでもその場合(明示的に指定する場合)は、FastAPIのちょっと裏技的な方法として--port=0を指定したらランダムなポートを割り当ててくれるっぽいので、それをドキュメントに書くのもありな気がしました。
これだとスキャンする時間が不要になるので高速かもなのですが、どうでしょう 👀

Comment on lines +22 to +28
for proc in process_iter():
try:
if has_process_port(proc):
processes.append(Process(proc.pid))
except AccessDenied:
has_checked_all = False
return GetProcessResult(processes, has_checked_all)
Copy link
Member

@Hiroshiba Hiroshiba May 25, 2023

Choose a reason for hiding this comment

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

全プロセスを網羅的に探索していて、結構効率が悪い気がしました。
またそのプロセスが権限的に情報取得できない場合、ポートが使われているのに取得できないということが起こり得そうです。

例えばlinuxだとlsofコマンドを使えば一発でそのportを使用しているプロセスがわかったりします。
windowsだとややこしいのですが、このあたりを良い感じにラップしているpythonライブラリもあると思うので、psutilではなくそちらを利用したほうが良いのかなと思いました!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

確かに、大抵の場合ポートが被ることはなくてnet_connections()のぶんだけ起動する処理は遅くなると思います。(lsofにしてもpsutilにしてもポートが使われているかはスキャンしないとわからないはず)
ただ、process_iterが呼び出されるのはポートが被った場合で、現実的にそこまで遅くなるとは思ってなくて(単体テストは遅いですが)取得できなくてもポートが使用されていることはメッセージに出しているのでそこでわかるかなと思いました。

Suggested change
for proc in process_iter():
try:
if has_process_port(proc):
processes.append(Process(proc.pid))
except AccessDenied:
has_checked_all = False
return GetProcessResult(processes, has_checked_all)
if check_port_is_live(port):
print(
f"\033[31mWARNING\033[0m: port {port} already in use",
file=sys.stderr,
)
for proc in get_process_by_port(port).processes:
print(
f"\033[31mWARNING\033[0m: port {port} is used by {proc.name()}",
file=sys.stderr,
)
continue
else:
break

もしパフォーマンスを気にするならrun.pyに書いた方がいいのかもしれませんがそうなるとただでさえ分厚いrun.pyが分厚くなる上にテストがしづらいかなと思い切り出しました。

psutilは具体的なOSごとの処理はCで書かれててそれの差分吸収しているのでここまで安定しているライブラリもあまりないと思うのでpsutilでいいのではないかなと思うのですが…

Copy link
Member

@Hiroshiba Hiroshiba Jun 5, 2023

Choose a reason for hiding this comment

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

テストは時間かかる、なるほどです。原因は全プロセスを探索しているからでしょうか?

思いつきですが、起動に失敗したらport=0で再度試す処理をrun.pyに書くとかどうでしょう?
再試行がちゃんと動くのかちょっと不安ですが・・・

@misogihagi
Copy link
Contributor Author

vueは別で起動すると別のポートを使います。自動的にポートを割り当てるのは珍しいことではありません。

image

linuxならいいですけどwindowsでポート使っているプロセス落とすとなったら面倒な気がしてもともとのissueにあるプロセス名を通知する案別のポート使う案はいいと思いました。
もしエラーが起こったとしてそれらを解消するのはかなりのOSの知識を必要として、再起動すれば大抵なんとかなるとはいえ再起動できない場合はつんでしまうのではないかと思いました。
ちょくちょくプロセスが残るのであれば一回一回再起動するより別のプロセス立ち上がってある程度してから再起動したほうがDevelopper Experienceは高いのではないかなと思いました。(実際Vueの次々立ち上がる仕様は複数のプロジェクトを並行運用している時に役立ちました)

--port=0を指定したらランダムなポートを割り当ててくれるのをドキュメントに書くのはいいと思います!

@Hiroshiba
Copy link
Member

Hiroshiba commented May 26, 2023

Vueなどはフレームワークなので、開発者に複数起動されることを想定しているんだろうと確かに思います。Python系統だと例えばJupyter Notebookも同じような挙動でした。
ただエンジンは複数起動されることがほぼないと思います。

誰かがそのポートを使っている場合、プロセスを停止することなく、手動で別のポートを指定すればOKなのかなと思いました。

一方で先ほども書いたとおり、これはサードパーティーアプリにとって仕様変更となってしまうため、かなりの混乱が予想されます。
(今まではエラーになっていたのがエラーが帰らなくなるためです)
--port=0が便利だということをドキュメントに書くに留める、あるいはポートを自動で変更するためのオプションが指定されていたら実行するというのはどうでしょう。

(せっかくプルリクエストを作ってくださったのでできれば取り込みたいのですが、やはり需要に比べて混乱が大きそうなので・・・。申し訳ないです。)

@Hiroshiba
Copy link
Member

Hiroshiba commented May 31, 2023

port=0の場合は未使用ポートが使用される件について、uvicorn側にプルリクエストを送ってドキュメントに取り込まれました!

エンジン側でも案内しちゃえると思います 🙏

@Hiroshiba
Copy link
Member

@misogihagi ちょうどエディターの方から、ポートが塞がれていた時に開いているポートで起動する引数の需要が発生したのでご連絡です・・・!!

@misogihagi
Copy link
Contributor Author

ま、まあもし需要が多くて仕様変えたい時にメジャーバージョン変えてリリースすればいいと思いますので…

@tarepan
Copy link
Contributor

tarepan commented Feb 26, 2024

@Hiroshiba
「現段階ではマージしない(メジャーアップデート時に再判断する)」との理解で良いでしょうか?
その場合、draftPR 化が適切かと思います。

@Hiroshiba
Copy link
Member

@tarepan ありがとうございます!

一旦需要が見えない and 混乱が予想されるので、後ほど再考か、申し訳ないですが一旦closeが良いのかなと思いました!
どういうユースケースの時に便利なのかがあれば話は全然変わってくると思います!

@tarepan
Copy link
Contributor

tarepan commented Feb 27, 2024

一旦需要が見えない and 混乱が予想されるので ... 一旦close

👍
close します。

ポート自動切り替えの実装例として、課題の明確化に大きく寄与した PR でした。
別目的で同様の実装をする際、参考にできそうです。
@misogihagi さんありがとうございました!

@tarepan tarepan closed this Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants