Attachment 7218 Details for Bug 37402 – Improve multi-threaded access in UCR

[patch] Improve multi-threaded access in UCR

37402_ucr-umc-threading.diff (text/plain), 13.23 KB, created by Philipp Hahn on 2015-10-21 14:07:13 CEST

(hide)

Description:

Filename:

MIME Type:

Creator: Philipp Hahn

Created: 2015-10-21 14:07:13 CEST

Size: 13.23 KB

patch

obsolete

>From bd3919dc69b207a5b8b883380abef0c69f2b227d Mon Sep 17 00:00:00 2001
>Message-Id: <bd3919dc69b207a5b8b883380abef0c69f2b227d.1445429124.git.hahn@univention.de>
>From: Philipp Hahn <hahn@univention.de>
>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 <hahn@univention.de>  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: <bd3919dc69b207a5b8b883380abef0c69f2b227d.1445429124.git.hahn@univention.de>
>References: <bd3919dc69b207a5b8b883380abef0c69f2b227d.1445429124.git.hahn@univention.de>
>From: Philipp Hahn <hahn@univention.de>
>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
>

Actions: View | Diff

Attachments on bug 37402: 7218