[TrackerIcons] Cleanup tmp files created by downloading page

Temporary files created while download host html page are no cleaned up
if the download fails.

Fixed by adding a 'finally' step in the callback chain to delete any
created temporary files.

Added tests to ensure the temporary files are deleted, using a fixture
that creates a known filename for the test.

Trac: https://dev.deluge-torrent.org/ticket/3167

deluge/conftest.py

@@ -4,8 +4,9 @@
 # See LICENSE for more details.
 #
 
-import unittest.mock
+import tempfile
 import warnings
+from unittest.mock import Mock, patch
 
 import pytest
 import pytest_twisted
@@ -49,7 +50,7 @@ def mock_callback():
         mock.side_effect = lambda *args, **kw: deferred.callback((args, kw))
         mock.deferred = deferred
 
-    mock = unittest.mock.Mock()
+    mock = Mock()
     original_reset_mock = mock.reset_mock
     mock.reset_mock = reset
     mock.reset_mock()
@@ -181,3 +182,11 @@ class BaseTestCase:
     have finished.
 
     """
+
+
+@pytest.fixture
+def mock_mkstemp(tmp_path):
+    """Return known tempfile location to verify file deleted"""
+    tmp_file = tempfile.mkstemp(dir=tmp_path)
+    with patch('tempfile.mkstemp', return_value=tmp_file):
+        yield tmp_file

deluge/tests/test_tracker_icons.py

@@ -3,6 +3,7 @@
 # the additional special exception to link portions of this program with the OpenSSL library.
 # See LICENSE for more details.
 #
+import os.path
 
 import pytest
 import pytest_twisted
@@ -28,11 +29,12 @@ class TestTrackerIcons(BaseTestCase):
         return component.shutdown()
 
     @pytest_twisted.ensureDeferred
-    async def test_get_deluge_png(self):
+    async def test_get_deluge_png(self, mock_mkstemp):
         # Deluge has a png favicon link
         icon = TrackerIcon(common.get_test_data_file('deluge.png'))
         result = await self.icons.fetch('deluge-torrent.org')
         assert result == icon
+        assert not os.path.isfile(mock_mkstemp[1])
 
     @pytest_twisted.ensureDeferred
     async def test_get_google_ico(self):
@@ -68,3 +70,10 @@ class TestTrackerIcons(BaseTestCase):
     async def test_get_empty_string_tracker(self):
         result = await self.icons.fetch('')
         assert result is None
+
+    @pytest_twisted.ensureDeferred
+    async def test_invalid_host(self, mock_mkstemp):
+        """Test that TrackerIcon can handle invalid hostname"""
+        result = await self.icons.fetch('deluge.example.com')
+        assert not result
+        assert not os.path.isfile(mock_mkstemp[1])

deluge/ui/tracker_icons.py

@@ -8,8 +8,8 @@
 
 import logging
 import os
+import tempfile
 from html.parser import HTMLParser
-from tempfile import mkstemp
 from urllib.parse import urljoin, urlparse
 
 from twisted.internet import defer, threads
@@ -203,8 +203,10 @@ class TrackerIcons(Component):
         else:
             # We need to fetch it
             self.pending[host] = []
+            tmp_file = tempfile.mkstemp(prefix='deluge_trackericon_html.')
+            filename = tmp_file[1]
             # Start callback chain
-            d = self.download_page(host)
+            d = self.download_page(host, filename)
             d.addCallbacks(
                 self.on_download_page_complete,
                 self.on_download_page_fail,
@@ -213,6 +215,7 @@ class TrackerIcons(Component):
             d.addCallbacks(
                 self.on_parse_complete, self.on_parse_fail, callbackArgs=(host,)
             )
+            d.addBoth(self.del_tmp_file, tmp_file)
             d.addCallback(self.download_icon, host)
             d.addCallbacks(
                 self.on_download_icon_complete,
@@ -224,24 +227,38 @@ class TrackerIcons(Component):
             d.addCallback(self.store_icon, host)
         return d
 
-    def download_page(self, host, url=None):
+    @staticmethod
-        """
+    def del_tmp_file(result, tmp_file):
-        Downloads a tracker host's page
+        """Remove tmp_file created when downloading tracker page"""
+        fd, filename = tmp_file
+        try:
+            os.close(fd)
+            os.remove(filename)
+        except OSError:
+            log.debug(f'Unable to delete temporary file: {filename}')
+
+        return result
+
+    def download_page(
+        self, host: str, filename: str, url: str = None
+    ) -> 'defer.Deferred[str]':
+        """Downloads a tracker host's page
+
         If no url is provided, it bases the url on the host
 
-        :param host: the tracker host
+        Args:
-        :type host: string
+            host: The tracker host
-        :param url: the (optional) url of the host
+            filename: Location to download page
-        :type url: string
+            url: The url of the host
-        :returns: the filename of the tracker host's page
+
-        :rtype: Deferred
+        Returns:
+            The filename of the tracker host's page
         """
         if not url:
             url = self.host_to_url(host)
-        log.debug('Downloading %s %s', host, url)
+
-        tmp_fd, tmp_file = mkstemp(prefix='deluge_ticon.')
+        log.debug(f'Downloading {host} {url} to {filename}')
-        os.close(tmp_fd)
+        return download_file(url, filename, force_filename=True)
-        return download_file(url, tmp_file, force_filename=True)
 
     def on_download_page_complete(self, page):
         """
@@ -291,10 +308,6 @@ class TrackerIcons(Component):
                 if parser.left_head:
                     break
             parser.close()
-        try:
-            os.remove(page)
-        except OSError as ex:
-            log.warning('Could not remove temp file: %s', ex)
 
         return parser.get_icons()