Merge pull request #6828 from kozlovsky/fix/tribler_start_shutdown_gu…

…i_tests

Fix Tribler startup/shutdown and GUI tests stability

src/tribler/gui/app_manager.py

@@ -0,0 +1,32 @@
+from typing import Optional
+
+from PyQt5.QtWidgets import QApplication
+
+from tribler.gui.utilities import connect
+
+
+class AppManager:
+    """
+    A helper class that calls QApplication.quit()
+
+    You should never call `QApplication.quit()` directly. Call `app_manager.quit_application()` instead.
+    It is necessary to avoid runtime errors like "wrapped C/C++ object of type ... has been deleted".
+
+    After `app_manager.quit_application()` was called, it is not safe to access Qt objects anymore.
+    If a signal can be emitted during the application shutdown, you can check `app_manager.quitting_app` flag
+    inside the signal handler to be sure that it is still safe to access Qt objects.
+    """
+
+    def __init__(self, app: Optional[QApplication] = None):
+        self.quitting_app = False
+        if app is not None:
+            # app can be None in tests where Qt application is not created
+            connect(app.aboutToQuit, self.on_about_to_quit)
+
+    def on_about_to_quit(self):
+        self.quitting_app = True
+
+    def quit_application(self):
+        if not self.quitting_app:
+            self.quitting_app = True
+            QApplication.quit()

src/tribler/gui/core_manager.py

@@ -1,10 +1,11 @@
 import logging
 import sys
+from pathlib import Path
 
 from PyQt5.QtCore import QObject, QProcess, QProcessEnvironment, pyqtSignal
 from PyQt5.QtNetwork import QNetworkRequest
-from PyQt5.QtWidgets import QApplication
 
+from tribler.gui.app_manager import AppManager
 from tribler.gui.event_request_manager import EventRequestManager
 from tribler.gui.exceptions import CoreCrashedError
 from tribler.gui.tribler_request_manager import TriblerNetworkRequest
