Recreate aiohttp session on connectivity check #5332
Conversation
agners
changed the title
|
This really feels wrong, and I don't understand why it is necessary. Without this, connectivity_check always returns with |
|
I'm guessing its a http keep alive reusing the open connection, and the second request ends up timing out because there is nothing on the other end to send an RST |
|
Maybe make a new session thats only used for connectivity check with a Connector that does |
agners
force-pushed
the
recreate-client-session
branch
from
f0d32b0
to
a8d65c6
Compare
There is no http connection setup here, DNS is non-responsive. So this all hangs already on resolve time.
Due to the above I don't think this will help. However, I've noticed that a separate client session indeed speeds this up. |
agners
force-pushed
the
recreate-client-session
branch
from
a8d65c6
to
c694b7f
Compare
|
There is definitely some weirdness going on in aiohttp or more likely in aiodns: When I drop packets to my DNS server, I get the following stack trace: So far so good, this is expected. When I use this small reproducer: import aiohttp
import asyncio
import datetime
import logging
async def fetch(client):
timeout = aiohttp.ClientTimeout(total=10)
async with client.head("https://checkonline.home-assistant.io/online.txt", timeout=timeout) as resp:
assert resp.status == 200
return await resp.text()
async def main():
async with aiohttp.ClientSession() as client:
while True:
print(datetime.datetime.now())
try:
html = await fetch(client)
print(html)
except Exception as e:
logging.exception(e)
logging.basicConfig(
format='%(asctime)s %(levelname)s %(message)s',
level=logging.INFO,
datefmt='%Y-%m-%d %H:%M:%S'
)
asyncio.run(main())Returns the following: It is a bit weird that sometimes we get a different exception. This does indicate that the previous request is somehow kept around and reused. What is weird though that even when I add another DNS server in my resolver (which fixes resolving) this script continuous to return errors 😢 It really seems as if the aiohttp/aiodns instance used keeps reusing cached or queued resolve attempts. |
agners
force-pushed
the
recreate-client-session
branch
from
c694b7f
to
761acd6
Compare
|
I've tried other approaches here, e.g. removing the timeout (hoping the DNS resolutions wouldn't queue) and lazy checking internet connectivity (hoping we would less overwhelm aiohttp/aiodns). But I did run into the same problem ultimately: Once the aiohttp session returns It feels rather safer to me to recreate the aiohttp session, at least for now. In a way, it does make sense to recreate the session when Internet connectivity changes, it will make sure we start from a clean slate. But maybe it would be better to create separate session for the connectivity check on every check, and only recreate the global one when going from no connectivity -> connectivity. 🤔 |
|
This is starting to feel like a bug in aiohttp. I’m going to spend some time this weekend digging into this more
|
|
It might also have to do with aiohttp’s internal dns cache. Might get better results with disabling that on the check session
|
Hm, at least this change doesn't seem to alter the behavior: diff --git a/supervisor/supervisor.py b/supervisor/supervisor.py
index 126cf739..f3adb795 100644
--- a/supervisor/supervisor.py
+++ b/supervisor/supervisor.py
@@ -8,12 +8,13 @@ from ipaddress import IPv4Address
import logging
from pathlib import Path
from tempfile import TemporaryDirectory
+from types import MappingProxyType
import aiohttp
from aiohttp.client_exceptions import ClientError
from awesomeversion import AwesomeVersion, AwesomeVersionException
-from .const import ATTR_SUPERVISOR_INTERNET, SUPERVISOR_VERSION, URL_HASSIO_APPARMOR
+from .const import ATTR_SUPERVISOR_INTERNET, SERVER_SOFTWARE, SUPERVISOR_VERSION, URL_HASSIO_APPARMOR
from .coresys import CoreSys, CoreSysAttributes
from .docker.stats import DockerStats
from .docker.supervisor import DockerSupervisor
@@ -52,6 +53,11 @@ class Supervisor(CoreSysAttributes):
self.coresys: CoreSys = coresys
self.instance: DockerSupervisor = DockerSupervisor(coresys)
self._connectivity: bool = True
+ conn = aiohttp.TCPConnector(use_dns_cache=False)
+ self._session_con_check = aiohttp.ClientSession(connector=conn)
+ self._session_con_check._default_headers = MappingProxyType(
+ {aiohttp.hdrs.USER_AGENT: SERVER_SOFTWARE}
+ )
async def load(self) -> None:
"""Prepare Supervisor object."""
@@ -270,7 +276,7 @@ class Supervisor(CoreSysAttributes):
"""Check the connection."""
timeout = aiohttp.ClientTimeout(total=10)
try:
- await self.sys_websession.head(
+ await self._session_con_check.head(
"https://checkonline.home-assistant.io/online.txt", timeout=timeout
)
except (ClientError, TimeoutError):Connection check continues to fail 😢 |
It seems to actually get a proper connectivity check run, we need a new vanilla ClientSession() object.
agners
force-pushed
the
recreate-client-session
branch
from
761acd6
to
fc3fa63
Compare
agners
changed the title
📝 WalkthroughWalkthroughThe changes involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant CoreSys
participant Supervisor
participant WebSession
CoreSys->>WebSession: create_websession()
WebSession-->>CoreSys: New session created
Supervisor->>CoreSys: check_connectivity()
alt Connectivity check fails
Supervisor->>CoreSys: recreate web session
CoreSys->>WebSession: create_websession()
WebSession-->>CoreSys: New session created
end
Supervisor-->>CoreSys: Connectivity check result
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
supervisor/supervisor.py (1)
277-278: LGTM! Good solution for addressing connectivity issues.The addition of
self.coresys.create_websession()before settingself.connectivity = Falseis a good approach to ensure a fresh session for subsequent connectivity checks. This change aligns with the PR objectives and should help mitigate issues related to stale connections.Consider adding a log message to indicate when a new websession is created, which could be helpful for debugging:
_LOGGER.debug("Recreating websession due to connectivity check failure") self.coresys.create_websession()supervisor/coresys.py (1)
555-557: Simplify headers by removingMappingProxyTypeThe use of
MappingProxyTypeto wrap the headers dictionary may be unnecessary here. Sinceaiohttp.ClientSessionaccepts a standard dictionary for headers, you can directly pass the dictionary without wrapping it.Apply this diff to simplify the code:
-self._websession: aiohttp.ClientSession = aiohttp.ClientSession( - headers=MappingProxyType({aiohttp.hdrs.USER_AGENT: SERVER_SOFTWARE}) -) +self._websession: aiohttp.ClientSession = aiohttp.ClientSession( + headers={aiohttp.hdrs.USER_AGENT: SERVER_SOFTWARE} +)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- supervisor/coresys.py (3 hunks)
- supervisor/supervisor.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
supervisor/coresys.py (6)
Pattern
*/**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern
*/**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern
*/**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern
*/**(html|markdown|md): - Use sentence-style capitalization also in headings.
Pattern
*/**(html|markdown|md): do not comment on HTML used for icons
Pattern
*/**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.supervisor/supervisor.py (6)
Pattern
*/**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern
*/**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern
*/**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern
*/**(html|markdown|md): - Use sentence-style capitalization also in headings.
Pattern
*/**(html|markdown|md): do not comment on HTML used for icons
Pattern
*/**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
🔇 Additional comments (1)
supervisor/supervisor.py (1)
277-278: Verify impact on overall system performance.While the changes look good and address the immediate issue, it's important to consider the potential impact on the overall system:
- Ensure that recreating the websession doesn't introduce any noticeable delay in the connectivity check process.
- Verify that other parts of the system that might be using the websession are not adversely affected by this change.
- Consider adding integration tests that simulate connectivity issues to ensure the new behavior works as expected in various scenarios.
To help with verification, you can run the following script to check for other uses of
sys_websessionin the codebase:This will help identify any other parts of the codebase that might be affected by the websession recreation.
|
I filed aio-libs/aiohttp#9447 |
Proposed change
It seems to actually get a proper connectivity check run, we need a new vanilla ClientSession() object.
Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit