Use our own implementation of dbPutField

This works around issues with potential infinite loops, even if
process=False, as seen in issue #201.

Author: AlexanderWells-diamond

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):

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"},

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.'''

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: