Repository navigation
Wait on _ever_connected in connect(), not _connected - #144
Merged
cdheiser merged 2 commits intoSep 12, 2026
Merged
Conversation
connect() could block forever. It waited on `self._connected or self._done`, but _main_loop sets _connected, notifies, and then reads -- and if that read returns nothing, the loop falls through to _disconnect_locked() and clears the flag again, possibly before the waiting thread reacquires the lock and re-evaluates. _done is False because the loop is retrying, so the predicate is false in both terms and wait_for waits again. Nothing sets _connected back unless a later reconnect happens to be observed by the waiter, so in practice connect() never returns. A controller that accepts a session and drops it is not exotic: it happens at the connection limit, during a reboot, or while another integration session is being torn down. Home Assistant calls connect() from async_setup_entry without an executor, so this hangs its event loop rather than just one thread. _ever_connected latches and is never cleared, which is the property the predicate needs. This weakens what a successful connect() means -- "the initial connection succeeded" rather than "the connection is up right now". That is the only guarantee available: the controller can close the session at any moment, so no predicate here can promise a live connection to the caller. The docstring now says so, and test_thread_start_and_connect asserts _ever_connected, since its own fixture drops the session after one line. The regression test runs connect() on a helper thread so a reappearance fails the test rather than hanging the suite. Closes thecynic#141.
Missed in the previous commit. Like test_thread_start_and_connect, this test's fixture makes readline return b"" so the session drops immediately after login, then asserts _connected right after connect() returns. Under the old predicate that assertion was guaranteed, because connect() could not return until it had observed _connected -- which is the same property that made it hang when the flag was cleared first. Under the new one connect() returns on _ever_connected, so _connected is a race. It passed locally and on 3.10/3.12, and failed on 3.9/3.11/3.13/3.14: a timing race, not a version difference. Assert what connect() actually promises.
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
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.
Closes #141.
connect()waited on the wrong flag and could block forever:_main_loopsets_connected, notifies, then reads. If that read returns nothing the loop falls straight through to_disconnect_locked()and clears the flag again — possibly before the waiting thread reacquires the lock and re-evaluates the predicate._doneisFalsebecause the loop is retrying, so both terms are false andwait_forgoes back to sleep. Nothing sets_connectedagain unless a later reconnect happens to be observed by the waiter, so in practiceconnect()never returns.A controller that accepts a session and immediately drops it is not exotic — the connection limit, a reboot, or another integration's session being torn down all produce it.
The fix is the flag added in #138:
_ever_connectedlatches and is never cleared, which is exactly the property the predicate needs.Why it matters beyond one blocked thread
Home Assistant calls
connect()fromasync_setup_entrywithout wrapping it in an executor, so this does not hang a worker — it hangs HA's event loop, with no timeout and no error. See home-assistant/core#181995.Behaviour change
A successful
connect()now means "the initial connection succeeded", not "the connection is up right now".That is the only guarantee this function can honestly make. The controller may close the session at any moment, including between the connect and the return, so no predicate here can promise the caller a live connection — the old one did not either, it just blocked instead of admitting it. Consumers that need current state should check it rather than infer it from
connect()returning.The docstring now says this explicitly, and
test_thread_start_and_connectasserts_ever_connectedinstead of_connected: its own fixture drops the session after a single line, so it was quietly depending on winning that race.Test
test_connect_returns_when_the_session_drops_immediatelyreproduces it with no hardware —_do_loginsucceeds and the firstreadline()returnsb"".connect()runs on a helper thread so a reappearance fails the test in 10s rather than hanging the suite.Verified by reverting the one-line predicate change: the new test fails on the
returned.wait(10.0)assertion. 83 tests pass,mypyclean.