Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,14 @@ addopts = """
"""
# https://iscinumpy.gitlab.io/post/bound-version-constraints/#watch-for-warnings
# https://github.com/DiamondLightSource/FastCS/issues/230
filterwarnings = "error"
filterwarnings = [
"error",
# DeviceTestContext (pytango) leaks Unix IPC sockets and its internal asyncio
# event loop on some Python 3.11.x versions. These cannot be closed from
# Python because they are Tango C-level resources. Suppress rather than fail.
"ignore:unclosed <socket\\.socket:ResourceWarning",
"ignore:unclosed event loop:ResourceWarning",
]
# Doctest python code in docs, python code in src docstrings, test functions in tests
testpaths = "docs src tests"
timeout = 5
Expand Down
26 changes: 14 additions & 12 deletions src/fastcs/control_system.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,9 @@ def __init__(
self,
controller: Controller,
transports: Sequence[Transport],
loop: asyncio.AbstractEventLoop | None = None,
):
self._controller = controller
self._transports = transports
self._loop = loop or asyncio.get_event_loop()

self._scan_coros: list[ScanCallback] = []
self._initial_coros: list[ScanCallback] = []
Expand All @@ -47,24 +45,20 @@ def run(self, interactive: bool = True):
"""Run the application

This is a convenience method to call `serve` in a synchronous context.
To use in an async context, call `serve` directly.

Args:
interactive: Whether to create an interactive IPython shell

"""
serve = asyncio.ensure_future(self.serve(interactive=interactive))

if os.name != "nt":
self._loop.add_signal_handler(signal.SIGINT, serve.cancel)
self._loop.add_signal_handler(signal.SIGTERM, serve.cancel)
self._loop.run_until_complete(serve)
asyncio.run(self.serve(interactive=interactive))

async def _run_initial_coros(self):
for coro in self._initial_coros:
await coro()

async def _start_scan_tasks(self):
self._scan_tasks = {self._loop.create_task(coro()) for coro in self._scan_coros}
self._scan_tasks = {asyncio.create_task(coro()) for coro in self._scan_coros}

def _stop_scan_tasks(self):
for task in self._scan_tasks:
Expand Down Expand Up @@ -93,6 +87,13 @@ async def serve(self, interactive: bool = True) -> None:
interactive: Whether to create an interactive IPython shell

