From bd3919dc69b207a5b8b883380abef0c69f2b227d Mon Sep 17 00:00:00 2001 Message-Id: From: Philipp Hahn Date: Wed, 21 Oct 2015 08:26:42 +0200 Subject: [PATCH 1/2] Bug #37402 UCR: Make boolean handling more robust. Organization: Univention GmbH, Bremen, Germany The underlying problem looks like a multi-threading bug: > File "%PY2.7%/univention/management/console/modules/udm/__init__.py", line 532, in _thread For now lets add a work-around: 1. Switch "if key" to "if value is None" to protect against "key" and "value" both being "None". 2. Move the ".lower()" to the later evaluation to protect against ".get()" returning "None". This changes the API, but should be more consistent. 3. Fix unit test to return more useful message on errors. --- .../univention-config-registry/debian/changelog | 6 ++++++ .../python/univention/config_registry/backend.py | 19 ++++++++-------- .../tests/test_backend.py | 25 +++++++++++----------- 3 files changed, 28 insertions(+), 22 deletions(-) diff --git a/branches/ucs-4.1/ucs-4.1-0/base/univention-config-registry/debian/changelog b/branches/ucs-4.1/ucs-4.1-0/base/univention-config-registry/debian/changelog index ff0b0b3..b54f48c 100644 --- a/branches/ucs-4.1/ucs-4.1-0/base/univention-config-registry/debian/changelog +++ b/branches/ucs-4.1/ucs-4.1-0/base/univention-config-registry/debian/changelog @@ -1,3 +1,9 @@ +univention-config-registry (11.0.0-2) unstable; urgency=low + + * Bug #37402: Make boolean handling more robust. + + -- Philipp Hahn Wed, 21 Oct 2015 08:09:46 +0200 + univention-config-registry (11.0.0-1) unstable; urgency=medium * Bump version for UCS 4.1 (Bug #38011) diff --git a/branches/ucs-4.1/ucs-4.1-0/base/univention-config-registry/python/univention/config_registry/backend.py b/branches/ucs-4.1/ucs-4.1-0/base/univention-config-registry/python/univention/config_registry/backend.py index 5abc042..1991c09 100644 --- a/branches/ucs-4.1/ucs-4.1-0/base/univention-config-registry/python/univention/config_registry/backend.py +++ b/branches/ucs-4.1/ucs-4.1-0/base/univention-config-registry/python/univention/config_registry/backend.py @@ -245,21 +245,19 @@ class ConfigRegistry(dict): def is_true(self, key=None, default=False, value=None): """Return if the strings value of key is considered as true.""" - if key: - if key in self: - value = self.get(key).lower() # pylint: disable-msg=E1103 - else: + if value is None: + value = self.get(key) + if value is None: return default - return value in ('yes', 'true', '1', 'enable', 'enabled', 'on') + return value.lower() in ('yes', 'true', '1', 'enable', 'enabled', 'on') def is_false(self, key=None, default=False, value=None): """Return if the strings value of key is considered as false.""" - if key: - if key in self: - value = self.get(key).lower() # pylint: disable-msg=E1103 - else: + if value is None: + value = self.get(key) + if value is None: return default - return value in ('no', 'false', '0', 'disable', 'disabled', 'off') + return value.lower() in ('no', 'false', '0', 'disable', 'disabled', 'off') def update(self, changes): """ @@ -281,6 +279,7 @@ class ConfigRegistry(dict): changed[key] = (old_value, new_value) return changed + class _ConfigRegistry(dict): """ Persistent value store. diff --git a/branches/ucs-4.1/ucs-4.1-0/base/univention-config-registry/tests/test_backend.py b/branches/ucs-4.1/ucs-4.1-0/base/univention-config-registry/tests/test_backend.py index c50adec..d90c80f 100644 --- a/branches/ucs-4.1/ucs-4.1-0/base/univention-config-registry/tests/test_backend.py +++ b/branches/ucs-4.1/ucs-4.1-0/base/univention-config-registry/tests/test_backend.py @@ -10,6 +10,7 @@ import time sys.path.insert(0, os.path.join(os.path.dirname(__file__), os.path.pardir, 'python')) from univention.config_registry.backend import ConfigRegistry + class TestConfigRegistry(unittest.TestCase): """Unit test for univention.config_registry.backend.ConfigRegistry""" def setUp(self): @@ -219,25 +220,25 @@ class TestConfigRegistry(unittest.TestCase): """Test valid is_true().""" ucr = ConfigRegistry() for ucr['foo'] in ('YES', 'yes', 'Yes', 'true', '1', 'enable', 'enabled', 'on'): - self.assertTrue(ucr.is_true('foo')) + self.assertTrue(ucr.is_true('foo'), 'is_true(%(foo)r)' % ucr) def test_is_true_invalid(self): """Test invalid is_true().""" ucr = ConfigRegistry() for ucr['foo'] in ('yes ', ' yes', ''): - self.assertFalse(ucr.is_true('foo')) + self.assertFalse(ucr.is_true('foo'), 'is_true(%(foo)r)' % ucr) def test_is_true_valid_direct(self): """Test valid is_true(value).""" ucr = ConfigRegistry() - for value in ('yes', 'true', '1', 'enable', 'enabled', 'on'): - self.assertTrue(ucr.is_true(value=value)) + for value in ('YES', 'Yes', 'yes', 'true', '1', 'enable', 'enabled', 'on'): + self.assertTrue(ucr.is_true(value=value), 'is_true(v=%r)' % value) def test_is_true_invalid_direct(self): """Test invalid is_true(value).""" ucr = ConfigRegistry() - for value in ('YES', 'Yes', 'yes ', ' yes', ''): - self.assertFalse(ucr.is_true(value=value)) + for value in ('yes ', ' yes', ''): + self.assertFalse(ucr.is_true(value=value), 'is_true(v=%r)' % value) def test_is_false_unset(self): """Test unset is_false().""" @@ -254,25 +255,25 @@ class TestConfigRegistry(unittest.TestCase): """Test valid is_false().""" ucr = ConfigRegistry() for ucr['foo'] in ('NO', 'no', 'No', 'false', '0', 'disable', 'disabled', 'off'): - self.assertTrue(ucr.is_false('foo')) + self.assertTrue(ucr.is_false('foo'), 'is_false(%(foo)r)' % ucr) def test_is_false_invalid(self): """Test invalid is_false().""" ucr = ConfigRegistry() for ucr['foo'] in ('no ', ' no', ''): - self.assertFalse(ucr.is_false('foo')) + self.assertFalse(ucr.is_false('foo'), 'is_false(%(foo)r)' % ucr) def test_is_false_valid_direct(self): """Test valid is_false(value).""" ucr = ConfigRegistry() - for value in ('no', 'false', '0', 'disable', 'disabled', 'off'): - self.assertTrue(ucr.is_false(value=value)) + for value in ('NO', 'No', 'no', 'false', '0', 'disable', 'disabled', 'off'): + self.assertTrue(ucr.is_false(value=value), 'is_false(v=%r)' % value) def test_is_false_invalid_direct(self): """Test valid is_false(value).""" ucr = ConfigRegistry() - for value in ('NO', 'No', 'no ', ' no', ''): - self.assertFalse(ucr.is_false(value=value)) + for value in ('no ', ' no', ''): + self.assertFalse(ucr.is_false(value=value), 'is_false(v=%r)' % value) def test_update(self): """Test update().""" -- 2.1.4 From 0e5c0ae555ae3af4aef89d0113dce4d56ae9d400 Mon Sep 17 00:00:00 2001 Message-Id: <0e5c0ae555ae3af4aef89d0113dce4d56ae9d400.1445429124.git.hahn@univention.de> In-Reply-To: References: From: Philipp Hahn Date: Wed, 21 Oct 2015 12:39:19 +0200 Subject: [PATCH 2/2] Bug #37402 UCR: Improve multi-threaded load Organization: Univention GmbH, Bremen, Germany The UCR instance is cleared at the beginning of load(). If loading the old file fails or is taking a long time, a concurrent access will find the UCR being empty. 1. Make creating the file atomic; otherwise there is a (small) time-slot between the "os.path.exists()" test and the "open()" creating the file. 2. Only read two lines instead of the full file to check for validity, which is significantly slower. 3. Just clear() the dictionary before reading the new content. 4. Add a unit test to check concurrent access. - UMC doesn't do the locking, but even with it it fails. - moving the "assertEqual()" inside the locks makes the problem go away, as then a concurrent "clear()" can not get in between. --- .../base/univention-config-registry/debian/rules | 1 + .../python/univention/config_registry/backend.py | 27 ++++---- .../tests/test_backend_threading.py | 78 ++++++++++++++++++++++ 3 files changed, 92 insertions(+), 14 deletions(-) create mode 100644 branches/ucs-4.1/ucs-4.1-0/base/univention-config-registry/tests/test_backend_threading.py diff --git a/branches/ucs-4.1/ucs-4.1-0/base/univention-config-registry/debian/rules b/branches/ucs-4.1/ucs-4.1-0/base/univention-config-registry/debian/rules index 8e2dac9..f192fb7 100755 --- a/branches/ucs-4.1/ucs-4.1-0/base/univention-config-registry/debian/rules +++ b/branches/ucs-4.1/ucs-4.1-0/base/univention-config-registry/debian/rules @@ -50,6 +50,7 @@ override_dh_auto_test: ucslint python tests/test_info_tools.py python tests/test_backend.py + python tests/test_backend_threading.py python python/univention/debhelper.py python python/univention/config_registry/misc.py UNIVENTION_BASECONF=tests/base.conf python python/univention/config_registry/interfaces.py diff --git a/branches/ucs-4.1/ucs-4.1-0/base/univention-config-registry/python/univention/config_registry/backend.py b/branches/ucs-4.1/ucs-4.1-0/base/univention-config-registry/python/univention/config_registry/backend.py index 1991c09..c2f906b 100644 --- a/branches/ucs-4.1/ucs-4.1-0/base/univention-config-registry/python/univention/config_registry/backend.py +++ b/branches/ucs-4.1/ucs-4.1-0/base/univention-config-registry/python/univention/config_registry/backend.py @@ -302,15 +302,13 @@ class _ConfigRegistry(dict): def load(self): """Load sub registry from file.""" - self.clear() import_failed = False try: reg_file = open(self.file, 'r') except EnvironmentError: import_failed = True else: - if len(reg_file.readlines()) < 2: # comment or nothing - import_failed = True + import_failed = reg_file.read() == '' and reg_file.read() == '' if import_failed: try: @@ -319,7 +317,8 @@ class _ConfigRegistry(dict): return reg_file.seek(0) - for line in reg_file.readlines(): + new = {} + for line in reg_file: line = re.sub(r'^[^:]*#.*$', "", line) if line == '': continue @@ -327,23 +326,23 @@ class _ConfigRegistry(dict): continue key, value = line.split(': ', 1) - value = value.strip() - if len(value) == 0: # if variable was set without an value - value = '' - - self[key] = value + new[key] = value.strip() reg_file.close() + self.update(new) + for key in set(self.keys()) - set(new.keys()): + self.pop(key, None) + if import_failed: self.__save_file(self.file) def __create_base_conf(self): """Create sub registry file.""" - if not os.path.exists(self.file): - try: - reg_file = os.open(self.file, os.O_CREAT | os.O_RDONLY, 0644) - os.close(reg_file) - except EnvironmentError: + try: + reg_file = os.open(self.file, os.O_CREAT | os.O_EXCL | os.O_WRONLY, 0644) + os.close(reg_file) + except EnvironmentError as ex: + if ex.errno != errno.EEXIST: msg = "E: file '%s' does not exist and could not be created" print >> sys.stderr, msg % (self.file,) exception_occured() diff --git a/branches/ucs-4.1/ucs-4.1-0/base/univention-config-registry/tests/test_backend_threading.py b/branches/ucs-4.1/ucs-4.1-0/base/univention-config-registry/tests/test_backend_threading.py new file mode 100644 index 0000000..8227c43 --- /dev/null +++ b/branches/ucs-4.1/ucs-4.1-0/base/univention-config-registry/tests/test_backend_threading.py @@ -0,0 +1,78 @@ +#!/usr/bin/python +"""Unit test for univention.config_registry.backend.""" +# pylint: disable-msg=C0103,E0611,R0904 +import unittest +import os +import sys +from tempfile import mkdtemp +from shutil import rmtree +from threading import Thread, Lock +sys.path.insert(0, os.path.join(os.path.dirname(__file__), os.path.pardir, 'python')) +from univention.config_registry.backend import ConfigRegistry + + +class DummyLock(object): + def __enter__(self): + pass + + def __exit__(self, exc_type, exc_value, traceback): + pass + + +class TestConfigRegistry(unittest.TestCase): + """Unit test for univention.config_registry.backend.ConfigRegistry""" + def setUp(self): + """Create object.""" + self.work_dir = mkdtemp() + ConfigRegistry.PREFIX = self.work_dir + + def tearDown(self): + """Destroy object.""" + # os.kill(os.getpid(), 19) # signal.SIGSTOP + rmtree(self.work_dir) + + def test_threading(self): + """Multiple threads accessing same registry.""" + DO_LOCKING = True + THREADS = 10 + ITERATIONS = 1000 + BASE, PRIME = 7, 23 + KEY = 'x' * PRIME + + SKEY, SVALUE = 'always', 'there' + ucr = ConfigRegistry() + ucr[SKEY] = SVALUE + ucr.save() + + lock = Lock() if DO_LOCKING else DummyLock() + + def run(tid): + for iteration in xrange(ITERATIONS): + i = tid + iteration + random = pow(BASE, i, PRIME) + key = KEY[:random + 1] + + with lock: + ucr.load() + self.assertEqual(ucr[SKEY], SVALUE, 'tid=%d iter=%d %r' % (tid, iteration, ucr.items())) + + try: + del ucr[key] + except LookupError: + ucr[key] = '%d %d' % (tid, iteration) + if i % 10 == 0 and tid % 10 == 0: + with lock: + ucr.save() + + threads = [] + for tid in xrange(THREADS): + thread = Thread(target=run, name='%d' % tid, args=(tid,)) + threads.append(thread) + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + +if __name__ == '__main__': + unittest.main() -- 2.1.4