fix: daemonize the monitor heartbeat thread and scope its locks per instance#529
Merged
Conversation
…nstance Two defects in AtMonitorConnection's lifecycle: 1. The heartbeat thread was non-daemon and runs an infinite loop, so any process that ever constructed a monitor connection could not exit, and every abandoned/replaced connection leaked a live thread. 2. should_be_running_lock and running_lock were module-level singletons (in atconstants, star-imported here and used from AtClient), so every monitor connection in a process shared one lock pair and serialized against the others' state transitions. The heartbeat thread is now created with daemon=True and kept on the instance (_heartbeat_thread). The locks are now instance attributes; AtClient uses the connection's own locks. The now-unused atconstants star imports are removed from both files; the definitions in atconstants are left in place so any external importer keeps working. Adds network-free tests: daemon flag, per-instance lock identity, and that one connection's lock does not block another's.
Aligns with the reference Dart SDK, whose heartbeat is a cancellable Timer (stopHeartbeat() in monitor.dart): the loop now waits on a threading.Event instead of time.sleep, and stop_heart_beat() sets it, so the thread exits promptly rather than only at process exit. Adds a test that the thread ends within the join timeout after stop_heart_beat().
cpswan
approved these changes
Jul 8, 2026
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
2 participants
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.
Two lifecycle defects in
AtMonitorConnection:should_be_running_lock/running_lockwere module-level singletons (defined inatconstants, star-imported here and used fromAtClient), so every monitor connection in a process shared one lock pair and serialized against the others' state transitions.Changes
daemon=Trueand kept on the instance as_heartbeat_thread.threading.Eventinstead oftime.sleep, and a newstop_heart_beat()sets it — so the thread ends promptly when a connection is retired, not only at process exit.AtMonitorConnection;AtClient.start_monitor/stop_monitoruse the connection's own locks.atconstantsstar imports are removed from both files. The lock definitions inatconstantsare deliberately left in place so any external importer keeps working.Reference Dart SDK parity: the Dart
at_clientmonitor's heartbeat is a cancellableTimer(heartbeatTimer/stopHeartbeat()inpackages/at_client/lib/src/manager/monitor.dart), stopped when the connection closes, with all state per-instance. The Event-based stoppable loop is the Python equivalent.Tests (
test/monitor_heartbeat_test.py, network-free): heartbeat thread is a daemon; locks are per-instance; one connection's held lock does not block another's;stop_heart_beat()ends the thread within a 2s join. Note the test suite itself would hang at interpreter exit without the daemon fix.No behaviour change to the monitor protocol or reconnect logic.