Conversation
📝 WalkthroughWalkthroughAdds optional aiohttp.ClientSession injection and lazy session properties to OpenEVSE and OpenEVSEWebsocket; HTTP flows (process_request, firmware_check) and websocket initialization now reuse a shared session. Tests converted to async, propagate session through a new charger factory, and add session lifecycle/unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OpenEVSE
participant OpenEVSEWebsocket
participant aiohttp as "aiohttp.ClientSession"
Client->>OpenEVSE: instantiate(host, ..., session=None|provided)
alt session not provided
OpenEVSE-->>aiohttp: lazily create session on first access
else session provided
OpenEVSE-->>aiohttp: use provided session
end
Client->>OpenEVSE: update()
OpenEVSE->>aiohttp: HTTP requests via self.session (process_request / firmware_check)
OpenEVSE->>OpenEVSEWebsocket: init(..., session=self.session)
OpenEVSEWebsocket->>aiohttp: use provided session for websocket connection
Client->>OpenEVSEWebsocket: close()
OpenEVSEWebsocket->>OpenEVSEWebsocket: close internal _client (do not close shared session)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Pylint (4.0.4)tests/test_main.pyThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openevsehttp/websocket.py (1)
28-55:⚠️ Potential issue | 🟠 MajorClose internally created aiohttp sessions when the websocket is stopped.
The
sessionproperty lazily creates aClientSessionwhen none is provided, butclose()only closes_client. This leaves internally created sessions open, causing resource leaks and "unclosed session" warnings when usingOpenEVSEWebsocketdirectly. Track session ownership and close only when this instance created it.🔧 Proposed fix (track ownership and close owned sessions)
class OpenEVSEWebsocket: @@ def __init__( self, server, callback, user=None, password=None, session=None, ): """Initialize a OpenEVSEWebsocket instance.""" self._session = session + self._owns_session = session is None @@ `@property` def session(self): """Return the aiohttp session, creating one if needed.""" if self._session is None: self._session = aiohttp.ClientSession() + self._owns_session = True return self._session @@ async def close(self): """Close the listening websocket.""" await OpenEVSEWebsocket.state.fset(self, STATE_STOPPED) if self._client: await self._client.close() + if self._owns_session and self._session and not self._session.closed: + await self._session.close()openevsehttp/__main__.py (1)
87-114:⚠️ Potential issue | 🟠 MajorAdd cleanup method for internally-created aiohttp session.
OpenEVSEandOpenEVSEWebsocketboth lazily create and retainClientSessioninstances when none is provided, but neither class exposes a cleanup method. This causes socket/connector leaks in short-lived integrations (tests, CLI scripts). Tracking session ownership is necessary sinceOpenEVSEWebsocketreceivesself.sessionfromOpenEVSEand must not close sessions it doesn't own.Recommend adding an
_owns_sessionflag and asyncclose()method toOpenEVSEto release the websocket and any internally-created session.🔧 Proposed fix (owning session + close method)
def __init__( self, host: str, user: str = "", pwd: str = "", session: aiohttp.ClientSession | None = None, ) -> None: @@ self.url = f"http://{host}/" self._session = session + self._owns_session = session is None @@ `@property` def session(self) -> aiohttp.ClientSession: """Return the aiohttp session, creating one if needed.""" if self._session is None: self._session = aiohttp.ClientSession() + self._owns_session = True return self._session + + async def close(self) -> None: + """Close websocket and any owned aiohttp session.""" + if self.websocket: + await self.websocket.close() + if self._owns_session and self._session and not self._session.closed: + await self._session.close()
|
Thanks man, this was on my todo list lol |
firstof9
left a comment
firstof9
left a comment
There was a problem hiding this comment.
Can you run black on this to fix the formatting? I haven't swapped to ruff yet on this one.
|
@firstof9 Done |
|
I'll try to update the tests this week as well. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/conftest.py (1)
31-65:⚠️ Potential issue | 🟡 MinorAdd
noqa: S106comments to suppress hardcoded-password lints in test fixtures.S106 is flagged at lines 50 and 64 for
pwd="fakepassword". Since ruff's bandit rules (S) are enabled in the project configuration and S106 is not suppressed forconftest.py, these violations will fail CI. Add an inlinenoqa: S106comment to each return statement.Suggested fix
- return main.OpenEVSE(TEST_TLD, user="testuser", pwd="fakepassword", session=session) + return main.OpenEVSE( + TEST_TLD, + user="testuser", + pwd="fakepassword", # noqa: S106 - test credential + session=session, + )openevsehttp/__main__.py (1)
87-113:⚠️ Potential issue | 🟠 MajorAdd explicit shutdown for internally created sessions.
self.sessionnow lazily creates aClientSession, but there is no teardown path. This can leak connectors on long-running processes or reloads (e.g., Home Assistant). Track ownership and add aclose()method that shuts down the websocket and internal session only.✅ Suggested fix
class OpenEVSE: @@ def __init__( self, host: str, user: str = "", pwd: str = "", session: aiohttp.ClientSession | None = None, ) -> None: @@ self.url = f"http://{host}/" self._session = session + self._owns_session = False @@ `@property` def session(self) -> aiohttp.ClientSession: """Return the aiohttp session, creating one if needed.""" if self._session is None: self._session = aiohttp.ClientSession() + self._owns_session = True return self._session + + async def close(self) -> None: + """Close websocket and any internally created session.""" + if self.websocket: + await self.websocket.close() + if self._owns_session and self._session and not self._session.closed: + await self._session.close()
🤖 Fix all issues with AI agents
In `@openevsehttp/websocket.py`:
- Around line 49-54: The session property lazily creates an
aiohttp.ClientSession but doesn't track ownership, causing a connection-pool
leak because close() only closes _client; add an _owns_session boolean on
OpenEVSEWebsocket, set _owns_session = True when session() creates a new
aiohttp.ClientSession and set _owns_session = False when an external session is
injected or passed into __init__, then update the close() method to await/close
self._session only if self._session is not None and self._owns_session is True
(and still close self._client as before); ensure any assignment paths to
self._session set the ownership flag appropriately.
In `@tests/test_main.py`:
- Around line 346-355: The test_get_ir_temperature function currently ignores
the parametrized expected value causing ARG001; modify the assertion to compare
charger.ir_temperature to the expected parameter (replace the hardcoded "assert
status is None" with "assert status == expected") so the test uses the provided
expected values; keep the surrounding calls (charger.update() and
charger.ws_disconnect()) as-is and ensure the test signature (async def
test_get_ir_temperature(charger, expected)) remains unchanged.
| @property | ||
| def session(self): | ||
| """Return the aiohttp session, creating one if needed.""" | ||
| if self._session is None: | ||
| self._session = aiohttp.ClientSession() | ||
| return self._session |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Get the file structure and size
wc -l openevsehttp/websocket.pyRepository: firstof9/python-openevse-http
Length of output: 102
🏁 Script executed:
# Find all close methods and session cleanup in the file
grep -n "def close\|async def close\|__aexit__\|_session" openevsehttp/websocket.py | head -30Repository: firstof9/python-openevse-http
Length of output: 262
🏁 Script executed:
# Look for session-related patterns
rg -n "session.*close|close.*session" openevsehttp/ --type pyRepository: firstof9/python-openevse-http
Length of output: 55
🏁 Script executed:
# Check for context manager implementation
rg -n "__aenter__|__aexit__" openevsehttp/ --type pyRepository: firstof9/python-openevse-http
Length of output: 55
🏁 Script executed:
sed -n '30,60p' openevsehttp/websocket.pyRepository: firstof9/python-openevse-http
Length of output: 909
🏁 Script executed:
sed -n '145,165p' openevsehttp/websocket.pyRepository: firstof9/python-openevse-http
Length of output: 880
Track ownership and close internally created sessions.
The session property lazily creates an aiohttp.ClientSession without tracking ownership, but close() only closes _client and never closes _session. If OpenEVSEWebsocket is used directly without injecting a session, the internally created session's connection pool leaks and is never cleaned up. Add an _owns_session flag to track when you create the session internally, and close it only in that case.
Suggested fix
class OpenEVSEWebsocket:
def __init__(
self,
server,
callback,
user=None,
password=None,
session=None,
):
"""Initialize a OpenEVSEWebsocket instance."""
self._session = session
+ self._owns_session = False
`@property`
def session(self):
"""Return the aiohttp session, creating one if needed."""
if self._session is None:
self._session = aiohttp.ClientSession()
+ self._owns_session = True
return self._session
async def close(self):
"""Close the listening websocket."""
await OpenEVSEWebsocket.state.fset(self, STATE_STOPPED)
if self._client:
await self._client.close()
+ if self._owns_session and self._session and not self._session.closed:
+ await self._session.close()🤖 Prompt for AI Agents
In `@openevsehttp/websocket.py` around lines 49 - 54, The session property lazily
creates an aiohttp.ClientSession but doesn't track ownership, causing a
connection-pool leak because close() only closes _client; add an _owns_session
boolean on OpenEVSEWebsocket, set _owns_session = True when session() creates a
new aiohttp.ClientSession and set _owns_session = False when an external session
is injected or passed into __init__, then update the close() method to
await/close self._session only if self._session is not None and
self._owns_session is True (and still close self._client as before); ensure any
assignment paths to self._session set the ownership flag appropriately.
| "charger, expected", | ||
| [("test_charger", None), ("test_charger_v2", None)], | ||
| indirect=["charger"], | ||
| ) | ||
| async def test_get_ir_temperature(fixture, expected, request): | ||
| async def test_get_ir_temperature(charger, expected): | ||
| """Test v4 Status reply.""" | ||
| charger = request.getfixturevalue(fixture) | ||
| await charger.update() | ||
| status = charger.ir_temperature | ||
| assert status is None | ||
| await charger.ws_disconnect() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for ARG001 warnings in the test file
rg -n "def test_get_ir_temperature" tests/test_main.py -A 10Repository: firstof9/python-openevse-http
Length of output: 435
🏁 Script executed:
#!/bin/bash
# Run ruff check to confirm ARG001 warning
ruff check tests/test_main.py --select ARG001Repository: firstof9/python-openevse-http
Length of output: 1466
Use the parametrized expected value (ARG001).
The expected parameter is unused and triggers an ARG001 lint error. Change the assertion to use it:
Suggested fix
async def test_get_ir_temperature(charger, expected):
"""Test v4 Status reply."""
await charger.update()
status = charger.ir_temperature
- assert status is None
+ assert status == expected
await charger.ws_disconnect()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "charger, expected", | |
| [("test_charger", None), ("test_charger_v2", None)], | |
| indirect=["charger"], | |
| ) | |
| async def test_get_ir_temperature(fixture, expected, request): | |
| async def test_get_ir_temperature(charger, expected): | |
| """Test v4 Status reply.""" | |
| charger = request.getfixturevalue(fixture) | |
| await charger.update() | |
| status = charger.ir_temperature | |
| assert status is None | |
| await charger.ws_disconnect() | |
| "charger, expected", | |
| [("test_charger", None), ("test_charger_v2", None)], | |
| indirect=["charger"], | |
| ) | |
| async def test_get_ir_temperature(charger, expected): | |
| """Test v4 Status reply.""" | |
| await charger.update() | |
| status = charger.ir_temperature | |
| assert status == expected | |
| await charger.ws_disconnect() |
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 350-350: Unused function argument: expected
(ARG001)
🤖 Prompt for AI Agents
In `@tests/test_main.py` around lines 346 - 355, The test_get_ir_temperature
function currently ignores the parametrized expected value causing ARG001;
modify the assertion to compare charger.ir_temperature to the expected parameter
(replace the hardcoded "assert status is None" with "assert status == expected")
so the test uses the provided expected values; keep the surrounding calls
(charger.update() and charger.ws_disconnect()) as-is and ensure the test
signature (async def test_get_ir_temperature(charger, expected)) remains
unchanged.
|
Oh boy, something's really wonky here. |
|
Looks like a rebase is needed. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openevsehttp/__main__.py (1)
87-113:⚠️ Potential issue | 🟠 MajorAdd ownership tracking and explicit close method for internally created sessions.
The OpenEVSE instance can create an aiohttp.ClientSession internally via the session property, but has no mechanism to close it. This causes connection pool leaks, especially when the websocket is never initialized (only HTTP requests are made). While OpenEVSEWebsocket.close() does close its session, OpenEVSE itself lacks cleanup logic. Add an
_owns_sessionflag to track internal session creation and provide anasync close()method that closes only owned sessions, leaving injected sessions to callers.Suggested implementation
def __init__( self, host: str, user: str = "", pwd: str = "", session: aiohttp.ClientSession | None = None, ) -> None: """Connect to an OpenEVSE charger equipped with wifi or ethernet.""" self._user = user self._pwd = pwd self.url = f"http://{host}/" self._session = session + self._owns_session = False self._status: dict = {} self._config: dict = {} `@property` def session(self) -> aiohttp.ClientSession: """Return the aiohttp session, creating one if needed.""" if self._session is None: self._session = aiohttp.ClientSession() + self._owns_session = True return self._session + + async def close(self) -> None: + """Close websocket and owned session.""" + if self.websocket: + await self.websocket.close() + if self._owns_session and self._session and not self._session.closed: + await self._session.close()
🧹 Nitpick comments (1)
openevsehttp/__main__.py (1)
130-180: Preserve tracebacks withlogging.exception+ bareraise.
This keeps the original stack trace while still logging the error message.♻️ Proposed tweak
- except (TimeoutError, ServerTimeoutError) as err: - _LOGGER.error("%s: %s", ERROR_TIMEOUT, url) - raise err - except ContentTypeError as err: - _LOGGER.error("Content error: %s", err.message) - raise err + except (TimeoutError, ServerTimeoutError): + _LOGGER.exception("%s: %s", ERROR_TIMEOUT, url) + raise + except ContentTypeError as err: + _LOGGER.exception("Content error: %s", err.message) + raise
2 participants
This lets us inject the session externally, which homeassistant wants to reduce memory usage.
Summary by CodeRabbit
New Features
Improvements
Tests