Apply finite network inactivity timeouts to shared downloads

Author: jonsch1

pypath/share/downloads.py

@@ -80,6 +80,24 @@ def __getattr__(self, name: str):
         return getattr(get_download_manager(), name)
 
 
+class _PypathDownloadManager(DownloadManager):
+    """Finite connect/read waits for shared downloads, including legacy callers."""
+
+    def download(self, url, *args, **kwargs):
+        # Requests interprets timeout as socket inactivity, not total transfer
+        # duration: a large download can continue while it is making progress.
+        for key, env, default in (
+            ('connecttimeout', 'PYPATH_CONNECT_TIMEOUT', 30),
+            ('timeout', 'PYPATH_READ_TIMEOUT', 120),
+        ):
+            value = float(os.environ.get(env, default))
+            if not 0 < value < float('inf'):
+                raise ValueError(f'{env} must be a finite positive number')
+            kwargs.setdefault(key, value)
+        kwargs.setdefault('retries', 1)
+        return super().download(url, *args, **kwargs)
+
+
 def get_download_manager() -> DownloadManager:
     """
     Get the shared DownloadManager instance configured with pypath's data folder.
@@ -89,7 +107,7 @@ def get_download_manager() -> DownloadManager:
     """
     manager = getattr(_thread_local, 'download_manager', None)
     if manager is None:
-        manager = DownloadManager(
+        manager = _PypathDownloadManager(
             path=str(_resolve_data_dir()),
             config={'backend': 'requests'},
         )

test/test_download_timeouts.py

@@ -0,0 +1,43 @@
+"""Shared download timeouts also apply to legacy dm.download callers."""
+import threading
+import time
+from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer
+
+import pytest
+from pypath.share import downloads
+
+
+def test_default_timeouts_and_explicit_override(monkeypatch):
+    calls = []
+    monkeypatch.setattr(downloads.DownloadManager, 'download', lambda self, url, *args, **kw: calls.append(kw))
+    manager = object.__new__(downloads._PypathDownloadManager)
+    manager.download('https://example.invalid/data')
+    assert calls[-1]['connecttimeout'] == 30
+    assert calls[-1]['timeout'] == 120
+    assert calls[-1]['retries'] == 1
+    manager.download('https://example.invalid/data', timeout=600)
+    assert calls[-1]['timeout'] == 600
+    monkeypatch.setenv('PYPATH_READ_TIMEOUT', '0.1')
+    manager.download('https://example.invalid/data')
+    assert calls[-1]['timeout'] == 0.1
+
+
+def test_unresponsive_http_source_times_out(tmp_path, monkeypatch):
+    class StalledHandler(BaseHTTPRequestHandler):
+        def do_GET(self):
+            time.sleep(1)
+        def log_message(self, *args):
+            pass
+    server = ThreadingHTTPServer(('127.0.0.1', 0), StalledHandler)
+    thread = threading.Thread(target=server.serve_forever, daemon=True)
+    thread.start()
+    monkeypatch.setenv('PYPATH_READ_TIMEOUT', '0.1')
+    manager = downloads._PypathDownloadManager(path=str(tmp_path / 'cache'), config={'backend': 'requests'})
+    start = time.monotonic()
+    try:
+        with pytest.raises(Exception):
+            manager.download(f'http://127.0.0.1:{server.server_port}/stalled', dest=str(tmp_path / 'result'))
+        assert time.monotonic() - start < 1
+    finally:
+        server.shutdown()
+        server.server_close()