From 012d7fd04d76594a9d022d85fb08820e56ee423b Mon Sep 17 00:00:00 2001 From: Ken Sanislo Date: Thu, 16 Jul 2026 22:39:53 -0700 Subject: [PATCH] Fix crash on exit from device cleanup during interpreter shutdown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `solaar show` (and any exit path) printed a cascade of "I/O operation on closed file" / "sys.meta_path is None" logging errors at the bottom. Two compounding causes: - Device.close() was not idempotent. The CLI closes devices explicitly, then __del__ calls close() again during interpreter shutdown, re-running every device cleanup a second time. Guard with a _closed flag (not self.handle, which is legitimately None for receiver-paired devices). - rgb_power.cleanup() read device.settings — the lazy property — which forces a first-time settings build. Running that build during shutdown fails because gettext can no longer import (sys.meta_path is None), raising inside a __del__ handler. Read the already-built settings (or the persister) via a new _rgb_control_off() helper that never triggers the build. Either guard alone stops the crash; both are correct hardening for a __del__-invoked path. --- lib/logitech_receiver/device.py | 7 ++++ lib/logitech_receiver/rgb_power.py | 17 +++++++++- tests/logitech_receiver/test_device.py | 17 ++++++++++ tests/logitech_receiver/test_rgb_power.py | 39 +++++++++++++++++++++++ 4 files changed, 79 insertions(+), 1 deletion(-) diff --git a/lib/logitech_receiver/device.py b/lib/logitech_receiver/device.py index 725a7259..1285bf30 100644 --- a/lib/logitech_receiver/device.py +++ b/lib/logitech_receiver/device.py @@ -172,6 +172,7 @@ class Device: self.present = True # used for devices that are integral with their receiver but that separately be disconnected self._feature_settings_checked = False + self._closed = False self._gestures_lock = threading.Lock() self._settings_lock = threading.Lock() self._persister_lock = threading.Lock() @@ -1037,6 +1038,12 @@ class Device: pass def close(self): + # Idempotent: __del__ re-invokes close() at interpreter shutdown, where + # re-running cleanups can't safely build settings. Flag, not self.handle + # (None for receiver-paired devices that borrow the receiver's handle). + if getattr(self, "_closed", False): + return None + self._closed = True # Run device.cleanups before clearing self.handle — cleanup callbacks # typically need to issue final feature_request() writes (e.g. release # SW control, restore device-side state) and feature_request() relies diff --git a/lib/logitech_receiver/rgb_power.py b/lib/logitech_receiver/rgb_power.py index 96ae6e2f..ba471710 100644 --- a/lib/logitech_receiver/rgb_power.py +++ b/lib/logitech_receiver/rgb_power.py @@ -316,6 +316,21 @@ def stop(device): mgr.stop() +def _rgb_control_off(device): + # Reads already-built settings/persister, never device.settings (the lazy + # property): cleanup() runs from __del__, where a settings build fails at + # shutdown. Unknown state ⇒ False (proceed). + for s in getattr(device, "_settings", None) or []: + if s.name == "rgb_control": + return not s._value + persister = getattr(device, "persister", None) + if persister is not None: + value = persister.get("rgb_control") + if value is not None: + return value in (False, 0) + return False + + def cleanup(device): """device.cleanups handler — restore firmware control on device close. @@ -330,7 +345,7 @@ def cleanup(device): animation would visibly contradict the user's "leave my lighting alone". """ stop(device) - if any(s.name == "rgb_control" and not s._value for s in getattr(device, "settings", []) or []): + if _rgb_control_off(device): return try: device.feature_request(SupportedFeature.RGB_EFFECTS, 0x50, SW_RELEASE) diff --git a/tests/logitech_receiver/test_device.py b/tests/logitech_receiver/test_device.py index c22f439e..d1606a54 100644 --- a/tests/logitech_receiver/test_device.py +++ b/tests/logitech_receiver/test_device.py @@ -423,3 +423,20 @@ def test_device_battery(device_info, responses, protocol, expected_battery, chan assert test_device.battery() == expected_battery test_device.read_battery() spy_changed.assert_called_with(**changed) + + +def test_close_runs_cleanups_once(): + """close() is idempotent: __del__ calls it again at interpreter shutdown, + and cleanups must not run a second time (a settings build there fails).""" + handle = 0x1 + test_device = device.Device( + LowLevelInterfaceFake(fake_hidpp.r_empty), None, None, None, handle=handle, device_info=di_CCCC + ) + calls = [] + test_device.cleanups = [lambda d: calls.append(d)] + + test_device.close() + test_device.close() # simulates the __del__ re-entry + test_device.__del__() + + assert calls == [test_device] diff --git a/tests/logitech_receiver/test_rgb_power.py b/tests/logitech_receiver/test_rgb_power.py index cafe6d59..047396b7 100644 --- a/tests/logitech_receiver/test_rgb_power.py +++ b/tests/logitech_receiver/test_rgb_power.py @@ -672,3 +672,42 @@ def test_perkey_write_key_value_skipped_when_zone_is_animation(monkeypatch): s.write_key_value(7, 0xFF0000) s._send_zone_color.assert_not_called() + + +class _SettingsPropertyExplodes: + """A device whose `.settings` property raises — mirrors the interpreter + -shutdown case where forcing a lazy settings build fails. cleanup() must + never touch it.""" + + def __init__(self, settings_list=None, persister=None): + self._settings = settings_list + self.persister = persister + + @property + def settings(self): + raise AssertionError("cleanup must not access the settings property") + + +def _rgb_control_setting(value): + class _S: + name = "rgb_control" + + s = _S() + s._value = value + return s + + +def test_rgb_control_off_reads_built_setting_without_building(): + dev = _SettingsPropertyExplodes(settings_list=[_rgb_control_setting(False)]) + assert rgb_power._rgb_control_off(dev) is True + + dev = _SettingsPropertyExplodes(settings_list=[_rgb_control_setting(True)]) + assert rgb_power._rgb_control_off(dev) is False + + +def test_rgb_control_off_falls_back_to_persister(): + dev = _SettingsPropertyExplodes(settings_list=None, persister={"rgb_control": False}) + assert rgb_power._rgb_control_off(dev) is True + + dev = _SettingsPropertyExplodes(settings_list=None, persister={}) + assert rgb_power._rgb_control_off(dev) is False