Skip to content

Commit

Permalink
Ignore old responses for the DownloadsPage
Browse files Browse the repository at this point in the history
  • Loading branch information
drew2a committed Apr 26, 2023
1 parent d8bc6a6 commit 732cb18
Show file tree
Hide file tree
Showing 7 changed files with 117 additions and 11 deletions.
5 changes: 5 additions & 0 deletions src/tribler/gui/network/request.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@

from tribler.gui.utilities import connect

REQUEST_ID = '_request_id'

if TYPE_CHECKING:
from tribler.gui.network.request_manager import RequestManager

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
7 changes: 6 additions & 1 deletion src/tribler/gui/network/request_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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()

Expand Down
28 changes: 26 additions & 2 deletions src/tribler/gui/network/tests/test_request.py
Original file line number Diff line number Diff line change
@@ -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():
Expand Down Expand Up @@ -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
12 changes: 12 additions & 0 deletions src/tribler/gui/network/tests/test_request_manager.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from unittest.mock import Mock, patch

import pytest

from tribler.gui.network.request_manager import RequestManager
Expand Down Expand Up @@ -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
50 changes: 50 additions & 0 deletions src/tribler/gui/tests/test_downloadspage.py
Original file line number Diff line number Diff line change
@@ -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
1 change: 0 additions & 1 deletion src/tribler/gui/tests/test_gui.py
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,6 @@ def tst_channels_widget(window, widget, widget_name, sort_column=1, test_filter=
QTest.qWait(100)
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)
Expand Down
25 changes: 18 additions & 7 deletions src/tribler/gui/widgets/downloadspage.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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']
Expand All @@ -164,15 +176,15 @@ 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

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
Expand Down Expand Up @@ -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()):
Expand Down Expand Up @@ -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(
Expand Down

0 comments on commit 732cb18

Please sign in to comment.