@@ -17,16 +18,17 @@ class CoreManager(QObject):
     a fake API will be started.
     """
 
-    def __init__(self, root_state_dir, api_port, api_key, error_handler):
+    def __init__(self, root_state_dir: Path, api_port: int, api_key: str, app_manager: AppManager,
+                 events_manager: EventRequestManager):
         QObject.__init__(self, None)
 
         self._logger = logging.getLogger(self.__class__.__name__)
-
+        self.app_manager = app_manager
         self.root_state_dir = root_state_dir
         self.core_process = None
         self.api_port = api_port
         self.api_key = api_key
-        self.events_manager = EventRequestManager(self.api_port, self.api_key, error_handler)
+        self.events_manager = events_manager
 
         self.upgrade_manager = None
         self.core_args = None
@@ -37,22 +39,14 @@ def __init__(self, root_state_dir, api_port, api_key, error_handler):
         self.core_connected = False
         self.shutting_down = False
         self.core_finished = False
-        self.quitting_app = False
 
         self.should_quit_app_on_core_finished = False
 
         self.use_existing_core = True
         self.last_core_stdout_output: str = ''
         self.last_core_stderr_output: str = ''
 
-        connect(self.events_manager.tribler_started, self.on_core_connected)
-        app = QApplication.instance()
-        if app is not None:
-            # app can be None in tests where Qt application is not created
-            connect(app.aboutToQuit, self.on_about_to_quit)
-
-    def on_about_to_quit(self):
-        self.quitting_app = True
+        connect(self.events_manager.core_connected, self.on_core_connected)
 
     def on_core_connected(self, _):
         if not self.core_finished:
@@ -108,7 +102,7 @@ def on_core_started(self):
         self.core_running = True
 
     def on_core_stdout_read_ready(self):
-        if self.quitting_app:
+        if self.app_manager.quitting_app:
             # Reading at this stage can lead to the error "wrapped C/C++ object of type QProcess has been deleted"
             return
 
@@ -122,7 +116,7 @@ def on_core_stdout_read_ready(self):
             pass
 
     def on_core_stderr_read_ready(self):
-        if self.quitting_app:
+        if self.app_manager.quitting_app:
             # Reading at this stage can lead to the error "wrapped C/C++ object of type QProcess has been deleted"
             return
 
@@ -154,21 +148,17 @@ def on_core_finished(self, exit_code, exit_status):
         self.core_finished = True
         if self.shutting_down:
             if self.should_quit_app_on_core_finished:
-                self.quit_application()
+                self.app_manager.quit_application()
         else:
             error_message = (
                 f"The Tribler core has unexpectedly finished with exit code {exit_code} and status: {exit_status}!\n"
                 f"Last core output: \n {self.last_core_stderr_output or self.last_core_stdout_output}"
             )
             self._logger.warning(error_message)
 
-            # Stop the event manager loop if it is running
-            if self.events_manager.connect_timer and self.events_manager.connect_timer.isActive():
-                self.events_manager.connect_timer.stop()
+            if not self.app_manager.quitting_app:
+                # Stop the event manager loop if it is running
+                if self.events_manager.connect_timer and self.events_manager.connect_timer.isActive():
+                    self.events_manager.connect_timer.stop()
 
             raise CoreCrashedError(error_message)
-
-    def quit_application(self):
-        if not self.quitting_app:
-            self.quitting_app = True
-            QApplication.quit()

src/tribler/gui/dialogs/feedbackdialog.py

@@ -1,3 +1,5 @@
+from __future__ import annotations
+
 import json
 import os
 import platform
@@ -6,13 +8,13 @@
 from collections import defaultdict
 
 from PyQt5 import uic
-from PyQt5.QtWidgets import QAction, QApplication, QDialog, QMessageBox, QTreeWidgetItem
+from PyQt5.QtWidgets import QAction, QDialog, QMessageBox, QTreeWidgetItem
 
 from tribler.core.components.reporter.reported_error import ReportedError
 from tribler.core.sentry_reporter.sentry_reporter import SentryReporter
 from tribler.core.sentry_reporter.sentry_scrubber import SentryScrubber
 from tribler.core.sentry_reporter.sentry_tools import CONTEXT_DELIMITER, LONG_TEXT_DELIMITER
-
+from tribler.gui.app_manager import AppManager
 from tribler.gui.event_request_manager import received_events
 from tribler.gui.sentry_mixin import AddBreadcrumbOnShowMixin
 from tribler.gui.tribler_action_menu import TriblerActionMenu
@@ -33,6 +35,7 @@ def __init__(  # pylint: disable=too-many-arguments, too-many-locals
         retrieve_error_message_from_stacktrace=False,
     ):
         QDialog.__init__(self, parent)
+        self.app_manager: AppManager = parent.app_manager
 
         uic.loadUi(get_ui_file_path('feedback_dialog.ui'), self)
 
@@ -193,5 +196,5 @@ def on_report_sent(self):
 
     def closeEvent(self, close_event):
         if self.stop_application_on_close:
-            QApplication.quit()
+            self.app_manager.quit_application()
             close_event.ignore()

src/tribler/gui/error_handler.py

@@ -1,10 +1,12 @@
+from __future__ import annotations
+
 import logging
 import traceback
 
 from tribler.core.components.reporter.reported_error import ReportedError
 from tribler.core.sentry_reporter.sentry_reporter import SentryStrategy
-
 from tribler.gui import gui_sentry_reporter
+from tribler.gui.app_manager import AppManager
 from tribler.gui.dialogs.feedbackdialog import FeedbackDialog
 from tribler.gui.exceptions import CoreError
 
@@ -18,6 +20,7 @@ def __init__(self, tribler_window):
         gui_sentry_reporter.ignore_logger(logger_name)
 
         self.tribler_window = tribler_window
+        self.app_manager: AppManager = tribler_window.app_manager
 
         self._handled_exceptions = set()
         self._tribler_stopped = False
@@ -50,6 +53,9 @@ def gui_error(self, *exc_info):
             event=gui_sentry_reporter.event_from_exception(info_error),
         )
 
+        if self.app_manager.quitting_app:
+            return
+
         FeedbackDialog(
             parent=self.tribler_window,
             sentry_reporter=gui_sentry_reporter,

src/tribler/gui/event_request_manager.py

@@ -8,7 +8,6 @@
 from tribler.core import notifications
 from tribler.core.components.reporter.reported_error import ReportedError
 from tribler.core.utilities.notifier import Notifier
-
 from tribler.gui import gui_sentry_reporter
 from tribler.gui.exceptions import CoreConnectTimeoutError, CoreConnectionError
 from tribler.gui.utilities import connect
@@ -26,7 +25,7 @@ class EventRequestManager(QNetworkAccessManager):
 
     node_info_updated = pyqtSignal(object)
     received_remote_query_results = pyqtSignal(object)
-    tribler_started = pyqtSignal(object)
+    core_connected = pyqtSignal(object)
     new_version_available = pyqtSignal(str)
     discovered_channel = pyqtSignal(object)
     torrent_finished = pyqtSignal(object)
@@ -47,8 +46,6 @@ def __init__(self, api_port, api_key, error_handler):
         self.shutting_down = False
         self.error_handler = error_handler
         self._logger = logging.getLogger(self.__class__.__name__)
-        # This flag is used to prevent race condition when starting GUI tests
-        self.tribler_started_flag = False
 
         self.connect_timer.setSingleShot(True)
         connect(self.connect_timer.timeout, self.connect)
@@ -66,13 +63,10 @@ def __init__(self, api_port, api_key, error_handler):
         notifier.add_observer(notifications.report_config_error, self.on_report_config_error)
 
     def on_events_start(self, public_key: str, version: str):
-        if version:
-            self.tribler_started_flag = True
-            self.tribler_started.emit(version)
-            # if public key format will be changed, don't forget to change it
-            # at the core side as well
-            if public_key:
-                gui_sentry_reporter.set_user(public_key.encode('utf-8'))
+        # if public key format is changed, don't forget to change it at the core side as well
+        if public_key:
+            gui_sentry_reporter.set_user(public_key.encode('utf-8'))
+        self.core_connected.emit(version)
 
     def on_tribler_exception(self, error: dict):
         self.error_handler.core_error(ReportedError(**error))

src/tribler/gui/start_gui.py

@@ -15,8 +15,8 @@
 from tribler.core.logger.logger import load_logger_config
 from tribler.core.sentry_reporter.sentry_reporter import SentryStrategy
 from tribler.core.utilities.rest_utils import path_to_uri
-
 from tribler.gui import gui_sentry_reporter
+from tribler.gui.app_manager import AppManager
 from tribler.gui.tribler_app import TriblerApplication
 from tribler.gui.tribler_window import TriblerWindow
 from tribler.gui.utilities import get_translator
@@ -49,6 +49,7 @@ def run_gui(api_port, api_key, root_state_dir, parsed_args):
 
         app_name = os.environ.get('TRIBLER_APP_NAME', 'triblerapp')
         app = TriblerApplication(app_name, sys.argv)
+        app_manager = AppManager(app)
 
         # Note (@ichorid): translator MUST BE created and assigned to a separate variable
         # before calling installTranslator on app. Otherwise, it won't work for some reason
@@ -69,7 +70,7 @@ def run_gui(api_port, api_key, root_state_dir, parsed_args):
             sys.exit(1)
 
         logger.info('Start Tribler Window')
-        window = TriblerWindow(settings, root_state_dir, api_port=api_port, api_key=api_key)
+        window = TriblerWindow(app_manager, settings, root_state_dir, api_port=api_port, api_key=api_key)
         window.setWindowTitle("Tribler")
         app.set_activation_window(window)
         app.parse_sys_args(sys.argv)

src/tribler/gui/tests/test_core_manager.py

@@ -1,66 +1,63 @@
+import sys
 from unittest.mock import MagicMock, patch
 
 import pytest
 
 from tribler.gui.core_manager import CoreCrashedError, CoreManager
 
-pytestmark = pytest.mark.asyncio
 
+@pytest.fixture(name='core_manager')
+def fixture_core_manager():
+    core_manager = CoreManager(root_state_dir=MagicMock(), api_port=MagicMock(), api_key=MagicMock(),
+                               app_manager=MagicMock(),
+                               events_manager=MagicMock())
+    core_manager.core_process = MagicMock(readAllStandardOutput=MagicMock(return_value=b'core stdout'),
+                                          readAllStandardError=MagicMock(return_value=b'core stderr'))
+    return core_manager
 
-# fmt: off
 
-@patch.object(CoreManager, 'quit_application')
-@patch('tribler.gui.core_manager.EventRequestManager', new=MagicMock())
-async def test_on_core_finished_call_on_finished(mocked_quit_application: MagicMock):
+def test_on_core_finished_calls_quit_application(core_manager):
     # test that in case of `shutting_down` and `should_quit_app_on_core_finished` flags have been set to True
-    # then `on_finished` function will be called and Exception will not be raised
-    core_manager = CoreManager(MagicMock(), MagicMock(), MagicMock(), MagicMock())
+    # then `quit_application` method will be called and Exception will not be raised
     core_manager.shutting_down = True
     core_manager.should_quit_app_on_core_finished = True
-
     core_manager.on_core_finished(exit_code=1, exit_status='exit status')
-    mocked_quit_application.assert_called_once()
+    core_manager.app_manager.quit_application.assert_called_once()
 
 
-@patch('tribler.gui.core_manager.EventRequestManager', new=MagicMock())
-async def test_on_core_finished_raises_error():
+def test_on_core_finished_raises_error(core_manager):
     # test that in case of flag `shutting_down` has been set to True and
     # exit_code is not equal to 0, then CoreRuntimeError should be raised
-    core_manager = CoreManager(MagicMock(), MagicMock(), MagicMock(), MagicMock())
-
     with pytest.raises(CoreCrashedError):
         core_manager.on_core_finished(exit_code=1, exit_status='exit status')
 
 
-@patch('tribler.gui.core_manager.print')
-@patch('tribler.gui.core_manager.EventRequestManager', new=MagicMock())
-async def test_on_core_stdout_read_ready(mocked_print: MagicMock):
+@patch('builtins.print')
+def test_on_core_stdout_read_ready(mocked_print, core_manager):
     # test that method `on_core_stdout_read_ready` converts byte output to a string and prints it
-    core_manager = CoreManager(MagicMock(), MagicMock(), MagicMock(), MagicMock())
-    core_manager.core_process = MagicMock(readAllStandardOutput=MagicMock(return_value=b'binary string'))
+    core_manager.app_manager.quitting_app = False
     core_manager.on_core_stdout_read_ready()
-    mocked_print.assert_called_with('binary string')
+    mocked_print.assert_called_with('core stdout')
 
 
-@patch('tribler.gui.core_manager.print')
-@patch('tribler.gui.core_manager.EventRequestManager', new=MagicMock())
-@patch('sys.stderr')
-async def test_on_core_stderr_read_ready(mocked_stderr, mocked_print: MagicMock):
+@patch('builtins.print')
+def test_on_core_stderr_read_ready(mocked_print, core_manager):
     # test that method `on_core_stdout_read_ready` converts byte output to a string and prints it
-    core_manager = CoreManager(MagicMock(), MagicMock(), MagicMock(), MagicMock())
-    core_manager.core_process = MagicMock(readAllStandardError=MagicMock(return_value=b'binary string'))
+    core_manager.app_manager.quitting_app = False
     core_manager.on_core_stderr_read_ready()
-    mocked_print.assert_called_with('binary string', file=mocked_stderr)
+    mocked_print.assert_called_with('core stderr', file=sys.stderr)
 
 
-@patch('tribler.gui.core_manager.EventRequestManager', new=MagicMock())
 @patch('builtins.print', MagicMock(side_effect=OSError()))
-def test_on_core_stdout_stderr_read_ready_os_error():
-    # test that OSError on writing to stdout is suppressed when quitting the application
-
-    core_manager = CoreManager(MagicMock(), MagicMock(), MagicMock(), MagicMock())
-    core_manager.core_process = MagicMock(read_all=MagicMock(return_value=''))
+def test_on_core_read_ready_os_error_suppressed(core_manager):
+    # OSError exceptions when writing to stdout and stderr are suppressed
+    core_manager.app_manager.quitting_app = False
+    core_manager.on_core_stdout_read_ready()
+    core_manager.on_core_stderr_read_ready()
+    assert print.call_count == 2
 
-    # check that OSError exception is suppressed when writing to stdout and stderr
+    # if app is quitting, core_manager does not write to stdout/stderr at all, and so the call counter does not grow
+    core_manager.app_manager.quitting_app = True
     core_manager.on_core_stdout_read_ready()
     core_manager.on_core_stderr_read_ready()
+    assert print.call_count == 2

src/tribler/gui/tests/test_error_handler.py

@@ -4,7 +4,6 @@
 
 from tribler.core.components.reporter.reported_error import ReportedError
 from tribler.core.sentry_reporter.sentry_reporter import SentryReporter, SentryStrategy
-
 from tribler.gui.error_handler import ErrorHandler
 from tribler.gui.exceptions import CoreConnectTimeoutError, CoreCrashedError
 
@@ -16,7 +15,9 @@
 
 @pytest.fixture
 def error_handler():
-    return ErrorHandler(MagicMock())
+    handler = ErrorHandler(MagicMock())
+    handler.app_manager.quitting_app = False
+    return handler
 
 
 @pytest.fixture