Skip to content

Close instruments when the daemon is done with them, not in __del__ - #99

Merged
sequoiap merged 2 commits into
masterfrom
fix/instrument-release
Oct 5, 2026
Merged

sequoiap merged 2 commits into
masterfrom
fix/instrument-release

Conversation

@sequoiap

@sequoiap sequoiap commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator

Fixes #61. This is independent of #98: no shared files, so they can merge in either order.

Problem

Instrument.__del__ called close(), which caused three problems:

  • The base class's close() raises NotImplementedError, and many drivers' close() fails on an instrument that never connected (self.device doesn't exist).
  • Errors raised in __del__ go to stderr, and a daemon runs in a windowless child process, so nobody sees them.
  • An instrument in a reference cycle (threads, callbacks) is only freed when Python's garbage collector next runs, which may be never before the process exits.

Change

  • When the daemon stops: DaemonRunner.run closes every single-mode instance it created, through Daemon.release_instruments(). This happens on the error path too.
  • When a client disconnects: Daemon.clientDisconnect closes that client's session-mode instances. Pyro discards them as soon as the connection closes anyway. LockableDaemon.clientDisconnect now chains to it after releasing locks.
  • Instrument._release() handles every call:
    • it calls close() at most once, so shutdown followed by garbage collection doesn't close an instrument twice;
    • it skips classes that don't override close();
    • it logs errors as warnings with the traceback, instead of raising them.
  • __del__ remains a last resort through the same path, logging at debug level and never raising.
  • Docs: the base close() docstring and CLAUDE.md now say when close() is called, and that it must cope with an instrument that never connected.

Not covered: a session-mode instance whose client is still connected when the daemon stops. That still depends on __del__, which is now harmless. Covering it would mean going through Pyro's internal connection list for each server type.

Things to know

  • Drivers whose close() fails on an instrument that never connected (ppcl55x, the RTO, KDC101 and others) will log a warning at shutdown if their instrument was never connected. Before, that error went to an invisible stderr. Each of those close() methods can be made tolerant in a later PR.
  • Instruments now really are closed when a daemon stops. Before, that only happened if garbage collection got to them before the process exited.

Testing

  • test_service.py:
    • _release closes an instrument once, leaves a class without close() alone, and logs errors;
    • garbage collection never raises. This test fails on master: the base class's __del__ reports an unraisable exception.
  • test_server.py, end to end over real sockets:
    • disconnecting closes that client's session instruments only, with both Daemon and LockableDaemon;
    • single-mode instruments survive disconnects and are closed by release_instruments().
  • test_registration.py: a real DaemonRunner with an instrument in a reference cycle, and the garbage collector paused. Stopping the runner closes the instrument. On master it is never closed.
  • Full suite: 442 passed, with pydantic's deprecation warnings turned into errors.

🤖 Generated with Claude Code

sequoiap and others added 2 commits October 5, 2026 11:30
Instrument.__del__ called close(), which raises NotImplementedError in the
base class and fails on an instrument that never connected. Exceptions in
__del__ go to stderr, which a windowless daemon process doesn't have, and
__del__ may never run at all for an instrument in a reference cycle.

Now the daemon releases instruments deterministically: DaemonRunner closes
every single-mode instance when it stops (also when it fails), and Daemon
closes a session-mode instance when its client disconnects (LockableDaemon
now chains to that). Instrument._release() calls close() at most once,
skips classes without their own close(), and logs errors instead of
raising them. __del__ remains a quiet last resort through the same path.

Fixes #61

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sequoiap sequoiap mentioned this pull request Oct 5, 2026
@sequoiap
sequoiap merged commit 0da026a into master Oct 5, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Instrument.__del__ calls close(), which raises NotImplementedError in the base class

1 participant