From a233bbfd9b3795b5ea35cc3c7330cbdf60abe226 Mon Sep 17 00:00:00 2001 From: Sunil Mohan Adapa Date: Mon, 2 Oct 2023 18:45:21 -0700 Subject: [PATCH] operation: Add unique ID for each operation - Helps in retrieving an operation that is currently running. - Prevent starting an operation that is already running. Tests: - Unit tests work. - Installing, uninstalling an app works. - For upgrading an app works. - Running background diagnostics works. - Updating tor configuration works. - Updating torproxy configuration works. Signed-off-by: Sunil Mohan Adapa Reviewed-by: James Valleroy --- plinth/modules/diagnostics/__init__.py | 5 +- plinth/modules/tor/views.py | 2 +- plinth/modules/torproxy/views.py | 2 +- plinth/operation.py | 32 ++++++---- plinth/setup.py | 10 ++-- plinth/tests/test_operation.py | 82 +++++++++++++++++--------- 6 files changed, 85 insertions(+), 48 deletions(-) diff --git a/plinth/modules/diagnostics/__init__.py b/plinth/modules/diagnostics/__init__.py index d16ef8666..3bb18a41b 100644 --- a/plinth/modules/diagnostics/__init__.py +++ b/plinth/modules/diagnostics/__init__.py @@ -264,8 +264,9 @@ def _warn_about_low_ram_space(request): def _start_background_diagnostics(request): """Start daily diagnostics as a background operation.""" operation = operation_module.manager.new( - 'diagnostics', gettext_noop('Running background diagnostics'), - _run_background_diagnostics, [], show_message=False, + op_id='diagnostics-full', app_id='diagnostics', + name=gettext_noop('Running background diagnostics'), + target=_run_background_diagnostics, [], show_message=False, show_notification=False) operation.join() diff --git a/plinth/modules/tor/views.py b/plinth/modules/tor/views.py index 1e6004093..f5ca2e01d 100644 --- a/plinth/modules/tor/views.py +++ b/plinth/modules/tor/views.py @@ -49,7 +49,7 @@ class TorAppView(AppView): def form_valid(self, form): """Configure tor app on successful form submission.""" - operation_module.manager.new(self.app_id, + operation_module.manager.new('tor-configuration', self.app_id, gettext_noop('Updating configuration'), _apply_changes, [form.initial, form.cleaned_data], diff --git a/plinth/modules/torproxy/views.py b/plinth/modules/torproxy/views.py index b4de80d9e..31fca07dc 100644 --- a/plinth/modules/torproxy/views.py +++ b/plinth/modules/torproxy/views.py @@ -47,7 +47,7 @@ class TorProxyAppView(AppView): def form_valid(self, form): """Configure tor app on successful form submission.""" - operation_module.manager.new(self.app_id, + operation_module.manager.new('torproxy-configuration', self.app_id, gettext_noop('Updating configuration'), _apply_changes, [form.initial, form.cleaned_data], diff --git a/plinth/operation.py b/plinth/operation.py index 8d61ed09d..acbcbe016 100644 --- a/plinth/operation.py +++ b/plinth/operation.py @@ -4,6 +4,7 @@ import enum import logging import threading +from collections import OrderedDict from typing import Callable from . import app as app_module @@ -21,12 +22,13 @@ class Operation: RUNNING: str = 'running' COMPLETED: str = 'completed' - def __init__(self, app_id: str, name: str, target: Callable, + def __init__(self, op_id: str, app_id: str, name: str, target: Callable, args: list | None = None, kwargs: dict | None = None, show_message: bool = True, show_notification: bool = False, thread_data: dict | None = None, on_complete: Callable | None = None): """Initialize to no operation.""" + self.op_id = op_id self.app_id = app_id self.name = name self.show_message = show_message @@ -171,7 +173,7 @@ class OperationsManager: def __init__(self) -> None: """Initialize the object.""" - self._operations: list[Operation] = [] + self._operations: OrderedDict[str, Operation] = OrderedDict() self._current_operation: Operation | None = None # Assume that operations manager will be called from various threads @@ -182,16 +184,24 @@ class OperationsManager: # when done from the same thread which holds the lock. self._lock = threading.RLock() - def new(self, *args, **kwargs) -> Operation: + def new(self, op_id: str, *args, **kwargs) -> Operation: """Create a new operation instance and add to global list.""" with self._lock: + if op_id in self._operations: + raise KeyError('Operation in progress/scheduled') + kwargs['on_complete'] = self._on_operation_complete - operation = Operation(*args, **kwargs) - self._operations.append(operation) + operation = Operation(op_id, *args, **kwargs) + self._operations[op_id] = operation logger.info('%s: added', operation) self._schedule_next() return operation + def get(self, op_id: str) -> Operation: + """Return an operation with given operation ID.""" + with self._lock: + return self._operations[op_id] + def _on_operation_complete(self, operation: Operation): """Trigger next operation. Called from within previous thread.""" logger.debug('%s: on_complete called', operation) @@ -199,7 +209,7 @@ class OperationsManager: self._current_operation = None if not operation.show_message: # No need to keep it lingering for later collection - self._operations.remove(operation) + del self._operations[operation.op_id] self._schedule_next() @@ -209,7 +219,7 @@ class OperationsManager: if self._current_operation: return - for operation in self._operations: + for operation in self._operations.values(): if operation.state == Operation.State.WAITING: logger.debug('%s: scheduling', operation) self._current_operation = operation @@ -220,22 +230,22 @@ class OperationsManager: """Return operations matching a pattern.""" with self._lock: return [ - operation for operation in self._operations + operation for operation in self._operations.values() if operation.app_id == app_id ] def collect_results(self, app_id: str) -> list[Operation]: """Return the finished operations for an app.""" results: list[Operation] = [] - remaining: list[Operation] = [] + remaining: OrderedDict[str, Operation] = OrderedDict() with self._lock: - for operation in self._operations: + for operation in self._operations.values(): if (operation.app_id == app_id and operation.state == Operation.State.COMPLETED): results.append(operation) else: - remaining.append(operation) + remaining[operation.op_id] = operation if results: self._operations = remaining diff --git a/plinth/setup.py b/plinth/setup.py index 6d4ed71ab..04c0dfd3f 100644 --- a/plinth/setup.py +++ b/plinth/setup.py @@ -46,8 +46,9 @@ def run_setup_on_app(app_id, allow_install=True, rerun=False): show_notification = show_message = (current_version or not app.info.is_essential) return operation_module.manager.new( - app_id, name, _run_setup_on_app, [app, current_version], - show_message=show_message, show_notification=show_notification, + f'{app_id}-setup', app_id, name, _run_setup_on_app, + [app, current_version], show_message=show_message, + show_notification=show_notification, thread_data={'allow_install': allow_install}) @@ -101,7 +102,7 @@ def run_uninstall_on_app(app_id): return logger.debug('Creating operation to uninstall app: %s', app_id) - return operation_module.manager.new(app_id, + return operation_module.manager.new(f'{app_id}-uninstall', app_id, gettext_noop('Uninstalling app'), _run_uninstall_on_app, [app], show_notification=True) @@ -450,7 +451,8 @@ class ForceUpgrader(): def _run_force_upgrade_as_operation(self, app, packages): """Start an operation for force upgrading.""" name = gettext_noop('Updating app packages') - operation = operation_module.manager.new(app.app_id, name, + operation = operation_module.manager.new(f'{app.app_id}-force-upgrade', + app.app_id, name, app.force_upgrade, [packages], show_message=False, show_notification=False) diff --git a/plinth/tests/test_operation.py b/plinth/tests/test_operation.py index 21af4dcc6..f4baaeeb1 100644 --- a/plinth/tests/test_operation.py +++ b/plinth/tests/test_operation.py @@ -3,6 +3,7 @@ import threading import time +from collections import OrderedDict from unittest.mock import Mock, call, patch import pytest @@ -28,7 +29,8 @@ class AppTest(app.App): def test_operation_default_initialization(update_notification): """Test Operation initialization with default values.""" target = Mock() - operation = Operation('testapp', 'op1', target) + operation = Operation('testid', 'testapp', 'op1', target) + assert operation.op_id == 'testid' assert operation.app_id == 'testapp' assert operation.name == 'op1' assert operation.show_message @@ -50,7 +52,7 @@ def test_operation_default_initialization(update_notification): def test_operation_initialization(update_notification): """Test Operation initialization with explicit values.""" on_complete = Mock() - operation = Operation('testapp', 'op1', Mock(), ['arg1'], + operation = Operation('testid', 'testapp', 'op1', Mock(), ['arg1'], {'arg2': 'value2'}, False, True, {'data1': 'datavalue1'}, on_complete) assert not operation.show_message @@ -67,7 +69,7 @@ def test_operation_initialization(update_notification): def test_operation_str(): """Test string representation of operation.""" - operation = Operation('testapp', 'op1', Mock()) + operation = Operation('testid', 'testapp', 'op1', Mock()) assert str(operation) == 'Operation: testapp: op1' @@ -77,7 +79,7 @@ def test_successful_operation(update_notification): target = Mock() target.return_value = 'test-return' on_complete = Mock() - operation = Operation('testapp', 'op1', target, ['arg1'], + operation = Operation('testid', 'testapp', 'op1', target, ['arg1'], {'arg2': 'value2'}, on_complete=on_complete) operation.run() assert operation.join() == 'test-return' @@ -94,7 +96,7 @@ def test_error_operation(update_notification): target = Mock() target.side_effect = RuntimeError('error1') on_complete = Mock() - operation = Operation('testapp', 'op1', target, ['arg1'], + operation = Operation('testid', 'testapp', 'op1', target, ['arg1'], {'arg2': 'value2'}, on_complete=on_complete) operation.run() with pytest.raises(RuntimeError): @@ -111,7 +113,7 @@ def test_error_operation(update_notification): def test_join_before_start(update_notification): """Test waiting until operation finishes..""" event = threading.Event() - operation = Operation('testapp', 'op1', Mock) + operation = Operation('testid', 'testapp', 'op1', Mock) success = [] def _wait(): @@ -135,7 +137,7 @@ def test_join_raises_exception(update_notification): target = Mock() target.side_effect = RuntimeError('error1') on_complete = Mock() - operation = Operation('testapp', 'op1', target, ['arg1'], + operation = Operation('testid', 'testapp', 'op1', target, ['arg1'], {'arg2': 'value2'}, on_complete=on_complete) operation.run() with pytest.raises(RuntimeError): @@ -149,7 +151,7 @@ def test_getting_operation_from_thread(): operation = Operation.get_operation() operation.thread_data['test_operation'] = operation - operation = Operation('testapp', 'op1', target) + operation = Operation('testid', 'testapp', 'op1', target) operation.run() operation.join() assert operation.thread_data['test_operation'] == operation @@ -164,7 +166,7 @@ def test_updating_operation(update_notification): operation = Operation.get_operation() operation.on_update('message1', exception) - operation = Operation('testapp', 'op1', target) + operation = Operation('testid', 'testapp', 'op1', target) operation.run() with pytest.raises(RuntimeError): operation.join() @@ -177,7 +179,7 @@ def test_updating_operation(update_notification): @patch('plinth.app.App.get') def test_message(app_get): """Test getting the operation's message.""" - operation = Operation('testapp', 'op1', Mock()) + operation = Operation('testid', 'testapp', 'op1', Mock()) operation._message = 'message1' operation.exception = RuntimeError('error1') assert operation.message == 'message1' @@ -208,7 +210,8 @@ def test_message(app_get): def test_update_notification(app_get): """Test that operation notification is created.""" app_get.return_value = AppTest() - operation = Operation('testapp', 'op1', Mock(), show_notification=True) + operation = Operation('testid', 'testapp', 'op1', Mock(), + show_notification=True) note = Notification.get('testapp-operation') assert note.id == 'testapp-operation' assert note.app_id == 'testapp' @@ -239,7 +242,7 @@ def test_manager_global_instance(): def test_manager_init(): """Test initializing operations manager.""" manager = OperationsManager() - assert manager._operations == [] + assert manager._operations == {} assert manager._current_operation is None assert isinstance(manager._lock, threading.RLock().__class__) @@ -252,14 +255,15 @@ def test_manager_new(): def target(): event.wait() - operation = manager.new('testapp', 'op1', target) + operation = manager.new('testop', 'testapp', 'op1', target) assert isinstance(operation, Operation) assert manager._current_operation == operation - assert manager._operations == [operation] + assert manager._operations == {'testop': operation} + event.set() operation.join() assert manager._current_operation is None - assert manager._operations == [operation] + assert manager._operations == OrderedDict(testop=operation) def test_manager_new_without_show_message(): @@ -270,11 +274,28 @@ def test_manager_new_without_show_message(): def target(): event.wait() - operation = manager.new('testapp', 'op1', target, show_message=False) + operation = manager.new('testop', 'testapp', 'op1', target, + show_message=False) event.set() operation.join() assert manager._current_operation is None - assert manager._operations == [] + assert manager._operations == {} + + +def test_manager_new_raises(): + """Test that a new operation is always unique.""" + manager = OperationsManager() + operation1 = manager.new('testop1', 'testapp', 'op1', Mock()) + + # Creating operation with same id throws exception + with pytest.raises(KeyError): + manager.new('testop1', 'testapp', 'op1', Mock()) + + # Creating operation with different ID works + operation2 = manager.new('testop2', 'testapp', 'op3', Mock()) + + assert manager._operations == OrderedDict(testop1=operation1, + testop2=operation2) def test_manager_scheduling(): @@ -284,13 +305,15 @@ def test_manager_scheduling(): event2 = threading.Event() event3 = threading.Event() - operation1 = manager.new('testapp', 'op1', event1.wait) - operation2 = manager.new('testapp', 'op2', event2.wait) - operation3 = manager.new('testapp', 'op3', event3.wait) + operation1 = manager.new('testop1', 'testapp', 'op1', event1.wait) + operation2 = manager.new('testop2', 'testapp', 'op2', event2.wait) + operation3 = manager.new('testop3', 'testapp', 'op3', event3.wait) def _assert_is_running(current_operation): assert manager._current_operation == current_operation - assert manager._operations == [operation1, operation2, operation3] + assert manager._operations == OrderedDict(testop1=operation1, + testop2=operation2, + testop3=operation3) for operation in [operation1, operation2, operation3]: alive = (operation == current_operation) assert operation.thread.is_alive() == alive @@ -311,9 +334,9 @@ def test_manager_scheduling(): def test_manager_filter(): """Test returning filtered operations.""" manager = OperationsManager() - operation1 = manager.new('testapp1', 'op1', Mock()) - operation2 = manager.new('testapp1', 'op2', Mock()) - operation3 = manager.new('testapp2', 'op3', Mock()) + operation1 = manager.new('testop1', 'testapp1', 'op1', Mock()) + operation2 = manager.new('testop2', 'testapp1', 'op2', Mock()) + operation3 = manager.new('testop3', 'testapp2', 'op3', Mock()) manager.filter('testapp1') == [operation1, operation2] manager.filter('testapp2') == [operation3] @@ -323,14 +346,15 @@ def test_manager_collect_results(): manager = OperationsManager() event = threading.Event() - operation1 = manager.new('testapp1', 'op1', Mock()) - operation2 = manager.new('testapp2', 'op2', Mock()) - operation3 = manager.new('testapp1', 'op3', event.wait) + operation1 = manager.new('testop1', 'testapp1', 'op1', Mock()) + operation2 = manager.new('testop2', 'testapp2', 'op2', Mock()) + operation3 = manager.new('testop3', 'testapp1', 'op3', event.wait) operation1.join() operation2.join() assert manager.collect_results('testapp1') == [operation1] - assert manager._operations == [operation2, operation3] + assert manager._operations == OrderedDict(testop2=operation2, + testop3=operation3) event.set() operation3.join() assert manager.collect_results('testapp1') == [operation3] - assert manager._operations == [operation2] + assert manager._operations == OrderedDict(testop2=operation2)