Improve cleanup lifecycle readability

Signed-off-by: Loren Eteval <loren.eteval@proton.me>

Furious/Application/DesktopApplication.py

@@ -1033,12 +1033,16 @@ class DesktopApplication(ApplicationRunner, SingletonApplication):
                     '_connectionShutdownFinished',
                     sender=self.connectionController,
                 )
+
             try:
                 self.connectionController.shutdown()
             except Exception as ex:
                 # Any non-exit exceptions
+
                 logger.error(f'connection shutdown prevented application exit: {ex}')
+
                 self._connectionShutdownFinished(False)
+
             return
 
         self._completeExit()
@@ -1046,6 +1050,7 @@ class DesktopApplication(ApplicationRunner, SingletonApplication):
     def _connectionShutdownFinished(self, success):
         if not self._exitRequested or self._exitCommitted:
             return
+
         if success:
             singleShotWeakly(0, self, '_completeExit')
         else:
@@ -1055,7 +1060,9 @@ class DesktopApplication(ApplicationRunner, SingletonApplication):
     def _completeExit(self):
         if not self._exitRequested or self._exitCommitted:
             return
+
         self._exitCommitted = True
+
         QtCore.QCoreApplication.exit(self._exitCode)
 
     def run(self):

Furious/Controllers/ConnectionController.py

@@ -224,9 +224,12 @@ class ConnectionController(QtCore.QObject):
         self._pendingHttpProxy = ''
 
         self.progressFinished.emit(True)
+
         if not isValid(self):
             return
+
         self._setActiveProfile(None)
+
         if not isValid(self):
             return
 
@@ -589,6 +592,7 @@ class ConnectionController(QtCore.QObject):
         if self.isDisconnecting():
             self._reconnectAfterStop = False
             return False
