Webhost: Added ability to clamp port range and retry mechanism #4902
Conversation
github-actions
bot
added
the
affects: webhost
WebHostLib/customserver.py
Outdated
| for room_port_try in range(room_port_alloc_tries): | ||
| next_port = get_random_port(room_port_min, room_port_max) | ||
| try: | ||
| ctx.server = websockets.serve( |
There was a problem hiding this comment.
i would definitely move the loop outside instead of having the duplicate code here
There was a problem hiding this comment.
Not sure I've resolved this, but I've made two separate methods - one attempting to start the server and returning the port (start_ws_server), and another to do the room port retrying and overflow-pooling (start_room_server).
There was a problem hiding this comment.
I've been thinking maybe something like this
attempts = 0
port: int | None = None
while port is None:
attempts += 1
if attempts > max_attempt:
if not allow_fallback:
raise e
else:
port = 0 # ignore range
else:
port = get_random_port(...)
try:
...
except ... as e:
if port = 0:
raise
port = None # try another port… to a tuple and moved port-fetching code to separate method calls
|
@black-sliver I realise that there's a large flaw in allowing 0 to be a port when passed to If Does this matter? At the moment, we only keep the IPv4 port against the Room, and not the IPv6 port, so whilst the IPv6 port will be bound, it won't be shown to the user - but it might also lead to connection oddities. |
…"max_attempts" room config
|
At the moment archipelago.gg is only reachable on IPv4, so this is a future us problem :-D -- I'll have a bunch more nitpicks, but I don't have time to do a proper review at the moment. I'll hope to get to it later tonight. |
| @@ -171,8 +170,8 @@ def get_save(self) -> dict: | |||
| return d | |||
|
|
|||
|
|
|||
| def get_random_port(): | |||
| return random.randint(49152, 65535) | |||
| def get_random_port(room_ports_config: dict): | |||
There was a problem hiding this comment.
Type hint for dict takes key and value types
| def get_random_port(room_ports_config: dict): | |
| def get_random_port(room_ports_config: dict[str, typing.Any]): |
| async def start_room(room_id): | ||
| with Locker(f"RoomLocker {room_id}"): | ||
| try: | ||
| logger = set_up_logging(room_id) | ||
| ctx = WebHostContext(static_server_data, logger) | ||
| ctx.load(room_id) | ||
| ctx.load(room_id, room_ports_config) |
There was a problem hiding this comment.
Semantically, I think it makes no sense to pass room_ports_config into load. I'd suggest to move it to be an attribute of the context (pass it into WebHostContext's __init__ in the line above.
| def run_server_process(name: str, ponyconfig: dict, | ||
| room_ports_config: dict, | ||
| static_server_data: dict, | ||
| cert_file: typing.Optional[str], cert_key_file: typing.Optional[str], | ||
| host: str, rooms_to_run: multiprocessing.Queue, rooms_shutting_down: multiprocessing.Queue): |
There was a problem hiding this comment.
This is getting out of hand 😅
| def run_server_process(name: str, ponyconfig: dict, | |
| room_ports_config: dict, | |
| static_server_data: dict, | |
| cert_file: typing.Optional[str], cert_key_file: typing.Optional[str], | |
| host: str, rooms_to_run: multiprocessing.Queue, rooms_shutting_down: multiprocessing.Queue): | |
| def run_server_process( | |
| name: str, | |
| ponyconfig: dict[str, typing.Any], | |
| room_ports_config: dict, | |
| static_server_data: dict, | |
| cert_file: typing.Optional[str], | |
| cert_key_file: typing.Optional[str], | |
| host: str, | |
| rooms_to_run: multiprocessing.Queue, | |
| rooms_shutting_down: multiprocessing.Queue, | |
| ): |
| "max_attempts": 20, | ||
| # "overflow" controls whether, once reaching the limit of retried ports available, | ||
| # the room port numbers will go to a port number potentially outside of "min" and "max". | ||
| "overflow": True |
There was a problem hiding this comment.
I think this name is not great. Maybe call it fallback_to_zero or fallback_to_auto or something?
I also just realized that the you did not add it to docs/webhost configuration sample.yaml. Would be great for easier configuration.
|
@black-sliver Thanks for the feedback! I'm going to be busy for the next couple of weeks, but I'll look to get this done ASAP. |
What is this fixing or adding?
This tries to add a new config option,
"ROOM_PORTS", with the following options:MIN,MAX)ALLOC_TRIES)websocketsbehaviour on/off, if configured (OVERFLOW)How was this tested?
Tested locally, with a far tighter port range (5 open ports). I would like to add unit tests, if reviewer(s) believe this would be useful.
If this makes graphical changes, please attach screenshots.
N/A
If direct communication would be easier to assist, please reach out on the Archipelago Discord server, as I may misunderstand multiple times. This is also the first time working on a public Python repo, and my knowledge of the project's structure is not the best.