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 4c0ec6df..fc0d3d75 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. @@ -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(*, error_if_unavailable: bool = True) + + 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 @@ -354,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: diff --git a/src/labthings_fastapi/actions.py b/src/labthings_fastapi/actions.py index fc27af6f..888acfef 100644 --- a/src/labthings_fastapi/actions.py +++ b/src/labthings_fastapi/actions.py @@ -15,10 +15,12 @@ """ from __future__ import annotations +from collections.abc import Iterator +from contextlib import contextmanager 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 +31,7 @@ Callable, Concatenate, Generic, + Literal, Optional, ParamSpec, TypeVar, @@ -39,6 +42,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, @@ -49,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, @@ -287,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: @@ -308,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: @@ -665,8 +681,9 @@ 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. + 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 @@ -683,6 +700,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, + 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, for example using + `~lt.ThingServerInterface.hold_global_lock`\ . """ super().__init__() self.func = func @@ -692,6 +719,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 +753,69 @@ def __set_name__(self, owner: type[OwnerT], name: str) -> None: f"'{self.func.__name__}'", ) + @contextmanager + def context_for_func(self, obj: OwnerT) -> Iterator[None]: + """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. + """ + with obj._thing_server_interface._optionally_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. + + Accessing a regular Python method returns the method bound to the instance, + i.e. the `self` argument is supplied. - This currently doesn't validate the arguments, though it may do so - in future. In its present form, this is equivalent to a regular - Python method, i.e. all we do is supply the first argument, `self`. + 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.func, 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/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/exceptions.py b/src/labthings_fastapi/exceptions.py index d9454c1d..472d5269 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 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. + """ + + 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..e3de1b6b 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,16 @@ 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._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) + 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 +867,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 +880,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 +1035,10 @@ 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._optionally_hold_global_lock( + self.use_global_lock + ): + self.fset(obj, value) @builtins.property def default(self) -> Value: @@ -1274,12 +1310,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 +1335,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 +1382,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 +1423,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..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 @@ -31,6 +34,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, @@ -140,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) @@ -147,6 +152,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() @@ -230,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/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..a16334e0 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") @@ -138,6 +153,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. @@ -156,6 +172,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` @@ -169,7 +186,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 @@ -182,6 +203,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 d518c344..6dedea5f 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,61 @@ 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 _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. + 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: + 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 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]: + """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_global_lock.py b/tests/test_global_lock.py new file mode 100644 index 00000000..a1c74076 --- /dev/null +++ b/tests/test_global_lock.py @@ -0,0 +1,511 @@ +"""Test code for the global lock.""" + +from collections.abc import Iterator +import logging +from threading import Thread, Event +import pytest +from contextlib import contextmanager + +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 + +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 + + +class ConcurrencyChecker(lt.Thing): + """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) + self._tick_event = Event() + self._tock_event = Event() + 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. + + 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) + 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 + + 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) -> None: + r"""Check if any properties have changed. + + This function does not acquire the global lock. + + 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} + self.logger.info("Checking for changes") + while self.keep_checking_for_changes: + 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() + 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 + 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 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 + + +@contextmanager +def assert_changes(thing: ConcurrencyChecker): + """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() + assert thing.changes_detected is True + + +@contextmanager +def assert_fails(thing: ConcurrencyChecker) -> Iterator[None]: + """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 + thing.tick() + assert thing.changes_detected is False + + +@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: + thing.tick() + 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(): + """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_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() + 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 + + # 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 + + # 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() + + +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. + 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. + # 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): + thing.increment_prop1() + with assert_changes(thing): + thing.increment_fprop2_unlocked() + + +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 + 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 + 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. + with assert_fails(thing): + thing.prop1 += 1 + with assert_fails(thing): + thing.fprop1 += 1 + + # The properties excluded from the lock may still be written + with assert_changes(thing): + thing.prop2 += 1 + with assert_changes(thing): + thing.fprop2 += 1 + + # By default actions won't run + with assert_fails(thing): + 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. + # (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): + thing.increment_fprop2_unlocked() + + # Actions that use locked resources (like prop1) should also fail + with assert_fails(thing): + 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 TestClient, with locking enabled. + """ + server = lt.ThingServer.from_things( + {"checker": ConcurrencyChecker}, enable_global_lock=True + ) + 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) + 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 TestClient, with locking disabled. + """ + server = lt.ThingServer.from_things( + {"checker": ConcurrencyChecker}, enable_global_lock=False + ) + 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) + 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() + + +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 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_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_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/test_thing_server_interface.py b/tests/test_thing_server_interface.py index 8bed3a6e..2ce6ed1b 100644 --- a/tests/test_thing_server_interface.py +++ b/tests/test_thing_server_interface.py @@ -6,10 +6,15 @@ from typing import Mapping from unittest.mock import Mock +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 +24,8 @@ create_thing_without_server, ) +from .test_global_lock import lock_is_available + NAME = "testname" EXAMPLE_THING_STATE = {"foo": "bar"} @@ -310,3 +317,67 @@ 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.from_things({}, enable_global_lock=enable) + interface = 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.from_things({}) + 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. + with interface._optionally_hold_global_lock(False): + pass # hold_global_lock should be a no-op + with interface._optionally_hold_global_lock(None): + pass # hold_global_lock should be a no-op + with pytest.raises(FeatureNotEnabledError): + 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: + interface = MockThingServerInterface("thing_name", enable_global_lock=True) + else: + 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. + with interface._optionally_hold_global_lock(False): + assert lock_is_available(interface.global_lock) + with interface._optionally_hold_global_lock(None): + assert not lock_is_available(interface.global_lock) + 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) diff --git a/tests/utilities.py b/tests/utilities.py index c5d3a574..3a032ad0 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,18 @@ 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.""" + 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() + duration = after - before + if min_t is not None: + assert duration >= min_t + if max_t is not None: + assert duration <= max_t