add custom command handler into command server#139
Conversation
umadein
changed the title
umadein
force-pushed
the
feature/custom-command
branch
from
67ca6b2 to
8e5c6d6
Compare
umadein
force-pushed
the
feature/custom-command
branch
from
ad29f7b to
2942320
Compare
umadein
force-pushed
the
feature/custom-command
branch
from
a2aaf99 to
936bc86
Compare
toshifumi-arai
left a comment
toshifumi-arai
left a comment
There was a problem hiding this comment.
LGTMです。
カスタムコマンド追加に伴うスレッド関連を中心に見ましたが、大丈夫そうに見えました。
JSON文字列をパースした結果の構造を確認していませんが、そこが落ちても APP_ERROR が返るので破綻はないように思います。そもそも、JSONの内容が変であれば、agent側でエラーが出るでしょうし。
| import socket | ||
| from dataclasses import dataclass | ||
|
|
||
| from typing_extensions import Self |
There was a problem hiding this comment.
typing_extensions が pyproject.toml に入っていなそうなため、直接の依存として入れた方が壊れにくくなるかもしれません
| def _handle_custom_command(self, request: CommandRequest) -> CommandResponse: | ||
| if self.custom_command_handler is None: | ||
| return CommandResponse( | ||
| copy.copy(request.id_), Status.GENERAL_ERROR, b"Unreachable Error: custom command handler is not set" |
There was a problem hiding this comment.
Unreachable という語は、コード上で到達する方法がないことをイメージすることが多い気がします。
どちらかというと、先に availability を check して OK が返ることを確認していない protocol error のイメージの方が近いかもしれません。
There was a problem hiding this comment.
基本的にはここには入ってきてはいけないという意味で Unreachable にしてました
このコメントとしてはProtocol Error に変更してエラーメッセージを変えるべき的なことですかね👀
(私としてもめちゃくちゃ強いこだわりがある訳ではないです！！)
| ) | ||
|
|
||
| try: | ||
| command_server_request: CustomCommandRequest = json.loads(request.data.decode()) |
There was a problem hiding this comment.
json.loads は Python の型をランタイムで確認しないはずなので、ここで保証されるのは json として valid なことまでな気がします。
(agent 側の動作に問題がなければほぼありえないはずですが、id を持っていることまでは確認してもよいかもしれません。)
(あと、ここで落ちる場合は agent がおかしい場合だと思うため、エラーメッセージではユーザーにとっては対処のしようがないということは明示しておきたい気がします)
There was a problem hiding this comment.
json.loads は Python の型をランタイムで確認しないはずなので、ここで保証されるのは json として valid なことまでな気がします。
はい、これは基本的に payload が JSON であるということのみを確認していて, ここで落ちるということは agent の request の形式がおかしいので GENERAL_ERROR として返してます！
id チェックとかしたいならもしかしたら pydantic とか導入した方が幸せな気がしないでもないですね🤔
一旦は可能な限り標準に寄せようとしてますが.
(あと、ここで落ちる場合は agent がおかしい場合だと思うため、エラーメッセージではユーザーにとっては対処のしようがないということは明示しておきたい気がします)
基本的には我々がバグを発見するためのもので, ユーザーに何かを求める用途でないのはそうです.
このエラーメッセージ自体は agent 側から syslog に出るだけとかじゃないんですかね？
そうすると他のエラーメッセージの扱いとあまり違わないような気がするんですが, とはいえエラーメッセージ自体に気にしなくていいよ的なのを書いた方がいいってことですかね？
There was a problem hiding this comment.
と思ったけど、エラーメッセージなんてあっても別に困らないし書いてもいい気がしてきた
なんなら agent が JSON を渡すことを前提に try-catch しないでもありかもしれない.
handle_request のトップレベルに try-catch を書いて GENERAL_ERROR で吸収するようにして
ny-a
left a comment
ny-a
left a comment
There was a problem hiding this comment.
(API response に渡している箇所だけコメントしましたが) Error の種類が入るように repr(e) や f"{e!r}" を使うのはどうでしょうか。
| command_server_request: CustomCommandRequest = json.loads(request.data.decode()) | ||
| except Exception as e: | ||
| return CommandResponse( | ||
| copy.copy(request.id_), Status.GENERAL_ERROR, f"Failed to parse custom command payload: {e}".encode() |
There was a problem hiding this comment.
Error の種類が分かるようにしたいです。
| copy.copy(request.id_), Status.GENERAL_ERROR, f"Failed to parse custom command payload: {e}".encode() | |
| copy.copy(request.id_), Status.GENERAL_ERROR, f"Failed to parse custom command payload: {e!r}".encode() |
| payload = self.custom_command_handler(command_server_request) | ||
| return CommandResponse(copy.copy(request.id_), Status.OK, payload.encode()) | ||
| except Exception as e: | ||
| error_message = str(e) |
There was a problem hiding this comment.
Error の種類が分かるようにしたいです。
| error_message = str(e) | |
| error_message = repr(e) |
3 participants
Check list
Summary