+
         if (
             self.state is ConnectionState.Disconnected
             and not self._startAdmissionPending
@@ -610,8 +614,10 @@ class ConnectionController(QtCore.QObject):
         self._startOperation = None
 
         self._setState(ConnectionState.Disconnecting)
+
         if not isValid(self) or generation != self._connectionGeneration:
             return True
+
         self._actionTimer.stop()
 
         if operation is not None and isValid(operation):
@@ -637,9 +643,11 @@ class ConnectionController(QtCore.QObject):
         self._disconnectionNotification = notification
 
         asyncStop = getattr(self._coreManager, 'stopAsync', None)
+
         if callable(asyncStop):
             try:
                 self._stopOperation = stopOperation = asyncStop()
+
                 connectWeakly(
                     stopOperation.finished,
                     self,
@@ -652,13 +660,19 @@ class ConnectionController(QtCore.QObject):
                     '_connectionStopDestroyed',
                     sender=stopOperation,
                 )
+
                 stopOperation.start()
+
                 return True
             except Exception as ex:
                 # Any non-exit exceptions
+
                 self._runtimeCleanupFailed = True
+
                 logger.error(f'failed to stop connection runtime: {ex}')
+
                 self._finishDisconnection(generation, notification)
+
                 return True
 
         try:
@@ -678,10 +692,14 @@ class ConnectionController(QtCore.QObject):
     def _connectionStopFinished(self, operation, error):
         if operation is not self._stopOperation:
             return
+
         self._stopOperation = None
+
         if error:
             self._runtimeCleanupFailed = True
+
             logger.error(f'failed to stop connection runtime: {error}')
+
         self._finishDisconnection(
             self._disconnectionGeneration, self._disconnectionNotification
         )
@@ -690,15 +708,19 @@ class ConnectionController(QtCore.QObject):
         if self._stopOperation is not None and not isValid(self._stopOperation):
             self._stopOperation = None
             self._runtimeCleanupFailed = True
+
             self._finishDisconnection(
                 self._disconnectionGeneration, self._disconnectionNotification
             )
 
     def _finishDisconnection(self, generation, notification):
         reconnect, self._reconnectAfterStop = self._reconnectAfterStop, False
+
         self._emitRuntimesChanged()
+
         if not isValid(self):
             return
+
         self._reset()
 
         if not isValid(self) or generation != self._connectionGeneration:
@@ -713,22 +735,28 @@ class ConnectionController(QtCore.QObject):
                 pass
 
         Mixins.ConnectionAware.callDisconnectedCallback()
+
         if not isValid(self) or generation != self._connectionGeneration:
             return
 
         if notification:
             self.notificationRequested.emit(notification)
+
             if not isValid(self):
                 return
 
         if self._shuttingDown:
             if self._shutdownConnectPreference:
                 AppSettings.turnON_('Connect')
+
             success = not self._runtimeCleanupFailed
+
             self._shutdownComplete = success
+
             if not success:
                 self._shuttingDown = False
                 self._shutdownConnectPreference = None
+
             self.shutdownFinished.emit(success)
         elif (
             reconnect
@@ -761,10 +789,14 @@ class ConnectionController(QtCore.QObject):
         """Stop runtime resources without changing the next-start preference."""
         if self._shutdownComplete:
             self.shutdownFinished.emit(True)
+
             return True
+
         reconnectOnStartup = AppSettings.isStateON_('Connect')
+
         if self._shutdownConnectPreference is None:
             self._shutdownConnectPreference = reconnectOnStartup
+
         reconnectOnStartup = self._shutdownConnectPreference
 
         self._shuttingDown = True
@@ -789,12 +821,15 @@ class ConnectionController(QtCore.QObject):
             self._runtimeCleanupFailed = False
             self._shutdownComplete = True
             self.shutdownFinished.emit(True)
+
             return True
         except Exception:
             # Any non-exit exceptions
+
             self._shuttingDown = False
             self._shutdownConnectPreference = None
             self.shutdownFinished.emit(False)
+
             raise
         finally:
             if reconnectOnStartup:

Furious/Frozenlib/SingTUNRouting.py

@@ -416,6 +416,7 @@ class SingTUNLinuxRouting:
         """Report loss of an authorized helper while forwarding is active."""
         if self._holding and self._ready and self._process.poll() is not None:
             return 'sing-tun privileged helper exited unexpectedly'
+
         return ''
 
     def drain(self, *, wait=True):
@@ -449,6 +450,7 @@ class SingTUNLinuxRouting:
         self._directory = None
         self._holding = False
         self._ready = False
+
         return True
 
     def execute(self, commands, *, preparing=False, guard=False, holding=False):
@@ -509,6 +511,7 @@ class SingTUNLinuxRouting:
         if holding:
             ready = pathlib.Path(directory.name, 'ready')
             staging = pathlib.Path(directory.name, 'ready.tmp')
+
             lines.extend(
                 [
                     'printf "%s\\n" "$createdIndex" > ' + shlex.quote(str(staging)),
@@ -587,6 +590,7 @@ class SingTUNLinuxRouting:
         """Pair successful additions with exact deletions in the same helper."""
         name = shlex.quote(self.tun['Name'])
         index = shlex.quote('/sys/class/net/' + self.tun['Name'] + '/ifindex')
+
         lines = [
             'createdIndex=; rulesAdded=0',
             'cleanup() {',
@@ -618,6 +622,7 @@ class SingTUNLinuxRouting:
                 "trap 'exit 1' HUP INT TERM",
             ]
         )
+
         return lines
 
     def prepare(self, dnsAddress='1.1.1.1'):

Furious/Service/ConnectionManager.py

@@ -1379,8 +1379,10 @@ class ConnectionStartOperation(QtCore.QObject):
 
 def _cleanupDestroyedStopOperation(reference, *_args):
     operation = reference()
+
     if operation is not None:
         operation._terminal = True
+
         if operation.manager._activeStopOperation is operation:
             operation.manager._activeStopOperation = None
 
@@ -1392,11 +1394,15 @@ class ConnectionStopOperation(QtCore.QObject):
 
     def __init__(self, manager):
         super().__init__()
+
         self.manager = manager
         self._terminal = False
+
         self._timer = QtCore.QTimer(self)
         self._timer.setInterval(100)
+
         connectWeakly(self._timer.timeout, self, '_poll')
+
         self.destroyed.connect(
             functools.partial(_cleanupDestroyedStopOperation, weakref.ref(self))
         )
@@ -1404,22 +1410,27 @@ class ConnectionStopOperation(QtCore.QObject):
     def start(self):
         if self._terminal or self._timer.isActive():
             return
+
         self._timer.start()
         self._poll()
 
     def _poll(self):
         if self._terminal:
             return
+
         try:
             complete = self.manager._pollPendingReleases()
         except Exception as ex:
             # Any non-exit exceptions
+
             if isValid(self):
                 self._finish(str(ex) or type(ex).__name__)
+
             return
 
         if not isValid(self) or self._terminal:
             return
+
         if complete:
             self._finish('')
         elif any(lease.releaseError for lease in self.manager._pendingReleases):
@@ -1428,8 +1439,10 @@ class ConnectionStopOperation(QtCore.QObject):
     def _finish(self, error):
         self._terminal = True
         self._timer.stop()
+
         if self.manager._activeStopOperation is self:
             self.manager._activeStopOperation = None
+
         try:
             self.finished.emit(self, error)
         finally:
@@ -1602,6 +1615,7 @@ class ConnectionManager(Mixins.CleanupOnExit):
         """
         if self._activeStopOperation is not None and isValid(self._activeStopOperation):
             raise RuntimeError('Connection runtime cleanup is incomplete')
+
         self._retryPendingReleases()
 
         self._lastStartError = ''
@@ -1643,6 +1657,7 @@ class ConnectionManager(Mixins.CleanupOnExit):
         """Create and schedule one manager-owned startup transaction."""
         if self._activeStopOperation is not None and isValid(self._activeStopOperation):
             raise RuntimeError('Connection runtime cleanup is incomplete')
+
         if self._activeStartOperation is not None:
             self._activeStartOperation.cancel()
 
@@ -2263,23 +2278,30 @@ class ConnectionManager(Mixins.CleanupOnExit):
             operation = ConnectionStopOperation(self)
         except BaseException:
             self._retainStopLeases()
+
             raise
+
         self._activeStopOperation = operation
+
         try:
             self._retainStopLeases()
             self.cancelStart()
         except BaseException:
             self._retainStopLeases()
             self._activeStopOperation = None
+
             operation.deleteLater()
+
             raise
 
         singleShotWeakly(0, operation, 'start')
+
         return operation
 
     def _retainStopLeases(self):
         self._pendingReleases.extend(reversed(self._leases))
         self._leases.clear()
+
         for lease in self._pendingReleases:
             lease.router.beginRelease()
 

Furious/Service/SingTUNHost.py

@@ -188,12 +188,14 @@ class SingTUNHostPlan:
                 self._done.set()
 
         self._worker = threading.Thread(target=run, name='sing-tun-host')
+
         try:
             self._worker.start()
         except BaseException:
             self._worker = None
             self._cleanupMethod = None
             self._done.set()
+
             raise
 
     @property
@@ -223,6 +225,7 @@ class SingTUNHostPlan:
             if self._worker.is_alive():
                 if self.platform == 'Linux':
                     return False
+
                 raise RuntimeError('sing-tun host preparation is still stopping')
 
             self._worker = None
@@ -585,6 +588,7 @@ class SingTUNHostPlan:
             or (self._linux is not None and self._linux.dnsTouched)
         ):
             return self._finishCleanupWork('restoreDNS')
+
         return True
 
     def _finishCleanupWork(self, method, *args):
@@ -599,9 +603,11 @@ class SingTUNHostPlan:
         if self._worker.is_alive():
             if self.platform == 'Linux':
                 return False
+
             raise RuntimeError('sing-tun host restoration is still running')
 
         completedMethod = self._cleanupMethod
+
         self._worker = None
         self._cleanupMethod = None
 
@@ -610,6 +616,7 @@ class SingTUNHostPlan:
 
         if completedMethod is not None and completedMethod != method:
             return self._finishCleanupWork(method, *args)
+
         return True
 
     def _recoverNative(self):

tests/test_architecture_refactors.py

@@ -1175,14 +1175,17 @@ class ApplicationLifecycleTransactionTest(TestCase):
 
                 eventFilter = CloseFilter(window)
                 window.installEventFilter(eventFilter)
+
                 try:
                     window.show()
                     processQtEvents()
+
                     with mock.patch(
                         'Furious.Application.DesktopApplication.AppQMessageBox',
                         messageType,
                     ):
                         window.close()
+
                     self.assertTrue(window.isVisible())
                     owner.exit.assert_called_once_with()
                 finally:

tests/test_runtime_lifecycle.py

@@ -111,16 +111,20 @@ class RuntimeLifecycleTest(TestCase):
             manager = ConnectionManager()
             runtime = _Runtime()
             manager._leases.append(RuntimeLease(runtime, RuntimeEventRouter()))
+
             operation = manager.stopAsync()
             operation.finished.connect(lambda current, _error: deleteQObject(current))
+
             with mock.patch('sys.excepthook') as qtErrors:
                 operation.start()
                 processQtEvents()
+
                 qtErrors.assert_not_called()
             self.assertFalse(isValid(operation))
             self.assertFalse(isValid(operation._timer))
             self.assertIsNone(manager._activeStopOperation)
             self.assertEqual(manager._pendingReleases, [])
+
             manager.cleanup()
 
     def testFailedStopObserverConstructionKeepsRuntimesOwnedAndBlocksReplacement(self):
@@ -128,16 +132,19 @@ class RuntimeLifecycleTest(TestCase):
         runtime = _Runtime()
         lease = RuntimeLease(runtime, RuntimeEventRouter())
         manager._leases.append(lease)
+
         with mock.patch(
             'Furious.Service.ConnectionManager.ConnectionStopOperation',
             side_effect=RuntimeError('observer factory'),
         ):
             with self.assertRaisesRegex(RuntimeError, 'observer factory'):
                 manager.stopAsync()
+
         self.assertEqual(manager.runtimes, [])
         self.assertEqual(manager._pendingReleases, [lease])
         self.assertEqual(lease.state, RuntimeLeaseState.Releasing)
         self.assertIsNone(manager._activeStopOperation)
+
         manager.cleanup()
 
     def testFailedLeaseReleaseRemainsOwnedUntilSuccessfulRetry(self):

tests/test_sing_tun.py

@@ -1344,38 +1344,47 @@ class SingTUNChildTest(unittest.TestCase):
         controllers = importlib.import_module(
             'Furious.Controllers.ConnectionController'
         )
+
         plan = SingTUNHostPlan({}, platform='Linux')
         tun = plan.configuration['tun_options']
         tun['Name'] = 'utun101'
         plan._linux = helper = SingTUNLinuxRouting(tun, 123456, 2000, threading.Event())
         helper.index = 77
         helper.dnsTouched = plan._activated = True
+
         entered, authorized = threading.Event(), threading.Event()
         present, refused, processes = [True], [denied], []
 
         def command(arguments):
             if not present[0]:
                 return '[]'
+
             if arguments == ['ip', '-N', '-j', 'link', 'show']:
                 return '[{"ifname":"utun101","ifindex":77}]'
             if arguments == ['ip', '-4', '-N', '-j', 'rule', 'show']:
                 return '[{"priority":2001,"src":"all","table":123456,"suppress_prefixlen":0}]'
+
             return '[]'
 
         def launch(arguments, **kwargs):
             self.assertEqual(arguments[:2], ['pkexec', 'bash'])
+
             process = mock.Mock()
             process.poll.return_value = None
 
             def wait(timeout=None):
                 self.assertIsNone(timeout)
                 entered.set()
+
                 if not authorized.wait(10):
                     raise RuntimeError('fixture authorization was not released')
+
                 code = 126 if refused[0] else 0
                 process.poll.return_value = code
+
                 if not code:
                     present[0] = False
+
                 return code
 
             process.wait.side_effect = wait
@@ -1401,10 +1410,12 @@ class SingTUNChildTest(unittest.TestCase):
             router.attach(runtime, manager)
             lease = RuntimeLease(runtime, router, applicationTUN=True)
             manager._leases.append(lease)
+
             controller = ConnectionController(
                 coreManager=manager, updatesManager=mock.Mock()
             )
             controller._state = ConnectionState.Connected
+
             try:
                 yield SimpleNamespace(
                     controller=controller,
@@ -1422,12 +1433,15 @@ class SingTUNChildTest(unittest.TestCase):
             finally:
                 refused[0] = False
                 authorized.set()
+
                 operation = manager.stopAsync()
                 operation.start()
                 self.assertTrue(waitFor(lambda: operation._terminal, timeout=10))
+
                 manager.cleanup()
                 controller.deleteLater()
                 processQtEvents()
+
                 qtErrors.assert_not_called()
 
     def testDisconnectAuthorizationLeavesQtResponsiveAndKeepsItsLease(self):
@@ -1435,15 +1449,18 @@ class SingTUNChildTest(unittest.TestCase):
             controller, runtime = fixture.controller, fixture.runtime
             states, ticks = [], []
             controller.stateChanged.connect(states.append)
+
             self.assertTrue(controller.startDisconnection())
             self.assertTrue(fixture.entered.wait(2))
             QtCore.QTimer.singleShot(0, lambda: ticks.append(True))
             self.assertTrue(waitFor(lambda: bool(ticks)))
+
             self.assertEqual(controller.state, ConnectionState.Disconnecting)
             self.assertTrue(runtime.cleanupPending)
             self.assertFalse(runtime._control.closed)
             self.assertEqual(fixture.manager._pendingReleases, [fixture.lease])
             self.assertEqual(len(fixture.processes), 1)
+
             with self.assertRaisesRegex(RuntimeError, 'cleanup is incomplete'):
                 fixture.manager.startAsync(None, '')
 
@@ -1467,6 +1484,7 @@ class SingTUNChildTest(unittest.TestCase):
             fixture.controller.startDisconnection()
             self.assertTrue(fixture.entered.wait(2))
             fixture.authorized.set()
+
             self.assertTrue(
                 waitFor(
                     lambda: fixture.controller.state is ConnectionState.Disconnected
@@ -1485,6 +1503,7 @@ class SingTUNChildTest(unittest.TestCase):
             self.assertTrue(fixture.entered.wait(2))
             processQtEvents()
             reconnect.assert_not_called()
+
             fixture.authorized.set()
             self.assertTrue(waitFor(lambda: reconnect.call_count == 1))
             self.assertEqual(fixture.manager._pendingReleases, [])
@@ -1493,8 +1512,10 @@ class SingTUNChildTest(unittest.TestCase):
         with self._delayedLinuxRecovery() as fixture:
             fixture.controller.startDisconnection()
             self.assertTrue(fixture.entered.wait(2))
+
             operation = fixture.controller._stopOperation
             deleteQObject(operation)
+
             self.assertFalse(isValid(operation._timer))
             self.assertIsNone(fixture.manager._activeStopOperation)
             self.assertEqual(fixture.manager._pendingReleases, [fixture.lease])
@@ -1506,12 +1527,14 @@ class SingTUNChildTest(unittest.TestCase):
             AppSettings.turnON_('Connect')
             results, ticks = [], []
             fixture.controller.shutdownFinished.connect(results.append)
+
             self.assertIs(fixture.controller.shutdown(), False)
             self.assertTrue(fixture.entered.wait(2))
             QtCore.QTimer.singleShot(0, lambda: ticks.append(True))
             self.assertTrue(waitFor(lambda: bool(ticks)))
             self.assertEqual(results, [])
             self.assertTrue(AppSettings.isStateON_('Connect'))
+
             fixture.authorized.set()
             self.assertTrue(waitFor(lambda: results == [True]))
             self.assertTrue(AppSettings.isStateON_('Connect'))
@@ -1522,11 +1545,13 @@ class SingTUNChildTest(unittest.TestCase):
         controllers = importlib.import_module(
             'Furious.Controllers.ConnectionController'
         )
+
         entered, released = threading.Event(), threading.Event()
         plan = SingTUNHostPlan({}, platform='Linux')
 
         def prepare(_addresses):
             entered.set()
+
             if not released.wait(5):
                 raise RuntimeError('fixture preparation was not released')
 
@@ -1541,6 +1566,7 @@ class SingTUNChildTest(unittest.TestCase):
             router.attach(runtime, manager)
             lease = RuntimeLease(runtime, router, applicationTUN=True)
             manager._leases.append(lease)
+
             controller = ConnectionController(
                 coreManager=manager, updatesManager=mock.Mock()
             )
@@ -1555,6 +1581,7 @@ class SingTUNChildTest(unittest.TestCase):
                 self.assertTrue(waitFor(lambda: bool(ticks)))
                 self.assertEqual(controller.state, ConnectionState.Disconnecting)
                 self.assertEqual(manager._pendingReleases, [lease])
+
                 released.set()
                 self.assertTrue(
                     waitFor(lambda: controller.state is ConnectionState.Disconnected)
@@ -1563,9 +1590,11 @@ class SingTUNChildTest(unittest.TestCase):
                 self.assertTrue(runtime._control.closed)
             finally:
                 released.set()
+
                 cleanup = manager.stopAsync()
                 cleanup.start()
                 self.assertTrue(waitFor(lambda: cleanup._terminal))
+
                 manager.cleanup()
                 controller.deleteLater()
                 processQtEvents()

tests/test_sing_tun_linux_host.py

@@ -161,8 +161,10 @@ class SingTUNLinuxHostTest(unittest.TestCase):
         def wait(timeout=None):
             self.assertIsNone(timeout)
             entered.set()
+
             if not authorized.wait(5):
                 raise RuntimeError('fixture authorization was not released')
+
             process.poll.return_value = 0
             return 0
 
@@ -173,6 +175,7 @@ class SingTUNLinuxHostTest(unittest.TestCase):
                 self.host.execute([['ip', 'link', 'delete', 'dev', 'utun101']])
             except Exception as ex:
                 # Any non-exit exceptions
+
                 errors.append(ex)
             finally:
                 complete.set()
@@ -182,6 +185,7 @@ class SingTUNLinuxHostTest(unittest.TestCase):
         ), mock.patch.object(self.module.subprocess, 'Popen', return_value=process):
             worker = threading.Thread(target=recover)
             worker.start()
+
             try:
                 self.assertTrue(entered.wait(2))
                 self.assertFalse(complete.is_set())
@@ -269,8 +273,10 @@ class SingTUNLinuxHostTest(unittest.TestCase):
 
         self.assertIs(self.host._process, process)
         self.assertTrue(directory.exists())
+
         process.poll.return_value = 126
         self.host.drain()
+
         self.assertFalse(directory.exists())
 
     def testLiveForwarderDnsCannotBeReportedAsRestored(self):