From 7953826facd7d938285ea2f0b47e337c235a7519 Mon Sep 17 00:00:00 2001 From: Markus Zolliker Date: Fri, 15 May 2020 14:21:34 +0200 Subject: [PATCH] improve error handling on client connections - send a heartbeat, if no events for 5 sec. an interrupted connection (not closed by the other end) may not be detected for a long time when nothing is sent + make the error reply on a non SECoPEror more verbose e.g. "KeyError('foo')" instead of just "foo" + allow cfg file without nodeinterface + shorter logger name in HasIodev Change-Id: I6b1ff23f9bf8c96feb25af44935596437b7d726f Reviewed-on: https://forge.frm2.tum.de/review/c/sine2020/secop/playground/+/23098 Tested-by: JenkinsCodeReview Reviewed-by: Markus Zolliker --- secop/client/__init__.py | 11 +++++++++-- secop/lib/asynconn.py | 10 ++++++++-- secop/protocol/interface/tcp.py | 2 +- secop/server.py | 6 ++++-- secop/stringio.py | 2 +- 5 files changed, 23 insertions(+), 8 deletions(-) diff --git a/secop/client/__init__.py b/secop/client/__init__.py index fa73c53..82b1c04 100644 --- a/secop/client/__init__.py +++ b/secop/client/__init__.py @@ -30,10 +30,11 @@ from collections import defaultdict from secop.lib import mkthread, formatExtendedTraceback, formatExtendedStack from secop.lib.asynconn import AsynConn, ConnectionClosed -from secop.datatypes import get_datatype, EnumType +from secop.datatypes import get_datatype from secop.protocol.interface import encode_msg_frame, decode_msg from secop.protocol.messages import REQUEST2REPLY, ERRORPREFIX, EVENTREPLY, WRITEREQUEST, WRITEREPLY, \ - READREQUEST, READREPLY, IDENTREQUEST, IDENTPREFIX, ENABLEEVENTSREQUEST, COMMANDREQUEST, DESCRIPTIONREQUEST + READREQUEST, READREPLY, IDENTREQUEST, IDENTPREFIX, ENABLEEVENTSREQUEST, COMMANDREQUEST, \ + DESCRIPTIONREQUEST, HEARTBEATREQUEST import secop.errors import secop.params @@ -290,13 +291,19 @@ class SecopClient(ProxyClient): self.disconnect(False) def __rxthread(self): + noactivity = 0 while self._running: try: reply = self.io.readline() if reply is None: + noactivity += 1 + if noactivity % 5 == 0: + # send ping to check if the connection is still alive + self.queue_request(HEARTBEATREQUEST, str(noactivity)) continue except ConnectionClosed: break + noactivity = 0 action, ident, data = decode_msg(reply) if ident == '.': ident = None diff --git a/secop/lib/asynconn.py b/secop/lib/asynconn.py index b874b31..3c45718 100644 --- a/secop/lib/asynconn.py +++ b/secop/lib/asynconn.py @@ -159,14 +159,20 @@ class AsynTcp(AsynConn): return b''.join(data) def recv(self): - """return bytes received within 1 sec""" + """return bytes in the recv buffer + + or bytes received within 1 sec + """ try: data = self.connection.recv(8192) if data: return data - except socket.timeout: + except (socket.timeout, TimeoutError): # timeout while waiting return b'' + # note that when no data is sent on a connection, an interruption might + # not be detected within a reasonable time. sending a heartbeat should + # help in this case. raise ConnectionClosed() # marks end of connection diff --git a/secop/protocol/interface/tcp.py b/secop/protocol/interface/tcp.py index 3f1a3fa..d4f6267 100644 --- a/secop/protocol/interface/tcp.py +++ b/secop/protocol/interface/tcp.py @@ -145,7 +145,7 @@ class TCPRequestHandler(socketserver.BaseRequestHandler): 'traceback': formatExtendedStack()}]) except Exception as err: # create Error Obj instead - result = (ERRORPREFIX + msg[0], msg[1], ['InternalError', str(err), + result = (ERRORPREFIX + msg[0], msg[1], ['InternalError', repr(err), {'exception': formatException(), 'traceback': formatExtendedStack()}]) print('--------------------') diff --git a/secop/server.py b/secop/server.py index 47384ed..4dae2f3 100644 --- a/secop/server.py +++ b/secop/server.py @@ -100,8 +100,8 @@ class Server: cfgdict = self.loadCfgFile(filename) ambiguous_sections |= set(merged_cfg) & set(cfgdict) merged_cfg.update(cfgdict) - self.node_cfg = merged_cfg.pop('NODE') - self.interface_cfg = merged_cfg.pop('INTERFACE') + self.node_cfg = merged_cfg.pop('NODE', {}) + self.interface_cfg = merged_cfg.pop('INTERFACE', {}) self.module_cfg = merged_cfg if interface: ambiguous_sections.discard('interface') @@ -109,6 +109,8 @@ class Server: self.node_cfg['name'] = name self.node_cfg['id'] = cfgfiles self.interface_cfg['uri'] = str(interface) + elif 'uri' not in self.interface_cfg: + raise ConfigError('missing interface uri') if ambiguous_sections: self.log.warning('ambiguous sections in %s: %r' % (cfgfiles, tuple(ambiguous_sections))) self._cfgfiles = cfgfiles diff --git a/secop/stringio.py b/secop/stringio.py index cb6bd9f..00d845c 100644 --- a/secop/stringio.py +++ b/secop/stringio.py @@ -208,7 +208,7 @@ class HasIodev(Module): opts = {'uri': self.uri, 'description': 'communication device for %s' % name, 'export': False} ioname = name + '_iodev' - iodev = self.iodevClass(ioname, self.log.getChild(ioname), opts, srv) + iodev = self.iodevClass(ioname, srv.log.getChild(ioname), opts, srv) srv.modules[ioname] = iodev self.setProperty('iodev', ioname)