From 10ac4466ea05c40abda1b68b56d35c9a48c23128 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 22 Apr 2026 15:34:28 +0100 Subject: [PATCH 01/17] Implement a global lock This commit adds: * A server config key for the global lock * A class to implement the global lock * Context managers used when running actions or setting properties, to acquire said global lock Properties will either use the lock for setting, or not use the lock. If functional properties should use the lock for reading, they can acquire it in the getter. No documentation yet, but there are some tests for the lock. We still need some tests of the lock in the context of properties and actions, including the argument to opt out. --- src/labthings_fastapi/actions.py | 50 +++++++- src/labthings_fastapi/exceptions.py | 18 +++ src/labthings_fastapi/global_lock.py | 79 ++++++++++++ src/labthings_fastapi/properties.py | 83 ++++++++++--- src/labthings_fastapi/server/__init__.py | 2 + src/labthings_fastapi/server/config_model.py | 13 ++ src/labthings_fastapi/testing.py | 17 ++- .../thing_server_interface.py | 45 ++++++- tests/test_global_lock.py | 112 ++++++++++++++++++ tests/test_invocation_contexts.py | 16 +-- tests/test_property.py | 6 +- tests/utilities.py | 14 +++ 12 files changed, 415 insertions(+), 40 deletions(-) create mode 100644 src/labthings_fastapi/global_lock.py create mode 100644 tests/test_global_lock.py diff --git a/src/labthings_fastapi/actions.py b/src/labthings_fastapi/actions.py index fc27af6f..9dd97ea8 100644 --- a/src/labthings_fastapi/actions.py +++ b/src/labthings_fastapi/actions.py @@ -18,7 +18,7 @@ import datetime import logging from collections import deque -from functools import partial +from functools import partial, wraps import inspect from threading import Thread, Lock import uuid @@ -29,6 +29,7 @@ Callable, Concatenate, Generic, + Literal, Optional, ParamSpec, TypeVar, @@ -39,6 +40,7 @@ from fastapi import APIRouter, FastAPI, HTTPException, Request, Body, BackgroundTasks from pydantic import BaseModel, create_model + from .middleware.url_for import URLFor from .base_descriptor import ( BaseDescriptor, @@ -665,6 +667,7 @@ def __init__( func: Callable[Concatenate[OwnerT, ActionParams], ActionReturn], response_timeout: float = 1, retention_time: float = 300, + use_global_lock: Literal[False] | None = None, ) -> None: """Create a new action descriptor. @@ -683,6 +686,17 @@ def __init__( of the action. :param retention_time: how long, in seconds, the action should be kept for after it has completed. + :param use_global_lock: If the global lock is enabled in `lt.FEATURE_FLAGS` + this parameter may be used to opt out. When the global lock is enabled, + by default all actions acquire the global lock before starting, and + release it after they finish. That means only one action thread may + run at a time. The same lock is used to set properties. + + If this parameter is `False` then the lock will not be acquired, even + if global locking is enabled. That is appropriate if the action does + not have side effects that would cause problems for other actions, or + if more nuanced locking behaviour is required meaning the lock is + acquired directly in the action code. """ super().__init__() self.func = func @@ -692,6 +706,7 @@ def __init__( name = func.__name__ # this is checked in __set_name__ self.response_timeout = response_timeout self.retention_time = retention_time + self.use_global_lock = use_global_lock self.dependency_params = fastapi_dependency_params(func) self.input_model = input_model_from_signature( func, @@ -725,19 +740,48 @@ def __set_name__(self, owner: type[OwnerT], name: str) -> None: f"'{self.func.__name__}'", ) + def wrapped_func( + self, obj: OwnerT + ) -> Callable[Concatenate[OwnerT, ActionParams], ActionReturn]: + """Wrap the action function if necessary, so that it holds the global lock. + + If global locking is enabled and this action hasn't opted out, this function + will wrap `func` such that it holds the global lock while it is running. + + :param obj: The object on which the method is being called. + :return: the function, wrapped if necessary. + """ + # hold_global_lock returns a context manager. It won't hold the lock + # until we enter the context in `wrapped` (defined below). + lock_context_manager = obj._thing_server_interface.hold_global_lock( + self.use_global_lock + ) + func = self.func + + @wraps(func) + def wrapped(*args: Any, **kwargs: Any) -> Any: # noqa: DOC + """Acquire the lock then run `func` with supplied arguments.""" + with lock_context_manager: + return func(*args, **kwargs) + + return wrapped + def instance_get(self, obj: OwnerT) -> Callable[ActionParams, ActionReturn]: """Return the function, bound to an object as for a normal method. This currently doesn't validate the arguments, though it may do so - in future. In its present form, this is equivalent to a regular + in future. If locking is disabled this is equivalent to a regular Python method, i.e. all we do is supply the first argument, `self`. + If locking is enabled, we return a wrapped function that holds the + global lock while the action runs. + :param obj: the `~lt.Thing` to which we are attached. This will be the first argument supplied to the function wrapped by this descriptor. :return: the action function, bound to ``obj``. """ - return partial(self.func, obj) + return partial(self.wrapped_func(obj), obj) def _observers_set(self, obj: Thing) -> WeakSet: """Return a set used to notify changes. diff --git a/src/labthings_fastapi/exceptions.py b/src/labthings_fastapi/exceptions.py index d9454c1d..d60b36f8 100644 --- a/src/labthings_fastapi/exceptions.py +++ b/src/labthings_fastapi/exceptions.py @@ -348,6 +348,15 @@ class InvalidClassSettingsError(ValueError): """ +class FeatureNotEnabledError(RuntimeError): + """A feature is being used that is currently disabled. + + Some new or optional features are only available if the relevant feature flag + is set. See `lt.FEATURE_FLAGS` for a list of features that may be enabled. + This error is raised if a feature is used when it is not enabled. + """ + + class PropertyRedefinitionError(AttributeError): """A property is being incorrectly redefined. @@ -363,3 +372,12 @@ class DefaultWillChangeWarning(DeprecationWarning): A default value will change in the future. This warning can usually be eliminated by setting the value explicitly. """ + + +class GlobalLockBusyError(TimeoutError): + """The global lock is already in use. + + This exception is raised when code needs the global lock but cannot acquire + it. It indicates that the LabThings server is busy running another action or + property setter. + """ diff --git a/src/labthings_fastapi/global_lock.py b/src/labthings_fastapi/global_lock.py new file mode 100644 index 00000000..8d29fbfa --- /dev/null +++ b/src/labthings_fastapi/global_lock.py @@ -0,0 +1,79 @@ +"""Global locking. + +If the feature is enabled, a global lock is used to restrict running actions +and setting properties. This module defines a wrapper for `threading.RLock` +with a context manager that acquires the lock using a short timeout. +""" + +from threading import RLock +from types import EllipsisType, TracebackType + +from .exceptions import GlobalLockBusyError + + +class GlobalLock: + """An RLock wrapper and work-a-like with a default timeout.""" + + def __init__(self) -> None: + """Initialise the global lock.""" + self._lock = RLock() + + default_timeout: float = 0.05 + + def acquire( + self, blocking: bool = True, timeout: float | EllipsisType = ... + ) -> bool: + """Acquire the lock. + + This wraps the underlying `threading.RLock.acquire` but will by default + block with a short timeout. + + :param blocking: whether to wait for the lock to become free. `True` (the + default) will block until the lock is available or we time out. `False` + will always return immediately. + :param timeout: the length of time to wait for the lock, if ``blocking`` is + `True` - or `-1` to specify waiting forever. + + :return: whether the lock was successfully acquired. + """ + if blocking is False: + return self._lock.acquire(blocking=False) + if timeout is ...: + timeout = self.default_timeout + return self._lock.acquire(blocking=blocking, timeout=timeout) + + def release(self) -> None: + """Release the lock. + + This wraps `threading.RLock.release` without modification. + """ + self._lock.release() + + def __enter__(self) -> None: + """Allow the lock to be used as a context manager. + + The behaviour when used as a context manager is different from a regular + `threading.RLock` because it will use the default timeout rather than + blocking forever. + + :raises GlobalLockBusyError: if the lock is in use by another thread. + """ + result = self.acquire(blocking=True, timeout=self.default_timeout) + if not result: + raise GlobalLockBusyError("The global lock could not be acquired.") + + def __exit__( + self, + exc_type: type[BaseException] | None, + exc_value: BaseException | None, + traceback: TracebackType | None, + ) -> None: + """Allow the lock to be used as a context manager. + + The lock is released when the context ends. No error handling is done. + + :param exc_type: the exception type, if one was raised (ignored). + :param exc_value: the exception, if one was raised (ignored). + :param traceback: the traceback, if an error was raised (ignored). + """ + self.release() diff --git a/src/labthings_fastapi/properties.py b/src/labthings_fastapi/properties.py index 92b6317e..e5491a4f 100644 --- a/src/labthings_fastapi/properties.py +++ b/src/labthings_fastapi/properties.py @@ -54,6 +54,7 @@ class attribute. Documentation is in strings immediately following the Any, Callable, Generic, + Literal, TypeVar, overload, TYPE_CHECKING, @@ -238,13 +239,21 @@ def property( @overload # use as `field: int = property(default=0)` def property( - *, default: Value, readonly: bool = False, **constraints: Any + *, + default: Value, + readonly: bool = False, + use_global_lock: Literal[False] | None = None, + **constraints: Any, ) -> Value: ... @overload # use as `field: int = property(default_factory=lambda: 0)` def property( - *, default_factory: Callable[[], Value], readonly: bool = False, **constraints: Any + *, + default_factory: Callable[[], Value], + readonly: bool = False, + use_global_lock: Literal[False] | None = None, + **constraints: Any, ) -> Value: ... @@ -254,6 +263,7 @@ def property( default: Value | EllipsisType = ..., default_factory: Callable[[], Value] | None = None, readonly: bool = False, + use_global_lock: Literal[False] | None = None, **constraints: Any, ) -> Value | FunctionalProperty[Owner, Value]: r"""Define a Property on a `~lt.Thing`\ . @@ -285,12 +295,14 @@ def property( a `.DirectThingClient`). This is automatically true if ``property`` is used as a decorator and no setter is specified. + :param use_global_lock: may be set to `False` to disable the global lock + for setting this property. By default, if global locking is enabled, + we hold the global lock while setting the property. :param \**constraints: additional keyword arguments are passed to `pydantic.Field` and allow constraints to be added to the property. For example, ``ge=0`` constrains a numeric property to be non-negative. See `pydantic.Field` for the full range of constraint arguments. - :return: a property descriptor, either a `.FunctionalProperty` if used as a decorator, or a `~lt.DataProperty` if used as a field. @@ -348,6 +360,7 @@ def property( default_factory=default_factory_from_arguments(default, default_factory), readonly=readonly, constraints=constraints, + use_global_lock=use_global_lock, ) @@ -362,13 +375,20 @@ class BaseProperty(FieldTypedBaseDescriptor[Owner, Value], Generic[Owner, Value] use `~lt.property` to declare properties on your `~lt.Thing` subclass. """ - def __init__(self, constraints: Mapping[str, Any] | None = None) -> None: + def __init__( + self, + constraints: Mapping[str, Any] | None = None, + use_global_lock: Literal[False] | None = None, + ) -> None: """Initialise a BaseProperty. :param constraints: is passed as keyword arguments to `pydantic.Field` to add validation constraints to the property. See `pydantic.Field` for details. The module-level constant `CONSTRAINT_ARGS` lists the supported constraint arguments. + :param use_global_lock: may be set to `False` to disable the global lock + for setting this property. By default, if global locking is enabled, + we hold the global lock while setting the property. :raises UnsupportedConstraintError: if unsupported constraint arguments are supplied. See `CONSTRAINT_ARGS` for the supported arguments. @@ -377,6 +397,7 @@ def __init__(self, constraints: Mapping[str, Any] | None = None) -> None: self._model: type[BaseModel] | None = None self.readonly: bool = False self._constraints: FieldConstraints = {} + self.use_global_lock = use_global_lock try: self.constraints = self._validate_constraints(constraints or {}) except UnsupportedConstraintError: @@ -656,6 +677,7 @@ def __init__( # noqa: DOC101,DOC103 *, readonly: bool = False, constraints: Mapping[str, Any] | None = None, + use_global_lock: Literal[False] | None = None, ) -> None: ... @overload @@ -665,6 +687,7 @@ def __init__( # noqa: DOC101,DOC103 default_factory: Callable[[], Value], readonly: bool = False, constraints: Mapping[str, Any] | None = None, + use_global_lock: Literal[False] | None = None, ) -> None: ... def __init__( @@ -674,6 +697,7 @@ def __init__( default_factory: Callable[[], Value] | None = None, readonly: bool = False, constraints: Mapping[str, Any] | None = None, + use_global_lock: Literal[False] | None = None, ) -> None: """Create a property that acts like a regular variable. @@ -707,8 +731,11 @@ def __init__( :param constraints: is passed as keyword arguments to `pydantic.Field` to add validation constraints to the property. See `pydantic.Field` for details. + :param use_global_lock: may be set to `False` to disable the global lock + for setting this property. By default, if global locking is enabled, + we hold the global lock while setting the property. """ - super().__init__(constraints=constraints) + super().__init__(constraints=constraints, use_global_lock=use_global_lock) self._default_factory = default_factory_from_arguments( default=default, default_factory=default_factory ) @@ -743,13 +770,14 @@ def __set__( :param value: the new value for the property. :param emit_changed_event: whether to emit a changed event. """ - if get_validate_properties_on_set(obj.__class__): - property_info = self.descriptor_info(obj) - obj.__dict__[self.name] = property_info.validate(value) - else: - obj.__dict__[self.name] = value - if emit_changed_event: - self.emit_changed_event(obj, value) + with obj._thing_server_interface.hold_global_lock(self.use_global_lock): + if get_validate_properties_on_set(obj.__class__): + property_info = self.descriptor_info(obj) + obj.__dict__[self.name] = property_info.validate(value) + else: + obj.__dict__[self.name] = value + if emit_changed_event: + self.emit_changed_event(obj, value) def get_default(self, obj: Owner | None) -> Value: """Return the default value of this property. @@ -837,6 +865,7 @@ def __init__( self, fget: Callable[[Owner], Value], constraints: Mapping[str, Any] | None = None, + use_global_lock: Literal[False] | None = None, ) -> None: """Set up a FunctionalProperty. @@ -849,10 +878,13 @@ def __init__( :param constraints: is passed as keyword arguments to `pydantic.Field` to add validation constraints to the property. See `pydantic.Field` for details. + :param use_global_lock: may be set to `False` to disable the global lock + for setting this property. By default, if global locking is enabled, + we hold the global lock while setting the property. :raises MissingTypeError: if the getter does not have a return type annotation. """ - super().__init__(constraints=constraints) + super().__init__(constraints=constraints, use_global_lock=use_global_lock) self._fget = fget self._type = return_type(self._fget) if fget.__doc__: @@ -1001,8 +1033,8 @@ def __set__(self, obj: Owner, value: Value) -> None: if get_validate_properties_on_set(obj.__class__): property_info = self.descriptor_info(obj) value = property_info.validate(value) - - self.fset(obj, value) + with obj._thing_server_interface.hold_global_lock(self.use_global_lock): + self.fset(obj, value) @builtins.property def default(self) -> Value: @@ -1274,12 +1306,22 @@ def setting( @overload # use as `field: int = setting(default=0)`` -def setting(*, default: Value, readonly: bool = False, **constraints: Any) -> Value: ... +def setting( + *, + default: Value, + readonly: bool = False, + use_global_lock: Literal[False] | None = None, + **constraints: Any, +) -> Value: ... @overload # use as `field: int = setting(default_factory=lambda: 0)` def setting( - *, default_factory: Callable[[], Value], readonly: bool = False, **constraints: Any + *, + default_factory: Callable[[], Value], + readonly: bool = False, + use_global_lock: Literal[False] | None = None, + **constraints: Any, ) -> Value: ... @@ -1289,6 +1331,7 @@ def setting( default: Value | EllipsisType = ..., default_factory: Callable[[], Value] | None = None, readonly: bool = False, + use_global_lock: Literal[False] | None = None, **constraints: Any, ) -> FunctionalSetting[Owner, Value] | Value: r"""Define a Setting on a `~lt.Thing`\ . @@ -1335,9 +1378,12 @@ def setting( :param readonly: whether the setting should be read-only via the `~lt.ThingClient` interface (i.e. over HTTP or via a `.DirectThingClient`). + :param use_global_lock: may be set to `False` to disable the global lock + for setting this setting. By default, if global locking is enabled, + we hold the global lock while setting the setting. :param \**constraints: additional keyword arguments are passed to `pydantic.Field` and allow constraints to be added to the - property. For example, ``ge=0`` constrains a numeric property + setting. For example, ``ge=0`` constrains a numeric setting to be non-negative. See `pydantic.Field` for the full range of constraint arguments. @@ -1373,6 +1419,7 @@ def setting( default_factory=default_factory_from_arguments(default, default_factory), readonly=readonly, constraints=constraints, + use_global_lock=use_global_lock, ) diff --git a/src/labthings_fastapi/server/__init__.py b/src/labthings_fastapi/server/__init__.py index ae8a8fd1..265b0e43 100644 --- a/src/labthings_fastapi/server/__init__.py +++ b/src/labthings_fastapi/server/__init__.py @@ -31,6 +31,7 @@ from ..thing import Thing from ..thing_server_interface import ThingServerInterface from ..thing_description._model import ThingDescription +from ..global_lock import GlobalLock from .config_model import ( ThingsConfig, ThingServerConfig, @@ -147,6 +148,7 @@ def __init__( self.blocking_portal: Optional[BlockingPortal] = None self.startup_status: dict[str, str | dict] = {"things": {}} global _thing_servers # noqa: F824 + self.global_lock = GlobalLock() if self._config.enable_global_lock else None # The function calls below create and set up the Things. self._things = self._create_things() self._connect_things() diff --git a/src/labthings_fastapi/server/config_model.py b/src/labthings_fastapi/server/config_model.py index 8cff6a97..fb9cd2cf 100644 --- a/src/labthings_fastapi/server/config_model.py +++ b/src/labthings_fastapi/server/config_model.py @@ -222,6 +222,19 @@ def thing_configs(self) -> Mapping[ThingName, ThingConfig]: ), ) + enable_global_lock: bool = Field( + default=False, + description=( + """Whether a global lock should be used to simplify concurrency. + + If this setting is `True`, actions will acquire a lock, meaning that + only one action can run at any time. The same applies to setting properties. + The intention here is that a running action shouldn't need to worry about + other code on the server changing things while it runs. + """ + ), + ) + application_config: dict[str, Any] | None = Field( default=None, description=( diff --git a/src/labthings_fastapi/testing.py b/src/labthings_fastapi/testing.py index fd37209e..95a9d70c 100644 --- a/src/labthings_fastapi/testing.py +++ b/src/labthings_fastapi/testing.py @@ -17,6 +17,8 @@ from tempfile import TemporaryDirectory from unittest.mock import Mock +from labthings_fastapi.global_lock import GlobalLock + from .utilities import class_attributes from .thing_slots import ThingSlot from .thing_server_interface import ThingServerInterface @@ -44,18 +46,26 @@ class MockThingServerInterface(ThingServerInterface): * `get_thing_states` will return an empty dictionary. """ - def __init__(self, name: str, settings_folder: str | None = None) -> None: + def __init__( + self, + name: str, + settings_folder: str | None = None, + enable_global_lock: bool = False, + ) -> None: """Initialise a ThingServerInterface. :param name: The name of the Thing we're providing an interface to. :param settings_folder: The location where we should save settings. By default, this is a temporary directory. + :param enable_global_lock: Whether to create a global lock object, to + mock the server setting of the same name. """ # We deliberately don't call super().__init__(), as it won't work without # a server. self._name: str = name self._settings_tempdir: TemporaryDirectory | None = None self._settings_folder = settings_folder + self._global_lock = GlobalLock() if enable_global_lock else None self._mocks: list[Mock] = [] def start_async_task_soon( @@ -129,6 +139,11 @@ def application_config(self) -> None: """ return None + @property + def global_lock(self) -> GlobalLock | None: + """Return a global lock.""" + return self._global_lock + ThingSubclass = TypeVar("ThingSubclass", bound="Thing") diff --git a/src/labthings_fastapi/thing_server_interface.py b/src/labthings_fastapi/thing_server_interface.py index d518c344..6952128b 100644 --- a/src/labthings_fastapi/thing_server_interface.py +++ b/src/labthings_fastapi/thing_server_interface.py @@ -1,7 +1,9 @@ r"""Interface between `~lt.Thing` subclasses and the `~lt.ThingServer`\ .""" from __future__ import annotations +from collections.abc import Iterator from concurrent.futures import Future +from contextlib import contextmanager from copy import deepcopy import os from typing import ( @@ -15,7 +17,9 @@ ) from weakref import ref, ReferenceType -from .exceptions import ServerNotRunningError +from labthings_fastapi.global_lock import GlobalLock + +from .exceptions import FeatureNotEnabledError, ServerNotRunningError if TYPE_CHECKING: from .server import ThingServer @@ -181,3 +185,42 @@ def _action_manager(self) -> ActionManager: This property may be removed in future, and is for internal use only. """ return self._get_server().action_manager + + @property + def global_lock(self) -> GlobalLock | None: + r"""A lock that ensures property writes and actions are one-at-a-time. + + If global locking is not enabled, this property will return None. + """ + return self._get_server().global_lock + + @contextmanager + def hold_global_lock(self, enabled: bool | None = True) -> Iterator[None]: + """Hold the global lock, if required, as a context manager. + + This function will hold the global lock if necessary while a block of code runs. + Its behaviour is controlled by the `enabled` parameter: if `enabled` is `False` + this function does nothing. If it is `None` (the default when called from a + property or action that's not otherwise configured), the global lock is + held if it exists, but no error is raised if global locking is disabled. + + If ``enabled`` is `True` (the default if no arguments are passed), an error + will be raised if there is no global lock. + + :param enabled: whether to use the global lock. `True` and `False` have the + obvious meanings described above, `None` will use the lock if it is enabled + globally but won't raise an error if it is unavailable. + :raises FeatureNotEnabledError: if `enabled` is `True` but the global lock is + not enabled. + """ + if self.global_lock is None: + if enabled is True: + msg = "The global lock is required, but is not enabled." + raise FeatureNotEnabledError(msg) + # If we get here, the global lock is disabled so we do nothing. + yield + else: + if enabled is False: # The lock is being explicitly skipped + yield + with self.global_lock: + yield diff --git a/tests/test_global_lock.py b/tests/test_global_lock.py new file mode 100644 index 00000000..4329c32f --- /dev/null +++ b/tests/test_global_lock.py @@ -0,0 +1,112 @@ +"""Test code for the global lock.""" + +from threading import Thread, Event +from labthings_fastapi.exceptions import GlobalLockBusyError +import pytest + +from labthings_fastapi.global_lock import GlobalLock + +from .utilities import assert_takes_time + + +class LockChecker(Thread): + def __init__(self, lock: GlobalLock): + super().__init__() + self._lock = lock + + def run(self): + self.acquired = self._lock.acquire(blocking=False) + if self.acquired: + self._lock.release() + + +def lock_is_available(lock: GlobalLock) -> bool: + """Check whether a lock is locked. + + This is needed for Python < 3.14 as there's no `locked` property. + """ + checker = LockChecker(lock) + checker.start() + checker.join() + return checker.acquired + + +def test_global_lock_unthreaded(): + """Test that the global lock acquires and releases the underlying `RLock`""" + lock = GlobalLock() + lock.default_timeout = 0.001 + + # The lock starts out available + assert lock_is_available(lock) + + # Once acquired, it's not available to other threads + lock.acquire() + assert not lock_is_available(lock) + + # It should be acquireable several times in this thread + lock.acquire() + assert not lock_is_available(lock) + lock.release() + + # It needs to be released once per acquire call + assert not lock_is_available(lock) + lock.release() + assert lock_is_available(lock) + + # The same thing should work with context manager use + with lock: + assert not lock_is_available(lock) + with lock: + assert not lock_is_available(lock) + assert not lock_is_available(lock) + assert lock_is_available(lock) + + # Or mixed use + with lock: + assert not lock_is_available(lock) + lock.acquire() + assert not lock_is_available(lock) + with lock: + lock.acquire() + assert not lock_is_available(lock) + lock.release() + assert not lock_is_available(lock) + lock.release() + assert not lock_is_available(lock) + assert lock_is_available(lock) + + +def test_global_lock_timeout(): + """Check the global lock times out correctly.""" + lock = GlobalLock() + lock.default_timeout = 0.05 + finished = Event() + + def hold_lock_in_background(): + with lock: + finished.wait(5) + + # Hold the lock in another thread + t = Thread(target=hold_lock_in_background) + t.start() + + # acquire() with no arguments should use the default timeout + with assert_takes_time(0.045, 0.1): + assert lock.acquire() is False + with assert_takes_time(0.045, 0.1): + assert lock.acquire(blocking=True) is False + + # check non-blocking acquire() works + with assert_takes_time(None, 0.001): + assert lock.acquire(blocking=False) is False + + # context manager use should also use the default timeout + with assert_takes_time(0.045, 0.1): + with pytest.raises(GlobalLockBusyError): + with lock: + pass + + # check the lock is still being held + assert t.is_alive + finished.set() + t.join() diff --git a/tests/test_invocation_contexts.py b/tests/test_invocation_contexts.py index 657e6347..ce4bb4b5 100644 --- a/tests/test_invocation_contexts.py +++ b/tests/test_invocation_contexts.py @@ -5,8 +5,6 @@ ``test_action_cancel`` . """ -from contextlib import contextmanager -import time import pytest import uuid from threading import Thread @@ -25,6 +23,7 @@ NoInvocationContextError, InvocationCancelledError, ) +from .utilities import assert_takes_time def append_invocation_id(ids: list): @@ -77,19 +76,6 @@ def test_getting_and_setting_id(): assert isinstance(ids[0], NoInvocationContextError) -@contextmanager -def assert_takes_time(min_t: float | None, max_t: float | None): - """Assert that a code block takes a certain amount of time.""" - before = time.time() - yield - after = time.time() - duration = after - before - if min_t is not None: - assert duration >= min_t - if max_t is not None: - assert duration <= max_t - - def test_cancel_event(): """Check the cancel event works as intended.""" id = uuid.uuid4() diff --git a/tests/test_property.py b/tests/test_property.py index 4eb29f77..a778700c 100644 --- a/tests/test_property.py +++ b/tests/test_property.py @@ -141,7 +141,8 @@ def getter(self) -> str: assert prop.kwargs["default_factory"]() == 0 assert prop.kwargs["readonly"] is False assert prop.kwargs["constraints"] == {} - assert len(prop.kwargs) == 3 + assert prop.kwargs["use_global_lock"] is None + assert len(prop.kwargs) == 4 # The same thing should happen when we use a factory, # except it should pass through the factory function unchanged. @@ -151,7 +152,8 @@ def getter(self) -> str: assert prop.kwargs["default_factory"] is list assert prop.kwargs["readonly"] is False assert prop.kwargs["constraints"] == {} - assert len(prop.kwargs) == 3 + assert prop.kwargs["use_global_lock"] is None + assert len(prop.kwargs) == 4 # The positional argument is the setter, so `None` is not valid # and probably means someone forgot to add `default=`. diff --git a/tests/utilities.py b/tests/utilities.py index c5d3a574..7b0b87e4 100644 --- a/tests/utilities.py +++ b/tests/utilities.py @@ -2,6 +2,7 @@ from contextlib import contextmanager from typing import Iterator +import time import pytest @@ -30,3 +31,16 @@ def raises_or_is_caused_by( # already have failed. traceback = excinfo._excinfo[2] excinfo._excinfo = (exception_cls, excinfo.value.__cause__, traceback) + + +@contextmanager +def assert_takes_time(min_t: float | None, max_t: float | None): + """Assert that a code block takes a certain amount of time.""" + before = time.time() + yield + after = time.time() + duration = after - before + if min_t is not None: + assert duration >= min_t + if max_t is not None: + assert duration <= max_t From f7caa9657c9f14a4ac864fd8b9d29194ecf2d04b Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 23 Apr 2026 00:39:22 +0100 Subject: [PATCH 02/17] Add tests for lock-related functions and fix bugs I've added tests for the ThingServerInterface functions related to the lock. This revealed a bug that is now fixed. I also added a convenience function to mock Thing instances, which provides a few attributes that are needed by the various descriptors and should help keep test suites tidy. --- docs/source/public_api.rst | 11 ++++ src/labthings_fastapi/testing.py | 25 ++++++++ .../thing_server_interface.py | 5 +- tests/test_properties.py | 5 +- tests/test_thing_server_interface.py | 58 ++++++++++++++++++- 5 files changed, 98 insertions(+), 6 deletions(-) diff --git a/docs/source/public_api.rst b/docs/source/public_api.rst index 4c0ec6df..bc055789 100644 --- a/docs/source/public_api.rst +++ b/docs/source/public_api.rst @@ -302,6 +302,17 @@ This page summarises the parts of the LabThings API that should be most frequent .. automethod:: labthings_fastapi.thing_server_interface.ThingServerInterface.get_thing_states :no-index: + .. py:property:: global_lock + :type GlobalLock | None: + + A global lock object that is used to restrict concurrent execution of actions and setting of properties. + + .. py:method:: hold_global_lock(enabled: bool | None = True) + + A context manager that holds the global lock. The `enabled` parameter sets + whether the lock is held. `False` ignores the lock, `None` uses the lock if + available, and `True` uses the lock or raises an error if it is missing. + .. py:class:: ThingClassSettings diff --git a/src/labthings_fastapi/testing.py b/src/labthings_fastapi/testing.py index 95a9d70c..79e99b8d 100644 --- a/src/labthings_fastapi/testing.py +++ b/src/labthings_fastapi/testing.py @@ -144,6 +144,17 @@ def global_lock(self) -> GlobalLock | None: """Return a global lock.""" return self._global_lock + @contextmanager + def hold_global_lock(self, enabled: bool | None = True) -> Iterator[None]: + """Hold the global lock while a block of code executes. + + See `lt.ThingServerInterface.hold_global_lock` for full documentation. + + :param enabled: whether or not the lock must be held. + """ + with ThingServerInterface.hold_global_lock(self, enabled): + yield + ThingSubclass = TypeVar("ThingSubclass", bound="Thing") @@ -197,6 +208,20 @@ def create_thing_without_server( return thing +def mock_thing_instance(spec: type[ThingSubclass]) -> ThingSubclass: + """Create a mock Thing instance, with some important attributes. + + :param spec: the Thing subclass we're mocking an instance of. Pass + `lt.Thing` if it doesn't matter. + :return: a Mock instance that pretends to be an instance of `spec`. + """ + mock = Mock(spec=spec) + mock.__name__ = "Mock{spec.__name__}" + mock.__module__ = "mock_module" + mock._thing_server_interface = MockThingServerInterface(mock.__name__) + return mock + + def _mock_slots(thing: Thing) -> None: """Mock the slots of a thing created by create_thing_without_server. diff --git a/src/labthings_fastapi/thing_server_interface.py b/src/labthings_fastapi/thing_server_interface.py index 6952128b..67225f2d 100644 --- a/src/labthings_fastapi/thing_server_interface.py +++ b/src/labthings_fastapi/thing_server_interface.py @@ -222,5 +222,6 @@ def hold_global_lock(self, enabled: bool | None = True) -> Iterator[None]: else: if enabled is False: # The lock is being explicitly skipped yield - with self.global_lock: - yield + else: + with self.global_lock: + yield diff --git a/tests/test_properties.py b/tests/test_properties.py index 7e0700d9..a1806b12 100644 --- a/tests/test_properties.py +++ b/tests/test_properties.py @@ -14,7 +14,7 @@ UnsupportedConstraintError, ) from labthings_fastapi.properties import BaseProperty, PropertyInfo -from labthings_fastapi.testing import create_thing_without_server +from labthings_fastapi.testing import create_thing_without_server, mock_thing_instance from .temp_client import poll_task @@ -410,8 +410,7 @@ def test_constrained_properties(prop_info, mocker): assert prop.value_type is prop_info.value_type m = prop.model assert issubclass(m, RootModel) - mock_thing = mocker.Mock(spec=PropertyTestThing) - mock_thing._thing_server_interface = mocker.Mock() + mock_thing = mock_thing_instance(spec=PropertyTestThing) descriptorinfo = prop.descriptor_info(mock_thing) assert isinstance(descriptorinfo, PropertyInfo) for ann in prop_info.constraints: diff --git a/tests/test_thing_server_interface.py b/tests/test_thing_server_interface.py index 8bed3a6e..9cba438c 100644 --- a/tests/test_thing_server_interface.py +++ b/tests/test_thing_server_interface.py @@ -6,10 +6,16 @@ from typing import Mapping from unittest.mock import Mock +from fastapi.testclient import TestClient +from labthings_fastapi.global_lock import GlobalLock import pytest import labthings_fastapi as lt -from labthings_fastapi.exceptions import ServerNotRunningError, ThingNotConnectedError +from labthings_fastapi.exceptions import ( + FeatureNotEnabledError, + ServerNotRunningError, + ThingNotConnectedError, +) from labthings_fastapi.thing_server_interface import ( ThingServerInterface, ThingServerMissingError, @@ -19,6 +25,8 @@ create_thing_without_server, ) +from .test_global_lock import lock_is_available + NAME = "testname" EXAMPLE_THING_STATE = {"foo": "bar"} @@ -310,3 +318,51 @@ def test_mocking_slots(): # These should also be the thing names grouped_thing_names = {i.name for i in slotty.dif_grouped_things.values()} assert set(DIF_GROUPED_NAMES) == grouped_thing_names + + +@pytest.mark.parametrize("enable", (False, True)) +def test_global_lock(enable): + """Test that the global lock is accessible, if configured.""" + server = lt.ThingServer({}, enable_global_lock=enable) + interface = lt.ThingServerInterface(server, "thing_name") + if enable: + assert isinstance(interface.global_lock, GlobalLock) + else: + assert interface.global_lock is None + + +@pytest.mark.parametrize("mock", (False, True)) +def test_mock_hold_global_lock(mock): + """Test the `hold_global_lock` method, with and without a global lock.""" + # By default, there is no global lock. + if mock: + interface = MockThingServerInterface("thing_name") + else: + server = lt.ThingServer({}) + interface = lt.ThingServerInterface(server, "thing_name") + assert interface.global_lock is None + # With no global lock, the context manager should be a no-op, unless we + # specify `enabled=True` at which point it errors. + with interface.hold_global_lock(False): + pass # hold_global_lock should be a no-op + with interface.hold_global_lock(None): + pass # hold_global_lock should be a no-op + with pytest.raises(FeatureNotEnabledError): + with interface.hold_global_lock(True): + pass # hold_global_lock should error, as there's no lock + + # If specified, there will be a global lock. + if mock: + interface = MockThingServerInterface("thing_name", enable_global_lock=True) + else: + server = lt.ThingServer({}, enable_global_lock=True) + interface = ThingServerInterface(server, "thing_name") + assert isinstance(interface.global_lock, GlobalLock) + # That means the context manager should work for all three arguments. + with interface.hold_global_lock(False): + assert lock_is_available(interface.global_lock) + with interface.hold_global_lock(None): + assert not lock_is_available(interface.global_lock) + with interface.hold_global_lock(True): + assert not lock_is_available(interface.global_lock) + assert lock_is_available(interface.global_lock) From 6f173111fd399247c4c1da9ff2838381a005c7c4 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 23 Apr 2026 01:32:07 +0100 Subject: [PATCH 03/17] Basic test of locking, without server. --- src/labthings_fastapi/testing.py | 8 +- tests/test_global_lock.py | 166 +++++++++++++++++++++++++++++++ 2 files changed, 173 insertions(+), 1 deletion(-) diff --git a/src/labthings_fastapi/testing.py b/src/labthings_fastapi/testing.py index 79e99b8d..6c000c1f 100644 --- a/src/labthings_fastapi/testing.py +++ b/src/labthings_fastapi/testing.py @@ -164,6 +164,7 @@ def create_thing_without_server( *args: Any, settings_folder: str | None = None, mock_all_slots: bool = False, + enable_global_lock: bool = True, **kwargs: Any, ) -> ThingSubclass: r"""Create a `~lt.Thing` and supply a mock ThingServerInterface. @@ -182,6 +183,7 @@ def create_thing_without_server( connected to each thing slot. It follows the default of the specified to the slot. So if an optional slot has a default of `None`, no mock will be provided. + :param enable_global_lock: Whether a global lock should be provided. :param \**kwargs: keyword arguments to ``__init__``. :returns: an instance of ``cls`` with a `.MockThingServerInterface` @@ -195,7 +197,11 @@ def create_thing_without_server( msg = "You may not supply a keyword argument called 'thing_server_interface'." raise ValueError(msg) - msi = MockThingServerInterface(name=name, settings_folder=settings_folder) + msi = MockThingServerInterface( + name=name, + settings_folder=settings_folder, + enable_global_lock=enable_global_lock, + ) # Note: we must ignore misc typing errors above because mypy flags an error # that `thing_server_interface` is multiply specified. # This is a conflict with *args, if we had only **kwargs it would not flag diff --git a/tests/test_global_lock.py b/tests/test_global_lock.py index 4329c32f..e30d8b73 100644 --- a/tests/test_global_lock.py +++ b/tests/test_global_lock.py @@ -2,9 +2,11 @@ from threading import Thread, Event from labthings_fastapi.exceptions import GlobalLockBusyError +from labthings_fastapi.testing import create_thing_without_server import pytest from labthings_fastapi.global_lock import GlobalLock +import labthings_fastapi as lt from .utilities import assert_takes_time @@ -20,6 +22,99 @@ def run(self): self._lock.release() +class ConcurrencyChecker(lt.Thing): + """A class to check if actions may run concurrently.""" + + def __init__(self, thing_server_interface: lt.ThingServerInterface): + super().__init__(thing_server_interface) + self._tick_event = Event() + self._tock_event = Event() + self._fprop1 = 0 + self._fprop2 = 0 + + def tick(self): + """Set the tick event and block until it's acknowledged. + + This avoids race conditions in the test code. + """ + self._tick_event.set() + self._tock_event.wait(0.1) + self._tock_event.clear() + + changes_detected: bool = lt.property(default=False, use_global_lock=False) + + prop1: int = lt.property(default=0) + """A data property, subject to the global lock by default.""" + + prop2: int = lt.property(default=0, use_global_lock=False) + """A data property that may be changed without the lock.""" + + @lt.property + def fprop1(self) -> int: + """A functional property that is locked (by default).""" + return self._fprop1 + + @fprop1.setter + def _set_fprop1(self, val: int) -> None: + self._fprop1 = val + + @lt.property + def fprop2(self) -> int: + """A functional property that is not locked.""" + return self._fprop2 + + fprop2.use_global_lock = False + + @fprop2.setter + def _set_fprop2(self, val: int) -> None: + self._fprop2 = val + + @lt.action + def check_for_changes_unlocked(self, ticks=2) -> None: + """Check if any properties have changed. + + This function does not acquire the global lock. + + :param ticks: the number of times to wait for the tick event. + :return: whether any changes were detected. + """ + names = ["prop1", "prop2", "fprop1", "fprop2"] + initial_values = {n: getattr(self, n) for n in names} + print(f"initial: {initial_values}") + for _i in range(ticks): + self._tick_event.wait(timeout=0.1) + self._tick_event.clear() + for n in names: + # Check for changes and reset to initial state + if getattr(self, n) != initial_values[n]: + self.changes_detected = True + setattr(self, n, initial_values[n]) + self._tock_event.set() + + check_for_changes_unlocked.use_global_lock = False + + @lt.action + def check_for_changes_locked(self, ticks=2): + return self.check_for_changes_unlocked(ticks=ticks) + + @lt.action + def increment_fprop2(self): + self._fprop2 += 1 + + @lt.action + def increment_fprop2_unlocked(self): + self._fprop2 += 1 + + increment_fprop2_unlocked.use_global_lock = False + + @lt.action + def increment_prop1(self): + """This function is excluded from the lock - but prop1 is locked.""" + self.prop1 += 1 + + increment_prop1.use_global_lock = False + + def lock_is_available(lock: GlobalLock) -> bool: """Check whether a lock is locked. @@ -110,3 +205,74 @@ def hold_lock_in_background(): assert t.is_alive finished.set() t.join() + + +def test_global_lock_with_thing(): + """Ensure the global lock stops multiple things happening at once.""" + thing = create_thing_without_server(ConcurrencyChecker, enable_global_lock=True) + + # Start the background action that checks for changes. + monitor_thread = Thread( + target=thing.check_for_changes_unlocked, kwargs={"ticks": 8} + ) + monitor_thread.start() + + # When we are using the non-blocking checker, all the properties should work. + for name in ["prop1", "prop2", "fprop1", "fprop2"]: + thing.changes_detected = False + val = getattr(thing, name) + setattr(thing, name, val + 1) + thing.tick() + assert thing.changes_detected is True + + # Increment actions should work too. + for name in ["increment_fprop2", "increment_fprop2_unlocked", "increment_prop1"]: + thing.changes_detected = False + action = getattr(thing, name) + action() + thing.tick() + assert thing.changes_detected is True + + assert monitor_thread.is_alive() + thing.tick() + monitor_thread.join() + + # Start the background action that checks for changes, and holds the lock + monitor_thread = Thread(target=thing.check_for_changes_locked, kwargs={"ticks": 8}) + monitor_thread.start() + + # When we are holding the lock, by default properties can't be written. + for name in ["prop1", "fprop1"]: + thing.changes_detected = False + val = getattr(thing, name) # read should always succeed + with pytest.raises(GlobalLockBusyError): + setattr(thing, name, val + 1) + thing.tick() + assert thing.changes_detected is False + + # The properties excluded from the lock may still be written + for name in ["prop2", "fprop2"]: + thing.changes_detected = False + val = getattr(thing, name) + setattr(thing, name, val + 1) + thing.tick() + assert thing.changes_detected is True + + # By default, other actions won't run + for name in ["increment_fprop2", "increment_prop1"]: + thing.changes_detected = False + action = getattr(thing, name) + with pytest.raises(GlobalLockBusyError): + action() + thing.tick() + assert thing.changes_detected is False + + # Actions may run if they're excluded from the lock. + thing.changes_detected = False + thing.increment_fprop2_unlocked() + thing.tick() + assert thing.changes_detected is True + + assert monitor_thread.is_alive() + thing.tick() + monitor_thread.join() From 434561782ea1429d721bedc0e70843d6b5876768 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 23 Apr 2026 12:47:48 +0100 Subject: [PATCH 04/17] Tidy up tests with some helper functions --- tests/test_global_lock.py | 90 +++++++++++++++++++++++++-------------- 1 file changed, 58 insertions(+), 32 deletions(-) diff --git a/tests/test_global_lock.py b/tests/test_global_lock.py index e30d8b73..011806bc 100644 --- a/tests/test_global_lock.py +++ b/tests/test_global_lock.py @@ -80,7 +80,6 @@ def check_for_changes_unlocked(self, ticks=2) -> None: """ names = ["prop1", "prop2", "fprop1", "fprop2"] initial_values = {n: getattr(self, n) for n in names} - print(f"initial: {initial_values}") for _i in range(ticks): self._tick_event.wait(timeout=0.1) self._tick_event.clear() @@ -115,6 +114,58 @@ def increment_prop1(self): increment_prop1.use_global_lock = False +def assert_can_change_property(thing: ConcurrencyChecker, name: str): + """Check whether we can change a property of a Thing. + + :param thing: The ConcurrencyChecker instance being checked. + :param name: The name of the property. + :return: `True` if the property can be changed. + """ + thing.changes_detected = False + val = getattr(thing, name) # read should always succeed + setattr(thing, name, val + 1) + thing.tick() + assert thing.changes_detected is True + + +def assert_cannot_change_property( + thing: ConcurrencyChecker, name: str, error: Exception = GlobalLockBusyError +): + """Check whether we cannot change a property of a Thing. + + :param thing: The ConcurrencyChecker instance being checked. + :param name: The name of the property. + :return: `True` if setting the property raises an error. + """ + thing.changes_detected = False + val = getattr(thing, name) # read should always succeed + with pytest.raises(error): + setattr(thing, name, val + 1) + thing.tick() + assert thing.changes_detected is False + + +def assert_action_makes_change(thing: ConcurrencyChecker, name: str): + """Assert an action runs OK and causes properties to change.""" + thing.changes_detected = False + action = getattr(thing, name) + action() + thing.tick() + assert thing.changes_detected is True + + +def assert_action_fails( + thing: ConcurrencyChecker, name: str, error: Exception = GlobalLockBusyError +): + """Assert an action fails with an error and doesn't cause a change.""" + thing.changes_detected = False + action = getattr(thing, name) + with pytest.raises(error): + action() + thing.tick() + assert thing.changes_detected is False + + def lock_is_available(lock: GlobalLock) -> bool: """Check whether a lock is locked. @@ -219,19 +270,11 @@ def test_global_lock_with_thing(): # When we are using the non-blocking checker, all the properties should work. for name in ["prop1", "prop2", "fprop1", "fprop2"]: - thing.changes_detected = False - val = getattr(thing, name) - setattr(thing, name, val + 1) - thing.tick() - assert thing.changes_detected is True + assert_can_change_property(thing, name) # Increment actions should work too. for name in ["increment_fprop2", "increment_fprop2_unlocked", "increment_prop1"]: - thing.changes_detected = False - action = getattr(thing, name) - action() - thing.tick() - assert thing.changes_detected is True + assert_action_makes_change(thing, name) assert monitor_thread.is_alive() thing.tick() @@ -243,35 +286,18 @@ def test_global_lock_with_thing(): # When we are holding the lock, by default properties can't be written. for name in ["prop1", "fprop1"]: - thing.changes_detected = False - val = getattr(thing, name) # read should always succeed - with pytest.raises(GlobalLockBusyError): - setattr(thing, name, val + 1) - thing.tick() - assert thing.changes_detected is False + assert_cannot_change_property(thing, name, GlobalLockBusyError) # The properties excluded from the lock may still be written for name in ["prop2", "fprop2"]: - thing.changes_detected = False - val = getattr(thing, name) - setattr(thing, name, val + 1) - thing.tick() - assert thing.changes_detected is True + assert_can_change_property(thing, name) # By default, other actions won't run for name in ["increment_fprop2", "increment_prop1"]: - thing.changes_detected = False - action = getattr(thing, name) - with pytest.raises(GlobalLockBusyError): - action() - thing.tick() - assert thing.changes_detected is False + assert_action_fails(thing, name, GlobalLockBusyError) # Actions may run if they're excluded from the lock. - thing.changes_detected = False - thing.increment_fprop2_unlocked() - thing.tick() - assert thing.changes_detected is True + assert_action_makes_change(thing, "increment_fprop2_unlocked") assert monitor_thread.is_alive() thing.tick() From 20503f36b045a752457bcebf388441d14df8824c Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 27 Apr 2026 22:02:39 +0100 Subject: [PATCH 05/17] Add tests for locking in the context of a mocked server. This now checks both directly and in the context of a `ThingServer` (simulated by a `TestClient`) that the global lock works as expected, if it is enabled. --- tests/test_global_lock.py | 260 +++++++++++++++++++++++++------------- 1 file changed, 173 insertions(+), 87 deletions(-) diff --git a/tests/test_global_lock.py b/tests/test_global_lock.py index 011806bc..c205c2c1 100644 --- a/tests/test_global_lock.py +++ b/tests/test_global_lock.py @@ -1,10 +1,13 @@ """Test code for the global lock.""" +from collections.abc import Iterator from threading import Thread, Event -from labthings_fastapi.exceptions import GlobalLockBusyError -from labthings_fastapi.testing import create_thing_without_server +from fastapi.testclient import TestClient import pytest +from contextlib import contextmanager +from labthings_fastapi.exceptions import GlobalLockBusyError, ServerActionError +from labthings_fastapi.testing import create_thing_without_server from labthings_fastapi.global_lock import GlobalLock import labthings_fastapi as lt @@ -22,6 +25,17 @@ def run(self): self._lock.release() +def lock_is_available(lock: GlobalLock) -> bool: + """Check whether a lock is locked. + + This is needed for Python < 3.14 as there's no `locked` property. + """ + checker = LockChecker(lock) + checker.start() + checker.join() + return checker.acquired + + class ConcurrencyChecker(lt.Thing): """A class to check if actions may run concurrently.""" @@ -32,6 +46,7 @@ def __init__(self, thing_server_interface: lt.ThingServerInterface): self._fprop1 = 0 self._fprop2 = 0 + @lt.action(use_global_lock=False) def tick(self): """Set the tick event and block until it's acknowledged. @@ -69,8 +84,11 @@ def fprop2(self) -> int: def _set_fprop2(self, val: int) -> None: self._fprop2 = val + keep_checking_for_changes: bool = lt.property(default=False, use_global_lock=False) + """Set this to False to stop checking for changes.""" + @lt.action - def check_for_changes_unlocked(self, ticks=2) -> None: + def check_for_changes_unlocked(self) -> None: """Check if any properties have changed. This function does not acquire the global lock. @@ -80,7 +98,7 @@ def check_for_changes_unlocked(self, ticks=2) -> None: """ names = ["prop1", "prop2", "fprop1", "fprop2"] initial_values = {n: getattr(self, n) for n in names} - for _i in range(ticks): + while self.keep_checking_for_changes: self._tick_event.wait(timeout=0.1) self._tick_event.clear() for n in names: @@ -93,88 +111,85 @@ def check_for_changes_unlocked(self, ticks=2) -> None: check_for_changes_unlocked.use_global_lock = False @lt.action - def check_for_changes_locked(self, ticks=2): - return self.check_for_changes_unlocked(ticks=ticks) + def check_for_changes_locked(self): + """This runs `check_for_changes_unlocked` but acquires the lock.""" + return self.check_for_changes_unlocked() @lt.action def increment_fprop2(self): + """Increment fprop2, subject to the global lock.""" self._fprop2 += 1 + self.logger.info(f"increment_fprop2 set _fprop2 to {self._fprop2}") @lt.action def increment_fprop2_unlocked(self): + """Increment fprop2, not subject to the global lock.""" self._fprop2 += 1 + self.logger.info(f"increment_fprop2_unlocked set _fprop2 to {self._fprop2}") increment_fprop2_unlocked.use_global_lock = False @lt.action def increment_prop1(self): - """This function is excluded from the lock - but prop1 is locked.""" + """This function is excluded from the lock - but prop1 is locked. + + This function should therefore fail if the lock is in use. + """ self.prop1 += 1 + self.logger.info(f"increment_prop1 set prop1 to {self.prop1}") increment_prop1.use_global_lock = False -def assert_can_change_property(thing: ConcurrencyChecker, name: str): - """Check whether we can change a property of a Thing. - - :param thing: The ConcurrencyChecker instance being checked. - :param name: The name of the property. - :return: `True` if the property can be changed. - """ - thing.changes_detected = False - val = getattr(thing, name) # read should always succeed - setattr(thing, name, val + 1) - thing.tick() - assert thing.changes_detected is True - - -def assert_cannot_change_property( - thing: ConcurrencyChecker, name: str, error: Exception = GlobalLockBusyError -): - """Check whether we cannot change a property of a Thing. - - :param thing: The ConcurrencyChecker instance being checked. - :param name: The name of the property. - :return: `True` if setting the property raises an error. - """ +@contextmanager +def assert_changes(thing: ConcurrencyChecker): + """Assert the code in a with block does or does not change properties.""" thing.changes_detected = False - val = getattr(thing, name) # read should always succeed - with pytest.raises(error): - setattr(thing, name, val + 1) - thing.tick() - assert thing.changes_detected is False - - -def assert_action_makes_change(thing: ConcurrencyChecker, name: str): - """Assert an action runs OK and causes properties to change.""" - thing.changes_detected = False - action = getattr(thing, name) - action() + yield thing.tick() assert thing.changes_detected is True -def assert_action_fails( - thing: ConcurrencyChecker, name: str, error: Exception = GlobalLockBusyError -): - """Assert an action fails with an error and doesn't cause a change.""" +@contextmanager +def assert_fails( + thing: ConcurrencyChecker, error: type[Exception] = GlobalLockBusyError +) -> Iterator[None]: + """Assert that the code in a with block doesn't change properties and errors.""" thing.changes_detected = False - action = getattr(thing, name) with pytest.raises(error): - action() + yield thing.tick() assert thing.changes_detected is False -def lock_is_available(lock: GlobalLock) -> bool: - """Check whether a lock is locked. - - This is needed for Python < 3.14 as there's no `locked` property. - """ - checker = LockChecker(lock) - checker.start() - checker.join() - return checker.acquired +@contextmanager +def monitor_for_changes(thing: ConcurrencyChecker, hold_lock: bool) -> Iterator[None]: + """Monitor for changes in a background thread""" + # Start the background action that checks for changes. + monitor_thread = Thread( + target=( + thing.check_for_changes_locked + if hold_lock + else thing.check_for_changes_unlocked + ), + ) + thing.keep_checking_for_changes = True + monitor_thread.start() + try: + yield + + assert monitor_thread.is_alive() + except Exception: + # If an exception occurs, send ticks so the background process terminates + print( + "monitor_for_changes caught an exception. " + f"Background thread is {'alive' if monitor_thread.is_alive() else 'dead'}." + ) + raise + finally: + thing.keep_checking_for_changes = False + thing.tick() + monitor_thread.join() def test_global_lock_unthreaded(): @@ -258,47 +273,118 @@ def hold_lock_in_background(): t.join() -def test_global_lock_with_thing(): - """Ensure the global lock stops multiple things happening at once.""" - thing = create_thing_without_server(ConcurrencyChecker, enable_global_lock=True) - - # Start the background action that checks for changes. - monitor_thread = Thread( - target=thing.check_for_changes_unlocked, kwargs={"ticks": 8} - ) - monitor_thread.start() +def assertions_without_locking(thing: ConcurrencyChecker): + """Test that all the actions and properties produce a change. + Note that this requires `check_for_changes` or `check_for_changes_unlocked` + to be running in a background thread. + """ # When we are using the non-blocking checker, all the properties should work. - for name in ["prop1", "prop2", "fprop1", "fprop2"]: - assert_can_change_property(thing, name) + with assert_changes(thing): + thing.prop1 += 1 + with assert_changes(thing): + thing.prop2 += 1 + with assert_changes(thing): + thing.fprop1 += 1 + with assert_changes(thing): + thing.fprop2 += 1 # Increment actions should work too. - for name in ["increment_fprop2", "increment_fprop2_unlocked", "increment_prop1"]: - assert_action_makes_change(thing, name) + with assert_changes(thing): + thing.increment_fprop2() + with assert_changes(thing): + thing.increment_prop1() + with assert_changes(thing): + thing.increment_fprop2_unlocked() - assert monitor_thread.is_alive() - thing.tick() - monitor_thread.join() - # Start the background action that checks for changes, and holds the lock - monitor_thread = Thread(target=thing.check_for_changes_locked, kwargs={"ticks": 8}) - monitor_thread.start() +def assertions_with_locking( + thing: ConcurrencyChecker, action_error: type[Exception] = GlobalLockBusyError +): + """Test that only the unlocked actions and properties produce a change. + + Note that this requires `check_for_changes_locked` to be running in a background + thread. See `assertions_without_locking` for a version that should work with locking + disabled. + """ + # Properties may always be read + assert thing.prop1 == 0 + assert thing.prop2 == 0 + assert thing.fprop1 == 0 + assert thing.fprop2 == 0 # When we are holding the lock, by default properties can't be written. - for name in ["prop1", "fprop1"]: - assert_cannot_change_property(thing, name, GlobalLockBusyError) + with assert_fails(thing): + thing.prop1 += 1 + with assert_fails(thing): + thing.fprop1 += 1 # The properties excluded from the lock may still be written - for name in ["prop2", "fprop2"]: - assert_can_change_property(thing, name) + with assert_changes(thing): + thing.prop2 += 1 + with assert_changes(thing): + thing.fprop2 += 1 - # By default, other actions won't run - for name in ["increment_fprop2", "increment_prop1"]: - assert_action_fails(thing, name, GlobalLockBusyError) + # By default actions won't run + with assert_fails(thing, error=action_error): + thing.increment_fprop2() # Actions may run if they're excluded from the lock. - assert_action_makes_change(thing, "increment_fprop2_unlocked") + with assert_changes(thing): + thing.increment_fprop2_unlocked() - assert monitor_thread.is_alive() - thing.tick() - monitor_thread.join() + # Actions that use locked resources (like prop1) should also fail + with assert_fails(thing, error=action_error): + thing.increment_prop1() + + +def test_actions_and_properties_direct_lock_enabled(): + """Ensure the global lock stops multiple things happening at once. + + This test uses a Thing instance directly, with locking enabled. + """ + thing = create_thing_without_server(ConcurrencyChecker, enable_global_lock=True) + with monitor_for_changes(thing, hold_lock=True): + assertions_with_locking(thing) + with monitor_for_changes(thing, hold_lock=False): + assertions_without_locking(thing) + + +def test_actions_and_properties_direct_lock_disabled(): + """Ensure the global lock stops multiple things happening at once. + + This test uses a Thing instance directly, with locking disabled. + """ + thing = create_thing_without_server(ConcurrencyChecker, enable_global_lock=False) + with monitor_for_changes(thing, hold_lock=True): + assertions_without_locking(thing) + with monitor_for_changes(thing, hold_lock=False): + assertions_without_locking(thing) + + +def test_actions_and_properties_testclient_lock_enabled(): + """Ensure the global lock stops multiple things happening at once. + + This test uses a Thing instance directly, with locking enabled. + """ + server = lt.ThingServer({"checker": ConcurrencyChecker}, enable_global_lock=True) + with TestClient(server.app) as client: + thing = lt.ThingClient.from_url("/checker/", client=client) + with monitor_for_changes(thing, hold_lock=True): + assertions_with_locking(thing, action_error=ServerActionError) + with monitor_for_changes(thing, hold_lock=False): + assertions_without_locking(thing) + + +def test_actions_and_properties_testclient_lock_disabled(): + """Ensure the global lock stops multiple things happening at once. + + This test uses a Thing instance directly, with locking enabled. + """ + server = lt.ThingServer({"checker": ConcurrencyChecker}, enable_global_lock=False) + with TestClient(server.app) as client: + thing = lt.ThingClient.from_url("/checker/", client=client) + with monitor_for_changes(thing, hold_lock=True): + assertions_without_locking(thing) + with monitor_for_changes(thing, hold_lock=False): + assertions_without_locking(thing) From 04491cea1ce9b977f74e56cfe1349509b2e725ed Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 28 Apr 2026 09:15:08 +0100 Subject: [PATCH 06/17] Improve error handling for properties. I've now installed a FastAPI error handler that will provide a more graceful error when the global lock is not available. Errors are handled for actions by the normal mechanism, but for property code any exceptions previously resulted in a 500 Internal Server Error with no details. I now explicitly catch `GlobalLockBusyError` and provide a 409 response with a message. --- src/labthings_fastapi/client/__init__.py | 2 +- src/labthings_fastapi/server/__init__.py | 16 ++++++++++++++++ tests/test_global_lock.py | 22 +++++++++++----------- 3 files changed, 28 insertions(+), 12 deletions(-) diff --git a/src/labthings_fastapi/client/__init__.py b/src/labthings_fastapi/client/__init__.py index bc0ed6d4..a2ddc247 100644 --- a/src/labthings_fastapi/client/__init__.py +++ b/src/labthings_fastapi/client/__init__.py @@ -171,7 +171,7 @@ def set_property(self, path: str, value: Any) -> None: """ response = self.client.put(urljoin(self.path, path), json=value) if response.is_error: - detail = response.json().get("detail") + detail = response.json().get("detail", None) err_msg = "Unknown error" if isinstance(detail, str): err_msg = detail diff --git a/src/labthings_fastapi/server/__init__.py b/src/labthings_fastapi/server/__init__.py index 265b0e43..4b46e4d8 100644 --- a/src/labthings_fastapi/server/__init__.py +++ b/src/labthings_fastapi/server/__init__.py @@ -10,6 +10,7 @@ from fastapi.testclient import TestClient from pydantic import ValidationError from typing import Any, AsyncGenerator, Optional, TypeVar, overload +from fastapi.responses import JSONResponse from typing_extensions import Self import os import logging @@ -22,6 +23,8 @@ from types import MappingProxyType import uvicorn +from labthings_fastapi.exceptions import GlobalLockBusyError + from ..middleware.url_for import url_for_middleware from ..thing_slots import ThingSlot from ..utilities import class_attributes @@ -141,6 +144,7 @@ def __init__( self.app = FastAPI(lifespan=self.lifespan) self._set_cors_middleware() self._set_url_for_middleware() + self._add_exception_handlers() self.action_manager = ActionManager() self.app.include_router(self.action_manager.router(), prefix=self._api_prefix) self.app.include_router(blob.router, prefix=self._api_prefix) @@ -232,6 +236,18 @@ def _set_url_for_middleware(self) -> None: """ self.app.middleware("http")(url_for_middleware) + def _add_exception_handlers(self) -> None: + """Add exception handlers to the FastAPI application.""" + + @self.app.exception_handler(GlobalLockBusyError) + async def global_lock_exception_handler( + _request: Request, exc: GlobalLockBusyError + ) -> JSONResponse: + return JSONResponse( + status_code=409, + content={"detail": repr(exc)}, + ) + @property def debug(self) -> bool: """Whether the server is in debug mode.""" diff --git a/tests/test_global_lock.py b/tests/test_global_lock.py index c205c2c1..9a9d9a1d 100644 --- a/tests/test_global_lock.py +++ b/tests/test_global_lock.py @@ -6,7 +6,11 @@ import pytest from contextlib import contextmanager -from labthings_fastapi.exceptions import GlobalLockBusyError, ServerActionError +from labthings_fastapi.exceptions import ( + ClientPropertyError, + GlobalLockBusyError, + ServerActionError, +) from labthings_fastapi.testing import create_thing_without_server from labthings_fastapi.global_lock import GlobalLock import labthings_fastapi as lt @@ -151,12 +155,10 @@ def assert_changes(thing: ConcurrencyChecker): @contextmanager -def assert_fails( - thing: ConcurrencyChecker, error: type[Exception] = GlobalLockBusyError -) -> Iterator[None]: +def assert_fails(thing: ConcurrencyChecker) -> Iterator[None]: """Assert that the code in a with block doesn't change properties and errors.""" thing.changes_detected = False - with pytest.raises(error): + with pytest.raises((GlobalLockBusyError, ServerActionError, ClientPropertyError)): yield thing.tick() assert thing.changes_detected is False @@ -298,9 +300,7 @@ def assertions_without_locking(thing: ConcurrencyChecker): thing.increment_fprop2_unlocked() -def assertions_with_locking( - thing: ConcurrencyChecker, action_error: type[Exception] = GlobalLockBusyError -): +def assertions_with_locking(thing: ConcurrencyChecker): """Test that only the unlocked actions and properties produce a change. Note that this requires `check_for_changes_locked` to be running in a background @@ -326,7 +326,7 @@ def assertions_with_locking( thing.fprop2 += 1 # By default actions won't run - with assert_fails(thing, error=action_error): + with assert_fails(thing): thing.increment_fprop2() # Actions may run if they're excluded from the lock. @@ -334,7 +334,7 @@ def assertions_with_locking( thing.increment_fprop2_unlocked() # Actions that use locked resources (like prop1) should also fail - with assert_fails(thing, error=action_error): + with assert_fails(thing): thing.increment_prop1() @@ -371,7 +371,7 @@ def test_actions_and_properties_testclient_lock_enabled(): with TestClient(server.app) as client: thing = lt.ThingClient.from_url("/checker/", client=client) with monitor_for_changes(thing, hold_lock=True): - assertions_with_locking(thing, action_error=ServerActionError) + assertions_with_locking(thing) with monitor_for_changes(thing, hold_lock=False): assertions_without_locking(thing) From 1ae3110aeb7a11d5db585e2870ea620095c9120a Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 28 Apr 2026 09:59:59 +0100 Subject: [PATCH 07/17] Cope with bound actions being called more than once I thought it was elegant to call the lock context manager and bind just the generator object it returns to the wrapped function. Unfortunately, said generator object may not be re-used. I have now added a test (that started off failing) to reproduce that problem, and have changed the offending code to use a fresh generator each time. This was caught by the OFM test suite :) --- src/labthings_fastapi/actions.py | 19 +++++---- tests/test_global_lock.py | 72 ++++++++++++++++++++++++++++---- 2 files changed, 74 insertions(+), 17 deletions(-) diff --git a/src/labthings_fastapi/actions.py b/src/labthings_fastapi/actions.py index 9dd97ea8..08ed1972 100644 --- a/src/labthings_fastapi/actions.py +++ b/src/labthings_fastapi/actions.py @@ -748,21 +748,22 @@ def wrapped_func( If global locking is enabled and this action hasn't opted out, this function will wrap `func` such that it holds the global lock while it is running. + .. note:: + + The returned function will hold a reference to both `obj` and `self` + (this descriptor). Given that accessing ``instance.method`` returns + a function that's already bound to the instance, this shouldn't cause + any problems. + :param obj: The object on which the method is being called. :return: the function, wrapped if necessary. """ - # hold_global_lock returns a context manager. It won't hold the lock - # until we enter the context in `wrapped` (defined below). - lock_context_manager = obj._thing_server_interface.hold_global_lock( - self.use_global_lock - ) - func = self.func - @wraps(func) + @wraps(self.func) def wrapped(*args: Any, **kwargs: Any) -> Any: # noqa: DOC """Acquire the lock then run `func` with supplied arguments.""" - with lock_context_manager: - return func(*args, **kwargs) + with obj._thing_server_interface.hold_global_lock(self.use_global_lock): + return self.func(*args, **kwargs) return wrapped diff --git a/tests/test_global_lock.py b/tests/test_global_lock.py index 9a9d9a1d..fbb24128 100644 --- a/tests/test_global_lock.py +++ b/tests/test_global_lock.py @@ -41,7 +41,10 @@ def lock_is_available(lock: GlobalLock) -> bool: class ConcurrencyChecker(lt.Thing): - """A class to check if actions may run concurrently.""" + """A class to check if actions may run concurrently. + + See `check_for_changes_unlocked` for some important concurrency notes. + """ def __init__(self, thing_server_interface: lt.ThingServerInterface): super().__init__(thing_server_interface) @@ -54,7 +57,10 @@ def __init__(self, thing_server_interface: lt.ThingServerInterface): def tick(self): """Set the tick event and block until it's acknowledged. - This avoids race conditions in the test code. + This avoids race conditions in the test code, by ensuring + the checks performed by `check_for_changes_unlocked` happen at + well-defined points in the foreground thread. See that method + for more details. """ self._tick_event.set() self._tock_event.wait(0.1) @@ -93,12 +99,28 @@ def _set_fprop2(self, val: int) -> None: @lt.action def check_for_changes_unlocked(self) -> None: - """Check if any properties have changed. + r"""Check if any properties have changed. This function does not acquire the global lock. - :param ticks: the number of times to wait for the tick event. - :return: whether any changes were detected. + In order to minimise dead time and remove the need for lots of `time.sleep` + calls, this method is synchronised by `_tick_event` and `_tock_event` and + terminated with `keep_checking_for_changes`\ . + + Code using this method should run it in a background thread or action + (most likely using the `monitor_for_changes` context manager) and then + set `changes_detected` to `False` then + call the `tick()` action whenever a check is required. Once the `tick()` + action has completed, `changes_detected` will be set to the right value + and the property values will be reset. + + The routine above is done automatically by `assert_changes` or `assert fails` + when run as context managers. + + At the end of the test (or when `monitor_for_changes` exits), you should + set `keep_checking_for_changes` to `False` and call `tick()` one last time + before `join()`\ ing the thread. Doing this using the context manager + should ensure your test code does not hang when it fails. """ names = ["prop1", "prop2", "fprop1", "fprop2"] initial_values = {n: getattr(self, n) for n in names} @@ -147,7 +169,10 @@ def increment_prop1(self): @contextmanager def assert_changes(thing: ConcurrencyChecker): - """Assert the code in a with block does or does not change properties.""" + """Assert the code in a with block does or does not change properties. + + See `ConcurrencyChecker.check_for_changes_unlocked` for notes on synchronisation. + """ thing.changes_detected = False yield thing.tick() @@ -156,7 +181,13 @@ def assert_changes(thing: ConcurrencyChecker): @contextmanager def assert_fails(thing: ConcurrencyChecker) -> Iterator[None]: - """Assert that the code in a with block doesn't change properties and errors.""" + """Assert that the code in a with block fails with an error. + + Currently, this will look for several exceptions, so that it works on both client + and server-side. + + See `ConcurrencyChecker.check_for_changes_unlocked` for notes on synchronisation. + """ thing.changes_detected = False with pytest.raises((GlobalLockBusyError, ServerActionError, ClientPropertyError)): yield @@ -292,6 +323,13 @@ def assertions_without_locking(thing: ConcurrencyChecker): thing.fprop2 += 1 # Increment actions should work too. + # Each action is called twice to check for reuse of context managers. + with assert_changes(thing): + thing.increment_fprop2() + with assert_changes(thing): + thing.increment_prop1() + with assert_changes(thing): + thing.increment_fprop2_unlocked() with assert_changes(thing): thing.increment_fprop2() with assert_changes(thing): @@ -306,6 +344,9 @@ def assertions_with_locking(thing: ConcurrencyChecker): Note that this requires `check_for_changes_locked` to be running in a background thread. See `assertions_without_locking` for a version that should work with locking disabled. + + This should run if either a `ConcurrencyChecker` or a `ThingClient` connected to + one is supplied. """ # Properties may always be read assert thing.prop1 == 0 @@ -330,6 +371,10 @@ def assertions_with_locking(thing: ConcurrencyChecker): thing.increment_fprop2() # Actions may run if they're excluded from the lock. + # Note this is done twice to check for reuse of context managers + # (which will fail on the second attempt) + with assert_changes(thing): + thing.increment_fprop2_unlocked() with assert_changes(thing): thing.increment_fprop2_unlocked() @@ -379,7 +424,7 @@ def test_actions_and_properties_testclient_lock_enabled(): def test_actions_and_properties_testclient_lock_disabled(): """Ensure the global lock stops multiple things happening at once. - This test uses a Thing instance directly, with locking enabled. + This test uses a Thing instance directly, with locking disabled. """ server = lt.ThingServer({"checker": ConcurrencyChecker}, enable_global_lock=False) with TestClient(server.app) as client: @@ -388,3 +433,14 @@ def test_actions_and_properties_testclient_lock_disabled(): assertions_without_locking(thing) with monitor_for_changes(thing, hold_lock=False): assertions_without_locking(thing) + + +def test_reuse_of_action_callables(): + """Test that it's OK to get a bound action and call it multiple times.""" + thing = create_thing_without_server(ConcurrencyChecker, enable_global_lock=True) + with monitor_for_changes(thing, hold_lock=False): + func = thing.increment_fprop2 + with assert_changes(thing): + func() + with assert_changes(thing): + func() From e745ef74b43114627d4ba91cf3c1b591eb9fb42a Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 28 Apr 2026 11:42:05 +0100 Subject: [PATCH 08/17] Add documentation on locking. --- docs/source/concurrency.rst | 15 +++++++++++++++ docs/source/public_api.rst | 13 ++++++++----- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/docs/source/concurrency.rst b/docs/source/concurrency.rst index 3d92265f..a7f43d6b 100644 --- a/docs/source/concurrency.rst +++ b/docs/source/concurrency.rst @@ -3,6 +3,10 @@ Concurrency in LabThings-FastAPI ================================== +.. note:: + + This page attempts to describe several aspects of concurrency in LabThings. If you just want an answer to the question "how do I make sure only one thing happens at a time", skip to :ref:`global_locking`\ . + One of the major challenges when controlling hardware, particularly from web frameworks, is concurrency. Most web frameworks assume resources (database connections, object storage, etc.) may be instantiated multiple times, and often initialise or destroy objects as required. In contrast, hardware can usually only be controlled from one process, and usually is initialised and shut down only once. LabThings-FastAPI instantiates each :class:`~lt.Thing` only once, and runs all code in a thread. More specifically, each time an action is invoked via HTTP, a new thread is created to run the action. Similarly, each time a property is read or written, a new thread is created to run the property method. This means that :class:`~lt.Thing` code should protect important variables or resources using locks from the `threading` module, and need not worry about writing asynchronous code. @@ -30,3 +34,14 @@ Each time an action is run ("invoked" in :ref:`wot_cc`), we create a new thread Usually, the best solution to this problem is to generate a new invocation ID for the thread. This means only the original action thread will receive cancellation events, and only the original action thread will log to the invocation logger. If the action is cancelled, you must cancel the background thread. This is the behaviour of `~lt.ThreadWithInvocationID`\ . It is also possible to copy the current invocation ID to a new thread. This is often a bad idea, as it's ill-defined whether the exception will arise in the original thread or the new one if the invocation is cancelled. Logs from the two threads will also be interleaved. If it's desirable to log from the background thread, the invocation logger may safely be passed as an argument, rather than accessed via ``lt.get_invocation_logger``\ . + +.. _global_locking: + +Global locking +-------------- + +It is possible to add a global lock object to the `~lt.ThingServer` by specifying `enable_global_lock=True` either as an argument or in the configuration file. When this is enabled, only one action may run at a given time. Setting properties also requires the lock, so you may assume that property values will not change while your action is running (unless you set them from the action). + +The `GlobalLock` is a work-a-like wrapper for `threading.RLock`\ . This means it can be acquired multiple times by the same thread - so actions can call other actions and set properties without worrying about locking, and everything is protected such that only one thread may make changes at a time. + +It is possible for individual actions or properties to opt out of the global lock, by specifying `use_global_lock=False` either as an argument to `~lt.property` or `~lt.action` or by setting the `use_global_lock` attribute on a functional property (see :ref:`properties`). Note that actions or setters that are exempted from the lock may not call other actions or properties that are locked: this will usually time out with a `GlobalLockBusyError`\ . diff --git a/docs/source/public_api.rst b/docs/source/public_api.rst index bc055789..7a235775 100644 --- a/docs/source/public_api.rst +++ b/docs/source/public_api.rst @@ -91,8 +91,8 @@ This page summarises the parts of the LabThings API that should be most frequent :no-index: .. py:function:: property(getter: Callable[[Owner], Value]) -> FunctionalProperty[Owner, Value] - property(*, default: Value, readonly: bool = False, **constraints: Any) -> Value - property(*, default_factory: Callable[[], Value], readonly: bool = False, **constraints: Any) -> Value + property(*, default: Value, readonly: bool = False, use_global_lock: bool | None = None, **constraints: Any) -> Value + property(*, default_factory: Callable[[], Value], readonly: bool = False, use_global_lock: bool | None = None, **constraints: Any) -> Value This function may be used to define :ref:`properties` either by decorating a function, or marking an attribute. Full documentation is available at `labthings_fastapi.properties.property` and a more in-depth discussion is available at :ref:`properties`\ . This page focuses on the most frequently used examples. @@ -143,14 +143,14 @@ This page summarises the parts of the LabThings API that should be most frequent .. py:function:: setting(getter: Callable[[Owner], Value]) -> FunctionalSetting[Owner, Value] - setting(*, default: Value, readonly: bool = False, **constraints: Any) -> Value - setting(*, default_factory: Callable[[], Value], readonly: bool = False, **constraints: Any) -> Value + setting(*, default: Value, readonly: bool = False, use_global_lock: bool | None = None, **constraints: Any) -> Value + setting(*, default_factory: Callable[[], Value], readonly: bool = False, use_global_lock: bool | None = None, **constraints: Any) -> Value A setting is a property that is saved to disk. It is defined in the same way as `property` but will be synchronised with the `Thing`\ 's settings file. Full documentation is available at `labthings_fastapi.properties.setting` .. py:decorator:: action - action(**kwargs: Any) + action(use_global_lock: bool | None = None, **kwargs: Any) Mark a method of a `~lt.Thing` as a LabThings Action. @@ -365,6 +365,9 @@ This page summarises the parts of the LabThings API that should be most frequent .. autoattribute:: labthings_fastapi.server.config_model.ThingServerConfig.settings_folder :no-index: + .. autoattribute:: labthings_fastapi.server.config_model.ThingServerConfig.enable_global_lock + :no-index: + .. autoattribute:: labthings_fastapi.server.config_model.ThingServerConfig.application_config :no-index: From cbb7c3d43fa3503af82621dd5cb9da2145a9db6f Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 5 May 2026 15:59:58 +0100 Subject: [PATCH 09/17] Update test syntax after rebasing. Some newly-added tests in this branch needed to be updated after rebasing. This was because they passed a dictionary of Things to `ThingServer`'s constructor. The tests now correctly use `from_things` and are otherwise unchanged. --- tests/test_global_lock.py | 8 ++++++-- tests/test_thing_server_interface.py | 7 +++---- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/tests/test_global_lock.py b/tests/test_global_lock.py index fbb24128..7fc21b8f 100644 --- a/tests/test_global_lock.py +++ b/tests/test_global_lock.py @@ -412,7 +412,9 @@ def test_actions_and_properties_testclient_lock_enabled(): This test uses a Thing instance directly, with locking enabled. """ - server = lt.ThingServer({"checker": ConcurrencyChecker}, enable_global_lock=True) + server = lt.ThingServer.from_things( + {"checker": ConcurrencyChecker}, enable_global_lock=True + ) with TestClient(server.app) as client: thing = lt.ThingClient.from_url("/checker/", client=client) with monitor_for_changes(thing, hold_lock=True): @@ -426,7 +428,9 @@ def test_actions_and_properties_testclient_lock_disabled(): This test uses a Thing instance directly, with locking disabled. """ - server = lt.ThingServer({"checker": ConcurrencyChecker}, enable_global_lock=False) + server = lt.ThingServer.from_things( + {"checker": ConcurrencyChecker}, enable_global_lock=False + ) with TestClient(server.app) as client: thing = lt.ThingClient.from_url("/checker/", client=client) with monitor_for_changes(thing, hold_lock=True): diff --git a/tests/test_thing_server_interface.py b/tests/test_thing_server_interface.py index 9cba438c..e4a85f68 100644 --- a/tests/test_thing_server_interface.py +++ b/tests/test_thing_server_interface.py @@ -6,7 +6,6 @@ from typing import Mapping from unittest.mock import Mock -from fastapi.testclient import TestClient from labthings_fastapi.global_lock import GlobalLock import pytest @@ -323,7 +322,7 @@ def test_mocking_slots(): @pytest.mark.parametrize("enable", (False, True)) def test_global_lock(enable): """Test that the global lock is accessible, if configured.""" - server = lt.ThingServer({}, enable_global_lock=enable) + server = lt.ThingServer.from_things({}, enable_global_lock=enable) interface = lt.ThingServerInterface(server, "thing_name") if enable: assert isinstance(interface.global_lock, GlobalLock) @@ -338,7 +337,7 @@ def test_mock_hold_global_lock(mock): if mock: interface = MockThingServerInterface("thing_name") else: - server = lt.ThingServer({}) + server = lt.ThingServer.from_things({}) interface = lt.ThingServerInterface(server, "thing_name") assert interface.global_lock is None # With no global lock, the context manager should be a no-op, unless we @@ -355,7 +354,7 @@ def test_mock_hold_global_lock(mock): if mock: interface = MockThingServerInterface("thing_name", enable_global_lock=True) else: - server = lt.ThingServer({}, enable_global_lock=True) + server = lt.ThingServer.from_things({}, enable_global_lock=True) interface = ThingServerInterface(server, "thing_name") assert isinstance(interface.global_lock, GlobalLock) # That means the context manager should work for all three arguments. From 790bf035abbe18627c2d15e6acabdf625cdb6af3 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 5 May 2026 16:03:11 +0100 Subject: [PATCH 10/17] Make it clear how to access the global lock. I've added text to the ActionDescriptor docstring directing the user to the global lock, in case they need to access it manually. This is in response to review comment from @julianstirling. --- src/labthings_fastapi/actions.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/labthings_fastapi/actions.py b/src/labthings_fastapi/actions.py index 08ed1972..4a40d103 100644 --- a/src/labthings_fastapi/actions.py +++ b/src/labthings_fastapi/actions.py @@ -669,7 +669,7 @@ def __init__( retention_time: float = 300, use_global_lock: Literal[False] | None = None, ) -> None: - """Create a new action descriptor. + r"""Create a new action descriptor. The action descriptor wraps a method of a `~lt.Thing`. It may still be called from Python in the same way, but it will also be added to the @@ -686,17 +686,16 @@ def __init__( of the action. :param retention_time: how long, in seconds, the action should be kept for after it has completed. - :param use_global_lock: If the global lock is enabled in `lt.FEATURE_FLAGS` - this parameter may be used to opt out. When the global lock is enabled, - by default all actions acquire the global lock before starting, and - release it after they finish. That means only one action thread may - run at a time. The same lock is used to set properties. + :param use_global_lock: If the global lock is enabled, + this parameter may be used to opt out. See :ref:`global_locking` + for details of how the global lock is implemented. If this parameter is `False` then the lock will not be acquired, even if global locking is enabled. That is appropriate if the action does not have side effects that would cause problems for other actions, or if more nuanced locking behaviour is required meaning the lock is - acquired directly in the action code. + acquired directly in the action code, for example using + `~lt.ThingServerInterface.hold_global_lock`\ . """ super().__init__() self.func = func From b203b0fe198cdb196507acb6689ec5296f725f21 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 5 May 2026 21:51:55 +0100 Subject: [PATCH 11/17] Neater error handling for errors that can't start. This splits out the context manager that holds the lock from the action function, when the action is called over HTTP. That means we will only log a single line if an action fails to start because of the global lock. I have deliberately left other global lock errors (e.g. for when an unlocked function calls locked code) to be handled as usual (i.e. log a traceback) because in my view that is an error, and should be handled by the action in the usual way. Re-raising the error as an InvocationError is appropriate, if terminating because the lock is busy is something we expect to happen. --- src/labthings_fastapi/actions.py | 97 ++++++++++++++++++++------------ tests/test_global_lock.py | 40 ++++++++++++- 2 files changed, 97 insertions(+), 40 deletions(-) diff --git a/src/labthings_fastapi/actions.py b/src/labthings_fastapi/actions.py index 4a40d103..00b3b1fb 100644 --- a/src/labthings_fastapi/actions.py +++ b/src/labthings_fastapi/actions.py @@ -15,6 +15,8 @@ """ from __future__ import annotations +from collections.abc import Iterator +from contextlib import contextmanager import datetime import logging from collections import deque @@ -51,6 +53,7 @@ from .utilities import model_to_dict, wrap_plain_types_in_rootmodel from .invocations import InvocationModel, InvocationStatus from .exceptions import ( + GlobalLockBusyError, InvocationCancelledError, InvocationError, NotConnectedToServerError, @@ -289,19 +292,22 @@ def run(self) -> None: # occur. raise RuntimeError("Cannot start an invocation without a Thing.") - with self._status_lock: - self._status = InvocationStatus.RUNNING - self._start_time = datetime.datetime.now() - action.emit_changed_event(self.thing, self._status.value) + # The action's `context_for_func` context manager will acquire the + # global lock if needed. + with action.context_for_func(thing): + with self._status_lock: + self._status = InvocationStatus.RUNNING + self._start_time = datetime.datetime.now() + action.emit_changed_event(self.thing, self._status.value) - bound_method = action.__get__(thing) - # Actually run the action - ret = bound_method(**kwargs, **self.dependencies) + # Actually run the action + ret = action.func(thing, **kwargs, **self.dependencies) + + with self._status_lock: + self._return_value = ret + self._status = InvocationStatus.COMPLETED + action.emit_changed_event(self.thing, self._status.value) - with self._status_lock: - self._return_value = ret - self._status = InvocationStatus.COMPLETED - action.emit_changed_event(self.thing, self._status.value) except InvocationCancelledError: logger.info(f"Invocation {self.id} was cancelled.") with self._status_lock: @@ -310,9 +316,17 @@ def run(self) -> None: except Exception as e: # skipcq: PYL-W0703 # First log if isinstance(e, InvocationError): - # Log without traceback + # Log without traceback for anticipated errors logger.error(e) + elif ( + isinstance(e, GlobalLockBusyError) + and self._status == InvocationStatus.PENDING + ): + # The global lock timed out before the function started. + # In this case, don't print a traceback. + logger.warning(f"Global lock was busy: didn't run {action.name}.") else: + # Other exceptions show up in the log with a traceback logger.exception(e) # Then set status with self._status_lock: @@ -739,49 +753,58 @@ def __set_name__(self, owner: type[OwnerT], name: str) -> None: f"'{self.func.__name__}'", ) - def wrapped_func( - self, obj: OwnerT - ) -> Callable[Concatenate[OwnerT, ActionParams], ActionReturn]: - """Wrap the action function if necessary, so that it holds the global lock. + @contextmanager + def context_for_func(self, obj: OwnerT) -> Iterator[None]: + """Create context in which ``func`` runs. - If global locking is enabled and this action hasn't opted out, this function - will wrap `func` such that it holds the global lock while it is running. - - .. note:: + This method is intended to create a hook for pre-run set-up and post-run + clean-up code. It should not perform slow or intensive tasks, and is mostly + intended as a good place to acquire and release locks and so on. - The returned function will hold a reference to both `obj` and `self` - (this descriptor). Given that accessing ``instance.method`` returns - a function that's already bound to the instance, this shouldn't cause - any problems. + Currently, if global locking is enabled and this action hasn't opted out, + this context manager will hold the global lock for the duration of the + action. :param obj: The object on which the method is being called. :return: the function, wrapped if necessary. """ - - @wraps(self.func) - def wrapped(*args: Any, **kwargs: Any) -> Any: # noqa: DOC - """Acquire the lock then run `func` with supplied arguments.""" - with obj._thing_server_interface.hold_global_lock(self.use_global_lock): - return self.func(*args, **kwargs) - - return wrapped + with obj._thing_server_interface.hold_global_lock(self.use_global_lock): + yield def instance_get(self, obj: OwnerT) -> Callable[ActionParams, ActionReturn]: - """Return the function, bound to an object as for a normal method. + """Return the function, bound to an object and wrapped in a context manager. - This currently doesn't validate the arguments, though it may do so - in future. If locking is disabled this is equivalent to a regular - Python method, i.e. all we do is supply the first argument, `self`. + Accessing a regular Python method returns the method bound to the instance, + i.e. the `self` argument is supplied. + LabThings Actions work the same way, but they also wrap the function in a + context manager. Currently, this context manager will handle acquiring the + global lock if required. + + If locking is disabled, the context manager does nothing. If locking is enabled, we return a wrapped function that holds the global lock while the action runs. + .. note:: + + The returned function will hold a reference to both `obj` and `self` + (this descriptor). Given that accessing ``instance.method`` returns + a function that's already bound to the instance, this shouldn't cause + any problems. + :param obj: the `~lt.Thing` to which we are attached. This will be the first argument supplied to the function wrapped by this descriptor. :return: the action function, bound to ``obj``. """ - return partial(self.wrapped_func(obj), obj) + + @wraps(self.func) + def wrapped(*args: Any, **kwargs: Any) -> Any: # noqa: DOC + """Acquire the lock then run `func` with supplied arguments.""" + with self.context_for_func(obj): + return self.func(*args, **kwargs) + + return partial(wrapped, obj) def _observers_set(self, obj: Thing) -> WeakSet: """Return a set used to notify changes. diff --git a/tests/test_global_lock.py b/tests/test_global_lock.py index 7fc21b8f..8ba0cf02 100644 --- a/tests/test_global_lock.py +++ b/tests/test_global_lock.py @@ -1,8 +1,8 @@ """Test code for the global lock.""" from collections.abc import Iterator +import logging from threading import Thread, Event -from fastapi.testclient import TestClient import pytest from contextlib import contextmanager @@ -124,6 +124,7 @@ def check_for_changes_unlocked(self) -> None: """ names = ["prop1", "prop2", "fprop1", "fprop2"] initial_values = {n: getattr(self, n) for n in names} + self.logger.info("Checking for changes") while self.keep_checking_for_changes: self._tick_event.wait(timeout=0.1) self._tick_event.clear() @@ -133,12 +134,14 @@ def check_for_changes_unlocked(self) -> None: self.changes_detected = True setattr(self, n, initial_values[n]) self._tock_event.set() + self.logger.info("Finished checking for changes.") check_for_changes_unlocked.use_global_lock = False @lt.action def check_for_changes_locked(self): """This runs `check_for_changes_unlocked` but acquires the lock.""" + self.logger.info("Checking for changes and holding lock.") return self.check_for_changes_unlocked() @lt.action @@ -209,6 +212,7 @@ def monitor_for_changes(thing: ConcurrencyChecker, hold_lock: bool) -> Iterator[ thing.keep_checking_for_changes = True monitor_thread.start() try: + thing.tick() yield assert monitor_thread.is_alive() @@ -415,7 +419,7 @@ def test_actions_and_properties_testclient_lock_enabled(): server = lt.ThingServer.from_things( {"checker": ConcurrencyChecker}, enable_global_lock=True ) - with TestClient(server.app) as client: + with server.test_client() as client: thing = lt.ThingClient.from_url("/checker/", client=client) with monitor_for_changes(thing, hold_lock=True): assertions_with_locking(thing) @@ -431,7 +435,7 @@ def test_actions_and_properties_testclient_lock_disabled(): server = lt.ThingServer.from_things( {"checker": ConcurrencyChecker}, enable_global_lock=False ) - with TestClient(server.app) as client: + with server.test_client() as client: thing = lt.ThingClient.from_url("/checker/", client=client) with monitor_for_changes(thing, hold_lock=True): assertions_without_locking(thing) @@ -448,3 +452,33 @@ def test_reuse_of_action_callables(): func() with assert_changes(thing): func() + + +def test_global_lock_log(caplog): + """Test that we get sensible errors when the lock is busy.""" + server = lt.ThingServer.from_things( + {"checker": ConcurrencyChecker}, enable_global_lock=True + ) + with server.test_client() as client: + checker = lt.ThingClient.from_url("/checker/", client=client) + + with monitor_for_changes(checker, hold_lock=True): + # First, try a function that uses the global lock. + # This should fail with a message about the global + # lock, but no traceback. + caplog.clear() + with pytest.raises(ServerActionError, match="Global lock was busy"): + checker.increment_fprop2() + matches = [r for r in caplog.records if "Global lock was busy" in r.message] + assert len(matches) == 1 + assert matches[0].levelno == logging.WARNING + assert "Traceback" not in caplog.text + + # Next, try the same thing with an action that does + # not hold the global lock, but calls a property that + # does. This should print a stack trace, as the + # exception is not handled. + caplog.clear() + with pytest.raises(ServerActionError, match="GlobalLockBusyError"): + checker.increment_prop1() + assert "Traceback" in caplog.text From 922e1d0cad376cd90ac397312c4f49fc26b74cc7 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 5 May 2026 22:09:25 +0100 Subject: [PATCH 12/17] Revise public API to have clearer argument The public API `hold_global_lock` no longer uses the True/False/None scheme, and instead has a more explicitly-named argument, as suggested by @julianstirling. --- docs/source/public_api.rst | 8 +++--- src/labthings_fastapi/actions.py | 4 ++- src/labthings_fastapi/properties.py | 8 ++++-- src/labthings_fastapi/testing.py | 11 -------- .../thing_server_interface.py | 18 +++++++++++- tests/test_thing_server_interface.py | 28 +++++++++++++++---- 6 files changed, 52 insertions(+), 25 deletions(-) diff --git a/docs/source/public_api.rst b/docs/source/public_api.rst index 7a235775..fc0d3d75 100644 --- a/docs/source/public_api.rst +++ b/docs/source/public_api.rst @@ -307,11 +307,11 @@ This page summarises the parts of the LabThings API that should be most frequent A global lock object that is used to restrict concurrent execution of actions and setting of properties. - .. py:method:: hold_global_lock(enabled: bool | None = True) + .. py:method:: hold_global_lock(*, error_if_unavailable: bool = True) - A context manager that holds the global lock. The `enabled` parameter sets - whether the lock is held. `False` ignores the lock, `None` uses the lock if - available, and `True` uses the lock or raises an error if it is missing. + A context manager that holds the global lock. By default, an exception is raised if the global lock + is not enabled. ``error_if_unavailable`` may be used to suppress that error, in which case the + context manager silently does nothing if there is no global lock to acquire. .. py:class:: ThingClassSettings diff --git a/src/labthings_fastapi/actions.py b/src/labthings_fastapi/actions.py index 00b3b1fb..386132f7 100644 --- a/src/labthings_fastapi/actions.py +++ b/src/labthings_fastapi/actions.py @@ -768,7 +768,9 @@ def context_for_func(self, obj: OwnerT) -> Iterator[None]: :param obj: The object on which the method is being called. :return: the function, wrapped if necessary. """ - with obj._thing_server_interface.hold_global_lock(self.use_global_lock): + with obj._thing_server_interface._optionally_hold_global_lock( + self.use_global_lock + ): yield def instance_get(self, obj: OwnerT) -> Callable[ActionParams, ActionReturn]: diff --git a/src/labthings_fastapi/properties.py b/src/labthings_fastapi/properties.py index e5491a4f..e3de1b6b 100644 --- a/src/labthings_fastapi/properties.py +++ b/src/labthings_fastapi/properties.py @@ -770,7 +770,9 @@ def __set__( :param value: the new value for the property. :param emit_changed_event: whether to emit a changed event. """ - with obj._thing_server_interface.hold_global_lock(self.use_global_lock): + with obj._thing_server_interface._optionally_hold_global_lock( + self.use_global_lock + ): if get_validate_properties_on_set(obj.__class__): property_info = self.descriptor_info(obj) obj.__dict__[self.name] = property_info.validate(value) @@ -1033,7 +1035,9 @@ def __set__(self, obj: Owner, value: Value) -> None: if get_validate_properties_on_set(obj.__class__): property_info = self.descriptor_info(obj) value = property_info.validate(value) - with obj._thing_server_interface.hold_global_lock(self.use_global_lock): + with obj._thing_server_interface._optionally_hold_global_lock( + self.use_global_lock + ): self.fset(obj, value) @builtins.property diff --git a/src/labthings_fastapi/testing.py b/src/labthings_fastapi/testing.py index 6c000c1f..a16334e0 100644 --- a/src/labthings_fastapi/testing.py +++ b/src/labthings_fastapi/testing.py @@ -144,17 +144,6 @@ def global_lock(self) -> GlobalLock | None: """Return a global lock.""" return self._global_lock - @contextmanager - def hold_global_lock(self, enabled: bool | None = True) -> Iterator[None]: - """Hold the global lock while a block of code executes. - - See `lt.ThingServerInterface.hold_global_lock` for full documentation. - - :param enabled: whether or not the lock must be held. - """ - with ThingServerInterface.hold_global_lock(self, enabled): - yield - ThingSubclass = TypeVar("ThingSubclass", bound="Thing") diff --git a/src/labthings_fastapi/thing_server_interface.py b/src/labthings_fastapi/thing_server_interface.py index 67225f2d..7f31bcf2 100644 --- a/src/labthings_fastapi/thing_server_interface.py +++ b/src/labthings_fastapi/thing_server_interface.py @@ -195,7 +195,9 @@ def global_lock(self) -> GlobalLock | None: return self._get_server().global_lock @contextmanager - def hold_global_lock(self, enabled: bool | None = True) -> Iterator[None]: + def _optionally_hold_global_lock( + self, enabled: bool | None = True + ) -> Iterator[None]: """Hold the global lock, if required, as a context manager. This function will hold the global lock if necessary while a block of code runs. @@ -225,3 +227,17 @@ def hold_global_lock(self, enabled: bool | None = True) -> Iterator[None]: else: with self.global_lock: yield + + @contextmanager + def hold_global_lock(self, *, error_if_unavailable: bool = True) -> Iterator[None]: + """Hold the global lock for the duration of a with block. + + This context manager will hold the global lock while a ``with:`` block runs. + By default, an exception will be raised if the global lock is not enabled. + + :param error_if_unavailable: may be set to `False` to suppress errors if the + global lock is not enabled. This means the context manager silently does + nothing, if the global lock is not available. + """ + with self._optionally_hold_global_lock(True if error_if_unavailable else None): + yield diff --git a/tests/test_thing_server_interface.py b/tests/test_thing_server_interface.py index e4a85f68..e4f5dc15 100644 --- a/tests/test_thing_server_interface.py +++ b/tests/test_thing_server_interface.py @@ -342,13 +342,22 @@ def test_mock_hold_global_lock(mock): assert interface.global_lock is None # With no global lock, the context manager should be a no-op, unless we # specify `enabled=True` at which point it errors. - with interface.hold_global_lock(False): + with interface._optionally_hold_global_lock(False): pass # hold_global_lock should be a no-op - with interface.hold_global_lock(None): + with interface._optionally_hold_global_lock(None): pass # hold_global_lock should be a no-op with pytest.raises(FeatureNotEnabledError): - with interface.hold_global_lock(True): + with interface._optionally_hold_global_lock(True): pass # hold_global_lock should error, as there's no lock + # The public API version only has two options - with error or without: + with pytest.raises(FeatureNotEnabledError): + with interface.hold_global_lock(): + pass # hold_global_lock should error by default + with pytest.raises(FeatureNotEnabledError): + with interface.hold_global_lock(error_if_unavailable=True): + pass # hold_global_lock should error + with interface.hold_global_lock(error_if_unavailable=False): + pass # The error was suppressed, so no errors here :) # If specified, there will be a global lock. if mock: @@ -358,10 +367,17 @@ def test_mock_hold_global_lock(mock): interface = ThingServerInterface(server, "thing_name") assert isinstance(interface.global_lock, GlobalLock) # That means the context manager should work for all three arguments. - with interface.hold_global_lock(False): + with interface._optionally_hold_global_lock(False): assert lock_is_available(interface.global_lock) - with interface.hold_global_lock(None): + with interface._optionally_hold_global_lock(None): assert not lock_is_available(interface.global_lock) - with interface.hold_global_lock(True): + with interface._optionally_hold_global_lock(True): assert not lock_is_available(interface.global_lock) assert lock_is_available(interface.global_lock) + # Also the public API version should work even with errors enabled. + with interface.hold_global_lock(): + assert not lock_is_available(interface.global_lock) + with interface.hold_global_lock(error_if_unavailable=True): + assert not lock_is_available(interface.global_lock) + with interface.hold_global_lock(error_if_unavailable=False): + assert not lock_is_available(interface.global_lock) From 0dfcc164946f89473525110355650804cc8f868f Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 6 May 2026 17:13:38 +0100 Subject: [PATCH 13/17] Docstring improvements from review Thanks to @bprobert97 --- src/labthings_fastapi/actions.py | 19 ++++++++++++++----- src/labthings_fastapi/exceptions.py | 4 ++-- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/labthings_fastapi/actions.py b/src/labthings_fastapi/actions.py index 386132f7..888acfef 100644 --- a/src/labthings_fastapi/actions.py +++ b/src/labthings_fastapi/actions.py @@ -755,16 +755,25 @@ def __set_name__(self, owner: type[OwnerT], name: str) -> None: @contextmanager def context_for_func(self, obj: OwnerT) -> Iterator[None]: - """Create context in which ``func`` runs. - - This method is intended to create a hook for pre-run set-up and post-run - clean-up code. It should not perform slow or intensive tasks, and is mostly - intended as a good place to acquire and release locks and so on. + """Create the context in which ``func`` runs. Currently, if global locking is enabled and this action hasn't opted out, this context manager will hold the global lock for the duration of the action. + This method is intended to create a hook for pre-run set-up and post-run + clean-up code that may be customised by `Thing` implementations in the future, + such as acquiring locks or other resources. + + When an action is run from Python code as ``thing.action()`` this context + manager is entered before executing `func` bound to the `Thing` instance. + + When an action is run from HTTP, this context manager is entered while the + action's status is ``pending`` and the status changes to ``running`` just + before `func` (the function decorated by `~lt.action`) runs. This allows + some slightly nicer error handling, for example not cluttering the log with + stack traces if an action can't start because the global lock is in use. + :param obj: The object on which the method is being called. :return: the function, wrapped if necessary. """ diff --git a/src/labthings_fastapi/exceptions.py b/src/labthings_fastapi/exceptions.py index d60b36f8..472d5269 100644 --- a/src/labthings_fastapi/exceptions.py +++ b/src/labthings_fastapi/exceptions.py @@ -351,8 +351,8 @@ class InvalidClassSettingsError(ValueError): class FeatureNotEnabledError(RuntimeError): """A feature is being used that is currently disabled. - Some new or optional features are only available if the relevant feature flag - is set. See `lt.FEATURE_FLAGS` for a list of features that may be enabled. + Some new or optional features must be enabled in the server settings or in + `~lt.Thing._class_settings` before they can be used. This error is raised if a feature is used when it is not enabled. """ From 5078fb626ae12e2874ef5e61615cc31f4b3963d8 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 6 May 2026 17:16:30 +0100 Subject: [PATCH 14/17] Add a sanity check to `assert_takes_time` From review by @bprobert97 --- tests/utilities.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/utilities.py b/tests/utilities.py index 7b0b87e4..3a032ad0 100644 --- a/tests/utilities.py +++ b/tests/utilities.py @@ -36,6 +36,8 @@ def raises_or_is_caused_by( @contextmanager def assert_takes_time(min_t: float | None, max_t: float | None): """Assert that a code block takes a certain amount of time.""" + if min_t is None and max_t is None: + raise ValueError("assert_takes_time(None, None) is meaningless!") before = time.time() yield after = time.time() From 3468d982f4791163f4d39a88350d497fb31159ae Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 6 May 2026 17:25:48 +0100 Subject: [PATCH 15/17] Use consisten import for ThingServerInterface --- tests/test_thing_server_interface.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_thing_server_interface.py b/tests/test_thing_server_interface.py index e4f5dc15..2ce6ed1b 100644 --- a/tests/test_thing_server_interface.py +++ b/tests/test_thing_server_interface.py @@ -323,7 +323,7 @@ def test_mocking_slots(): def test_global_lock(enable): """Test that the global lock is accessible, if configured.""" server = lt.ThingServer.from_things({}, enable_global_lock=enable) - interface = lt.ThingServerInterface(server, "thing_name") + interface = ThingServerInterface(server, "thing_name") if enable: assert isinstance(interface.global_lock, GlobalLock) else: @@ -338,7 +338,7 @@ def test_mock_hold_global_lock(mock): interface = MockThingServerInterface("thing_name") else: server = lt.ThingServer.from_things({}) - interface = lt.ThingServerInterface(server, "thing_name") + interface = ThingServerInterface(server, "thing_name") assert interface.global_lock is None # With no global lock, the context manager should be a no-op, unless we # specify `enabled=True` at which point it errors. From bbfc1e05460bf9b50222c4480c2307dc4d2db99e Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 6 May 2026 17:26:08 +0100 Subject: [PATCH 16/17] Make logic of _optionally_hold_global_lock clearer --- src/labthings_fastapi/thing_server_interface.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/labthings_fastapi/thing_server_interface.py b/src/labthings_fastapi/thing_server_interface.py index 7f31bcf2..6dedea5f 100644 --- a/src/labthings_fastapi/thing_server_interface.py +++ b/src/labthings_fastapi/thing_server_interface.py @@ -216,17 +216,19 @@ def _optionally_hold_global_lock( not enabled. """ if self.global_lock is None: - if enabled is True: + if enabled: msg = "The global lock is required, but is not enabled." raise FeatureNotEnabledError(msg) # If we get here, the global lock is disabled so we do nothing. yield else: - if enabled is False: # The lock is being explicitly skipped - yield - else: + if enabled is None or enabled: + # None means "use the lock if available", True means "use the lock". with self.global_lock: yield + else: + # enabled has been explicitly set to False, so skip the lock. + yield @contextmanager def hold_global_lock(self, *, error_if_unavailable: bool = True) -> Iterator[None]: From f0ca23b8d4ee021fa0b4e2769daca01c05d19023 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 6 May 2026 17:37:56 +0100 Subject: [PATCH 17/17] Respond to review comments on test_global_lock This fixes several things: * Adds a test for the expected error if an un-acquired lock is released. * Adds a test that the global lock is the same lock. * Adds a test for non-default timeouts. * Fixes some docstrings. * Clarifies a confusing comment. Thanks @bprobert97 --- tests/test_global_lock.py | 35 +++++++++++++++++++++++++++++++---- 1 file changed, 31 insertions(+), 4 deletions(-) diff --git a/tests/test_global_lock.py b/tests/test_global_lock.py index 8ba0cf02..a1c74076 100644 --- a/tests/test_global_lock.py +++ b/tests/test_global_lock.py @@ -274,6 +274,24 @@ def test_global_lock_unthreaded(): assert lock_is_available(lock) +def test_global_lock_release_unacquired(): + """Make sure the same error is raised as for RLock for spurious release.""" + lock = GlobalLock() + with pytest.raises(RuntimeError): + lock.release() # The lock was never acquired. + + +def test_global_lock_identity(): + """Ensure the property returns the exact same lock instance every time.""" + server = lt.ThingServer.from_things({}, enable_global_lock=True) + interface = lt.ThingServerInterface(server, "thing_name") + + lock_1 = interface.global_lock + lock_2 = interface.global_lock + + assert lock_1 is lock_2, "The interface is generating multiple distinct locks!" + + def test_global_lock_timeout(): """Check the global lock times out correctly.""" lock = GlobalLock() @@ -294,6 +312,12 @@ def hold_lock_in_background(): with assert_takes_time(0.045, 0.1): assert lock.acquire(blocking=True) is False + # acquire() should respect the timeout argument + with assert_takes_time(None, 0.04): + assert lock.acquire(timeout=0) is False + with assert_takes_time(0.06, 0.12): + assert lock.acquire(timeout=0.1) is False + # check non-blocking acquire() works with assert_takes_time(None, 0.001): assert lock.acquire(blocking=False) is False @@ -375,8 +399,11 @@ def assertions_with_locking(thing: ConcurrencyChecker): thing.increment_fprop2() # Actions may run if they're excluded from the lock. - # Note this is done twice to check for reuse of context managers - # (which will fail on the second attempt) + # Note this is done twice to check for reuse of context managers. + # (There is no expected failure, because we don't reuse the + # context manager. However, running the test below twice did + # fail, when a generator context manager was being inappropriately + # reused.) with assert_changes(thing): thing.increment_fprop2_unlocked() with assert_changes(thing): @@ -414,7 +441,7 @@ def test_actions_and_properties_direct_lock_disabled(): def test_actions_and_properties_testclient_lock_enabled(): """Ensure the global lock stops multiple things happening at once. - This test uses a Thing instance directly, with locking enabled. + This test uses TestClient, with locking enabled. """ server = lt.ThingServer.from_things( {"checker": ConcurrencyChecker}, enable_global_lock=True @@ -430,7 +457,7 @@ def test_actions_and_properties_testclient_lock_enabled(): def test_actions_and_properties_testclient_lock_disabled(): """Ensure the global lock stops multiple things happening at once. - This test uses a Thing instance directly, with locking disabled. + This test uses a TestClient, with locking disabled. """ server = lt.ThingServer.from_things( {"checker": ConcurrencyChecker}, enable_global_lock=False