Monitor: reliable non-blocking reconnection + bump to v1.20.0#724
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves the experimental Monitor’s reliability by making it “own” device connection lifecycle (fail-fast sends, explicit disconnect detection, safer command dispatch during reconnect) and rolls the release forward to v1.20.0 with corresponding release notes and new offline unit tests.
Changes:
- Make monitored devices fail-fast (
socketRetryLimit = 0) and route heartbeat/command send failures through Monitor’s disconnect → auto-reconnect path. - Improve reconnect/shutdown behavior via an interruptible backoff wait and selector-registration gating (
state.fileno) for queued commands. - Bump version to 1.20.0, add release rollup entry, and add initial offline
Monitorreliability tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
tinytuya/core/Monitor.py |
Reworks reconnection/send failure handling to avoid blocking the reactor and to keep selector registration authoritative. |
tinytuya/core/core.py |
Bumps library version tuple to 1.20.0. |
tests.py |
Adds new offline unit tests for Monitor reliability behaviors and helper _accepts_nowait. |
RELEASE.md |
Adds v1.20.0 rollup entry including Monitor reliability, security, and bug-fix bullets. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Close all device sockets and hand each device back with its own | ||
| # retry limit restored. | ||
| for state in list(self._devices.values()): | ||
| device = state.device | ||
| fd = state.fileno |
| def _get_socket(self, renew): | ||
| s, peer = _socket_mod.socketpair() | ||
| self.socket = s | ||
| self._peer = peer | ||
| return True |
jasonacox-sam
left a comment
There was a problem hiding this comment.
Review — PR #724: Monitor Reliability + v1.20.0
This is a well-architected fix for the core threading issues in Monitor. Reviewed all 4 changed files.
Design — ✅ Sound
The central insight is correct: Monitor must own the connection lifecycle for watched devices. Letting Device.heartbeat() run its own retry loop on the reactor thread was the root cause of both the stall (blocking all devices) and the silent socket leak (new socket the selector never watches).
The implementation is clean:
- Fail-fast enforcement (
socketRetryLimit = 0) is applied atadd()and restored atremove()/stop(). Saved limit is stored in_DeviceState.saved_retry_limit. - Post-heartbeat socket check catches the fail-fast case where the device closed its socket and returned an error dict instead of raising. This is the right pattern — the device's contract is "socket is None means disconnected."
- Command dispatch gating on
state.filenocloses the reconnect handshake race thatdevice.socketchecking couldn't.filenois only set after the connector hands off and the reactor registers the selector entry. _accepts_nowaitintrospection is the right fix for the proxy issue — checking the signature at call time rather than hardcoding which methods takenowait.
Threading — ✅ Safe
_stop_event(threading.Event) replacingtime.sleepin the connector loop is correct —stop()now returns in ~0.2s instead of up toreconnect_backoff + 5s.- Double-teardown guard in
_handle_disconnect(state.fileno is Noneearly return) prevents the race where both a recv error and a heartbeat failure fire for the same device in one reactor tick. - Unregistering by stored fd rather than
device.socketis correct — the socket may already be closed, makingfileno()invalid.
Tests — ✅ Good coverage
The _FakeDevice + TestMonitorReliability suite covers the key behaviors: fail-fast enforcement, heartbeat → disconnect routing, command gating on active state, nowait proxy behavior, and the _accepts_nowait helper.
Copilot findings — both valid
-
stop()doesn't restore retry limit for disconnected devices (still in_id_to_statebut not in_devices): agree this is a gap. Those devices left in_id_to_stateatstop()time had theirsocketRetryLimitforced to 0 and won't get it restored. An easy follow-up would be iterating_id_to_state.values()alongside_devices.values()instop(). -
_FakeDevice._peerfd leak: valid test-hygiene issue. Adding a__del__or cleanup hook to close_peerwould prevent flakes on low-fd systems.
Neither is a blocker — both are minor follow-up items. The core logic is solid.
Version bump — ✅ 1.20.0
Appropriate — this batch includes the Monitor rework (this PR), security fixes (#722), and bug fixes (#723).
Verdict: APPROVED. The threading model is correct, the tests prove it, and the interruptible shutdown is a real UX improvement.
— Sam ⚙️
|
Thanks @jasonacox-sam - can you address the copilot reviews and also test this PR against hardware? Perhaps a scan and exercise a status query of some of the various device versions? |
|
Addressed both Copilot findings and ran the full hardware battery. Results below. Copilot finding 1 —
|
| Device | IP | DPS |
|---|---|---|
| Kitchen Light | <LAN_IP> |
{'1': False, '9': 0} |
| Back Patio | <LAN_IP> |
{'20': True, '21': 'colour', '22': 1000, ...} (RGB bulb) |
| Chandelier | <LAN_IP> |
{'1': False, '3': 167, '101': False} |
| Front Porch | <LAN_IP> |
{'1': True, '9': 0} |
| Master Bedroom | <LAN_IP> |
{'1': False, '9': 0} |
| Guest Room Light | <LAN_IP> |
{'1': True, '7': 0, '14': 'memory', ...} |
| Tower Fan | <LAN_IP> |
{'1': True, '11': 0} |
| Sofa Lamp | <LAN_IP> |
{'1': True, '9': 0} |
All 8/8 responded correctly — outlet devices ({'1': bool}), RGB bulb (multi-DPS colour state), fan, and a power-monitoring outlet.
Hardware — Monitor end-to-end (3 devices, 10s run)
[connect] Kitchen Light error=None
[connect] Dining Room error=None
[connect] Back Patio error=None
[disconnect] Dining Room error=[Errno 104] Connection reset by peer
[connect] Dining Room error=None ← auto-reconnect fired
stop() latency: 0.00s
One device hit a real Connection reset by peer during the 10s window — the auto-reconnect detected it and brought it back cleanly. stop() returned in <0.01s (interruptible backoff doing its job).
Branch is ready for merge whenever you are. 🙏
— Sam ⚙️
|
@jasonacox-sam Can you help address the merge conflicts? |
….20.0 The experimental Monitor delegated sends to Device.heartbeat()/commands, whose internal retry loop would, on a broken connection, (a) block the single reactor thread for up to socketRetryLimit x (connect timeout + delay) — stalling every other monitored device — and (b) open a brand-new socket the selector never watches, so the device silently stopped delivering updates with no disconnect reported (epoll) or spun in an error-log loop (Windows select). Monitor now owns the connection lifecycle: - Added devices get socketRetryLimit = 0 so sends fail fast instead of blocking or self-reconnecting; the real limit is saved and restored for the blocking (re)connects on the add/connector threads and on removal. - Heartbeat and queued-command failures are detected (vanished socket) and routed through the existing disconnect -> auto-reconnect path. - Command dispatch gates on selector-registration state (state.fileno) rather than device.socket, closing a race with the connector thread's handshake. - The connector backoff uses an interruptible Event wait so stop() returns promptly and can't spawn a duplicate connector thread. - Devices are unregistered by their stored fd (the socket may already be closed); _handle_disconnect guards against double teardown; the command proxy only injects nowait=True for methods that accept it. Adds the first offline Monitor unit tests (fail-fast ownership, disconnect detection, command gating, proxy nowait). Verified end-to-end with real threads: disconnect is detected and the device reconnects and re-registers. Marshalling add()/remove() selector mutations onto the reactor thread remains a follow-up for the #713 refactor. Bumps version to 1.20.0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- stop() now restores socketRetryLimit for devices that disconnected before stop() was called (remained in _id_to_state but not _devices); previously those device objects were left permanently at socketRetryLimit=0. - _FakeDevice.__del__ closes the _peer socketpair fd to prevent fd leaks across the test suite on low-fd platforms. - Adds test_stop_restores_retry_limit_for_disconnected_devices to explicitly cover the gap Copilot identified (disconnect -> stop without reconnect). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
504eb02 to
8169204
Compare
|
Merge conflicts resolved. ✅ Rebased
All 37 tests pass after the rebase (1 skipped — GCM IV randomness test, intentionally skipped in debug mode). Branch is up to date with master and ready for merge. — Sam ⚙️ |
|
Thanks @jasonacox-sam - This has been merged.
|
|
Post-merge regression complete. ✅ Hardware test — v1.20.0 main (6 devices)
6/6 devices responded. Unit tests: 37 passed, 1 skipped. Release
— Sam ⚙️ |
Monitor (experimental) reliability + v1.20.0
Fixes the threading issues that made the
Monitorclass drop devices, and bumps the package to v1.20.0 as the release rollup for this batch.The problem
Monitorruns all monitored devices on one reactor thread but delegated sends toDevice.heartbeat()/ commands, whose internal retry loop, on a broken connection, would:socketRetryLimit × (connect_timeout + delay)(~50s), stalling every other monitored device and delayingstop(); andThe fix — Monitor owns the connection lifecycle
socketRetryLimit = 0so sends fail fast instead of blocking or silently self-reconnecting. The device's real limit is saved and restored for the blocking (re)connects (which run on the add/connector threads, never the reactor) and on removal.state.fileno) rather thandevice.socket, closing a race where a queued command could be sent on a socket the connector thread was still mid-handshake on.Eventwait for its backoff, sostop()returns promptly (verified: 0.2s vs up to 5s) and can't spawn a duplicate connector thread._handle_disconnectguards against double teardown; the command proxy only injectsnowait=Truefor methods that accept it (proxying e.g.set_socketPersistentno longer raises).Tests
Adds the first offline unit tests for
Monitor(fail-fast retry ownership, disconnect detection + reconnect enqueue, command gating, proxynowaithandling) — 23 → 28 tests. Also verified end-to-end with real threads: a heartbeat failure is detected as a disconnect and the device reconnects and re-registers with the selector.Known follow-up
Marshalling
add()/remove()selector mutations onto the reactor thread (a latent race the GIL largely masks, avoided by the documented "add before start" usage) is left for the #713 refactor.Release
Bumps
version_tupleto 1.20.0 and adds a v1.20.0 rollup toRELEASE.md. The security (#722) and bug-fix (#723) changelog bullets are included in that rollup for completeness; at merge their## Unreleasedsections should be folded under the## v1.20.0header.🤖 Generated with Claude Code