From dba7e78928cbda8f0428b264187118f861d2b9c8 Mon Sep 17 00:00:00 2001 From: Chris Hennes Date: Fri, 18 Sep 2026 13:00:57 -0500 Subject: [PATCH 1/5] Use threading.Event instead of bool (cherry picked from commit 12b0a34e0df28e4c21d154423f96cb0c43f2c2f1) --- AddonManagerTest/app/test_network_manager.py | 4 ++-- NetworkManager.py | 13 +++++-------- addonmanager_workers_utility.py | 12 +++++------- 3 files changed, 12 insertions(+), 17 deletions(-) diff --git a/AddonManagerTest/app/test_network_manager.py b/AddonManagerTest/app/test_network_manager.py index b83a76c4..2e7d3394 100644 --- a/AddonManagerTest/app/test_network_manager.py +++ b/AddonManagerTest/app/test_network_manager.py @@ -50,7 +50,7 @@ def __init__(self): def await_response(self, index: int, quiet: bool) -> None: """Set up the state that blocking_get() creates while it waits for a response.""" - self.synchronous_complete[index] = False + self.synchronous_complete[index] = threading.Event() if quiet: self.synchronous_quiet.add(index) @@ -87,7 +87,7 @@ def test_quiet_request_still_completes(self): self.requests.complete_request(1, 404, None) - self.assertTrue(self.requests.synchronous_complete[1]) + self.assertTrue(self.requests.synchronous_complete[1].is_set()) def test_quiet_does_not_affect_other_requests(self): """Marking one request quiet does not suppress the reporting of any other request.""" diff --git a/NetworkManager.py b/NetworkManager.py index 1f0fd2fd..a50bab02 100644 --- a/NetworkManager.py +++ b/NetworkManager.py @@ -138,7 +138,7 @@ def __init__(self): # We support an arbitrary number of threads using synchronous GET calls: self.synchronous_lock = threading.Lock() - self.synchronous_complete: Dict[int, bool] = {} + self.synchronous_complete: Dict[int, threading.Event] = {} self.synchronous_result_data: Dict[int, QtCore.QByteArray] = {} self.synchronous_quiet: Set[int] = set() # Indices whose failures are not reported @@ -368,8 +368,9 @@ def blocking_get( """ current_index = next(self.counting_iterator) # A thread-safe counter + completion = threading.Event() with self.synchronous_lock: - self.synchronous_complete[current_index] = False + self.synchronous_complete[current_index] = completion if quiet: self.synchronous_quiet.add(current_index) @@ -381,13 +382,9 @@ def blocking_get( ) ) self.__request_queued.emit() - while True: + while not completion.wait(0.1): if QtCore.QThread.currentThread().isInterruptionRequested(): return None - QtCore.QCoreApplication.processEvents() - with self.synchronous_lock: - if self.synchronous_complete[current_index]: - break with self.synchronous_lock: self.synchronous_complete.pop(current_index) @@ -423,7 +420,7 @@ def __synchronous_process_completion( ).format(code) + "\n" ) - self.synchronous_complete[index] = True + self.synchronous_complete[index].set() @staticmethod def __create_get_request( diff --git a/addonmanager_workers_utility.py b/addonmanager_workers_utility.py index 30aa6923..0477f90a 100644 --- a/addonmanager_workers_utility.py +++ b/addonmanager_workers_utility.py @@ -30,7 +30,7 @@ from PySide2 import QtCore import NetworkManager -import time +import threading import addonmanager_freecad_interface as fci @@ -48,7 +48,7 @@ class ConnectionChecker(QtCore.QThread): def __init__(self): QtCore.QThread.__init__(self) self.setObjectName("ConnectionChecker") - self.done = False + self.response_received = threading.Event() self.request_id = None self.data = None @@ -58,19 +58,17 @@ def run(self): fci.Console.PrintLog("Checking network connection...\n") url = fci.Preferences().get("status_test_url") - self.done = False + self.response_received.clear() NetworkManager.AM_NETWORK_MANAGER.completed.connect(self.connection_data_received) self.request_id = NetworkManager.AM_NETWORK_MANAGER.submit_unmonitored_get( url, timeout_ms=30000, disable_cache=True ) - while not self.done: + while not self.response_received.wait(0.1): if QtCore.QThread.currentThread().isInterruptionRequested(): fci.Console.PrintLog("Connection check cancelled\n") NetworkManager.AM_NETWORK_MANAGER.abort(self.request_id) self.disconnect_network_manager() return - QtCore.QCoreApplication.processEvents() - time.sleep(0.1) if not self.data: self.failure.emit( translate( @@ -91,7 +89,7 @@ def connection_data_received(self, id: int, status: int, data: QtCore.QByteArray else: fci.Console.PrintWarning(f"No data received: status returned was {status}\n") self.data = None - self.done = True + self.response_received.set() def disconnect_network_manager(self): NetworkManager.AM_NETWORK_MANAGER.completed.disconnect(self.connection_data_received) From 9f3b38564d5ea84b3c0efda286a1e915aa111635 Mon Sep 17 00:00:00 2001 From: Chris Hennes Date: Fri, 18 Sep 2026 13:18:31 -0500 Subject: [PATCH 2/5] Make ConnectionChecker retire old worker (cherry picked from commit 47025e30ea4e0084c76ee7ee009e10d68db90680) --- .../gui/test_connection_checker.py | 125 ++++++++++++++++++ AddonManagerTest/gui/test_workers_utility.py | 53 ++++++++ addonmanager_connection_checker.py | 20 ++- addonmanager_workers_utility.py | 2 + 4 files changed, 199 insertions(+), 1 deletion(-) create mode 100644 AddonManagerTest/gui/test_connection_checker.py diff --git a/AddonManagerTest/gui/test_connection_checker.py b/AddonManagerTest/gui/test_connection_checker.py new file mode 100644 index 00000000..7e73dedd --- /dev/null +++ b/AddonManagerTest/gui/test_connection_checker.py @@ -0,0 +1,125 @@ +# SPDX-License-Identifier: LGPL-2.1-or-later +# SPDX-FileCopyrightText: 2026 FreeCAD Project Association +# SPDX-FileNotice: Part of the AddonManager. + +################################################################################ +# # +# This addon is free software: you can redistribute it and/or modify # +# it under the terms of the GNU Lesser General Public License as # +# published by the Free Software Foundation, either version 2.1 # +# of the License, or (at your option) any later version. # +# # +# This addon is distributed in the hope that it will be useful, # +# but WITHOUT ANY WARRANTY; without even the implied warranty # +# of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. # +# See the GNU Lesser General Public License for more details. # +# # +# You should have received a copy of the GNU Lesser General Public # +# License along with this addon. If not, see https://www.gnu.org/licenses # +# # +################################################################################ + +"""Tests for the ConnectionCheckerGUI class.""" + +import unittest +from unittest.mock import patch + +from PySideWrapper import QtCore + +from addonmanager_connection_checker import ConnectionCheckerGUI + + +class FakeConnectionChecker(QtCore.QObject): + """Stands in for the ConnectionChecker worker, without a thread or a network connection.""" + + success = QtCore.Signal() + failure = QtCore.Signal(str) + + def __init__(self): + super().__init__() + self.running = False + self.interruption_requested = False + + def start(self): + self.running = True + + def isRunning(self) -> bool: + return self.running + + def isFinished(self) -> bool: + return not self.running + + def requestInterruption(self): + self.interruption_requested = True + + +class TestConnectionCheckerGUI(unittest.TestCase): + """A QThread cannot be restarted, so every check has to get a worker of its own.""" + + def setUp(self): + checker_patch = patch( + "addonmanager_connection_checker.ConnectionChecker", FakeConnectionChecker + ) + checker_patch.start() + self.addCleanup(checker_patch.stop) + self.checker_gui = ConnectionCheckerGUI() + self.addCleanup(self._mark_all_workers_finished) + + def _mark_all_workers_finished(self): + """Stop the delayed message that start() arms from finding a worker still running.""" + for checker in [self.checker_gui.connection_checker, *self.checker_gui.retired_checkers]: + if checker is not None: + checker.running = False + + def test_each_check_gets_a_new_worker(self): + self.checker_gui.start() + first_checker = self.checker_gui.connection_checker + first_checker.running = False + + self.checker_gui.start() + + self.assertIsNot(first_checker, self.checker_gui.connection_checker) + + def test_new_worker_is_started(self): + self.checker_gui.start() + + self.assertTrue(self.checker_gui.connection_checker.isRunning()) + + def test_unfinished_worker_is_asked_to_stop(self): + self.checker_gui.start() + first_checker = self.checker_gui.connection_checker + + self.checker_gui.start() + + self.assertTrue(first_checker.interruption_requested) + + def test_unfinished_worker_is_kept_alive(self): + """The worker still owns a network request, so it must outlive the check that started it.""" + self.checker_gui.start() + first_checker = self.checker_gui.connection_checker + + self.checker_gui.start() + + self.assertIn(first_checker, self.checker_gui.retired_checkers) + + def test_finished_worker_is_not_kept(self): + self.checker_gui.start() + self.checker_gui.connection_checker.running = False + + self.checker_gui.start() + + self.assertEqual([], self.checker_gui.retired_checkers) + + def test_retired_workers_are_released_once_they_finish(self): + self.checker_gui.start() + first_checker = self.checker_gui.connection_checker + self.checker_gui.start() + first_checker.running = False + + self.checker_gui.start() + + self.assertNotIn(first_checker, self.checker_gui.retired_checkers) + + +if __name__ == "__main__": + unittest.main() diff --git a/AddonManagerTest/gui/test_workers_utility.py b/AddonManagerTest/gui/test_workers_utility.py index 74899815..e765210e 100644 --- a/AddonManagerTest/gui/test_workers_utility.py +++ b/AddonManagerTest/gui/test_workers_utility.py @@ -21,6 +21,8 @@ import unittest import os +from unittest.mock import MagicMock, patch + from addonmanager_workers_utility import ConnectionChecker try: @@ -78,3 +80,54 @@ def connection_succeeded(self): def connection_failed(self): self.last_result = "FAILURE" + + +class TestConnectionCheckerRun(unittest.TestCase): + """The Addon Manager checks the connection every time it is opened, so a worker may be asked + to run more than once in a FreeCAD session.""" + + def setUp(self): + network_patch = patch("NetworkManager.AM_NETWORK_MANAGER", MagicMock()) + self.mock_network_manager = network_patch.start() + self.addCleanup(network_patch.stop) + self.checker = ConnectionChecker() + + def _respond_with(self, data): + """Complete the request as soon as it is submitted, recording what the worker knew at + that moment.""" + self.data_when_submitted = [] + + def submit(url, timeout_ms=30000, disable_cache=False): + self.data_when_submitted.append(self.checker.data) + self.checker.data = data + self.checker.response_received.set() + return 7 + + self.mock_network_manager.submit_unmonitored_get.side_effect = submit + + def test_response_from_a_previous_check_is_discarded(self): + """A response held over from an earlier check must not stand in for one that never + arrived.""" + self.checker.data = b"OK\n" + self._respond_with(None) + failures = [] + self.checker.failure.connect(failures.append) + + self.checker.run() + + self.assertEqual([None], self.data_when_submitted) + self.assertEqual(1, len(failures)) + + def test_response_to_a_previous_request_is_not_accepted(self): + """The worker listens again before it has an id for its new request, so a late response + to the request made by an earlier check can arrive in between.""" + self.checker.request_id = 7 + id_when_listening_resumed = [] + self.mock_network_manager.completed.connect.side_effect = ( + lambda slot: id_when_listening_resumed.append(self.checker.request_id) + ) + self._respond_with(b"OK\n") + + self.checker.run() + + self.assertEqual([None], id_when_listening_resumed) diff --git a/addonmanager_connection_checker.py b/addonmanager_connection_checker.py index ad48cc0d..36fcf114 100644 --- a/addonmanager_connection_checker.py +++ b/addonmanager_connection_checker.py @@ -42,7 +42,8 @@ def __init__(self): super().__init__() # Check the connection in a new thread, so FreeCAD stays responsive - self.connection_checker = ConnectionChecker() + self.connection_checker = None + self.retired_checkers = [] self.signals_connected = False self.connection_message_timer = None @@ -50,6 +51,8 @@ def __init__(self): def start(self): """Start the connection check""" + self._retire_current_checker() + self.connection_checker = ConnectionChecker() self.connection_checker.success.connect(self._check_succeeded) self.connection_checker.failure.connect(self._network_connection_failed) self.signals_connected = True @@ -58,6 +61,21 @@ def start(self): # If it takes longer than a half second to check the connection, show a message: QtCore.QTimer.singleShot(500, self._show_connection_check_message) + def _retire_current_checker(self): + """Set the worker from the previous check aside. A QThread cannot be restarted, and a + cancelled check may still be winding down, so one that is still running is asked to stop + and is kept alive until it has.""" + self.retired_checkers = [ + checker for checker in self.retired_checkers if not checker.isFinished() + ] + if self.connection_checker is None: + return + self._disconnect_signals() + if self.connection_checker.isRunning(): + self.connection_checker.requestInterruption() + self.retired_checkers.append(self.connection_checker) + self.connection_checker = None + def _show_connection_check_message(self): """Display a message informing the user that the check is in process""" if not self.connection_checker.isFinished(): diff --git a/addonmanager_workers_utility.py b/addonmanager_workers_utility.py index 0477f90a..91d19dc2 100644 --- a/addonmanager_workers_utility.py +++ b/addonmanager_workers_utility.py @@ -58,6 +58,8 @@ def run(self): fci.Console.PrintLog("Checking network connection...\n") url = fci.Preferences().get("status_test_url") + self.data = None + self.request_id = None self.response_received.clear() NetworkManager.AM_NETWORK_MANAGER.completed.connect(self.connection_data_received) self.request_id = NetworkManager.AM_NETWORK_MANAGER.submit_unmonitored_get( From 828e3c93240a7ec85d964e8c98de5a9f52a82578 Mon Sep 17 00:00:00 2001 From: Chris Hennes Date: Fri, 18 Sep 2026 13:25:55 -0500 Subject: [PATCH 3/5] Improve testing of NetworkManager (cherry picked from commit 74c287791f95a70f1909d0f17f83876847ab16c5) --- AddonManagerTest/gui/gui_mocks.py | 38 +++++ AddonManagerTest/gui/test_workers_utility.py | 139 +++++++++++++------ 2 files changed, 134 insertions(+), 43 deletions(-) diff --git a/AddonManagerTest/gui/gui_mocks.py b/AddonManagerTest/gui/gui_mocks.py index 763fa03b..ff20a187 100644 --- a/AddonManagerTest/gui/gui_mocks.py +++ b/AddonManagerTest/gui/gui_mocks.py @@ -201,3 +201,41 @@ def abort_all(self): def abort(self, index: int): pass + + +class FakeNetworkManager(QtCore.QObject): + """A stand-in for the NetworkManager singleton that never touches the network. + + Requests are recorded and answered by whichever thread calls answer_pending_requests(), so a + test decides when a worker sees its response. The completed signal is a real Qt signal, and is + therefore delivered across threads exactly as the real one is.""" + + completed = QtCore.Signal(int, int, QtCore.QByteArray) + + def __init__(self, status: int = 200, response: bytes = b"OK"): + super().__init__() + self.status = status + self.response = response + self.requested_urls = [] + self.pending_requests = [] + self.aborted_requests = [] + self.next_index = 0 + + def submit_unmonitored_get( + self, url: str, timeout_ms: int = 30000, disable_cache: bool = False + ) -> int: + index = self.next_index + self.next_index += 1 + self.requested_urls.append(url) + self.pending_requests.append(index) + return index + + def answer_pending_requests(self) -> None: + """Complete every request that has been submitted and not yet answered or aborted.""" + while self.pending_requests: + index = self.pending_requests.pop(0) + self.completed.emit(index, self.status, QtCore.QByteArray(self.response)) + + def abort(self, index: int) -> None: + self.aborted_requests.append(index) + self.pending_requests = [pending for pending in self.pending_requests if pending != index] diff --git a/AddonManagerTest/gui/test_workers_utility.py b/AddonManagerTest/gui/test_workers_utility.py index e765210e..fed95597 100644 --- a/AddonManagerTest/gui/test_workers_utility.py +++ b/AddonManagerTest/gui/test_workers_utility.py @@ -19,8 +19,8 @@ # # ################################################################################ +import time import unittest -import os from unittest.mock import MagicMock, patch from addonmanager_workers_utility import ConnectionChecker @@ -33,53 +33,106 @@ except ImportError: from PySide2 import QtCore -import NetworkManager +import addonmanager_freecad_interface as fci +from AddonManagerTest.gui.gui_mocks import FakeNetworkManager -class TestWorkersUtility(unittest.TestCase): +WORKER_TIMEOUT_MS = 5000 - MODULE = "test_workers_utility" # file name without extension - @unittest.skip("Test is slow and uses the network: refactor!") +class TestConnectionChecker(unittest.TestCase): + """The connection checker runs in a thread of its own, so it is driven here the way the Addon + Manager drives it: start the worker, then service the main thread until it reports a result.""" + def setUp(self): - self.test_dir = os.path.join(os.path.dirname(__file__), "..", "data") - self.last_result = None - - url = "https://api.github.com/zen" - NetworkManager.InitializeNetworkManager() - result = NetworkManager.AM_NETWORK_MANAGER.blocking_get(url) - if result is None: - self.skipTest("No active internet connection detected") - - def test_connection_checker_basic(self): - """Tests the connection checking worker's basic operation: does not exit until worker thread completes""" - worker = ConnectionChecker() - worker.success.connect(self.connection_succeeded) - worker.failure.connect(self.connection_failed) - self.last_result = None - worker.start() - while worker.isRunning(): - QtCore.QCoreApplication.processEvents(QtCore.QEventLoop.AllEvents, 50) - QtCore.QCoreApplication.processEvents(QtCore.QEventLoop.AllEvents) - self.assertEqual(self.last_result, "SUCCESS") - - def test_connection_checker_thread_interrupt(self): - worker = ConnectionChecker() - worker.success.connect(self.connection_succeeded) - worker.failure.connect(self.connection_failed) - self.last_result = None - worker.start() - worker.requestInterruption() - while worker.isRunning(): - QtCore.QCoreApplication.processEvents(QtCore.QEventLoop.AllEvents, 50) - QtCore.QCoreApplication.processEvents(QtCore.QEventLoop.AllEvents) - self.assertIsNone(self.last_result, "Requesting interruption of thread failed to interrupt") - - def connection_succeeded(self): - self.last_result = "SUCCESS" - - def connection_failed(self): - self.last_result = "FAILURE" + self.network_manager = FakeNetworkManager() + network_patch = patch("NetworkManager.AM_NETWORK_MANAGER", self.network_manager) + network_patch.start() + self.addCleanup(network_patch.stop) + + self.result = None + self.worker = ConnectionChecker() + self.worker.success.connect(self._record_success) + self.worker.failure.connect(self._record_failure) + self.addCleanup(self._stop_worker) + + def _record_success(self): + self.result = "SUCCESS" + + def _record_failure(self, _message: str): + self.result = "FAILURE" + + def _stop_worker(self): + if self.worker.isRunning(): + self.worker.requestInterruption() + self.worker.wait(WORKER_TIMEOUT_MS) + + def _run_until(self, condition, answer_requests: bool = True) -> bool: + """Process events on this thread while the worker runs on its own, answering its request + once it has been submitted. Returns whether the condition was met before the timeout.""" + deadline = time.monotonic() + WORKER_TIMEOUT_MS / 1000 + while not condition() and time.monotonic() < deadline: + if answer_requests and self.worker.request_id is not None: + self.network_manager.answer_pending_requests() + QtCore.QCoreApplication.processEvents(QtCore.QEventLoop.AllEvents, 10) + return condition() + + def _run_until_finished(self, answer_requests: bool = True) -> bool: + return self._run_until(lambda: not self.worker.isRunning(), answer_requests) + + def test_reachable_server_reports_success(self): + self.worker.start() + + self.assertTrue(self._run_until(lambda: self.result is not None)) + self.assertEqual("SUCCESS", self.result) + + def test_server_is_asked_for_the_status_url(self): + self.worker.start() + self._run_until(lambda: self.result is not None) + + self.assertEqual( + [fci.Preferences().get("status_test_url")], self.network_manager.requested_urls + ) + + def test_error_response_reports_failure(self): + self.network_manager.status = 404 + + self.worker.start() + + self.assertTrue(self._run_until(lambda: self.result is not None)) + self.assertEqual("FAILURE", self.result) + + def test_worker_exits_when_its_check_is_done(self): + self.worker.start() + self._run_until(lambda: self.result is not None) + + self.assertTrue(self._run_until_finished()) + + def test_interrupted_check_reports_nothing(self): + self.worker.start() + self.worker.requestInterruption() + + self.assertTrue(self._run_until_finished(answer_requests=False)) + self.assertIsNone(self.result) + + def test_interrupted_check_abandons_its_request(self): + self.worker.start() + self._run_until(lambda: self.worker.request_id is not None, answer_requests=False) + self.worker.requestInterruption() + + self.assertTrue(self._run_until_finished(answer_requests=False)) + self.assertEqual([self.worker.request_id], self.network_manager.aborted_requests) + + def test_worker_stops_listening_once_its_check_is_over(self): + """A response that arrives after the check has finished must not be handed to a worker + whose thread has already exited.""" + self.worker.start() + self._run_until(lambda: self.result is not None) + self._run_until_finished() + + self.network_manager.completed.emit(self.worker.request_id, 200, QtCore.QByteArray(b"LATE")) + + self.assertEqual(b"OK", self.worker.data) class TestConnectionCheckerRun(unittest.TestCase): From 20e27ced93dc2be0c03c8f78ae9150f4365daa7e Mon Sep 17 00:00:00 2001 From: Chris Hennes Date: Fri, 18 Sep 2026 13:51:22 -0500 Subject: [PATCH 4/5] Confine every QNAM operation to its own thread (cherry picked from commit ac357af5df474dcc0618214e3e84651c26d62237) --- .../gui/test_network_manager_threading.py | 220 ++++++++++++++++++ NetworkManager.py | 82 ++++++- 2 files changed, 295 insertions(+), 7 deletions(-) create mode 100644 AddonManagerTest/gui/test_network_manager_threading.py diff --git a/AddonManagerTest/gui/test_network_manager_threading.py b/AddonManagerTest/gui/test_network_manager_threading.py new file mode 100644 index 00000000..b4d6218b --- /dev/null +++ b/AddonManagerTest/gui/test_network_manager_threading.py @@ -0,0 +1,220 @@ +# SPDX-License-Identifier: LGPL-2.1-or-later +# SPDX-FileCopyrightText: 2026 FreeCAD Project Association +# SPDX-FileNotice: Part of the AddonManager. + +################################################################################ +# # +# This addon is free software: you can redistribute it and/or modify # +# it under the terms of the GNU Lesser General Public License as # +# published by the Free Software Foundation, either version 2.1 # +# of the License, or (at your option) any later version. # +# # +# This addon is distributed in the hope that it will be useful, # +# but WITHOUT ANY WARRANTY; without even the implied warranty # +# of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. # +# See the GNU Lesser General Public License for more details. # +# # +# You should have received a copy of the GNU Lesser General Public # +# License along with this addon. If not, see https://www.gnu.org/licenses # +# # +################################################################################ + +"""Tests that the NetworkManager only uses its QNetworkAccessManager from the thread that owns +it. Work from any other thread has to be handed over through the event loop.""" + +import threading +import unittest +from unittest.mock import patch + +from PySideWrapper import QtCore + +import NetworkManager + +TEST_URL = "https://example.com/test" +THREAD_TIMEOUT_MS = 5000 + + +class FakeReply(QtCore.QObject): + """Stands in for a QNetworkReply, offering only what launching and aborting a request use.""" + + finished = QtCore.Signal() + sslErrors = QtCore.Signal(list) + readyRead = QtCore.Signal() + downloadProgress = QtCore.Signal(int, int) + + def __init__(self): + super().__init__() + self.running = True + self.aborted = False + + def isRunning(self) -> bool: + return self.running + + def abort(self) -> None: + self.aborted = True + self.running = False + + +class FakeQNAM: + """Stands in for a QNetworkAccessManager, recording which thread each request was made on.""" + + def __init__(self): + self.replies = [] + self.launching_threads = [] + + def get(self, _request) -> FakeReply: + return self._new_reply() + + def head(self, _request) -> FakeReply: + return self._new_reply() + + def _new_reply(self) -> FakeReply: + self.launching_threads.append(threading.get_ident()) + reply = FakeReply() + self.replies.append(reply) + return reply + + +class CallFromAnotherThread(QtCore.QThread): + """Call into the network manager the way an Addon Manager worker does: from a thread of its + own, while the thread that owns the manager is busy elsewhere.""" + + def __init__(self, function): + super().__init__() + self.function = function + self.result = None + + def run(self): + self.result = self.function() + + +class NetworkManagerTestCase(unittest.TestCase): + """Builds a NetworkManager whose QNetworkAccessManager is replaced by a fake, so that no + request ever reaches the network. Proxy setup is skipped: outside FreeCAD it prompts on the + command line.""" + + def setUp(self): + proxy_patch = patch.object(NetworkManager.NetworkManager, "_setup_proxy") + proxy_patch.start() + self.addCleanup(proxy_patch.stop) + + self.manager = NetworkManager.NetworkManager() + self.manager.QNAM = FakeQNAM() + + +class TestRequestsAreLaunchedByTheOwningThread(NetworkManagerTestCase): + """A request launched by a worker thread gives the resulting reply, and its transfer timer, + the wrong thread affinity. That is the fault reported in issue 492.""" + + def setUp(self): + super().setUp() + self.owning_thread = threading.get_ident() + + def _call_from_another_thread(self, function): + """Run the call and wait for it to return, without letting this thread process events.""" + caller = CallFromAnotherThread(function) + caller.start() + self.assertTrue(caller.wait(THREAD_TIMEOUT_MS), "The calling thread never returned") + return caller.result + + def _launch_a_request(self) -> int: + index = self.manager.submit_unmonitored_get(TEST_URL) + QtCore.QCoreApplication.processEvents() + return index + + def test_request_from_another_thread_is_not_launched_by_it(self): + self._call_from_another_thread(lambda: self.manager.submit_unmonitored_get(TEST_URL)) + + self.assertEqual([], self.manager.QNAM.launching_threads) + + def test_request_from_another_thread_is_launched_by_the_owning_thread(self): + self._call_from_another_thread(lambda: self.manager.submit_unmonitored_get(TEST_URL)) + + QtCore.QCoreApplication.processEvents() + + self.assertEqual([self.owning_thread], self.manager.QNAM.launching_threads) + + def test_monitored_request_from_another_thread_is_launched_by_the_owning_thread(self): + self._call_from_another_thread(lambda: self.manager.submit_monitored_get(TEST_URL)) + + QtCore.QCoreApplication.processEvents() + + self.assertEqual([self.owning_thread], self.manager.QNAM.launching_threads) + + def test_size_query_from_another_thread_is_launched_by_the_owning_thread(self): + self._call_from_another_thread(lambda: self.manager.query_download_size(TEST_URL)) + + QtCore.QCoreApplication.processEvents() + + self.assertEqual([self.owning_thread], self.manager.QNAM.launching_threads) + + def test_abort_from_another_thread_is_run_by_the_owning_thread(self): + index = self._launch_a_request() + reply = self.manager.QNAM.replies[0] + + self._call_from_another_thread(lambda: self.manager.abort(index)) + self.assertFalse(reply.aborted, "The reply was aborted by the wrong thread") + QtCore.QCoreApplication.processEvents() + + self.assertTrue(reply.aborted) + + def test_abort_all_from_another_thread_is_run_by_the_owning_thread(self): + self._launch_a_request() + reply = self.manager.QNAM.replies[0] + + self._call_from_another_thread(self.manager.abort_all) + self.assertFalse(reply.aborted, "The reply was aborted by the wrong thread") + QtCore.QCoreApplication.processEvents() + + self.assertTrue(reply.aborted) + + def test_abort_on_the_owning_thread_takes_effect_at_once(self): + """The Addon Manager cancels its downloads from the GUI, and expects them to stop.""" + index = self._launch_a_request() + reply = self.manager.QNAM.replies[0] + + self.manager.abort(index) + + self.assertTrue(reply.aborted) + + def test_abort_all_on_the_owning_thread_takes_effect_at_once(self): + self._launch_a_request() + reply = self.manager.QNAM.replies[0] + + self.manager.abort_all() + + self.assertTrue(reply.aborted) + + +class TestBlockingRequestsAreRefusedOnTheOwningThread(NetworkManagerTestCase): + """A blocking request waits for the owning thread to launch it, so making one from that + thread can only hang.""" + + def setUp(self): + super().setUp() + console_patch = patch("NetworkManager.fci.Console") + self.mock_console = console_patch.start() + self.addCleanup(console_patch.stop) + + def test_blocking_request_returns_nothing(self): + self.assertIsNone(self.manager.blocking_get(TEST_URL)) + + def test_blocking_request_is_not_submitted(self): + self.manager.blocking_get(TEST_URL) + QtCore.QCoreApplication.processEvents() + + self.assertEqual([], self.manager.QNAM.launching_threads) + + def test_blocking_request_is_reported(self): + self.manager.blocking_get(TEST_URL) + + self.mock_console.PrintError.assert_called_once() + + def test_blocking_request_with_retries_is_refused_without_retrying(self): + self.manager.blocking_get_with_retries(TEST_URL, max_attempts=3) + + self.mock_console.PrintError.assert_called_once() + + +if __name__ == "__main__": + unittest.main() diff --git a/NetworkManager.py b/NetworkManager.py index a50bab02..04c3d609 100644 --- a/NetworkManager.py +++ b/NetworkManager.py @@ -48,7 +48,9 @@ # A secondary blocking interface is also provided, for very short network # accesses: the blocking_get() function blocks until the network transmission # is complete, directly returning a QByteArray object with the received data. -# Do not run on the main GUI thread! +# Do not run on the main GUI thread: a request made there can only be launched +# once control returns to the event loop, so blocking there would deadlock, and +# is refused. """ import threading @@ -125,6 +127,8 @@ class NetworkManager(QtCore.QObject): progress_complete = QtCore.Signal(int, int, os.PathLike) # Index, http response code, filename __request_queued = QtCore.Signal() + __abort_requested = QtCore.Signal(int) + __abort_all_requested = QtCore.Signal() def __init__(self): super().__init__() @@ -167,8 +171,52 @@ def __init__(self): # A helper connection for our blocking interface self.completed.connect(self.__complete_synchronous_request) - # Set up our worker connection - self.__request_queued.connect(self.__setup_network_request) + queued = QtCore.Qt.ConnectionType.QueuedConnection + self.__request_queued.connect(self.__setup_network_request, queued) + self.__abort_requested.connect(self._abort_request, queued) + self.__abort_all_requested.connect(self._abort_all_requests, queued) + + self._move_to_application_thread() + + def _move_to_application_thread(self): + """Put this object, and the QNetworkAccessManager it owns, on the thread running the + application. Whichever thread asks for the manager first is the one that constructs it, + and that may be a worker (which would later die).""" + application = QtCore.QCoreApplication.instance() + if application is None or self.thread() == application.thread(): + return + fci.Console.PrintWarning( + translate( + "AddonsInstaller", + "The Addon Manager network manager was created on a worker thread: moving it to" + " the main thread", + ) + + "\n" + ) + if self.diskCache.parent() is None: + self.diskCache.moveToThread(application.thread()) + self.QNAM.moveToThread(application.thread()) + self.moveToThread(application.thread()) + + def _on_owning_thread(self) -> bool: + """Whether the caller is running on the thread that owns the QNetworkAccessManager.""" + return QtCore.QThread.currentThread() == self.thread() + + def _refuse_blocking_request(self) -> bool: + """Whether a blocking request has to be turned away. One made on the owning thread stops + that thread from ever launching it, so it would wait forever.""" + if not self._on_owning_thread(): + return False + fci.Console.PrintError( + translate( + "AddonsInstaller", + "Addon Manager internal error: a blocking network request was made on the thread" + " that owns the network manager, where it could never complete. The request was" + " refused.", + ) + + "\n" + ) + return True def _setup_proxy(self): """Set up the proxy based on user preferences or prompts on command line""" @@ -221,8 +269,10 @@ def _setup_proxy_standalone(self): def __aboutToQuit(self): """Called when the application is about to quit. Not currently used.""" + @QtCore.Slot() def __setup_network_request(self): - """Get the next request off the queue and launch it.""" + """Get the next request off the queue and launch it. Runs on the thread that owns the + QNetworkAccessManager, whichever thread queued the request.""" try: item = self.queue.get_nowait() if item: @@ -283,7 +333,7 @@ def submit_unmonitored_get( is not called until the data transfer has finished and the connection is closed.""" current_index = next(self.counting_iterator) # A thread-safe counter - # Use a queue because we can only put things on the QNAM from the main event loop thread + # Use a queue because the QNAM may only be used from the thread that owns it self.queue.put( QueueItem( current_index, @@ -305,7 +355,7 @@ def submit_monitored_get( file when done with it (or move it into its final place, etc.).""" current_index = next(self.counting_iterator) # A thread-safe counter - # Use a queue because we can only put things on the QNAM from the main event loop thread + # Use a queue because the QNAM may only be used from the thread that owns it self.queue.put( QueueItem( current_index, @@ -338,6 +388,8 @@ def blocking_get_with_retries( raise ValueError("max_attempts must be greater than 0") if timeout_ms < 1: raise ValueError("timeout_ms must be greater than 0") + if self._refuse_blocking_request(): + return None attempt = 0 while True: attempt += 1 @@ -366,6 +418,8 @@ def blocking_get( :quiet: Do not report a failed request to the user: the file is allowed to be missing :returns: The response data, or None if the request failed after max_attempts attempts. """ + if self._refuse_blocking_request(): + return None current_index = next(self.counting_iterator) # A thread-safe counter completion = threading.Event() @@ -457,18 +511,32 @@ def __create_get_request( def abort_all(self): """Abort ALL network calls in progress, including clearing the queue""" + if self._on_owning_thread(): + self._abort_all_requests() + else: + self.__abort_all_requested.emit() + + @QtCore.Slot() + def _abort_all_requests(self): for reply in self.replies.values(): if reply.isRunning(): reply.abort() while True: try: - self.queue.get() + self.queue.get_nowait() self.queue.task_done() except queue.Empty: break def abort(self, index: int): """Abort a specific request""" + if self._on_owning_thread(): + self._abort_request(index) + else: + self.__abort_requested.emit(index) + + @QtCore.Slot(int) + def _abort_request(self, index: int): if index in self.replies and self.replies[index].isRunning(): self.replies[index].abort() elif index < self.__last_started_index: From 21456adf5e621c1c96cf54569a875094c6cef820 Mon Sep 17 00:00:00 2001 From: Chris Hennes Date: Wed, 23 Sep 2026 09:04:22 -0500 Subject: [PATCH 5/5] Switch from MagicMock to simple lambda in _setup_proxy test (cherry picked from commit 5f8de40414f6a64d5231a794a354f46aee59deb5) --- AddonManagerTest/gui/test_network_manager_threading.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/AddonManagerTest/gui/test_network_manager_threading.py b/AddonManagerTest/gui/test_network_manager_threading.py index b4d6218b..9a9bf31e 100644 --- a/AddonManagerTest/gui/test_network_manager_threading.py +++ b/AddonManagerTest/gui/test_network_manager_threading.py @@ -91,10 +91,13 @@ def run(self): class NetworkManagerTestCase(unittest.TestCase): """Builds a NetworkManager whose QNetworkAccessManager is replaced by a fake, so that no request ever reaches the network. Proxy setup is skipped: outside FreeCAD it prompts on the - command line.""" + command line. It is replaced with a plain function, not a mock: PySide6 6.10 crashes when it + builds the meta-object of a QObject class that holds a MagicMock.""" def setUp(self): - proxy_patch = patch.object(NetworkManager.NetworkManager, "_setup_proxy") + proxy_patch = patch.object( + NetworkManager.NetworkManager, "_setup_proxy", lambda _manager: None + ) proxy_patch.start() self.addCleanup(proxy_patch.stop)