From 6efe75d9501636640e2247a58b4990df338eff90 Mon Sep 17 00:00:00 2001 From: AlexWells Date: Tue, 3 Feb 2026 11:55:51 +0000 Subject: [PATCH] Use our own implementation of dbPutField This works around issues with potential infinite loops, even if process=False, as seen in issue #201. --- CHANGELOG.rst | 4 +- softioc/device.py | 6 +-- softioc/extension.c | 32 +++++++++++---- softioc/imports.py | 9 +++-- tests/test_records.py | 90 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 127 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index a71b6b93..ff70667c 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -10,7 +10,9 @@ Versioning `_. Unreleased_ ----------- -Nothing yet! +Fixed: + +- `Fix infinite loop when setting record's own value from on_update callback <../../pull/202>`_ 4.7.0_ - 2026-01-14 ------------------- diff --git a/softioc/device.py b/softioc/device.py index e468d46b..965428c4 100644 --- a/softioc/device.py +++ b/softioc/device.py @@ -11,7 +11,7 @@ dbLoadDatabase, signal_processing_complete, recGblResetAlarms, - db_put_field, + db_put_field_process, db_get_field, ) from .device_core import DeviceSupportCore, RecordLookup @@ -108,7 +108,7 @@ def set_field(self, field, value): data = (c_char * 40)() data.value = str(value).encode() + b'\0' name = self._name + '.' + field - db_put_field(name, fields.DBF_STRING, addressof(data), 1) + db_put_field_process(name, fields.DBF_STRING, addressof(data), 1, True) class ProcessDeviceSupportIn(ProcessDeviceSupportCore): _link_ = 'INP' @@ -288,7 +288,7 @@ def set(self, value, process=True, # The array parameter is used to keep the raw pointer alive dbf_code, length, data, array = self._value_to_dbr(value) self.__enable_write = process - db_put_field(_record.NAME, dbf_code, data, length) + db_put_field_process(_record.NAME, dbf_code, data, length, process) self.__enable_write = True def get(self): diff --git a/softioc/extension.c b/softioc/extension.c index bf8bfd9c..4d59b010 100644 --- a/softioc/extension.c +++ b/softioc/extension.c @@ -93,16 +93,17 @@ static PyObject *get_field_offsets(PyObject *self, PyObject *args) } -/* Updates PV field with integrated db lookup. Safer to do this in C as we need - * an intermediate copy of the dbAddr structure, which changes size between - * EPICS releases. */ -static PyObject *db_put_field(PyObject *self, PyObject *args) +/* This is our own re-implementation of EPICS's dbPutField function. +We do this to allow us to control when dbProcess is called. We use the +same logicical flow as the original function. */ +static PyObject *db_put_field_process(PyObject *self, PyObject *args) { const char *name; short dbrType; PyObject *buffer_ptr; long length; - if (!PyArg_ParseTuple(args, "shOl", &name, &dbrType, &buffer_ptr, &length)) + short process; + if (!PyArg_ParseTuple(args, "shOlh", &name, &dbrType, &buffer_ptr, &length, &process)) return NULL; void *pbuffer = PyLong_AsVoidPtr(buffer_ptr); if (!pbuffer) @@ -113,6 +114,8 @@ static PyObject *db_put_field(PyObject *self, PyObject *args) return PyErr_Format( PyExc_RuntimeError, "dbNameToAddr failed for %s", name); + struct dbCommon *precord = dbAddr.precord; + long put_result; /* There are two important locks to consider at this point: The Global * Interpreter Lock (GIL) and the EPICS record lock. A deadlock is possible @@ -125,7 +128,22 @@ static PyObject *db_put_field(PyObject *self, PyObject *args) * EPICS call, to avoid potential deadlocks. * See https://github.com/DiamondLightSource/pythonSoftIOC/issues/119. */ Py_BEGIN_ALLOW_THREADS - put_result = dbPutField(&dbAddr, dbrType, pbuffer, length); + dbScanLock(precord); + put_result = dbPut(&dbAddr, dbrType, pbuffer, length); + + if (put_result == 0 && process) + { + if (precord->pact) + { + precord->rpro = TRUE; + } + else + { + dbProcess(precord); + } + } + + dbScanUnlock(precord); Py_END_ALLOW_THREADS if (put_result) return PyErr_Format( @@ -314,7 +332,7 @@ static struct PyMethodDef softioc_methods[] = { "Get a map of DBF names to values"}, {"get_field_offsets", get_field_offsets, METH_VARARGS, "Get offset, size and type for each record field"}, - {"db_put_field", db_put_field, METH_VARARGS, + {"db_put_field_process", db_put_field_process, METH_VARARGS, "Put a database field to a value"}, {"db_get_field", db_get_field, METH_VARARGS, "Get a database field's value"}, diff --git a/softioc/imports.py b/softioc/imports.py index a54fc8ee..c27dd177 100644 --- a/softioc/imports.py +++ b/softioc/imports.py @@ -19,9 +19,12 @@ def get_field_offsets(record_type): '''Return {field_name: (offset, size, field_type)}''' return _extension.get_field_offsets(record_type) -def db_put_field(name, dbr_type, pbuffer, length): - '''Put field where pbuffer is void* pointer. Returns None.''' - return _extension.db_put_field(name, dbr_type, pbuffer, length) +def db_put_field_process(name, dbr_type, pbuffer, length, process): + '''Put field where pbuffer is void* pointer, conditionally processing + the record. Returns None.''' + return _extension.db_put_field_process( + name, dbr_type, pbuffer, length, process + ) def db_get_field(name, dbr_type, pbuffer, length): '''Get field where pbuffer is void* pointer. Returns None.''' diff --git a/tests/test_records.py b/tests/test_records.py index 58176ca0..7f125323 100644 --- a/tests/test_records.py +++ b/tests/test_records.py @@ -1,6 +1,7 @@ import asyncio import numpy import os +import re import pytest from enum import Enum @@ -682,6 +683,95 @@ def test_on_update_true_false(self, out_records): always_update is True and the put'ed value is always different""" self.on_update_runner(out_records, True, False) + def on_update_recursive_set_test_func( + self, device_name, conn + ): + log("CHILD: Child started") + + builder.SetDeviceName(device_name) + + async def on_update_func(new_val): + log("CHILD: on_update_func started") + record.set(0, process=False) + conn.send("C") # "Callback" + log("CHILD: on_update_func ended") + + record = builder.Action( + "ACTION", + on_update=on_update_func, + blocking=True, + initial_value=1 # A non-zero value, to check it changes + ) + + dispatcher = asyncio_dispatcher.AsyncioDispatcher() + builder.LoadDatabase() + softioc.iocInit(dispatcher) + + conn.send("R") # "Ready" + + log("CHILD: Sent R over Connection to Parent") + + # Keep process alive while main thread runs CAGET + if conn.poll(TIMEOUT): + val = conn.recv() + assert val == "D", "Did not receive expected Done character" + + log("CHILD: Received exit command, child exiting") + + async def test_on_update_recursive_set(self): + """Test that on_update functions correctly when the on_update + callback sets the value of the record again (with process=False). + See issue #201""" + + ctx = get_multiprocessing_context() + parent_conn, child_conn = ctx.Pipe() + + device_name = create_random_prefix() + + process = ctx.Process( + target=self.on_update_recursive_set_test_func, + args=(device_name, child_conn), + ) + + process.start() + + log("PARENT: Child started, waiting for R command") + + from aioca import caget, caput + + try: + # Wait for message that IOC has started + select_and_recv(parent_conn, "R") + + log("PARENT: received R command") + + record = f"{device_name}:ACTION" + + val = await caget(record) + + assert val == 1, "ACTION record did not start with value 1" + + await caput(record, 1, wait=True) + + val = await caget(record) + + assert val == 0, "ACTION record did not return to zero value" + + # Expect one "C" + select_and_recv(parent_conn, "C") + + # ...But if we receive another we know there's a problem + if parent_conn.poll(5): # Shorter timeout to make this quicker + pytest.fail("Received unexpected second message") + + finally: + log("PARENT:Sending Done command to child") + parent_conn.send("D") # "Done" + process.join(timeout=TIMEOUT) + log(f"PARENT: Join completed with exitcode {process.exitcode}") + if process.exitcode is None: + pytest.fail("Process did not terminate") + class TestBlocking: