Allow for a external session. #505
Conversation
📝 WalkthroughWalkthroughAdds optional aiohttp.ClientSession injection and lazy session properties to OpenEVSE and OpenEVSEWebsocket; refactors request flows to reuse a shared session (including firmware_check and websocket init). Tests updated to propagate and verify sessions and converted many fixtures to async with a charger factory. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OpenEVSE
participant OpenEVSEWebsocket
participant aiohttp as "aiohttp.ClientSession"
Client->>OpenEVSE: instantiate(session=None or 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🎯 3 (Moderate) | ⏱️ ~20 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/conftest.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. |
2 participants
This lets us inject the session externally, which homeassistant wants to reduce memory usage.
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.