diff --git a/nova/tests/unit/test_utils.py b/nova/tests/unit/test_utils.py index 55ee29b7fa..6d3f70e136 100644 --- a/nova/tests/unit/test_utils.py +++ b/nova/tests/unit/test_utils.py @@ -21,6 +21,7 @@ import threading import time from unittest import mock +import fasteners import fixtures from keystoneauth1 import adapter as ks_adapter from keystoneauth1.identity import base as ks_identity @@ -1997,22 +1998,81 @@ class TestFairLockGuard(test.NoDBTestCase): self.assertTrue(test_locks[0].is_writer()) self.assertTrue(test_locks[1].is_writer()) - # attempting to nest the same context manager instance - # should raise a TypeError. + # Same instance nesting now works - locks are already held, + # so nested entry just increments depth counter. with lock_guard: self.assertTrue(lock_guard.is_locked()) self.assertTrue(test_locks[0].is_writer()) self.assertTrue(test_locks[1].is_writer()) - with self.assertRaisesRegex( - TypeError, - "Cannot enter FairLockGuard while it is already active."): - with lock_guard: - pass - # after the TypeError, the outer context should still - # be active. + with lock_guard: + # Still locked in nested context + self.assertTrue(lock_guard.is_locked()) + self.assertTrue(test_locks[0].is_writer()) + self.assertTrue(test_locks[1].is_writer()) + # Still locked after inner exit self.assertTrue(lock_guard.is_locked()) self.assertTrue(test_locks[0].is_writer()) self.assertTrue(test_locks[1].is_writer()) + # Released after outer exit + self.assertFalse(lock_guard.is_locked()) + self.assertFalse(test_locks[0].is_writer()) + self.assertFalse(test_locks[1].is_writer()) + + @mock.patch.object(utils, 'NOVA_FAIR_LOCKS') + def test_deep_nesting(self, mock_fair_locks): + """Test that nesting works correctly at 3+ levels deep.""" + test_lock = fasteners.ReaderWriterLock() + mock_fair_locks.get.return_value = test_lock + lock_guard = utils.FairLockGuard(['deep-test']) + + with lock_guard: + self.assertTrue(lock_guard.is_locked()) + with lock_guard: + self.assertTrue(lock_guard.is_locked()) + with lock_guard: + # Third level of nesting + self.assertTrue(lock_guard.is_locked()) + self.assertTrue(test_lock.is_writer()) + # Still locked after level 3 exit + self.assertTrue(lock_guard.is_locked()) + # Still locked after level 2 exit + self.assertTrue(lock_guard.is_locked()) + # Released after outermost exit + self.assertFalse(lock_guard.is_locked()) + self.assertFalse(test_lock.is_writer()) + + @mock.patch.object(utils, 'NOVA_FAIR_LOCKS') + def test_nested_exception_outer_still_holds_locks(self, mock_fair_locks): + """Test that outer context retains locks when inner context raises.""" + test_lock = fasteners.ReaderWriterLock() + mock_fair_locks.get.return_value = test_lock + lock_guard = utils.FairLockGuard(['exception-test']) + + with lock_guard: + self.assertTrue(lock_guard.is_locked()) + try: + with lock_guard: + self.assertTrue(lock_guard.is_locked()) + raise ValueError("Test exception") + except ValueError: + pass + # Outer context still holds locks after inner raised + self.assertTrue(lock_guard.is_locked()) + self.assertTrue(test_lock.is_writer()) + # Released after outer exit + self.assertFalse(lock_guard.is_locked()) + self.assertFalse(test_lock.is_writer()) + + @mock.patch.object(utils, 'NOVA_FAIR_LOCKS') + def test_empty_lock_list(self, mock_fair_locks): + """Test FairLockGuard with empty lock list.""" + lock_guard = utils.FairLockGuard([]) + + with lock_guard: + # No locks to acquire, but should still work + self.assertFalse(lock_guard.is_locked()) + + mock_fair_locks.get.assert_not_called() class StaticallyDelayingCancellableTaskExecutorWrapperTest(test.NoDBTestCase): diff --git a/nova/utils.py b/nova/utils.py index fb02373243..0fd4d99646 100644 --- a/nova/utils.py +++ b/nova/utils.py @@ -1172,7 +1172,7 @@ def latch_error_on_raise(retryable=(_SentinelException,)): class FairLockGuard: """A lock guard context manager - This class support acquiring multiple locks safely by name + This class supports acquiring multiple locks safely by name and releasing them via the context manager protocol i.e. with FairLockGuard([list of lock names]): @@ -1181,33 +1181,42 @@ class FairLockGuard: the locks and passing them in. Lock creation and management is handled entirely internally. - If you are using this between threads, Thread-A and Thread-B - should both create there own context manager instead of sharing - a single context manager between treads. + Thread Safety: + While sharing a FairLockGuard instance between threads is + supported, it is discouraged. Each thread should create its + own context manager instance with the same set of lock names + for clarity and to avoid confusion about lock ownership. If + shared, threads will correctly wait for each other due to the + underlying fair locks. - Nesting is supported by creating a new context manager instance - for each nested context with the same or different lock names - as the outer context. Attempting to nest the same context manager - instance should raise a TypeError. + Nesting: + Same-instance nesting is supported but discouraged. When the + same thread re-enters an active context manager, the nesting + depth is tracked and locks are only released on the outermost + exit. For clarity, prefer creating separate FairLockGuard + instances for nested contexts. - Example Valid Usage: + Example Recommended Usage: ``` - with FairLockGuard(['lock1', 'lock2']) as lock_guard: - with FairLockGuard(['lock1', 'lock2']) as lock_guard2: + # Each context creates its own guard - clear ownership + with FairLockGuard(['lock1', 'lock2']): + with FairLockGuard(['lock1', 'lock2']): pass ``` - Example Invalid Usage: + Example Discouraged (but supported) Usage: ``` - with FairLockGuard(['lock1', 'lock2']) as lock_guard: - with lock_guard: - pass + # Same instance nesting - works but less clear + lock_guard = FairLockGuard(['lock1', 'lock2']) + with lock_guard: + with lock_guard: # Increments nesting depth + pass # Decrements but doesn't release + pass # Still holds locks + # Locks released here ``` - This will raise a TypeError because the same context manager instance - is being nested. In general you should avoid naming the context manager instance - and only construct it in the with statement to avoid incorrect usage. + and only construct it in the with statement to keep usage clear. """ def __init__(self, names): @@ -1227,33 +1236,87 @@ class FairLockGuard: # exclusive access for state modifications. self.locks_lock = lockutils.ReaderWriterLock() self._active = False + self._active_thread = None + self._nesting_depth = 0 + + @staticmethod + def _release_locks(locks): + for lock in locks: + if lock.is_writer(): + try: + lock.release_write_lock() + except Exception: + pass def __enter__(self): + current_thread = threading.current_thread() + + # Check for same-thread nesting - allow it by incrementing depth with self.locks_lock.write_lock(): - if self._active: - raise TypeError( - "Cannot enter FairLockGuard while it is already active. " - "Create a new instance for nested usage or wait for the " - "current context to exit.") + if self._active and self._active_thread == current_thread: + # Same thread re-entering - just increment depth. + # Locks are already held, no need to re-acquire. + self._nesting_depth += 1 + return + + # First entry (or different thread that will wait on named locks). + # IMPORTANT: Acquire named locks OUTSIDE of locks_lock to prevent + # deadlock. If we acquired named locks while holding locks_lock: + # 1. Thread-A holds locks_lock, acquires named locks, releases + # locks_lock, does work + # 2. Thread-B takes locks_lock, blocks waiting for named locks + # (still holding locks_lock) + # 3. Thread-A tries to __exit__, needs locks_lock - DEADLOCK! + # By acquiring named locks outside locks_lock, Thread-B blocks on + # named locks without holding locks_lock, allowing Thread-A to exit. + # + # NOTE: We append to acquired_locks BEFORE calling + # acquire_write_lock(). + # This ordering is intentional for exception safety: if acquire fails + # for a lock, we can still release all previously acquired locks + # (the is_writer() check ensures we only release locks that were + # actually acquired). + acquired_locks = [] + try: for name in self.names: named_lock = NOVA_FAIR_LOCKS.get(name) - self.locks.append(named_lock) - # we ensure we add it to the list - # before we acquire the lock to make sure - # we release it if there is an exception + acquired_locks.append(named_lock) named_lock.acquire_write_lock() + except Exception: + # Release any locks acquired so far on failure + self._release_locks(acquired_locks) + raise + + # Update internal state after acquiring locks + with self.locks_lock.write_lock(): + self.locks = acquired_locks self._active = True + self._active_thread = current_thread + self._nesting_depth = 1 def __exit__(self, exc_type, exc_value, traceback): + current_thread = threading.current_thread() + with self.locks_lock.write_lock(): + # Validate the exiting thread owns the lock. This handles the + # edge case where a shared guard's __exit__ is called from a + # thread that didn't successfully __enter__ (e.g., Thread-B + # calls exit while Thread-A holds nested locks). In normal + # context manager usage this shouldn't happen, but we handle + # it defensively by returning early without modifying state. + if self._active_thread != current_thread: + return + + self._nesting_depth -= 1 + if self._nesting_depth > 0: + # Still nested, don't release locks yet + return + # Release locks OUTSIDE of locks_lock to prevent holding it + # while other threads may be waiting + self._release_locks(self.locks) with self.locks_lock.write_lock(): - for lock in self.locks: - # This should always be true but since - # we add the lock to the list before we - # acquire it check anyway. - if lock.is_writer(): - lock.release_write_lock() self.locks = [] self._active = False + self._active_thread = None def is_locked(self): # NOTE(sean-k-mooney): LockGuards exist in several programming