Make check_port an async function #4677
Conversation
agners
added
the
refactor
|
Tweaked it a bit #4678 |
agners
force-pushed
the
use-asyncio-check-port
branch
from
192d473 to
79d10dd
Compare
|
I am not very happy how I have to handle the The current change converts the property to a async get method which takes the I/O out of the asyncio event loop. But when the port is exactly determinated is still very implicit. There is a tradeoff of reserving ports unnecessarily early and doing port reservation as late as possible. Ideally I think the port should be determined on first add-on startup. I don't think we need to clear the port on stop. But this would mean that the URL will not be valid until the add-on gets started, not sure if we handle that properly. In any case, this can be done in a follow-up PR I think. |
Its definitely not ideal. The property having the side effect of allocating a port was a bit scarier to me since it was non-obvious what was going on under the hood.
It seems like it should be refactored into a PortManager class or something similar that knows about all the ports
That makes more sense to me, I don't think there is a risk we run out of ports. I think this is a good change but a followup to refactor the ingress_port area would make me feel a lot better about it |
|
I'll throw this on my production now to make sure nothing breaks |
Looks like there might be an issue with the url construction |
|
|
There was a problem hiding this comment.
Looks like there is a missing await
diff --git a/supervisor/api/ingress.py b/supervisor/api/ingress.py
index 75179361..1817071a 100644
--- a/supervisor/api/ingress.py
+++ b/supervisor/api/ingress.py
@@ -201,7 +201,7 @@ class APIIngress(CoreSysAttributes):
session_data: IngressSessionData | None,
) -> web.Response | web.StreamResponse:
"""Ingress route for request."""
- url = self._create_url(addon, path)
+ url = await self._create_url(addon, path)
source_header = _init_header(request, addon, session_data)
# Passing the raw stream breaks requests for some webservers
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
testing this on my production now |
|
|
strace of strace of |
|
Both do a 0.5s poll, the async version also does a 0s poll (returns right away as well) and has to add/remove the epoll, but the async version doesn't have the executor overhead |
|
I guess another question is why do the check here https://github.com/home-assistant/supervisor/pull/4677/files#diff-063212876158cc6d68b7582cea3335ca047c893162509e7f1ee1a1bfa89abf2eR143 Maybe it would make sense to do the api calls with a shorter https://docs.aiohttp.org/en/stable/client_reference.html#aiohttp.ClientTimeout ? |
I guess the intention of the code was to fail faster when the port is still closed? But I'd expect that aiohttp returns quickly in that case too no? I do agree I think we should remove the port check in that case. |
|
Could we maybe do that change in a separate PR? I am a bit scared that the change will subtle change the behavior of |
This requires to change the ingress_port property to a async method.
mdegat01
force-pushed
the
use-asyncio-check-port
branch
from
dde9b06 to
5301baf
Compare
Proposed change
This makes
check_port()an async function. This also requires to change the ingress_port property to a async method.Type of change
Additional information
Checklist
black --fast supervisor tests)If API endpoints of add-on configuration are added/changed: