From 3efd47d004604eb32c81a032d9928ed0f1ce087b Mon Sep 17 00:00:00 2001 From: Roland Date: Tue, 14 Jun 2022 19:51:13 -0600 Subject: [PATCH] Global timeout can be set in the config and UI. Downloaders can specify a process timeout. --- app/src/components/Admin.js | 27 ++++++++++++++++++++ modules/videos/downloader.py | 4 +-- wrolpi/common.py | 9 +++++++ wrolpi/downloader.py | 45 ++++++++++++++++++++++------------ wrolpi/root_api.py | 1 + wrolpi/schema.py | 1 + wrolpi/test/test_downloader.py | 36 ++++++++++++++++++++++++++- wrolpi/version.py | 2 +- 8 files changed, 105 insertions(+), 20 deletions(-) diff --git a/app/src/components/Admin.js b/app/src/components/Admin.js index 9898582e..19ff55c8 100644 --- a/app/src/components/Admin.js +++ b/app/src/components/Admin.js @@ -26,6 +26,7 @@ import { import TimezoneSelect from 'react-timezone-select'; import { DisableDownloadsToggle, + HelpPopup, HotspotToggle, PageContainer, secondsToDate, @@ -51,6 +52,7 @@ class Settings extends React.Component { ready: false, download_on_startup: null, + download_timeout: null, hotspot_on_startup: null, hotspot_password: null, hotspot_ssid: null, @@ -71,6 +73,7 @@ class Settings extends React.Component { ready: true, disabled: settings.wrol_mode, download_on_startup: settings.download_on_startup, + download_timeout: settings.download_timeout || '', hotspot_on_startup: settings.hotspot_on_startup, hotspot_password: settings.hotspot_password, hotspot_ssid: settings.hotspot_ssid, @@ -89,6 +92,7 @@ class Settings extends React.Component { this.setState({disabled: true, pending: true}); let settings = { download_on_startup: this.state.download_on_startup, + download_timeout: this.state.download_timeout ? parseInt(this.state.download_timeout) : 0, hotspot_on_startup: this.state.hotspot_on_startup, hotspot_password: this.state.hotspot_password, hotspot_ssid: this.state.hotspot_ssid, @@ -113,6 +117,15 @@ class Settings extends React.Component { this.setState({qrCodeValue}); } + handleTimeoutChange = async (e, name, value) => { + if (e) { + e.preventDefault() + } + // Restrict timeout to numbers. + value = value.replace(/[^\d]/,''); + this.setState({[name]: value}); + } + render() { if (this.state.ready === false) { return @@ -121,6 +134,7 @@ class Settings extends React.Component { let { disabled, download_on_startup, + download_timeout, hotspot_on_startup, hotspot_password, hotspot_ssid, @@ -174,6 +188,19 @@ class Settings extends React.Component { onChange={(e, d) => this.handleInputChange(e, 'throttle_on_startup', d.checked)} /> + + + Download Timeout + + } + value={download_timeout} + disabled={disabled || download_timeout === null} + onChange={(e, d) => this.handleTimeoutChange(e, 'download_timeout', d.value)} + /> + + int: + return self._config['download_timeout'] + + @download_timeout.setter + def download_timeout(self, value: int): + self.update({'download_timeout': value}) + @property def hotspot_on_startup(self) -> bool: return self._config['hotspot_on_startup'] diff --git a/wrolpi/downloader.py b/wrolpi/downloader.py index 85a5fa74..a5a05254 100644 --- a/wrolpi/downloader.py +++ b/wrolpi/downloader.py @@ -18,7 +18,7 @@ from sqlalchemy import Column, Integer, String, Text from sqlalchemy.dialects.postgresql import JSONB from sqlalchemy.orm import Session -from wrolpi.common import Base, ModelHelper, logger, wrol_mode_check, zig_zag, ConfigFile +from wrolpi.common import Base, ModelHelper, logger, wrol_mode_check, zig_zag, ConfigFile, WROLPI_CONFIG from wrolpi.dates import TZDateTime, now, Seconds, local_timezone, recursive_replace_tz from wrolpi.db import get_db_session, get_db_curs, get_db_context, optional_session from wrolpi.errors import InvalidDownload, UnrecoverableDownloadError @@ -128,11 +128,12 @@ class Download(ModelHelper, Base): class Downloader: - name = None - pretty_name = None - listable = True + name: str = None + pretty_name: str = None + listable: bool = True + timeout: int = None - def __init__(self, priority: int = 50, name: str = None): + def __init__(self, priority: int = 50, name: str = None, timeout: int = None): """ Lower `priority` means the downloader will be checked first. Valid priority: 0-100 @@ -143,8 +144,9 @@ class Downloader: if not 0 <= priority <= 100: raise ValueError(f'Priority of {priority} of {self} is invalid!') - self.name = self.name or name - self.priority = priority + self.name: str = self.name or name + self.priority: int = priority + self.timeout: int = timeout or self.timeout self._kill = multiprocessing.Event() self._manager: DownloadManager = None # noqa @@ -176,28 +178,35 @@ class Downloader: self._manager = value def kill(self): - """ - Kill the running download for this Downloader. - """ + """Kill the running download for this Downloader.""" if not self._kill.is_set(): self._kill.set() def clear(self): - """ - Clear any "kill" request for this Downloader. - """ + """Clear any "kill" request for this Downloader.""" if self._kill.is_set(): self._kill.clear() - def process_runner(self, url: str, cmd: List[str], cwd: pathlib.Path, **kwargs) -> Tuple[int, dict]: + def process_runner(self, url: str, cmd: Tuple[str, ...], cwd: pathlib.Path, timeout: int = None, + **kwargs) -> Tuple[int, dict]: """ Run a subprocess using the provided arguments. This process can be killed by the Download Manager. + + Global timeout takes precedence over the timeout argument, unless it is 0. (Smaller global timeout wins) """ logger.debug(f'{self} launching download process with args: {" ".join(cmd)}') + start = now() proc = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=cwd, **kwargs) pid = proc.pid logger.debug(f'{self} launched download process {pid=} for {url}') + # Timeout can be any positive integer. Global download timeout takes precedence, unless it is 0. + # A timeout of 0 means the download will never be killed. + if timeout is None: + timeout = self.timeout + if WROLPI_CONFIG.download_timeout and timeout > WROLPI_CONFIG.download_timeout: + timeout = WROLPI_CONFIG.download_timeout + try: while True: try: @@ -207,13 +216,17 @@ class Downloader: except subprocess.TimeoutExpired: pass + if timeout and (elapsed := now() - start).total_seconds() > timeout: + logger.warning(f'Download has exceeded its timeout {elapsed=}') + self.kill() + if self._kill.is_set(): logger.warning(f'Killing download {pid=}') proc.kill() proc.poll() - raise ChildProcessError(f'Download {pid=} was killed!') + break finally: - self._kill.clear() + self.clear() # Output all logs from the process. # TODO is there a way to stream this output while the process is running? diff --git a/wrolpi/root_api.py b/wrolpi/root_api.py index f9d58fd0..37f7ffd0 100644 --- a/wrolpi/root_api.py +++ b/wrolpi/root_api.py @@ -120,6 +120,7 @@ def get_settings(_: Request): settings = { 'download_on_startup': config.download_on_startup, + 'download_timeout': config.download_timeout, 'hotspot_on_startup': config.hotspot_on_startup, 'hotspot_password': config.hotspot_password, 'hotspot_ssid': config.hotspot_ssid, diff --git a/wrolpi/schema.py b/wrolpi/schema.py index 120ceb66..8909b201 100644 --- a/wrolpi/schema.py +++ b/wrolpi/schema.py @@ -15,6 +15,7 @@ class SettingsResponse: @dataclass class SettingsRequest: download_on_startup: Optional[bool] = None + download_timeout: Optional[int] = None hotspot_on_startup: Optional[bool] = None hotspot_password: Optional[str] = None hotspot_ssid: Optional[str] = None diff --git a/wrolpi/test/test_downloader.py b/wrolpi/test/test_downloader.py index 93ee46af..3aa9b4a3 100644 --- a/wrolpi/test/test_downloader.py +++ b/wrolpi/test/test_downloader.py @@ -200,7 +200,7 @@ async def test_download_wrol_mode(test_session, test_download_manager): await test_download_manager.do_downloads_sync() -def test_download_get_downloader(test_session, test_download_manager,): +def test_download_get_downloader(test_session, test_download_manager, ): """ A Download can find it's Downloader. """ @@ -364,3 +364,37 @@ def test_skip_urls(test_session, test_download_manager): ], downloader_name=HTTPDownloader.name) downloads = test_download_manager.get_downloads(test_session) assert {i.url for i in downloads} == {'https://example.com/1', 'https://example.com/2'} + + +def test_process_runner_timeout(test_directory): + """A Downloader can cancel it's download using a timeout.""" + # Default timeout of 3 seconds. + downloader = Downloader(0, 'downloader', timeout=3) + + # Default timeout is obeyed. + start = datetime.now() + downloader.process_runner('https://example.com', ('sleep', '8'), test_directory) + elapsed = datetime.now() - start + assert 3 < elapsed.total_seconds() < 4 + + # One-off timeout is obeyed. + start = datetime.now() + downloader.process_runner('https://example.com', ('sleep', '8'), test_directory, timeout=1) + elapsed = datetime.now() - start + assert 1 < elapsed.total_seconds() < 2 + + # Zero timeout means no timeout. + start = datetime.now() + downloader.process_runner('https://example.com', ('sleep', '8'), test_directory, timeout=0) + elapsed = datetime.now() - start + assert 7 < elapsed.total_seconds() < 10 + + # This Downloader's timeout is ignored in favor of the global timeout. + from wrolpi.common import WROLPI_CONFIG + WROLPI_CONFIG.download_timeout = 3 + + # Global timeout is obeyed. + start = datetime.now() + downloader.process_runner('https://example.com', ('sleep', '8'), test_directory) + elapsed = datetime.now() - start + assert 2 < elapsed.total_seconds() < 4 diff --git a/wrolpi/version.py b/wrolpi/version.py index 5b450d27..7016889c 100644 --- a/wrolpi/version.py +++ b/wrolpi/version.py @@ -1,4 +1,4 @@ -__version__ = '0.5.0-beta' +__version__ = '0.5.1-beta' import subprocess