From 1d833bf073b6e7ca9731314230af1c3b725b1995 Mon Sep 17 00:00:00 2001 From: drew2a Date: Wed, 26 Apr 2023 10:41:00 +0200 Subject: [PATCH] Ignore old responses for the DownloadsPage --- src/tribler/gui/network/request.py | 5 ++ src/tribler/gui/network/request_manager.py | 7 ++- src/tribler/gui/network/tests/test_request.py | 28 ++++++++++- .../gui/network/tests/test_request_manager.py | 12 +++++ src/tribler/gui/tests/test_downloadspage.py | 50 +++++++++++++++++++ src/tribler/gui/tests/test_gui.py | 1 - src/tribler/gui/widgets/downloadspage.py | 25 +++++++--- 7 files changed, 117 insertions(+), 11 deletions(-) create mode 100644 src/tribler/gui/tests/test_downloadspage.py diff --git a/src/tribler/gui/network/request.py b/src/tribler/gui/network/request.py index 1f94bdb7dbf..754519ec14b 100644 --- a/src/tribler/gui/network/request.py +++ b/src/tribler/gui/network/request.py @@ -11,6 +11,8 @@ from tribler.gui.utilities import connect +REQUEST_ID = 'id' + if TYPE_CHECKING: from tribler.gui.network.request_manager import RequestManager @@ -84,6 +86,7 @@ def __init__( self.status_code = 0 self.status_text = "unknown" self.cancellable = True + self.id = 0 def set_manager(self, manager: RequestManager): self.manager = manager @@ -134,6 +137,8 @@ def on_finished(self): self.logger.debug('Create a json response') result = json.loads(data) + if isinstance(result, dict): + result[REQUEST_ID] = self.id is_error = 'error' in result if is_error and self.capture_errors: text = self.manager.show_error(self, result) diff --git a/src/tribler/gui/network/request_manager.py b/src/tribler/gui/network/request_manager.py index 7791b157fe2..e27c2abb256 100644 --- a/src/tribler/gui/network/request_manager.py +++ b/src/tribler/gui/network/request_manager.py @@ -34,9 +34,10 @@ def __init__(self, limit: int = 50, timeout_interval: int = 15): self.protocol = DEFAULT_API_PROTOCOL self.host = DEFAULT_API_HOST self.port = DEFAULT_API_PORT - self.key = b"" + self.key = '' self.limit = limit self.timeout_interval = timeout_interval + self.last_request_id = 0 def get(self, endpoint: str, @@ -114,6 +115,10 @@ def delete(self, return request def add(self, request: Request): + # Set last request id + self.last_request_id += 1 + request.id = self.last_request_id + if len(self.active_requests) > self.limit: self._drop_timed_out_requests() diff --git a/src/tribler/gui/network/tests/test_request.py b/src/tribler/gui/network/tests/test_request.py index ae9d6932d66..fdc281c7279 100644 --- a/src/tribler/gui/network/tests/test_request.py +++ b/src/tribler/gui/network/tests/test_request.py @@ -1,6 +1,9 @@ -from unittest.mock import MagicMock +from typing import Dict +from unittest.mock import MagicMock, Mock, patch -from tribler.gui.network.request import Request +from PyQt5.QtNetwork import QNetworkReply + +from tribler.gui.network.request import REQUEST_ID, Request def test_default_constructor(): @@ -49,3 +52,24 @@ def test_on_finished(): request.on_finished() on_success.assert_not_called() + + +@patch.object(Request, 'update_status', Mock()) +def test_set_id(): + # Test that the id is set correctly during `on_finished` callback call + actual_id = None + + def on_success(json: Dict): + nonlocal actual_id + actual_id = json[REQUEST_ID] + + request = Request('endpoint', on_success=on_success) + request.manager = Mock() + request.reply = Mock( + error=Mock(return_value=QNetworkReply.NoError), + readAll=Mock(return_value=b'{"a": "b"}') + ) + request.id = 10 + request.on_finished() + + assert actual_id == 10 diff --git a/src/tribler/gui/network/tests/test_request_manager.py b/src/tribler/gui/network/tests/test_request_manager.py index f065162b0ab..f123ab6346f 100644 --- a/src/tribler/gui/network/tests/test_request_manager.py +++ b/src/tribler/gui/network/tests/test_request_manager.py @@ -1,3 +1,5 @@ +from unittest.mock import Mock, patch + import pytest from tribler.gui.network.request_manager import RequestManager @@ -42,3 +44,13 @@ def test_get_message_from_error_any_dict(request_manager: RequestManager): } ) assert message == '{"key": "value"}' + + +@patch('tribler.gui.network.request_manager.QBuffer', Mock()) +@patch.object(RequestManager, 'sendCustomRequest', Mock()) +def test_request_id(request_manager: RequestManager): + request = request_manager.get('endpoint') + assert request.id == 1 + + request = request_manager.delete('endpoint') + assert request.id == 2 diff --git a/src/tribler/gui/tests/test_downloadspage.py b/src/tribler/gui/tests/test_downloadspage.py new file mode 100644 index 00000000000..418de3b6617 --- /dev/null +++ b/src/tribler/gui/tests/test_downloadspage.py @@ -0,0 +1,50 @@ +from unittest.mock import MagicMock, Mock, patch + +from PyQt5.QtWidgets import QWidget + +from tribler.gui.network.request import REQUEST_ID +from tribler.gui.widgets.downloadspage import DownloadsPage + + +def downloads_page() -> DownloadsPage: + window = MagicMock() + window.downloads_list.indexOfTopLevelItem = Mock(return_value=-1) + + page = DownloadsPage() + page.window = Mock(return_value=window) + page.received_downloads = Mock() + return page + + +@patch.object(QWidget, '__init__', Mock()) +def test_accept_requests(): + # Test that the page accepts requests with the greater request id + page = downloads_page() + + page.on_received_downloads(result={REQUEST_ID: 1, 'downloads': MagicMock()}) + assert page.received_downloads.emit.called + + page.received_downloads.emit.reset_mock() + page.on_received_downloads(result={REQUEST_ID: 2, 'downloads': MagicMock()}) + assert page.received_downloads.emit.called + + page.received_downloads.emit.reset_mock() + page.on_received_downloads(result={REQUEST_ID: 10, 'downloads': MagicMock()}) + assert page.received_downloads.emit.called + + +@patch.object(QWidget, '__init__', Mock()) +def test_ignore_request(): + # Test that the page ignores requests with a lower or equal request id + page = downloads_page() + + page.on_received_downloads(result={REQUEST_ID: 10, 'downloads': MagicMock()}) + assert page.received_downloads.emit.called + + page.received_downloads.emit.reset_mock() + page.on_received_downloads(result={REQUEST_ID: 10, 'downloads': MagicMock()}) + assert not page.received_downloads.emit.called + + page.received_downloads.emit.reset_mock() + page.on_received_downloads(result={REQUEST_ID: 9, 'downloads': MagicMock()}) + assert not page.received_downloads.emit.called diff --git a/src/tribler/gui/tests/test_gui.py b/src/tribler/gui/tests/test_gui.py index 0698a7d5272..4c63ee422bf 100644 --- a/src/tribler/gui/tests/test_gui.py +++ b/src/tribler/gui/tests/test_gui.py @@ -262,7 +262,6 @@ def tst_channels_widget(window, widget, widget_name, sort_column=1, test_filter= screenshot(window, name=f"{widget_name}-torrent_details") -@pytest.mark.guitest def test_discovered_page(window): QTest.mouseClick(window.left_menu_button_discovered, Qt.LeftButton) tst_channels_widget(window, window.discovered_page, "discovered_page", sort_column=2) diff --git a/src/tribler/gui/widgets/downloadspage.py b/src/tribler/gui/widgets/downloadspage.py index e089afa6371..0a10f891ec1 100644 --- a/src/tribler/gui/widgets/downloadspage.py +++ b/src/tribler/gui/widgets/downloadspage.py @@ -20,6 +20,7 @@ DOWNLOADS_FILTER_DOWNLOADING, DOWNLOADS_FILTER_INACTIVE, ) from tribler.gui.dialogs.confirmationdialog import ConfirmationDialog +from tribler.gui.network.request import REQUEST_ID from tribler.gui.network.request_manager import request_manager from tribler.gui.sentry_mixin import AddBreadcrumbOnShowMixin from tribler.gui.tribler_action_menu import TriblerActionMenu @@ -67,6 +68,9 @@ def __init__(self): self.total_download = 0 self.total_upload = 0 + # Used to keep track of the last processed request with a purpose of ignoring old requests + self.last_processed_request_id = 0 + def showEvent(self, QShowEvent): """ When the downloads tab is clicked, we want to update the downloads list immediately. @@ -142,11 +146,19 @@ def refresh_downloads(self): on_success=self.on_received_downloads, ) - def on_received_downloads(self, downloads): - if not downloads or "downloads" not in downloads: + def on_received_downloads(self, result): + if not result or "downloads" not in result: return # This might happen when closing Tribler + request_id = result[REQUEST_ID] + if self.last_processed_request_id >= request_id: + # This is an old request, ignore it. + # It could happen because some of requests processed a bit longer than others + msg = f'Ignoring old request {request_id} (last processed request id: {self.last_processed_request_id})' + self._logger.warning(msg) + return + self.last_processed_request_id = request_id - checkpoints = downloads.get('checkpoints', {}) + checkpoints = result.get('checkpoints', {}) if checkpoints and self.loading_message_widget: # If not all checkpoints are loaded, display the number of the loaded checkpoints total = checkpoints['total'] @@ -164,7 +176,7 @@ def on_received_downloads(self, downloads): self.window().downloads_list.takeTopLevelItem(loading_widget_index) self.window().downloads_list.setSelectionMode(QAbstractItemView.ExtendedSelection) - self.downloads = downloads + self.downloads = result self.total_download = 0 self.total_upload = 0 @@ -172,7 +184,7 @@ def on_received_downloads(self, downloads): download_infohashes = set() items = [] - for download in downloads["downloads"]: + for download in result["downloads"]: # Update download progress information for torrents in the Channels GUI. # We skip updating progress information for ChannelTorrents because otherwise it interferes # with channel processing progress updates @@ -218,7 +230,7 @@ def on_received_downloads(self, downloads): self.update_download_visibility() self.refresh_top_panel() - self.received_downloads.emit(downloads) + self.received_downloads.emit(result) def update_download_visibility(self): for i in range(self.window().downloads_list.topLevelItemCount()): @@ -280,7 +292,6 @@ def refresh_top_panel(self): self.window().start_download_button.setEnabled(DownloadsPage.start_download_enabled(self.selected_items)) self.window().stop_download_button.setEnabled(DownloadsPage.stop_download_enabled(self.selected_items)) - def on_start_download_clicked(self, checked): for item in self.selected_items: request_manager.patch(