linse/frappy
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

avoid race conditions in read_*/write_* methods

Browse Source 
using one RLock per Module
+ init generalConfig for all tests

Change-Id: I88db6cacdb4aaac2ecd56644ccd6a3e5fd2d1cf2
Reviewed-on: https://forge.frm2.tum.de/review/c/sine2020/secop/playground/+/28005
Tested-by: Jenkins Automated Tests <pedersen+jenkins@frm2.tum.de>
Reviewed-by: Markus Zolliker <markus.zolliker@psi.ch>
This commit is contained in:
zolliker 2022-03-03 15:42:29 +01:00
parent 9858973ba1
commit 16a9550080
3 changed files with 95 additions and 80 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

125
secop/modules.py
View File

@ -25,6 +25,7 @@
import time import time
from queue import Queue, Empty from queue import Queue, Empty
import threading
from collections import OrderedDict from collections import OrderedDict
from functools import wraps from functools import wraps
@ -136,16 +137,17 @@ class HasAccessibles(HasProperties):
@wraps(rfunc) # handles __wrapped__ and __doc__ @wraps(rfunc) # handles __wrapped__ and __doc__
def new_rfunc(self, pname=pname, rfunc=rfunc): def new_rfunc(self, pname=pname, rfunc=rfunc):
try: with self.accessLock:
value = rfunc(self) try:
self.log.debug("read_%s returned %r", pname, value) value = rfunc(self)
except Exception as e: self.log.debug("read_%s returned %r", pname, value)
self.log.debug("read_%s failed with %r", pname, e) except Exception as e:
raise self.log.debug("read_%s failed with %r", pname, e)
if value is Done: raise
return getattr(self, pname) if value is Done:
setattr(self, pname, value) # important! trigger the setter return getattr(self, pname)
return value setattr(self, pname, value) # important! trigger the setter
return value
new_rfunc.poll = getattr(rfunc, 'poll', True) new_rfunc.poll = getattr(rfunc, 'poll', True)
else: else:
@ -175,18 +177,19 @@ class HasAccessibles(HasProperties):
@wraps(wfunc) # handles __wrapped__ and __doc__ @wraps(wfunc) # handles __wrapped__ and __doc__
def new_wfunc(self, value, pname=pname, wfunc=wfunc): def new_wfunc(self, value, pname=pname, wfunc=wfunc):
pobj = self.accessibles[pname] with self.accessLock:
self.log.debug('validate %r for %r', value, pname) pobj = self.accessibles[pname]
# we do not need to handle errors here, we do not self.log.debug('validate %r for %r', value, pname)
# want to make a parameter invalid, when a write failed # we do not need to handle errors here, we do not
new_value = pobj.datatype(value) # want to make a parameter invalid, when a write failed
new_value = wfunc(self, new_value) new_value = pobj.datatype(value)
self.log.debug('write_%s(%r) returned %r', pname, value, new_value) new_value = wfunc(self, new_value)
if new_value is Done: self.log.debug('write_%s(%r) returned %r', pname, value, new_value)
# setattr(self, pname, getattr(self, pname)) if new_value is Done:
return getattr(self, pname) # setattr(self, pname, getattr(self, pname))
setattr(self, pname, new_value) # important! trigger the setter return getattr(self, pname)
return new_value setattr(self, pname, new_value) # important! trigger the setter
return new_value
else: else:
def new_wfunc(self, value, pname=pname): def new_wfunc(self, value, pname=pname):
@ -293,6 +296,7 @@ class Module(HasAccessibles):
self.startModuleDone = False self.startModuleDone = False
self.remoteLogHandler = None self.remoteLogHandler = None
self.changePollinterval = Queue() # used for waiting between polls and transmit info to the thread self.changePollinterval = Queue() # used for waiting between polls and transmit info to the thread
self.accessLock = threading.RLock()
errors = [] errors = []
# handle module properties # handle module properties
@ -468,45 +472,46 @@ class Module(HasAccessibles):
def announceUpdate(self, pname, value=None, err=None, timestamp=None): def announceUpdate(self, pname, value=None, err=None, timestamp=None):
"""announce a changed value or readerror""" """announce a changed value or readerror"""
# TODO: remove readerror 'property' and replace value with exception with self.accessLock:
pobj = self.parameters[pname] # TODO: remove readerror 'property' and replace value with exception
timestamp = timestamp or time.time() pobj = self.parameters[pname]
changed = pobj.value != value timestamp = timestamp or time.time()
try: changed = pobj.value != value
# store the value even in case of error
pobj.value = pobj.datatype(value)
except Exception as e:
if isinstance(e, DiscouragedConversion):
if DiscouragedConversion.log_message:
self.log.error(str(e))
self.log.error('you may disable this behaviour by running the server with --relaxed')
DiscouragedConversion.log_message = False
if not err: # do not overwrite given error
err = e
if err:
err = secop_error(err)
if str(err) == str(pobj.readerror):
return # do call updates for repeated errors
elif not changed and timestamp < (pobj.timestamp or 0) + self.omit_unchanged_within:
# no change within short time -> omit
return
pobj.timestamp = timestamp or time.time()
pobj.readerror = err
if pobj.export:
self.DISPATCHER.announce_update(self.name, pname, pobj)
if err:
callbacks = self.errorCallbacks
arg = err
else:
callbacks = self.valueCallbacks
arg = value
cblist = callbacks[pname]
for cb in cblist:
try: try:
cb(arg) # store the value even in case of error
except Exception: pobj.value = pobj.datatype(value)
# print(formatExtendedTraceback()) except Exception as e:
pass if isinstance(e, DiscouragedConversion):
if DiscouragedConversion.log_message:
self.log.error(str(e))
self.log.error('you may disable this behaviour by running the server with --relaxed')
DiscouragedConversion.log_message = False
if not err: # do not overwrite given error
err = e
if err:
err = secop_error(err)
if str(err) == str(pobj.readerror):
return # no updates for repeated errors
elif not changed and timestamp < (pobj.timestamp or 0) + self.omit_unchanged_within:
# no change within short time -> omit
return
pobj.timestamp = timestamp or time.time()
pobj.readerror = err
if pobj.export:
self.DISPATCHER.announce_update(self.name, pname, pobj)
if err:
callbacks = self.errorCallbacks
arg = err
else:
callbacks = self.valueCallbacks
arg = value
cblist = callbacks[pname]
for cb in cblist:
try:
cb(arg)
except Exception:
# print(formatExtendedTraceback())
pass
def registerCallbacks(self, modobj, autoupdate=()): def registerCallbacks(self, modobj, autoupdate=()):
"""register callbacks to another module <modobj> """register callbacks to another module <modobj>