"""
loop = asyncio.get_running_loop()
if os.name != "nt":
task = asyncio.current_task()
if task is not None:
loop.add_signal_handler(signal.SIGINT, task.cancel)
loop.add_signal_handler(signal.SIGTERM, task.cancel)

await self._controller.initialise()
self._controller.post_initialise()

Expand All @@ -110,7 +111,7 @@ async def serve(self, interactive: bool = True) -> None:

coros: list[Coroutine] = []
for transport in self._transports:
transport.connect(controller_api=self.controller_api, loop=self._loop)
transport.connect(controller_api=self.controller_api)
coros.append(transport.serve())
common_context = context.keys() & transport.context.keys()
if common_context:
Expand Down Expand Up @@ -155,14 +156,15 @@ async def block_forever():

async def _interactive_shell(self, context: dict[str, Any]):
"""Spawn interactive shell in another thread and wait for it to complete."""
loop = asyncio.get_running_loop()

def run(coro: Coroutine[None, None, None]):
"""Run coroutine on FastCS event loop from IPython thread."""

def wrapper():
asyncio.create_task(coro)

self._loop.call_soon_threadsafe(wrapper)
loop.call_soon_threadsafe(wrapper)
Comment on lines 164 to +167
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -n "create_task\\(|stop_event.wait|call_soon_threadsafe" src/fastcs/control_system.py -C3

Repository: DiamondLightSource/fastcs

Length of output: 904


🏁 Script executed:

# Get the full _interactive_shell method and interactive_shell function
sed -n '155,195p' src/fastcs/control_system.py

Repository: DiamondLightSource/fastcs

Length of output: 1103


🏁 Script executed:

# Also search for the interactive_shell function definition
rg -n "async def interactive_shell" src/fastcs/control_system.py -A 20

Repository: DiamondLightSource/fastcs

Length of output: 684


🏁 Script executed:

# Check for any try-except blocks around stop_event.wait() or in the method
sed -n '157,190p' src/fastcs/control_system.py

Repository: DiamondLightSource/fastcs

Length of output: 1054


🏁 Script executed:

# Check if InteractiveShellEmbed or shell.mainloop might handle exceptions internally
rg -n "class.*_interactive_shell|try:|except|finally:" src/fastcs/control_system.py -B2 -A2

Repository: DiamondLightSource/fastcs

Length of output: 857


🏁 Script executed:

# Check where _interactive_shell is called from
rg -n "_interactive_shell" src/fastcs/control_system.py

Repository: DiamondLightSource/fastcs

Length of output: 200


🏁 Script executed:

# Check if there's any outer error handling that might catch exceptions
rg -n "def.*_interactive_shell|await.*_interactive_shell" src/fastcs/ -B5 -A10

Repository: DiamondLightSource/fastcs

Length of output: 1183


🏁 Script executed:

# Get the full serve method context to understand the full call chain
sed -n '120,156p' src/fastcs/control_system.py

Repository: DiamondLightSource/fastcs

Length of output: 1265


🏁 Script executed:

# Verify if InteractiveShellEmbed could raise exceptions during mainloop
rg -n "InteractiveShellEmbed|from.*IPython" src/fastcs/control_system.py -B2 -A2

Repository: DiamondLightSource/fastcs

Length of output: 596


Handle exceptions from interactive_shell task to prevent blocking and silent failures.

At line 181, the interactive_shell task is created without a retained handle and without error supervision. If asyncio.to_thread(partial(shell.mainloop, ...)) raises an exception, stop_event.set() is never called, causing await stop_event.wait() at line 182 to block indefinitely and the exception to be silently lost.

Additionally, at line 165, tasks created via the run() function are not tracked, preventing visibility into task failures from the interactive shell.

🔧 Suggested fix
     async def _interactive_shell(self, context: dict[str, Any]):
@@
-        def run(coro: Coroutine[None, None, None]):
+        pending_tasks: set[asyncio.Task[Any]] = set()
+
+        def _track(task: asyncio.Task[Any]) -> asyncio.Task[Any]:
+            pending_tasks.add(task)
+            task.add_done_callback(pending_tasks.discard)
+            return task
+
+        def run(coro: Coroutine[None, None, None]):
@@
             def wrapper():
-                asyncio.create_task(coro)
+                _track(asyncio.create_task(coro))
@@
-        stop_event = asyncio.Event()
-        asyncio.create_task(interactive_shell(context, stop_event))
-        await stop_event.wait()
+        stop_event = asyncio.Event()
+        shell_task = _track(asyncio.create_task(interactive_shell(context, stop_event)))
+        wait_task = asyncio.create_task(stop_event.wait())
+        done, pending = await asyncio.wait(
+            {shell_task, wait_task},
+            return_when=asyncio.FIRST_COMPLETED,
+        )
+        for task in pending:
+            task.cancel()
+        if shell_task in done:
+            await shell_task
🧰 Tools
🪛 Ruff (0.15.5)

[warning] 165-165: Store a reference to the return value of asyncio.create_task

(RUF006)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/fastcs/control_system.py` around lines 164 - 167, The interactive_shell
task created inside run()/wrapper must be tracked and its exceptions handled:
when creating the coroutine (interactive_shell /
asyncio.to_thread(partial(shell.mainloop, ...))) retain the Task returned by
asyncio.create_task and attach a done callback that catches/logs exceptions and
always calls stop_event.set() on error; also ensure tasks spawned by run() are
appended to a local registry so failures are visible (e.g., add the created Task
to a list in run() and on completion remove it), and update wrapper to create
the task via that helper so any raised exceptions won't silently block await
stop_event.wait().


async def interactive_shell(
context: dict[str, object], stop_event: asyncio.Event
Expand All @@ -176,7 +178,7 @@ async def interactive_shell(
context["run"] = run

stop_event = asyncio.Event()
self._loop.create_task(interactive_shell(context, stop_event))
asyncio.create_task(interactive_shell(context, stop_event))
await stop_event.wait()

def __del__(self):
Expand Down
5 changes: 1 addition & 4 deletions src/fastcs/launch.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import asyncio
import inspect
import json
from pathlib import Path
Expand Down Expand Up @@ -158,9 +157,7 @@ def run(
else:
controller = controller_class()

instance = FastCS(
controller, instance_options.transport, loop=asyncio.get_event_loop()
)
instance = FastCS(controller, instance_options.transport)

instance.run()

Expand Down
7 changes: 2 additions & 5 deletions src/fastcs/transports/epics/ca/ioc.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,8 @@ def __init__(
_create_and_link_attribute_pvs(pv_prefix, controller_api)
_create_and_link_command_pvs(pv_prefix, controller_api)

def run(
self,
loop: asyncio.AbstractEventLoop,
) -> None:
dispatcher = AsyncioDispatcher(loop) # Needs running loop
def run(self) -> None:
dispatcher = AsyncioDispatcher(asyncio.get_running_loop())
builder.LoadDatabase()
softioc.iocInit(dispatcher)

Expand Down
10 changes: 2 additions & 8 deletions src/fastcs/transports/epics/ca/transport.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import asyncio
from dataclasses import dataclass, field
from typing import Any

Expand Down Expand Up @@ -28,13 +27,8 @@ class EpicsCATransport(Transport):
gui: EpicsGUIOptions | None = None
"""Options for the GUI. If not set, no GUI will be created."""

def connect(
self,
controller_api: ControllerAPI,
loop: asyncio.AbstractEventLoop,
) -> None:
def connect(self, controller_api: ControllerAPI) -> None:
self._controller_api = controller_api
self._loop = loop
self._pv_prefix = self.epicsca.pv_prefix
self._ioc = EpicsCAIOC(self.epicsca.pv_prefix, controller_api)

Expand All @@ -47,7 +41,7 @@ def connect(
async def serve(self) -> None:
"""Serve `ControllerAPI` over EPICS Channel Access"""
logger.info("Running IOC", pv_prefix=self._pv_prefix)
self._ioc.run(self._loop)
self._ioc.run()

@property
def context(self) -> dict[str, Any]:
Expand Down
13 changes: 2 additions & 11 deletions src/fastcs/transports/epics/pva/transport.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,8 @@
import asyncio
from dataclasses import dataclass, field

from fastcs.controllers import ControllerAPI
from fastcs.logging import logger
from fastcs.transports.epics import (
EpicsDocsOptions,
EpicsGUIOptions,
EpicsIOCOptions,
)
from fastcs.transports.epics import EpicsDocsOptions, EpicsGUIOptions, EpicsIOCOptions
from fastcs.transports.epics.docs import EpicsDocs
from fastcs.transports.epics.pva.gui import PvaEpicsGUI
from fastcs.transports.transport import Transport
Expand All @@ -23,11 +18,7 @@ class EpicsPVATransport(Transport):
docs: EpicsDocsOptions | None = None
gui: EpicsGUIOptions | None = None

def connect(
self,
controller_api: ControllerAPI,
loop: asyncio.AbstractEventLoop,
) -> None:
def connect(self, controller_api: ControllerAPI) -> None:
self._controller_api = controller_api
self._pv_prefix = self.epicspva.pv_prefix
self._ioc = P4PIOC(self.epicspva.pv_prefix, controller_api)
Expand Down
7 changes: 1 addition & 6 deletions src/fastcs/transports/graphql/transport.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import asyncio
from dataclasses import dataclass, field

from fastcs.controllers import ControllerAPI
Expand All @@ -14,11 +13,7 @@ class GraphQLTransport(Transport):

graphql: GraphQLServerOptions = field(default_factory=GraphQLServerOptions)

def connect(
self,
controller_api: ControllerAPI,
loop: asyncio.AbstractEventLoop,
):
def connect(self, controller_api: ControllerAPI):
self._server = GraphQLServer(controller_api)

async def serve(self) -> None:
Expand Down
7 changes: 1 addition & 6 deletions src/fastcs/transports/rest/transport.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import asyncio
from dataclasses import dataclass, field

from fastcs.controllers import ControllerAPI
Expand All @@ -14,11 +13,7 @@ class RestTransport(Transport):

rest: RestServerOptions = field(default_factory=RestServerOptions)

def connect(
self,
controller_api: ControllerAPI,
loop: asyncio.AbstractEventLoop,
):
def connect(self, controller_api: ControllerAPI):
self._server = RestServer(controller_api)

async def serve(self) -> None:
Expand Down
11 changes: 3 additions & 8 deletions src/fastcs/transports/tango/transport.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,8 @@ class TangoTransport(Transport):

tango: TangoDSROptions = field(default_factory=TangoDSROptions)

def connect(
self,
controller_api: ControllerAPI,
loop: asyncio.AbstractEventLoop,
):
self._dsr = TangoDSR(controller_api, loop)
def connect(self, controller_api: ControllerAPI):
self._dsr = TangoDSR(controller_api, asyncio.get_running_loop())

async def serve(self) -> None:
coro = asyncio.to_thread(self._dsr.run, self.tango)
await coro
await asyncio.to_thread(self._dsr.run, self.tango)
10 changes: 4 additions & 6 deletions src/fastcs/transports/transport.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import asyncio
from abc import abstractmethod
from dataclasses import dataclass
from typing import Any, ClassVar, Union
Expand All @@ -24,13 +23,12 @@ def union(cls):
return Union[tuple(cls.subclasses)] # noqa: UP007

@abstractmethod
def connect(
self, controller_api: ControllerAPI, loop: asyncio.AbstractEventLoop
) -> None:
def connect(self, controller_api: ControllerAPI) -> None:
"""Connect the ``Transport`` to the control system

