Fix elkm1 connection cleanup on setup failure #157208
Conversation
|
Hey there @gwww, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the elkm1 integration's connection synchronization logic to fix cleanup issues on setup failure. The main change converts the async_wait_for_elk_to_sync function into an ElkSyncWaiter class that uses futures with call_later for timeout handling instead of asyncio.timeout.
Key changes:
- Refactored synchronization logic from function to
ElkSyncWaiterclass with futures-based timeout handling - Replaced
asyncio.timeoutcontext manager withcall_laterto avoid CancelledError propagation - Moved disconnect logic from callbacks to finally blocks (with issues noted below)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| homeassistant/components/elkm1/init.py | Refactored async_wait_for_elk_to_sync function into ElkSyncWaiter class; updated async_setup_entry to use new class |
| homeassistant/components/elkm1/config_flow.py | Updated import and usage to use new ElkSyncWaiter class |
Comments suppressed due to low confidence (1)
homeassistant/components/elkm1/init.py:306
- Missing cleanup in
async_setup_entryfor all failure paths. The elk connection established at line 262 is not properly cleaned up when setup fails. There are several failure paths without cleanup:
-
Login failure (line 283): When
ElkSyncWaiter.async_wait()returnsFalse(login failed), no disconnect is called either by the waiter or by this function, leaving the connection open. -
Exceptions after successful sync (lines 287-304): If an exception occurs after sync succeeds (e.g., when accessing
elk.panel.temperature_unitsat line 287), the elk connection is not disconnected. -
Timeout (line 285): This case is handled correctly by
ElkSyncWaiterdisconnecting in its finally block before raising TimeoutError.
This incomplete cleanup causes the "multiple competing Elk instances" issue mentioned in the PR description.
Recommendation: Add try-finally block to ensure cleanup on all failure paths:
elk = Elk({...})
elk.connect()
try:
# ... keypad setup ...
if not await ElkSyncWaiter(elk, LOGIN_TIMEOUT, SYNC_TIMEOUT).async_wait():
return False
# ... rest of setup (lines 287-305) ...
entry.runtime_data = ELKM1Data(elk=elk, ...)
await hass.config_entries.async_forward_entry_setups(entry, PLATFORMS)
return True
except Exception:
# Clean up on any failure
elk.disconnect()
raiseNote: This assumes ElkSyncWaiter will handle its own cleanup for timeout cases, which it currently does at line 385.
elk.connect()
def _keypad_changed(keypad: Element, changeset: dict[str, Any]) -> None:
if (keypress := changeset.get("last_keypress")) is None:
return
hass.bus.async_fire(
EVENT_ELKM1_KEYPAD_KEY_PRESSED,
{
ATTR_KEYPAD_NAME: keypad.name,
ATTR_KEYPAD_ID: keypad.index + 1,
ATTR_KEY_NAME: keypress[0],
ATTR_KEY: keypress[1],
},
)
for keypad in elk.keypads:
keypad.add_callback(_keypad_changed)
try:
if not await ElkSyncWaiter(elk, LOGIN_TIMEOUT, SYNC_TIMEOUT).async_wait():
return False
except TimeoutError as exc:
raise ConfigEntryNotReady(f"Timed out connecting to {conf[CONF_HOST]}") from exc
elk_temp_unit = elk.panel.temperature_units
if elk_temp_unit == "C":
temperature_unit = UnitOfTemperature.CELSIUS
else:
temperature_unit = UnitOfTemperature.FAHRENHEIT
config["temperature_unit"] = temperature_unit
prefix: str = conf[CONF_PREFIX]
auto_configure: bool = conf[CONF_AUTO_CONFIGURE]
entry.runtime_data = ELKM1Data(
elk=elk,
prefix=prefix,
mac=entry.unique_id,
auto_configure=auto_configure,
config=config,
keypads={},
)
await hass.config_entries.async_forward_entry_setups(entry, PLATFORMS)
return True
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| _LOGGER.error("ElkM1 login failed for %s", conf[CONF_HOST]) | ||
| return False |
There was a problem hiding this comment.
in the future we could raise ConfigEntryAuthFailed but this integration doesn't support reauth right now
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
|
Would you be able to include a bump to the elkm1_lib in just a few moments. I have cleaned up a log message. |
We need to do that in another PR (lib bumps only allow code changes in the same PR to handle breaking changes), however if you open it today, should be no problem getting it into beta on Wednesday |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
|
The bump PR is there now. Big ❤️ for the PR. |
|
thanks |
Proposed change
Fix elkm1 integration failing to clean up connection when setup is cancelled or times out, which caused multiple competing Elk instances and connection instability.
Changes:
ElkSyncWaiterclass to avoid nonlocal variablesLoginFailedexception for explicit login failure handlingcall_laterfor timeout handling instead ofasyncio.timeoutto avoidCancelledErrorpropagation issues from elkm1-lib's internal task cancellationelk.disconnect()is called when any step fails (timeout, login failure, cancellation)remove_handlerin finally block to prevent leaksType of change
Additional information
Checklist
ruff format homeassistant tests)