From 6486ac95e77b6bb4ea7edd0579afb66cf8c4b0d0 Mon Sep 17 00:00:00 2001 From: Loren Eteval Date: Wed, 9 Sep 2026 15:58:31 +0800 Subject: [PATCH] Fix Qt object lifetime cleanup Signed-off-by: Loren Eteval --- .../qt-pyside6-object-lifetime-guidelines.md | 16 ++- Furious/Qt/QtWidgets.py | 4 + Furious/Qt/Signals.py | 20 +-- Furious/Qt/ThemeTransition.py | 18 +++ Furious/Service/DnsResolver.py | 6 +- Furious/Service/PluginUIManager.py | 11 +- Furious/Widget/ServerTableView.py | 2 + tests/fixtures/editor_lifetime_probe.py | 115 +++++++++++++++++- tests/test_connection_startup_async.py | 45 ++++++- tests/test_qt_lifetime.py | 41 +++++++ tests/test_service_runtime.py | 34 +++++- tests/test_theme_transition.py | 47 +++++++ 12 files changed, 336 insertions(+), 23 deletions(-) diff --git a/.agents/skills/manage-qt-pyside6-lifetimes/references/qt-pyside6-object-lifetime-guidelines.md b/.agents/skills/manage-qt-pyside6-lifetimes/references/qt-pyside6-object-lifetime-guidelines.md index 33976b2..34da11e 100644 --- a/.agents/skills/manage-qt-pyside6-lifetimes/references/qt-pyside6-object-lifetime-guidelines.md +++ b/.agents/skills/manage-qt-pyside6-lifetimes/references/qt-pyside6-object-lifetime-guidelines.md @@ -273,10 +273,12 @@ connectWeakly( `QObject.sender()`; - `_ownsQObject(receiver, sender)` walks from the sender through its `parent()` chain; - when that walk shows that the sender is not the receiver or its descendant, - `sender=` makes receiver destruction disconnect the otherwise dormant dispatcher; -- that cleanup captures only the opaque connection returned by `signal.connect()` and - calls `QtCore.QObject.disconnect(connection)`; it deliberately does not capture the - sender's `SignalInstance`, which may wrap an already-deleted sender during teardown; + `sender=` makes either endpoint's destruction release dispatch and both cleanup hooks; +- that cleanup captures only opaque connection handles and calls + `QtCore.QObject.disconnect(connection)`; it deliberately does not capture either + QObject or the sender's `SignalInstance`, which may wrap an already-deleted sender; +- releasing both cleanup hooks prevents short-lived senders from accumulating callbacks + on a surviving receiver; verify both destruction orders over repeated cycles; - the method name is static and must remain valid for the receiver's lifetime. Pass the sender whenever it is outside the receiver's QObject subtree. Omitting it can @@ -338,8 +340,10 @@ UI objects may observe controller state. Controllers should not become accidenta Long-running resources belong to the service that schedules them, not to a transient page callback. Furious's current profile-test and subscription services parent their manager-side QObjects/pools to durable owners, cross worker results back to the owning -Qt thread, reject stale generations/identities, and provide bounded idempotent -shutdown. Preserve that ownership shape when moving work off the GUI thread. +Qt thread, and reject stale generations/identities. Their shutdown is idempotent, but +not uniformly bounded: subscription preparation drains cooperatively and waits +synchronously after its diagnostic timeout. Preserve ownership until workers finish; +do not confuse a diagnostic timeout with a termination deadline. ## 12. Plugin and Registry Design diff --git a/Furious/Qt/QtWidgets.py b/Furious/Qt/QtWidgets.py index 8c95873..e340001 100644 --- a/Furious/Qt/QtWidgets.py +++ b/Furious/Qt/QtWidgets.py @@ -1856,7 +1856,11 @@ class AppQMessageBox(AppQTransientDialog): return self._removeWindowMask() + self._windowMask = _AppMessageBoxMask(owner) + # Native deletion can bypass done(); the window must not keep the mask. + self.destroyed.connect(self._windowMask.deleteLater) + self._windowMask.show() self._windowMask.raise_() diff --git a/Furious/Qt/Signals.py b/Furious/Qt/Signals.py index 4aae085..fbd46e3 100644 --- a/Furious/Qt/Signals.py +++ b/Furious/Qt/Signals.py @@ -146,15 +146,19 @@ def connectWeakly( and not _ownsQObject(receiver, sender) ): - def disconnect(*_args): - """Remove weak dispatch from an independently owned sender.""" - # Retain only Qt's opaque connection handle. Capturing ``signal`` - # here keeps the sender's SignalInstance wrapper alive until the - # receiver dies; when the native sender died first, PySide6 could - # then access that stale wrapper during application teardown. - QtCore.QObject.disconnect(connection) + connections = [connection] - receiver.destroyed.connect(disconnect) + def disconnect(*_args): + """Release dispatch and both cleanup hooks when either endpoint dies.""" + # Keep only opaque handles, never SignalInstance wrappers or QObjects. + # Removing both hooks also bounds retention when senders die first. + for ownedConnection in connections: + QtCore.QObject.disconnect(ownedConnection) + + connections.clear() + + connections.append(receiver.destroyed.connect(disconnect)) + connections.append(sender.destroyed.connect(disconnect)) return connection diff --git a/Furious/Qt/ThemeTransition.py b/Furious/Qt/ThemeTransition.py index a700c94..f5f5bda 100644 --- a/Furious/Qt/ThemeTransition.py +++ b/Furious/Qt/ThemeTransition.py @@ -180,6 +180,11 @@ class ThemeTransition(QtCore.QObject): b'opacityValue', self, ) + + # The snapshot belongs to the window, but must also die if its + # animation owner is destroyed early. This is a native Qt slot. + animation.destroyed.connect(overlay.deleteLater) + animation.setDuration(self._duration) animation.setStartValue(1.0) animation.setEndValue(0.0) @@ -198,6 +203,13 @@ class ThemeTransition(QtCore.QObject): forwardSender=True, ) + connectWeakly( + overlay.destroyed, + self, + '_releaseDestroyedOverlays', + sender=overlay, + ) + overlay.show() overlay.raise_() @@ -209,6 +221,12 @@ class ThemeTransition(QtCore.QObject): for animation in tuple(self._animations): animation.start() + def _releaseDestroyedOverlays(self): + """Native target destruction stops animations without emitting finished.""" + for animation, (_window, overlay) in tuple(self._animations.items()): + if not isValid(overlay): + self._releaseAnimation(animation) + def _releaseAnimation(self, animation, *, notify=True): """Release one animation and its transient overlay exactly once.""" transition = self._animations.pop(animation, None) diff --git a/Furious/Service/DnsResolver.py b/Furious/Service/DnsResolver.py index a6323e8..58616c6 100644 --- a/Furious/Service/DnsResolver.py +++ b/Furious/Service/DnsResolver.py @@ -27,6 +27,8 @@ from Furious.Qt.Signals import connectWeakly from PySide6 import QtCore from PySide6.QtNetwork import * +from shiboken6 import isValid + from typing import Tuple import logging @@ -104,6 +106,7 @@ class DnsResolutionOperation(QtCore.QObject): for networkReply in self._resultMap['reference']: if ( isinstance(networkReply, QNetworkReply) + and isValid(networkReply) and not networkReply.isFinished() ): networkReply.abort() @@ -372,6 +375,7 @@ class DnsResolver(HttpGetManager): for networkReply in resultMap['reference']: if ( isinstance(networkReply, QNetworkReply) + and isValid(networkReply) and not networkReply.isFinished() ): networkReply.abort() @@ -379,7 +383,7 @@ class DnsResolver(HttpGetManager): def dispose(self): """Abort pending replies and schedule this resolver for destruction.""" for networkReply in tuple(self._replyContexts): - if not networkReply.isFinished(): + if isValid(networkReply) and not networkReply.isFinished(): networkReply.abort() self._replyContexts.clear() diff --git a/Furious/Service/PluginUIManager.py b/Furious/Service/PluginUIManager.py index 96bb58c..c5559c8 100644 --- a/Furious/Service/PluginUIManager.py +++ b/Furious/Service/PluginUIManager.py @@ -29,6 +29,8 @@ from Furious.Plugins import ( from PySide6 import QtCore from PySide6.QtWidgets import QWidget +from shiboken6 import isValid + import logging __all__ = ['PluginNavigationManager', 'isCoreActive'] @@ -110,10 +112,10 @@ class PluginNavigationManager: continue - if not isinstance(page, QWidget): - logger.error(f'plugin page {pageId!r} did not create a QWidget') + if not isinstance(page, QWidget) or not isValid(page): + logger.error(f'plugin page {pageId!r} did not create a valid QWidget') - if isinstance(page, QtCore.QObject): + if isinstance(page, QtCore.QObject) and isValid(page): page.deleteLater() continue @@ -131,7 +133,8 @@ class PluginNavigationManager: logger.error(f'failed to register plugin page {pageId!r}: {ex}') - page.deleteLater() + if isValid(page): + page.deleteLater() continue diff --git a/Furious/Widget/ServerTableView.py b/Furious/Widget/ServerTableView.py index 3f26f54..f16ed27 100644 --- a/Furious/Widget/ServerTableView.py +++ b/Furious/Widget/ServerTableView.py @@ -753,6 +753,7 @@ class ServerTableView( self.proxyModel = UserServersSortFilterProxyModel(parent=self) self.proxyModel.setSourceModel(self.sourceModel) self.setModel(self.proxyModel) + self._sortSelectionSnapshot = None self.proxyModel.sortAboutToStart.connect(self._captureSortSelection) self.proxyModel.sortCompleted.connect(self._restoreSortSelection) @@ -878,6 +879,7 @@ class ServerTableView( self._subscriptionActions = [] self.importActions = tuple(importActionsFactory()) + self.testActions = ( AppQAction( _('Test Ping Latency'), diff --git a/tests/fixtures/editor_lifetime_probe.py b/tests/fixtures/editor_lifetime_probe.py index 913d0a6..0488894 100644 --- a/tests/fixtures/editor_lifetime_probe.py +++ b/tests/fixtures/editor_lifetime_probe.py @@ -21,11 +21,12 @@ from __future__ import annotations from Furious.Backends import OFFICIAL_PLUGIN_TYPES from Furious.Plugins import blankProfile, initializePluginRegistry -from Furious.Qt import AppQDialog, connectWeakly +from Furious.Qt import AppQDialog, AppQMessageBox, ThemeTransition, connectWeakly from Furious.Widget.ServerTableView import ServerTableView import PySide6 +from PySide6 import QtCore from PySide6.QtWidgets import QWidget from shiboken6 import isValid @@ -221,6 +222,116 @@ def runProbe( registry.shutdown() +class _SignalEndpoint(QtCore.QObject): + """Exercise compiled methods with independently destroyed Qt endpoints.""" + + emitted = QtCore.Signal() + + def __init__(self): + super().__init__() + self.calls = 0 + + def record(self): + self.calls += 1 + + +def runInfrastructureProbe(iterations=100): + """Check signal, mask, and animation ownership under real Qt destruction.""" + application() + result = {} + + for senderFirst in (True, False): + survivor = _SignalEndpoint() + counts = [] + + for _ in range(iterations): + transient = _SignalEndpoint() + sender, receiver = ( + (transient, survivor) if senderFirst else (survivor, transient) + ) + + connectWeakly(sender.emitted, receiver, 'record', sender=sender) + sender.emitted.emit() + + assert receiver.calls > 0 + + transient.deleteLater() + processQtEvents() + + assert not isValid(transient) + + survivor.emitted.emit() + counts.append(survivor.receivers(QtCore.SIGNAL('destroyed(QObject*)'))) + + assert counts == [counts[0]] * iterations, counts + result['senderFirst' if senderFirst else 'receiverFirst'] = iterations + + survivor.deleteLater() + processQtEvents() + + for windowFirst in (True, False): + for _ in range(iterations): + window = QWidget() + window.show() + processQtEvents() + + transition = ThemeTransition( + duration=100000, + windowProvider=lambda: (window,), + animationsEnabled=lambda: True, + ) + + transition.apply(lambda: None) + animation = next(iter(transition._animations)) + overlay = window.findChild(QWidget, transition.OverlayObjectName) + + if windowFirst: + window.deleteLater() + processQtEvents() + + assert not transition._animations + assert not transition._animationsByWindow + + transition.deleteLater() + else: + transition.deleteLater() + processQtEvents() + window.deleteLater() + + processQtEvents() + + assert not isValid(animation) + assert not isValid(overlay) + + result['themeWindowFirst' if windowFirst else 'themeCoordinatorFirst'] = ( + iterations + ) + + owner = QWidget() + owner.show() + processQtEvents() + + try: + for _ in range(iterations): + dialog = AppQMessageBox(parent=owner, text='Lifetime probe') + dialog.open() + mask = dialog._windowMask + + dialog.deleteLater() + processQtEvents() + + assert not isValid(dialog) + assert not isValid(mask) + + assert not AppQDialog._openDialogs + result['messageBoxDeleted'] = iterations + finally: + owner.deleteLater() + processQtEvents() + + return result + + def main(): """Run the probe as a standalone source or Nuitka executable.""" parser = argparse.ArgumentParser() @@ -231,6 +342,8 @@ def main(): parser.add_argument('--close-method', choices=CLOSE_METHODS, default='reject') arguments = parser.parse_args() + print(json.dumps(runInfrastructureProbe(arguments.iterations), sort_keys=True)) + print( json.dumps( runProbe( diff --git a/tests/test_connection_startup_async.py b/tests/test_connection_startup_async.py index 3ddad11..0e5b8f7 100644 --- a/tests/test_connection_startup_async.py +++ b/tests/test_connection_startup_async.py @@ -26,7 +26,7 @@ from Furious.Service.ConnectionManager import ( ConnectionManager, ConnectionStartStage, ) -from Furious.Service.DnsResolver import DnsResolutionOperation +from Furious.Service.DnsResolver import DnsResolutionOperation, DnsResolver from PySide6 import QtCore, QtNetwork @@ -438,6 +438,48 @@ class ConnectionStartupAsyncTest(TestCase): self.assertEqual(runtime.startOptions, [{}]) self.assertEqual(manager.runtimes, [runtime]) + def testDnsCancellationAndTimeoutIgnoreAlreadyDestroyedReplies(self): + """Earlier recursive replies may be deleted before later requests stop.""" + + class PendingReply(QtNetwork.QNetworkReply): + def __init__(self): + super().__init__() + self.abortCount = 0 + + def abort(self): + self.abortCount += 1 + self.setFinished(True) + + for synchronous in (False, True): + with self.subTest(synchronous=synchronous): + resolver = mock.Mock() + resolver._newResultMap = DnsResolver._newResultMap + operation = DnsResolutionOperation(resolver, 'example.test') + + completed = PendingReply() + pending = PendingReply() + operation._resultMap['reference'] = [completed, pending] + operation._resultMap['depth'] = 1 + + completed.deleteLater() + processQtEvents() + + try: + if synchronous: + with self.assertLogs( + 'Furious.Service.DnsResolver', level='ERROR' + ): + DnsResolver.wait(operation._resultMap, timeout=0) + else: + operation.cancel() + operation.cancel() + + self.assertEqual(pending.abortCount, 1) + finally: + pending.deleteLater() + operation.deleteLater() + processQtEvents() + def testDnsResolutionOperationCompletesAndCancelsWithoutNestedWait(self): """Observe recursive DNS state through timers and suppress stale cancel.""" resolver = _ResolverFixture() @@ -469,6 +511,7 @@ class ConnectionStartupAsyncTest(TestCase): processQtEvents(5) self.assertEqual(staleResults, []) + operation.deleteLater() cancelled.deleteLater() resolver.deleteLater() diff --git a/tests/test_qt_lifetime.py b/tests/test_qt_lifetime.py index 522e1f6..5b45f3c 100644 --- a/tests/test_qt_lifetime.py +++ b/tests/test_qt_lifetime.py @@ -140,6 +140,23 @@ class DelayedReceiver(QtCore.QObject): class QtLifetimeTest(unittest.TestCase): """Stress direct destruction evidence without relying on process RSS alone.""" + def testIndependentSenderDestructionReleasesReceiverCleanupHooks(self): + """A surviving receiver must not accumulate hooks for dead senders.""" + receiver = DelayedReceiver([]) + self.addCleanup(receiver.deleteLater) + counts = [] + + for _ in range(40): + sender = LongLivedEmitter() + connectWeakly(sender.emitted, receiver, 'record', sender=sender) + + sender.deleteLater() + processQtEvents() + + counts.append(receiver.receivers(QtCore.SIGNAL('destroyed(QObject*)'))) + + self.assertEqual(counts, [counts[0]] * len(counts)) + @classmethod def setUpClass(cls): """Create the one QApplication used by the entire test process.""" @@ -290,6 +307,30 @@ class QtLifetimeTest(unittest.TestCase): self.assertTrue(waitFor(lambda: reference() is None)) self.assertNotIn(key, AppQMainWindow._openWindows) + def testNativeMessageBoxDestructionReleasesItsWindowMask(self): + """Direct Qt deletion must release a mask parented to a surviving window.""" + owner = QWidget() + owner.show() + processQtEvents() + + try: + for _ in range(30): + messageBox = AppQMessageBox(parent=owner, text='Lifetime probe') + messageBox.open() + mask = messageBox._windowMask + + self.assertIsNotNone(mask) + + messageBox.deleteLater() + processQtEvents() + + self.assertFalse(isValid(messageBox)) + self.assertFalse(isValid(mask)) + self.assertEqual(owner.findChildren(_AppMessageBoxMask), []) + finally: + owner.deleteLater() + processQtEvents() + def testMessageBoxAndParentMaskHaveTransientOwnership(self): """Remove every parent event filter/mask over repeated modal presentation.""" iterations = 60 diff --git a/tests/test_service_runtime.py b/tests/test_service_runtime.py index 97ad51f..578b344 100644 --- a/tests/test_service_runtime.py +++ b/tests/test_service_runtime.py @@ -37,7 +37,7 @@ from PySide6.QtWidgets import QWidget from shiboken6 import isValid -from tests.support import application, collectAtBoundary, waitFor +from tests.support import processQtEvents, application, collectAtBoundary, waitFor from types import SimpleNamespace from unittest.mock import patch @@ -259,6 +259,31 @@ class PluginNavigationManagerTest(unittest.TestCase): """Create the process-wide headless QApplication.""" application() + def testDestroyedFactoryResultsAreRejectedBeforeRegistration(self): + """A retained Python wrapper is not proof of a valid plugin page.""" + for pageType in (QtCore.QObject, QWidget): + with self.subTest(pageType=pageType): + page = pageType() + page.deleteLater() + processQtEvents() + + provider = _NavigationProvider() + provider._invalidPage = lambda parent=None: page + host = _NavigationHost() + manager = PluginNavigationManager(_NavigationRegistry(provider)) + + try: + with self.assertLogs( + 'Furious.Service.PluginUIManager', level='ERROR' + ): + pages = manager.registerPages(host) + + self.assertEqual(len(pages), 1) + self.assertEqual(len(host.registrations), 1) + finally: + host.deleteLater() + processQtEvents() + def testRegistrationIsIdempotentAndDeletesInvalidQObject(self): """Construct each descriptor once and destroy rejected Qt objects.""" provider = _NavigationProvider() @@ -296,7 +321,12 @@ class ConnectivityManagerTest(unittest.TestCase): reply = object() manager._testingEnabled = True - with patch.object(manager, 'webGET', return_value=reply) as webGet: + with ( + patch.object(manager, 'webGET', return_value=reply) as webGet, + patch( + 'Furious.Service.ConnectivityManager.AppSettings.get', return_value=None + ), + ): manager.startSingleTest() manager.startSingleTest() diff --git a/tests/test_theme_transition.py b/tests/test_theme_transition.py index 9d83aea..f496884 100644 --- a/tests/test_theme_transition.py +++ b/tests/test_theme_transition.py @@ -87,6 +87,53 @@ class ThemeTransitionTest(unittest.TestCase): QtCore.Qt.FindChildOption.FindDirectChildrenOnly, ) + def testWindowDestructionReleasesCoordinatorAnimationState(self): + """Deleting a target must not leave its stopped animation registered.""" + windows = [] + transition = self.createTransition(windows, duration=100000) + + for _ in range(30): + window = QWidget() + window.show() + processQtEvents() + + windows[:] = [window] + + transition.apply(lambda: None) + animation = next(iter(transition._animations)) + + window.deleteLater() + processQtEvents() + + self.assertFalse(isValid(animation)) + self.assertEqual(transition._animations, {}) + self.assertEqual(transition._animationsByWindow, {}) + + def testCoordinatorDestructionReleasesWindowOwnedOverlays(self): + """Destroying the animation owner must also remove its snapshots.""" + window = self.createWindow() + + for _ in range(30): + owner = QtCore.QObject() + transition = ThemeTransition( + owner, + duration=100000, + windowProvider=lambda: (window,), + animationsEnabled=lambda: True, + ) + + transition.apply(lambda: None) + overlays = self.overlays(window) + + self.assertEqual(len(overlays), 1) + + owner.deleteLater() + processQtEvents() + + self.assertFalse(isValid(transition)) + self.assertFalse(isValid(overlays[0])) + self.assertEqual(self.overlays(window), []) + def testThemeIsAppliedImmediatelyThenSnapshotCompletesAndIsRemoved(self): """Keep destination state live beneath one real fading snapshot.""" window = self.createWindow()