From d7aee2352c023401b70e4cd930cde5c08a9374d5 Mon Sep 17 00:00:00 2001 From: Ben Nemec Date: Thu, 25 Jul 2013 10:56:55 -0500 Subject: [PATCH] Sync lockutils from Oslo 90b6a65 Fix locking bug 27d4b41 Move synchronized body to a first-class function 15c17fb Make lock_file_prefix optional 1a2df89 Enable H302 hacking check b41862d Use param keyword for docstrings Fixes bug 1065531 And bug 1162047 Change-Id: Ide79292fae6f779ecd4ac166d68c8f10ca728409 --- etc/nova/nova.conf.sample | 3 +- nova/openstack/common/lockutils.py | 185 ++++++++++++++--------------- 2 files changed, 92 insertions(+), 96 deletions(-) diff --git a/etc/nova/nova.conf.sample b/etc/nova/nova.conf.sample index 9a760cf3a63..896cb65077d 100644 --- a/etc/nova/nova.conf.sample +++ b/etc/nova/nova.conf.sample @@ -1383,8 +1383,7 @@ # Whether to disable inter-process locks (boolean value) #disable_process_locking=false -# Directory to use for lock files. Default to a temp directory -# (string value) +# Directory to use for lock files. (string value) #lock_path= diff --git a/nova/openstack/common/lockutils.py b/nova/openstack/common/lockutils.py index 18cf32fb5d2..7d69f488d7c 100644 --- a/nova/openstack/common/lockutils.py +++ b/nova/openstack/common/lockutils.py @@ -16,11 +16,10 @@ # under the License. +import contextlib import errno import functools import os -import shutil -import tempfile import time import weakref @@ -28,7 +27,7 @@ from oslo.config import cfg from nova.openstack.common import fileutils -from nova.openstack.common.gettextutils import _ +from nova.openstack.common.gettextutils import _ # noqa from nova.openstack.common import local from nova.openstack.common import log as logging @@ -40,8 +39,7 @@ cfg.BoolOpt('disable_process_locking', default=False, help='Whether to disable inter-process locks'), cfg.StrOpt('lock_path', - help=('Directory to use for lock files. Default to a ' - 'temp directory')) + help=('Directory to use for lock files.')) ] @@ -135,7 +133,87 @@ def unlock(self): _semaphores = weakref.WeakValueDictionary() -def synchronized(name, lock_file_prefix, external=False, lock_path=None): +@contextlib.contextmanager +def lock(name, lock_file_prefix=None, external=False, lock_path=None): + """Context based lock + + This function yields a `semaphore.Semaphore` instance unless external is + True, in which case, it'll yield an InterProcessLock instance. + + :param lock_file_prefix: The lock_file_prefix argument is used to provide + lock files on disk with a meaningful prefix. + + :param external: The external keyword argument denotes whether this lock + 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. + """ + # NOTE(soren): If we ever go natively threaded, this will be racy. + # See http://stackoverflow.com/questions/5390569/dyn + # amically-allocating-and-destroying-mutexes + sem = _semaphores.get(name, semaphore.Semaphore()) + if name not in _semaphores: + # this check is not racy - we're already holding ref locally + # so GC won't remove the item and there was no IO switch + # (only valid in greenthreads) + _semaphores[name] = sem + + with sem: + LOG.debug(_('Got semaphore "%(lock)s"'), {'lock': name}) + + # NOTE(mikal): I know this looks odd + if not hasattr(local.strong_store, 'locks_held'): + local.strong_store.locks_held = [] + local.strong_store.locks_held.append(name) + + try: + if external and not CONF.disable_process_locking: + LOG.debug(_('Attempting to grab file 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 + if not local_lock_path: + raise cfg.RequiredOptError('lock_path') + + if not os.path.exists(local_lock_path): + fileutils.ensure_tree(local_lock_path) + LOG.info(_('Created lock path: %s'), local_lock_path) + + def add_prefix(name, prefix): + if not prefix: + return name + sep = '' if prefix.endswith('-') else '-' + return '%s%s%s' % (prefix, sep, name) + + # NOTE(mikal): the lock name cannot contain directory + # separators + lock_file_name = add_prefix(name.replace(os.sep, '_'), + lock_file_prefix) + + lock_file_path = os.path.join(local_lock_path, lock_file_name) + + try: + lock = InterProcessLock(lock_file_path) + with lock as lock: + LOG.debug(_('Got file lock "%(lock)s" at %(path)s'), + {'lock': name, 'path': lock_file_path}) + yield lock + finally: + LOG.debug(_('Released file lock "%(lock)s" at %(path)s'), + {'lock': name, 'path': lock_file_path}) + else: + yield sem + + finally: + local.strong_store.locks_held.remove(name) + + +def synchronized(name, lock_file_prefix=None, external=False, lock_path=None): """Synchronization decorator. Decorating a method like so:: @@ -157,99 +235,18 @@ def bar(self, *args): ... This way only one of either foo or bar can be executing at a time. - - :param lock_file_prefix: The lock_file_prefix argument is used to provide - lock files on disk with a meaningful prefix. The prefix should end with a - hyphen ('-') if specified. - - :param external: The external keyword argument denotes whether this lock - 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. """ def wrap(f): @functools.wraps(f) def inner(*args, **kwargs): - # NOTE(soren): If we ever go natively threaded, this will be racy. - # See http://stackoverflow.com/questions/5390569/dyn - # amically-allocating-and-destroying-mutexes - sem = _semaphores.get(name, semaphore.Semaphore()) - if name not in _semaphores: - # this check is not racy - we're already holding ref locally - # so GC won't remove the item and there was no IO switch - # (only valid in greenthreads) - _semaphores[name] = sem - - with sem: - LOG.debug(_('Got semaphore "%(lock)s" for method ' - '"%(method)s"...'), {'lock': name, - 'method': f.__name__}) - - # NOTE(mikal): I know this looks odd - if not hasattr(local.strong_store, 'locks_held'): - local.strong_store.locks_held = [] - local.strong_store.locks_held.append(name) - - try: - if external and not CONF.disable_process_locking: - LOG.debug(_('Attempting to grab file lock "%(lock)s" ' - 'for method "%(method)s"...'), - {'lock': name, 'method': f.__name__}) - cleanup_dir = False - - # We need a copy of lock_path because it is non-local - local_lock_path = lock_path - if not local_lock_path: - local_lock_path = CONF.lock_path - - if not local_lock_path: - cleanup_dir = True - local_lock_path = tempfile.mkdtemp() - - if not os.path.exists(local_lock_path): - fileutils.ensure_tree(local_lock_path) - - # NOTE(mikal): the lock name cannot contain directory - # separators - safe_name = name.replace(os.sep, '_') - lock_file_name = '%s%s' % (lock_file_prefix, safe_name) - lock_file_path = os.path.join(local_lock_path, - lock_file_name) - - try: - lock = InterProcessLock(lock_file_path) - with lock: - LOG.debug(_('Got file lock "%(lock)s" at ' - '%(path)s for method ' - '"%(method)s"...'), - {'lock': name, - 'path': lock_file_path, - 'method': f.__name__}) - retval = f(*args, **kwargs) - finally: - LOG.debug(_('Released file lock "%(lock)s" at ' - '%(path)s for method "%(method)s"...'), - {'lock': name, - 'path': lock_file_path, - 'method': f.__name__}) - # NOTE(vish): This removes the tempdir if we needed - # to create one. This is used to - # cleanup the locks left behind by unit - # tests. - if cleanup_dir: - shutil.rmtree(local_lock_path) - else: - retval = f(*args, **kwargs) - - finally: - local.strong_store.locks_held.remove(name) + with lock(name, lock_file_prefix, external, lock_path): + LOG.debug(_('Got semaphore / lock "%(function)s"'), + {'function': f.__name__}) + return f(*args, **kwargs) - return retval + LOG.debug(_('Semaphore / lock released "%(function)s"'), + {'function': f.__name__}) return inner return wrap @@ -273,7 +270,7 @@ def bar(self, *args): ... The lock_file_prefix argument is used to provide lock files on disk with a - meaningful prefix. The prefix should end with a hyphen ('-') if specified. + meaningful prefix. """ return functools.partial(synchronized, lock_file_prefix=lock_file_prefix)