x/deb-oslo.concurrency
Code Issues Proposed changes

lockutils: remove lock_path parameter

Browse Source 
This remove the parameter lock_path so the configured lock_path is
always used instead. There's only a few places where this is used in
OpenStack, and it's not heavily justified to have it even in this
places.

This is needed to simplify the interface toward moving to IPC locking
mechanism. With such a locking mechanism in place, the lock_path
parameter has absolutely no sense. So isolating this to make it only
relevant to some part of the driver and not in the public interface
makes more sense.

Change-Id: I72bd683209090ae6e3911a709e4d213879508aca
Blueprint: lockutils-posix-ipc
This commit is contained in:
Julien Danjou
2013-12-26 14:15:27 +01:00
parent c3b4777630
commit fc8f4361de
2 changed files with 18 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files

20
openstack/common/lockutils.py
View File

@@ -138,13 +138,13 @@ _semaphores = weakref.WeakValueDictionary()
_semaphores_lock = threading.Lock()
def external_lock(name, lock_file_prefix=None, lock_path=None):
def external_lock(name, lock_file_prefix=None):
with internal_lock(name):
LOG.debug(_('Attempting to grab file lock "%(lock)s in %(lock_path)"'),
{'lock': name, 'lock_path': lock_path})
LOG.debug(_('Attempting to grab external lock "%(lock)s"'),
{'lock': name})
# We need a copy of lock_path because it is non-local
local_lock_path = lock_path or CONF.lock_path
local_lock_path = CONF.lock_path
if not local_lock_path:
raise cfg.RequiredOptError('lock_path')
@@ -177,7 +177,7 @@ def internal_lock(name):
@contextlib.contextmanager
def lock(name, lock_file_prefix=None, external=False, lock_path=None):
def lock(name, lock_file_prefix=None, external=False):
"""Context based lock
This function yields a `threading.Semaphore` instance (if we don't use
@@ -191,20 +191,16 @@ def lock(name, lock_file_prefix=None, external=False, lock_path=None):
should work across multiple processes. This means that if two different
workers both run a a method decorated with @synchronized('mylock',
external=True), only one of them will execute at a time.
:param lock_path: The lock_path keyword argument is used to specify a
special location for external lock files to live. If nothing is set, then
CONF.lock_path is used as a default.
"""
if external and not CONF.disable_process_locking:
lock = external_lock(name, lock_file_prefix, lock_path)
lock = external_lock(name, lock_file_prefix)
else:
lock = internal_lock(name)
with lock:
yield lock
def synchronized(name, lock_file_prefix=None, external=False, lock_path=None):
def synchronized(name, lock_file_prefix=None, external=False):
"""Synchronization decorator.
Decorating a method like so::
@@ -232,7 +228,7 @@ def synchronized(name, lock_file_prefix=None, external=False, lock_path=None):
@functools.wraps(f)
def inner(*args, **kwargs):
try:
with lock(name, lock_file_prefix, external, lock_path):
with lock(name, lock_file_prefix, external):
LOG.debug(_('Got semaphore / lock "%(function)s"'),
{'function': f.__name__})
return f(*args, **kwargs)

20
tests/unit/test_lockutils.py
View File

@@ -226,8 +226,9 @@ class LockTestCase(test.BaseTestCase):
def test_synchronized_without_prefix(self):
lock_dir = tempfile.mkdtemp()
self.config(lock_path=lock_dir)
@lockutils.synchronized('lock', external=True, lock_path=lock_dir)
@lockutils.synchronized('lock', external=True)
def test_without_prefix():
path = os.path.join(lock_dir, "lock")
self.assertTrue(os.path.exists(path))
@@ -240,8 +241,9 @@ class LockTestCase(test.BaseTestCase):
def test_synchronized_prefix_without_hypen(self):
lock_dir = tempfile.mkdtemp()
self.config(lock_path=lock_dir)
@lockutils.synchronized('lock', 'hypen', True, lock_dir)
@lockutils.synchronized('lock', 'hypen', True)
def test_without_hypen():
path = os.path.join(lock_dir, "hypen-lock")
self.assertTrue(os.path.exists(path))
@@ -254,6 +256,7 @@ class LockTestCase(test.BaseTestCase):
def test_contextlock(self):
lock_dir = tempfile.mkdtemp()
self.config(lock_path=lock_dir)
try:
# Note(flaper87): Lock is not external, which means
@@ -263,14 +266,12 @@ class LockTestCase(test.BaseTestCase):
# NOTE(flaper87): Lock is external so an InterProcessLock
# will be yielded.
with lockutils.lock("test2", external=True,
lock_path=lock_dir):
with lockutils.lock("test2", external=True):
path = os.path.join(lock_dir, "test2")
self.assertTrue(os.path.exists(path))
with lockutils.lock("test1",
external=True,
lock_path=lock_dir) as lock1:
external=True) as lock1:
self.assertTrue(isinstance(lock1,
lockutils.InterProcessLock))
finally:
@@ -279,6 +280,7 @@ class LockTestCase(test.BaseTestCase):
def test_contextlock_unlocks(self):
lock_dir = tempfile.mkdtemp()
self.config(lock_path=lock_dir)
sem = None
@@ -286,14 +288,12 @@ class LockTestCase(test.BaseTestCase):
with lockutils.lock("test") as sem:
self.assertTrue(isinstance(sem, threading._Semaphore))
with lockutils.lock("test2", external=True,
lock_path=lock_dir):
with lockutils.lock("test2", external=True):
path = os.path.join(lock_dir, "test2")
self.assertTrue(os.path.exists(path))
# NOTE(flaper87): Lock should be free
with lockutils.lock("test2", external=True,
lock_path=lock_dir):
with lockutils.lock("test2", external=True):
path = os.path.join(lock_dir, "test2")
self.assertTrue(os.path.exists(path))