From eb42e7fcd7bb67ab951c9bc6c80a78cd23011458 Mon Sep 17 00:00:00 2001 From: Vishvananda Ishaya Date: Fri, 16 Mar 2012 13:25:05 -0700 Subject: Workaround issue with greenthreads and lockfiles * Adds a GreenLockFile that always works in greenthreads * Adds test to verify that regular Lockfile is broken * Adds test to verify that GreenLockfile works * Adds note about limitation of external locks * Adds test showing limitation of nested locks * Fixes bug 956313 Change-Id: I11cd1206611aa4862dadd2fcc077c4c2e0f798f6 --- nova/tests/test_misc.py | 16 ++++++++++++++++ nova/tests/test_utils.py | 39 +++++++++++++++++++++++++++++++++++++++ nova/utils.py | 44 +++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 98 insertions(+), 1 deletion(-) diff --git a/nova/tests/test_misc.py b/nova/tests/test_misc.py index 0baf38236..63c2773bf 100644 --- a/nova/tests/test_misc.py +++ b/nova/tests/test_misc.py @@ -22,6 +22,7 @@ import select from eventlet import greenpool from eventlet import greenthread +import lockfile from nova import exception from nova import test @@ -134,6 +135,21 @@ class LockTestCase(test.TestCase): self.assertEqual(saved_sem_num, len(utils._semaphores), "Semaphore leak detected") + def test_nested_external_fails(self): + """We can not nest external syncs""" + + @utils.synchronized('testlock1', external=True) + def outer_lock(): + + @utils.synchronized('testlock2', external=True) + def inner_lock(): + pass + inner_lock() + try: + self.assertRaises(lockfile.NotMyLock, outer_lock) + finally: + utils.cleanup_file_locks() + def test_synchronized_externally(self): """We can lock across multiple processes""" rpipe1, wpipe1 = os.pipe() diff --git a/nova/tests/test_utils.py b/nova/tests/test_utils.py index f29da5466..501ff91d0 100644 --- a/nova/tests/test_utils.py +++ b/nova/tests/test_utils.py @@ -24,7 +24,10 @@ import shutil import StringIO import tempfile +import eventlet +from eventlet import greenpool import iso8601 +import lockfile import mox import nova @@ -832,6 +835,42 @@ class Iso8601TimeTest(test.TestCase): self._instaneous(normed, 2012, 2, 13, 23, 53, 07, 0) +class TestGreenLocks(test.TestCase): + def test_concurrent_green_lock_succeeds(self): + """Verify spawn_n greenthreads with two locks run concurrently. + + This succeeds with spawn but fails with spawn_n because lockfile + gets the same thread id for both spawn_n threads. Our workaround + of using the GreenLockFile will work even if the issue is fixed. + """ + self.completed = False + with utils.tempdir() as tmpdir: + + def locka(wait): + a = utils.GreenLockFile(os.path.join(tmpdir, 'a')) + a.acquire() + wait.wait() + a.release() + self.completed = True + + def lockb(wait): + b = utils.GreenLockFile(os.path.join(tmpdir, 'b')) + b.acquire() + wait.wait() + b.release() + + wait1 = eventlet.event.Event() + wait2 = eventlet.event.Event() + pool = greenpool.GreenPool() + pool.spawn_n(locka, wait1) + pool.spawn_n(lockb, wait2) + wait2.send() + eventlet.sleep(0) + wait1.send() + pool.waitall() + self.assertTrue(self.completed) + + class TestLockCleanup(test.TestCase): """unit tests for utils.cleanup_file_locks()""" diff --git a/nova/utils.py b/nova/utils.py index e375f11e5..a23feea73 100644 --- a/nova/utils.py +++ b/nova/utils.py @@ -36,12 +36,14 @@ import socket import struct import sys import tempfile +import threading import time import types import uuid import warnings from xml.sax import saxutils +from eventlet import corolocal from eventlet import event from eventlet import greenthread from eventlet import semaphore @@ -838,6 +840,33 @@ else: anyjson.force_implementation("nova.utils") +class GreenLockFile(lockfile.FileLock): + """Implementation of lockfile that allows for a lock per greenthread. + + Simply implements lockfile:LockBase init with an addiontall suffix + on the unique name of the greenthread identifier + """ + def __init__(self, path, threaded=True): + self.path = path + self.lock_file = os.path.abspath(path) + ".lock" + self.hostname = socket.gethostname() + self.pid = os.getpid() + if threaded: + t = threading.current_thread() + # Thread objects in Python 2.4 and earlier do not have ident + # attrs. Worm around that. + ident = getattr(t, "ident", hash(t)) + gident = corolocal.get_ident() + self.tname = "-%x-%x" % (ident & 0xffffffff, gident & 0xffffffff) + else: + self.tname = "" + dirname = os.path.dirname(self.lock_file) + self.unique_name = os.path.join(dirname, + "%s%s.%s" % (self.hostname, + self.tname, + self.pid)) + + _semaphores = {} @@ -869,6 +898,19 @@ def synchronized(name, external=False): a method decorated with @synchronized('mylock', external=True), only one of them will execute at a time. + Important limitation: you can only have one external lock running per + thread at a time. For example the following will fail: + + @utils.synchronized('testlock1', external=True) + def outer_lock(): + + @utils.synchronized('testlock2', external=True) + def inner_lock(): + pass + inner_lock() + + outer_lock() + """ def wrap(f): @@ -893,7 +935,7 @@ def synchronized(name, external=False): {'lock': name, 'method': f.__name__}) lock_file_path = os.path.join(FLAGS.lock_path, 'nova-%s' % name) - lock = lockfile.FileLock(lock_file_path) + lock = GreenLockFile(lock_file_path) with lock: LOG.debug(_('Got file lock "%(lock)s" for ' 'method "%(method)s"...') % -- cgit