Fix: re-raise exceptions instead of silently swallowing them - #27
Open
luizbon wants to merge 1 commit into
Open
Conversation
Every wrapper method that calls api_get()/api_post() (getESSList, getSumDataForCustomer, getOneDateEnergyBySn, getLastPowerData, getChargeConfigInfo, getDisChargeConfigInfo, getEvChargerConfigList, and 10 others) caught all exceptions, logged them, and returned None - without re-raising. api_get()/api_post() themselves already do this correctly (log then `raise`), but every caller one level up undid that by catching-and-swallowing again. Consequence: a transient failure (DNS blip, timeout, connection reset) anywhere in these calls comes back to the caller as a plain None instead of an exception. Home Assistant's alphaess integration (coordinator.py) wraps its calls in try/except for aiohttp.ClientConnectorError etc. specifically so HA's DataUpdateCoordinator can mark the update as failed and retry with backoff - but that except block can never fire, because the exception never reaches it. The coordinator treats the None as a normal (if empty) result, so entities can get stuck without ever triggering HA's retry path, sometimes for hours after the underlying network issue has already cleared. This matches CharlesGillanders/homeassistant-alphaESS#110 ("happens at least once a day, a quick reload sorts it") and the traceback in CharlesGillanders/homeassistant-alphaESS#114 ('NoneType' object is not iterable, from getESSList() returning None). PR CharlesGillanders#25 added `is None` guards around the two call sites inside getdata()/authenticate() to stop that traceback, but homeassistant-alphaESS's coordinator.py doesn't call getdata()/ authenticate() - it calls these per-field methods directly, so the guard doesn't apply there, and the underlying swallow is still present for every other caller of these methods. Fix: add `raise` after the existing `logger.error(...)` call in each of the 17 affected methods, matching the pattern already used correctly by api_get, api_post, getdata, authenticate, setbatterycharge, and setbatterydischarge in this same file. Logging behaviour is unchanged; the only difference is the exception now also propagates to the caller.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #26.
Problem
17 wrapper methods (
getESSList,getSumDataForCustomer,getOneDateEnergyBySn,getLastPowerData,getChargeConfigInfo,getDisChargeConfigInfo,getEvChargerConfigList,setEvChargerCurrentsBySn,getEvChargerCurrentsBySn,getEvChargerStatusBySn,remoteControlEvCharger,bindSn,getVerificationCode,unBindSn,updateChargeConfigInfo,updateDisChargeConfigInfo) catch exceptions fromapi_get()/api_post(), log them, and returnNonewithout re-raising — even thoughapi_get/api_postthemselves already correctlyraiseafter logging. Full detail and reproduction in #26.Net effect: any real failure (DNS blip, connection reset, etc.) reaching one of these methods comes back to the caller as
None.homeassistant-alphaESS'scoordinator.pycalls these methods directly and hasexcept (aiohttp.ClientConnectorError, ...)specifically to let Home Assistant'sDataUpdateCoordinatormark a failed update and retry with backoff — but that path can't be reached, because the exception never leaves this library. Observed effect on my own instance: entities stuckunavailablefor 5+ hours after a 3-minute network outage, on a 60-second poll interval, until a manual config-entry reload.Fix
Add
raiseafter the existinglogger.error(f"Error: {e} when calling {resource}")in each of the 17 methods — one line each, no other behavioural change. This matches the pattern already used correctly elsewhere in the same file:api_get,api_post,getdata,authenticate,setbatterycharge,setbatterydischargeall already dologger.error(...); raise.Also bumped
setup.pyto 0.0.20 following the convention from #25.Testing
I don't have
aiohttp/voluptuousinstallable in my current environment (no pip/venv available) to run this live against a real or mocked API failure, so I can't attach a test run. What I did verify:python3 -m py_compile alphaess/alphaess.pypasseslogger.error(f"Error: {e} when calling {resource}")) — confirmed exactly 17 occurrences, all now followed byraisetry/exceptstructure and indentation are unchanged aside from the added line — this is a purely additive change (raisere-raising the currently-caught exception), so there's no new control-flow path introduced, only the existing one becoming reachablegetdata()andauthenticate()'s existingif units is None:guards (from Guard against getESSList() returning None ('NoneType' object is not iterable) #25) are untouched and still meaningful —api_get()can still legitimately returnNoneon a successful-but-empty API response (not just on exceptions), so those guards aren't made redundant by this changeHappy to add a proper mocked test if there's an existing test setup I should target — didn't see one in the repo.