Repository navigation
Add disconnect() to stop the reader thread and free its resources - #142
Merged
Merged
Conversation
There is no way for a consumer to shut a connection down. Setting _done does nothing on its own: the reader is parked in readline(), which returns only on data or a closed socket, so the thread and its telnet session outlive the caller. The event loop created in __init__ is never closed either, so its selector fd leaks alongside them. This matters downstream. Home Assistant builds a fresh Lutron per config entry setup and has no way to tear the old one down, so every reload of a live entry leaks a thread, a repeater session and a loop, and the orphaned client keeps invoking callbacks on entities that no longer exist (home-assistant/core#181995). Run _main_loop inside a task so shutdown has something to cancel. Cancellation is the mechanism that does the work -- it unblocks a pending readline() and cuts short the reconnect backoff alike -- with the writer closed first so the socket goes away even if the task sits between awaits. run() then closes the loop in a finally, and wakes anyone blocked in connect() rather than leaving them waiting on a thread that has gone. disconnect() is idempotent, safe from any thread but the reader's own, and terminal: a threading.Thread cannot be restarted, so callers wanting a new session construct a new Lutron. Closes thecynic#140.
test_disconnect_interrupts_reconnect_backoff failed on Python 3.9 only. On 3.9 asyncio.Event.__init__ still binds a loop via get_event_loop(), and IsolatedAsyncioTestCase calls set_event_loop(None) on teardown. The disconnect tests are a plain TestCase that runs after those, so the main thread has no current loop by then and constructing the Event raises "There is no current event loop in thread 'MainThread'". 3.10 dropped the binding, which is why 3.10-3.14 passed. Create it inside _do_login instead, which runs on the reader thread's loop, and reach it through a dict once connect() has returned. The other three tests were never affected: their Event is constructed inside the parked coroutine, already on the loop thread. Verified on 3.9.2 with set_event_loop(None) applied first: the old form raises, the new form wakes its waiter and the thread exits.
Both found by an independent review of the branch. The loop-closed check was a TOCTOU. is_alive() can be true while run()'s finally is already closing the loop, so call_soon_threadsafe races closure and raises "Event loop is closed" out of disconnect(). Guarding with is_closed() first only narrows the window. Catch the RuntimeError instead: if the reader has already gone there is nothing left to wake. In Home Assistant this escaped into async_unload_entry and failed the unload, on exactly the path taken when something had already gone wrong. disconnect() from the reader thread itself -- a receive callback asking to shut down, which is a reasonable thing for a consumer to do -- reached self.join() and raised "cannot join current thread". Schedule the shutdown as usual, then return instead of joining; the task unwinds on its own. The docstring promised this was unsupported, but the failure mode was an exception in the consumer's callback rather than anything graceful. Tests for both, each verified by reverting the guard it covers. The reader-thread test holds its callback until connect() has returned, because disconnecting sooner trips the separate connect() predicate race in thecynic#141.
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 #140.
The problem
There is no way to shut a connection down.
_donedoes nothing on its own — the reader is parked inreadline(), which returns only on data or a closed socket:So the thread and its telnet session outlive the caller, and the event loop built in
__init__is never closed either, leaking its selector fd alongside them.Downstream this is the blocker for home-assistant/core#181995 (3). Home Assistant constructs a fresh
Lutronper config entry setup and has no way to tear the old one down, so every reload of a live entry leaks a thread, a repeater session and a loop — and the orphaned client keeps invoking callbacks on entities that no longer exist. That one cannot be fixed on the HA side.The change
Lutron.disconnect(timeout=5.0)andLutronConnection.disconnect(timeout=5.0). Idempotent, safe from any thread except the reader's own, and blocking until the thread exits or the timeout elapses.Three pieces make it work:
_main_loopruns inside_runner(), so shutdown has a task to cancel. Cancellation is the mechanism that does the work: it unblocks a pendingreadline()and cuts short the reconnect backoff alike. The writer is closed first so the socket goes away even if the task sits between awaits.run()closes the loop in afinally, afterrun_until_completereturns — it has to happen on the reader thread, which is the only one that may touch it.run()also wakes anyone blocked inconnect()on the way out, rather than leaving them waiting on a thread that is gone.disconnect()is terminal: athreading.Threadcannot be restarted, so callers wanting a new session construct a newLutron. The docstrings say so.HA's side then becomes:
Tests
Four, driving the real thread rather than
_main_loopdirectly, because the point is the cross-thread handshake. Each was verified by reverting the thing it covers:_main_task.cancel()test_disconnect_stops_thread_and_closes_loop,test_disconnect_is_idempotentself._loop.close()test_disconnect_stops_thread_and_closes_loopis_alive()guard beforejoin()test_disconnect_without_connect(RuntimeError)self._writer.close()test_disconnect_stops_thread_and_closes_loopMeasured: disconnecting a live connection and disconnecting mid-backoff both complete in 0.00s, with no threads left behind.
79 tests pass,
mypyclean.Notes
An earlier draft had more machinery. I had an
asyncio.Eventand await_forto make the 5s backoff interruptible. Reverting that to a plainasyncio.sleep(5)changed no test result — cancelling the task already interrupts the sleep — so it came out. The backoff comment now says why the plain sleep is fine.Related but deliberately not included: #141.
connect()can block forever if the session drops between the connect and the waiter re-checking its predicate. It is a separate pre-existing bug with a one-line fix, and it changes what a successfulconnect()guarantees, so it deserves its own review rather than riding along here. Worth noting the two interact: with #141 unfixed, a consumer can still hang inconnect()before it ever gets the chance to calldisconnect().Still open for HA: marking entities unavailable during an outage needs the
connectedproperty and connection-state callback from #140, which this PR does not add. Happy to follow up with those if the shape here looks right.