The `ControllerAPI` should be exposed over the transport. The provided event
loop should be used where required instead of creating a new one.
The `ControllerAPI` should be exposed over the transport. Transports that
require the event loop should retrieve it with `asyncio.get_running_loop`,
as this method is called from within an async context.

"""
pass
Expand Down
4 changes: 1 addition & 3 deletions tests/benchmarking/controller.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import asyncio

from fastcs import FastCS
from fastcs.attributes import AttrR, AttrW
from fastcs.controllers import Controller
Expand All @@ -25,7 +23,7 @@ def run():
),
TangoTransport(tango=TangoDSROptions(dev_name="MY/BENCHMARK/DEVICE")),
]
instance = FastCS(MyTestController(), transport_options, asyncio.get_event_loop())
instance = FastCS(MyTestController(), transport_options)
instance.run()


Expand Down
14 changes: 4 additions & 10 deletions tests/test_control_system.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,8 @@

@pytest.mark.asyncio
async def test_scan_tasks(controller):
loop = asyncio.get_event_loop()
transport_options = []
fastcs = FastCS(controller, transport_options, loop)
fastcs = FastCS(controller, transport_options)

asyncio.create_task(fastcs.serve(interactive=False))
await asyncio.sleep(0.1)
Expand Down Expand Up @@ -43,9 +42,8 @@ async def do_nothing_static(self):
pass

controller = MyTestController()
loop = asyncio.get_event_loop()
transport_options = []
fastcs = FastCS(controller, transport_options, loop)
fastcs = FastCS(controller, transport_options)

asyncio.create_task(fastcs.serve(interactive=False))
await asyncio.sleep(0.1)
Expand Down Expand Up @@ -79,9 +77,7 @@ class MyController(Controller):
)

controller = MyController(ios=[AttributeIOTimesCalled()])
loop = asyncio.get_event_loop()

fastcs = FastCS(controller, [], loop)
fastcs = FastCS(controller, [])

assert controller.update_quickly.get() == 0
assert controller.update_once.get() == 0
Expand All @@ -108,9 +104,7 @@ async def disconnect(self):
self.connected = False

controller = MyTestController()

loop = asyncio.get_event_loop()
fastcs = FastCS(controller, [], loop)
fastcs = FastCS(controller, [])

task = asyncio.create_task(fastcs.serve(interactive=False))

Expand Down
2 changes: 0 additions & 2 deletions tests/transports/epics/ca/test_initial_value.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,12 +51,10 @@ class InitialValuesController(Controller):
async def test_initial_values_set_in_ca(mocker):
pv_prefix = "SOFTIOC_INITIAL_DEVICE"

loop = asyncio.get_event_loop()
controller = InitialValuesController()
fastcs = FastCS(
controller,
[EpicsCATransport(epicsca=EpicsIOCOptions(pv_prefix=pv_prefix))],
loop,
)

record_spy = mocker.spy(ca_ioc, "_make_in_record")
Expand Down
Loading
Loading