Merge pull request #8033 from drew2a/fix/8014

Refactor proxy settings and add error handling

src/tribler/core/components/libtorrent/download_manager/download_manager.py

@@ -8,11 +8,11 @@
 import os
 import time as timemod
 from asyncio import CancelledError, gather, iscoroutine, shield, sleep, wait_for
-from binascii import unhexlify
 from copy import deepcopy
 from shutil import rmtree
-from typing import Callable, Dict, List, Optional
+from typing import Callable, Dict, List, Optional, Tuple
 
+from binascii import unhexlify
 from ipv8.taskmanager import TaskManager
 
 from tribler.core import notifications
@@ -362,21 +362,35 @@ def get_session(self, hops=0):
 
  return self.ltsessions[hops]
 
- def set_proxy_settings(self, ltsession, ptype, server=None, auth=None):
+ def set_proxy_settings(self, session, ptype, server: Optional[Tuple[str, str]] = None,
+ auth: Optional[Tuple[str, str]] = None):
  """
  Apply the proxy settings to a libtorrent session. This mechanism changed significantly in libtorrent 1.1.0.
  """
- settings = {}
- settings["proxy_type"] = ptype
- settings["proxy_hostnames"] = True
- settings["proxy_peer_connections"] = True
- if server and server[0] and server[1]:
- settings["proxy_hostname"] = server[0]
- settings["proxy_port"] = int(server[1])
- if auth:
- settings["proxy_username"] = auth[0]
- settings["proxy_password"] = auth[1]
- self.set_session_settings(ltsession, settings)
+ settings = {"proxy_type": ptype, "proxy_hostnames": True, "proxy_peer_connections": True}
+
+ try:
+ if not all(v is not None for v in server):
+ raise ValueError('Host and port should not be None')
+
+ host, port = server
+ port = int(port)
+ settings["proxy_hostname"] = host
+ settings["proxy_port"] = port
+ except (ValueError, TypeError) as e:
+ self._logger.exception(e)
+
+ try:
+ if not all(v is not None for v in auth):
+ raise ValueError('Username and password should not be None')
+
+ username, proxy_password = auth
+ settings["proxy_username"] = username
+ settings["proxy_password"] = proxy_password
+ except (ValueError, TypeError) as e:
+ self._logger.exception(e)
+
+ self.set_session_settings(session, settings)
 
  def set_upload_rate_limit(self, rate, hops=None):
  # Rate conversion due to the fact that we had a different system with Swift

src/tribler/core/components/libtorrent/tests/test_download_manager.py

@@ -1,5 +1,4 @@
 import asyncio
-import functools
 import itertools
 from asyncio import Future
 from unittest.mock import AsyncMock, MagicMock, Mock
@@ -347,28 +346,15 @@ def test_set_proxy_settings(fake_dlmgr):
  """
  Test setting the proxy settings
  """
-
- def on_proxy_set(settings):
- assert settings
- assert settings.hostname == 'a'
- assert settings.port == 1234
- assert settings.username == 'abc'
- assert settings.password == 'def'
-
- def on_set_settings(settings):
- assert settings
- assert settings['proxy_hostname'] == 'a'
- assert settings['proxy_port'] == 1234
- assert settings['proxy_username'] == 'abc'
- assert settings['proxy_password'] == 'def'
- assert settings['proxy_peer_connections']
- assert settings['proxy_hostnames']
-
- mock_lt_session = MagicMock()
- mock_lt_session.get_settings = dict
- mock_lt_session.set_settings = on_set_settings
- mock_lt_session.set_proxy = on_proxy_set # Libtorrent < 1.1.0 uses set_proxy to set proxy settings
- fake_dlmgr.set_proxy_settings(mock_lt_session, 0, ('a', "1234"), ('abc', 'def'))
+ session = Mock()
+ fake_dlmgr.set_proxy_settings(session, 0, ('a', "1234"), ('abc', 'def'))
+ settings, = session.method_calls[-1].args
+ assert settings['proxy_hostname'] == 'a'
+ assert settings['proxy_port'] == 1234
+ assert settings['proxy_username'] == 'abc'
+ assert settings['proxy_password'] == 'def'
+ assert settings['proxy_peer_connections']
+ assert settings['proxy_hostnames']
 
 
 def test_payout_on_disconnect(fake_dlmgr):
@@ -575,3 +561,53 @@ def test_update_trackers_list_append(fake_dlmgr) -> None:
  actual_trackers = set(itertools.chain.from_iterable(tracker_list))
  assert actual_trackers == {fake_tracker1, fake_tracker2}
 
+
+def test_set_proxy_settings_invalid_port(fake_dlmgr):
+ # Test setting the proxy settings for an invalid port number. In this case port and host should not be set.
+ session = Mock()
+ proxy_type = 2
+
+ fake_dlmgr.set_proxy_settings(session, proxy_type, ('host name', 'invalid port'))
+
+ settings, = session.method_calls[-1].args
+ assert 'proxy_port' not in settings
+ assert 'proxy_hostname' not in settings
+ assert settings['proxy_type'] == proxy_type
+
+
+def test_set_proxy_defaults(fake_dlmgr):
+ # Test setting the proxy settings with default values
+ session = Mock()
+ proxy_type = 2
+
+ fake_dlmgr.set_proxy_settings(session, proxy_type)
+ settings, = session.method_calls[-1].args
+ assert 'proxy_port' not in settings
+ assert 'proxy_hostname' not in settings
+ assert settings['proxy_type'] == proxy_type
+
+
+def test_set_proxy_corner_case(fake_dlmgr):
+ # Test setting the proxy settings with None values
+ session = Mock()
+
+ fake_dlmgr.set_proxy_settings(session, 2, (None, None))
+ settings, = session.method_calls[-1].args
+ assert settings['proxy_type'] == 2
+ assert 'proxy_port' not in settings
+
+ fake_dlmgr.set_proxy_settings(session, 3, (None,))
+ settings, = session.method_calls[-1].args
+ assert settings['proxy_type'] == 3
+ assert 'proxy_port' not in settings
+
+ fake_dlmgr.set_proxy_settings(session, 3, (None, 123))
+ settings, = session.method_calls[-1].args
+ assert settings['proxy_type'] == 3
+ assert 'proxy_port' not in settings
+
+ fake_dlmgr.set_proxy_settings(session, 3, (None, 123), ('name', None))
+ settings, = session.method_calls[-1].args
+ assert settings['proxy_type'] == 3
+ assert 'proxy_port' not in settings
+ assert 'proxy_username' not in settings