From ea5cdbbe4423d3e6fbdb8e9b41f1b0ea72811bcc Mon Sep 17 00:00:00 2001
From: Markus Zolliker <markus.zolliker@psi.ch>
Date: Thu, 13 Apr 2023 16:00:10 +0200
Subject: [PATCH] improve error handling on callbacks

errors in callback functions should be reported to log,
but not stop the callback chain

Change-Id: I4fc509b7121960ebe59e1ad4f4b4746dfb4d5ba3
Reviewed-on: https://forge.frm2.tum.de/review/c/secop/frappy/+/30950
Tested-by: Jenkins Automated Tests <pedersen+jenkins@frm2.tum.de>
Reviewed-by: Markus Zolliker <markus.zolliker@psi.ch>
---
 frappy/client/__init__.py | 86 +++++++++++++++++++++------------------
 frappy/protocol/router.py |  3 +-
 2 files changed, 47 insertions(+), 42 deletions(-)

diff --git a/frappy/client/__init__.py b/frappy/client/__init__.py
index 32df0b24..b3bcc6f8 100644
--- a/frappy/client/__init__.py
+++ b/frappy/client/__init__.py
@@ -46,10 +46,10 @@ UPDATE_MESSAGES = {EVENTREPLY, READREPLY, WRITEREPLY, ERRORPREFIX + READREQUEST,
 
 VERSIONFMT= re.compile(r'^[^,]*?ISSE[^,]*,SECoP,')
 
-class UNREGISTER:
-    """a magic value, used a returned value in a callback
 
-    to indicate it has to be unregistered
+class UnregisterCallback(Exception):
+    """raise in a callback to indicate it has to be unregistered
+
     used to implement one shot callbacks
     """
 
@@ -157,8 +157,8 @@ class CacheItem(tuple):
 class ProxyClient:
     """common functionality for proxy clients"""
 
-    CALLBACK_NAMES = ('updateEvent', 'updateItem', 'descriptiveDataChange',
-                      'nodeStateChange', 'unhandledMessage')
+    CALLBACK_NAMES = {'updateEvent', 'updateItem', 'descriptiveDataChange',
+                      'nodeStateChange', 'unhandledMessage'}
     online = False  # connected or reconnecting since a short time
     state = 'disconnected'  # further possible values: 'connecting', 'reconnecting', 'connected'
     log = None
@@ -181,42 +181,41 @@ class ProxyClient:
         """
         for cbfunc in args:
             kwds[cbfunc.__name__] = cbfunc
-        for cbname in self.CALLBACK_NAMES:
-            cbfunc = kwds.pop(cbname, None)
-            if not cbfunc:
-                continue
-            cbdict = self.callbacks[cbname]
-            cbdict[key].append(cbfunc)
+        for cbname, cbfunc in kwds.items():
+            if cbname not in self.CALLBACK_NAMES:
+                raise TypeError(f"unknown callback: {', '.join(kwds)}")
 
             # immediately call for some callback types
-            if cbname == 'updateItem':
-                if key is None:
-                    for (mname, pname), data in self.cache.items():
-                        cbfunc(mname, pname, data)
+            if cbname in ('updateItem', 'updateEvent'):
+                if key is None:  # case generic callback
+                    cbargs = [(m, p, d) for (m, p), d in self.cache.items()]
                 else:
                     data = self.cache.get(key, None)
-                    if data:
-                        cbfunc(*key, data)  # case single parameter
+                    if data:  # case single parameter
+                        cbargs = [key + (data,)]
                     else:  # case key = module
-                        for (mname, pname), data in self.cache.items():
-                            if mname == key:
-                                cbfunc(mname, pname, data)
-            elif cbname == 'updateEvent':
-                if key is None:
-                    for (mname, pname), data in self.cache.items():
-                        cbfunc(mname, pname, *data)
-                else:
-                    data = self.cache.get(key, None)
-                    if data:
-                        cbfunc(*key, *data)  # case single parameter
-                    else:  # case key = module
-                        for (mname, pname), data in self.cache.items():
-                            if mname == key:
-                                cbfunc(mname, pname, *data)
+                        cbargs = [(m, p, d) for (m, p), d in self.cache.items() if m == key]
+
+                if cbname == 'updateEvent':
+                    # expand entry argument to (value, timestamp, readerror)
+                    cbargs = [a[0:2] + a[2] for a in cbargs]
+
             elif cbname == 'nodeStateChange':
-                cbfunc(self.online, self.state)
-        if kwds:
-            raise TypeError(f"unknown callback: {', '.join(kwds)}")
+                cbargs = [(self.online, self.state)]
+            else:
+                cbargs = []
+
+            do_append = True
+            for args in cbargs:
+                try:
+                    cbfunc(*args)
+                except UnregisterCallback:
+                    do_append = False
+                except Exception as e:
+                    if self.log:
+                        self.log.error('error %r calling %s%r', e, cbfunc.__name__, args)
+            if do_append:
+                self.callbacks[cbname][key].append(cbfunc)
 
     def unregister_callback(self, key, *args, **kwds):
         """unregister a callback
@@ -240,20 +239,27 @@ class ProxyClient:
         key=(<module name>, <parameter name): callbacks for specified parameter
         """
         cblist = self.callbacks[cbname].get(key, [])
-        self.callbacks[cbname][key] = [cb for cb in cblist if cb(*args) is not UNREGISTER]
+        for cbfunc in list(cblist):
+            try:
+                cbfunc(*args)
+            except UnregisterCallback:
+                cblist.remove(cbfunc)
+            except Exception as e:
+                if self.log:
+                    self.log.error('error %r calling %s%r', e, cbfunc.__name__, args)
         return bool(cblist)
 
     def updateValue(self, module, param, value, timestamp, readerror):
         entry = CacheItem(value, timestamp, readerror,
-                        self.modules[module]['parameters'][param]['datatype'])
+                          self.modules[module]['parameters'][param]['datatype'])
         self.cache[(module, param)] = entry
         self.callback(None, 'updateItem', module, param, entry)
         self.callback(module, 'updateItem', module, param, entry)
         self.callback((module, param), 'updateItem', module, param, entry)
         # TODO: change clients to use updateItem instead of updateEvent
-        self.callback(None, 'updateEvent', module, param, value, timestamp, readerror)
-        self.callback(module, 'updateEvent', module, param, value, timestamp, readerror)
-        self.callback((module, param), 'updateEvent', module, param, value, timestamp, readerror)
+        self.callback(None, 'updateEvent', module, param, *entry)
+        self.callback(module, 'updateEvent', module, param, *entry)
+        self.callback((module, param), 'updateEvent', module, param, *entry)
 
 
 class SecopClient(ProxyClient):
diff --git a/frappy/protocol/router.py b/frappy/protocol/router.py
index 011cac8e..795ca656 100644
--- a/frappy/protocol/router.py
+++ b/frappy/protocol/router.py
@@ -122,8 +122,7 @@ class Router(frappy.protocol.dispatcher.Dispatcher):
                             self.node_by_module[module] = node
                         self.nodes.append(node)
                         self.restart()
-                        return frappy.client.UNREGISTER
-                    return None
+                        raise frappy.client.UnregisterCallback()
 
                 node.register_callback(None, nodeStateChange)
                 logger.warning('can not connect to node %r', node.nodename)