44
secop/rwhandler.py
View File

@ -127,11 +127,12 @@ class ReadHandler(Handler):
def wrap(self, key): def wrap(self, key):
def method(module, pname=key, func=self.func): def method(module, pname=key, func=self.func):
value = func(module, pname) with module.accessLock:
if value is Done: value = func(module, pname)
return getattr(module, pname) if value is Done:
setattr(module, pname, value) return getattr(module, pname)
return value setattr(module, pname, value)
return value
return wraps(self.func)(method) return wraps(self.func)(method)
@ -148,10 +149,11 @@ class CommonReadHandler(ReadHandler):
def wrap(self, key): def wrap(self, key):
def method(module, pname=key, func=self.func): def method(module, pname=key, func=self.func):
ret = func(module) with module.accessLock:
if ret not in (None, Done): ret = func(module)
raise ProgrammingError('a method wrapped with CommonReadHandler must not return any value') if ret not in (None, Done):
return getattr(module, pname) raise ProgrammingError('a method wrapped with CommonReadHandler must not return any value')
return getattr(module, pname)
method = wraps(self.func)(method) method = wraps(self.func)(method)
method.poll = self.poll and getattr(method, 'poll', True) if key == self.first_key else False method.poll = self.poll and getattr(method, 'poll', True) if key == self.first_key else False
@ -165,10 +167,11 @@ class WriteHandler(Handler):
def wrap(self, key): def wrap(self, key):
@wraps(self.func) @wraps(self.func)
def method(module, value, pname=key, func=self.func): def method(module, value, pname=key, func=self.func):
value = func(module, pname, value) with module.accessLock:
if value is not Done: value = func(module, pname, value)
setattr(module, pname, value) if value is not Done:
return value setattr(module, pname, value)
return value
return method return method
@ -201,13 +204,14 @@ class CommonWriteHandler(WriteHandler):
def wrap(self, key): def wrap(self, key):
@wraps(self.func) @wraps(self.func)
def method(module, value, pname=key, func=self.func): def method(module, value, pname=key, func=self.func):
values = WriteParameters(module) with module.accessLock:
values[pname] = value values = WriteParameters(module)
ret = func(module, values) values[pname] = value
if ret not in (None, Done): ret = func(module, values)
raise ProgrammingError('a method wrapped with CommonWriteHandler must not return any value') if ret not in (None, Done):
# remove pname from writeDict. this was not removed in WriteParameters, as it was not missing raise ProgrammingError('a method wrapped with CommonWriteHandler must not return any value')
module.writeDict.pop(pname, None) # remove pname from writeDict. this was not removed in WriteParameters, as it was not missing
module.writeDict.pop(pname, None)
return method return method

6
test/conftest.py
View File

@ -1,6 +1,12 @@
# content of conftest.py # content of conftest.py
import pytest import pytest
from secop.lib import generalConfig
@pytest.fixture(scope="session", autouse=True)
def general_config():
generalConfig.testinit()
@pytest.fixture(scope="module") @pytest.fixture(scope="module")