diff options
| -rw-r--r-- | nova/hacking/checks.py | 29 | ||||
| -rw-r--r-- | nova/tests/fixtures/nova.py | 21 | ||||
| -rw-r--r-- | nova/tests/unit/test_hacking.py | 20 | ||||
| -rw-r--r-- | nova/utils.py | 35 | ||||
| -rw-r--r-- | tox.ini | 1 |
5 files changed, 86 insertions, 20 deletions
diff --git a/nova/hacking/checks.py b/nova/hacking/checks.py index 28fbb47a90..abe6c0ab17 100644 --- a/nova/hacking/checks.py +++ b/nova/hacking/checks.py @@ -136,6 +136,10 @@ mock_class_aliasing_re = re.compile( # Regex for catching aliasing mock.Mock class in test mock_class_as_new_value_in_patching_re = re.compile( r"mock\.patch(\.object)?.* new=mock\.(Magic|NonCallable)?Mock[^(]") +# Regex for direct use of oslo.concurrency lockutils.ReaderWriterLock +rwlock_re = re.compile( + r"(?P<module_part>(oslo_concurrency\.)?(lockutils|fasteners))" + r"\.ReaderWriterLock\(.*\)") class BaseASTChecker(ast.NodeVisitor): @@ -1001,3 +1005,28 @@ def do_not_use_mock_class_as_new_mock_value(logical_line, filename): "leak out from the test and can cause interference. " "Use new=mock.Mock() or new_callable=mock.Mock instead." ) + + +@core.flake8ext +def check_lockutils_rwlocks(logical_line): + """Check for direct use of oslo.concurrency lockutils.ReaderWriterLock() + + oslo.concurrency lockutils uses fasteners.ReaderWriterLock to provide + read/write locks and fasteners calls threading.current_thread() to track + and identify lock holders and waiters. The eventlet implementation of + current_thread() only supports greenlets of type GreenThread, else it falls + back on the native threading.current_thread() method. + + See https://github.com/eventlet/eventlet/issues/731 for details. + + N369 + """ + msg = ("N369: %(module)s.ReaderWriterLock() does not " + "function correctly with eventlet patched code. " + "Use nova.utils.ReaderWriterLock() instead.") + match = re.match(rwlock_re, logical_line) + if match: + yield ( + 0, + msg % {'module': match.group('module_part')} + ) diff --git a/nova/tests/fixtures/nova.py b/nova/tests/fixtures/nova.py index 4d2b955e21..236c678b72 100644 --- a/nova/tests/fixtures/nova.py +++ b/nova/tests/fixtures/nova.py @@ -17,7 +17,6 @@ """Fixtures for Nova tests.""" import collections -import contextlib from contextlib import contextmanager import functools import logging as std_logging @@ -29,7 +28,6 @@ import fixtures import futurist import mock from openstack import service_description -from oslo_concurrency import lockutils from oslo_config import cfg from oslo_db import exception as db_exc from oslo_db.sqlalchemy import enginefacade @@ -407,24 +405,7 @@ class CellDatabases(fixtures.Fixture): # to point to a cell, we need to take an exclusive lock to # prevent any other calls to get_context_manager() until we # reset to the default. - # NOTE(melwitt): As of fasteners >= 0.15, the workaround code to use - # eventlet.getcurrent if eventlet patching is detected has been removed - # and threading.current_thread is being used instead. Although we are - # running in a greenlet in our test environment, we are not running in - # a greenlet of type GreenThread. A GreenThread is created by calling - # eventlet.spawn and spawn is not used to run our tests. At the time of - # this writing, the eventlet patched threading.current_thread method - # falls back to the original unpatched current_thread method if it is - # not called from a GreenThead [1] and that breaks our tests involving - # this fixture. - # We can work around this by patching threading.current_thread with - # eventlet.getcurrent during creation of the lock object. - # [1] https://github.com/eventlet/eventlet/blob/v0.32.0/eventlet/green/threading.py#L128 # noqa - eventlet_patched = eventlet.patcher.is_monkey_patched('thread') - with (contextlib.ExitStack() if not eventlet_patched else - fixtures.MonkeyPatch('threading.current_thread', - eventlet.getcurrent)): - self._cell_lock = lockutils.ReaderWriterLock() + self._cell_lock = utils.ReaderWriterLock() def _cache_schema(self, connection_str): # NOTE(melwitt): See the regular Database fixture for why diff --git a/nova/tests/unit/test_hacking.py b/nova/tests/unit/test_hacking.py index 1ed47e389f..03b7692217 100644 --- a/nova/tests/unit/test_hacking.py +++ b/nova/tests/unit/test_hacking.py @@ -1000,3 +1000,23 @@ class HackingTestCase(test.NoDBTestCase): self._assert_has_no_errors( code, checks.do_not_use_mock_class_as_new_mock_value, filename="nova/tests/unit/test_context.py") + + def test_check_lockutils_rwlocks(self): + code = """ + lockutils.ReaderWriterLock() + lockutils.ReaderWriterLock(condition_cls=MyClass) + oslo_concurrency.lockutils.ReaderWriterLock() + fasteners.ReaderWriterLock() + fasteners.ReaderWriterLock(condition_cls=MyClass) + """ + errors = [(x + 1, 0, 'N369') for x in range(5)] + self._assert_has_errors( + code, checks.check_lockutils_rwlocks, expected_errors=errors) + + code = """ + nova.utils.ReaderWriterLock() + utils.ReaderWriterLock() + utils.ReaderWriterLock(condition_cls=MyClass) + nova_utils.ReaderWriterLock() + """ + self._assert_has_no_errors(code, checks.check_lockutils_rwlocks) diff --git a/nova/utils.py b/nova/utils.py index ec5e6c9248..308cf6632b 100644 --- a/nova/utils.py +++ b/nova/utils.py @@ -29,6 +29,7 @@ import shutil import tempfile import eventlet +import fixtures from keystoneauth1 import loading as ks_loading import netaddr from openstack import connection @@ -1143,3 +1144,37 @@ def run_once(message, logger, cleanup=None): wrapper.reset = functools.partial(reset, wrapper) return wrapper return outer_wrapper + + +class ReaderWriterLock(lockutils.ReaderWriterLock): + """Wrap oslo.concurrency lockutils.ReaderWriterLock to support eventlet. + + As of fasteners >= 0.15, the workaround code to use eventlet.getcurrent() + if eventlet patching is detected has been removed and + threading.current_thread is being used instead. Although we are running in + a greenlet in our test environment, we are not running in a greenlet of + type GreenThread. A GreenThread is created by calling eventlet.spawn() and + spawn() is not used to run our tests. At the time of this writing, the + eventlet patched threading.current_thread() method falls back to the + original unpatched current_thread() method if it is not called from a + GreenThead [1] and that breaks our tests involving this fixture. + + We can work around this by patching threading.current_thread() with + eventlet.getcurrent() during creation of the lock object, if we detect we + are eventlet patched. If we are not eventlet patched, we use a no-op + context manager. + + Note: this wrapper should be used for any ReaderWriterLock because any lock + may possibly be running inside a plain greenlet created by spawn_n(). + + See https://github.com/eventlet/eventlet/issues/731 for details. + + [1] https://github.com/eventlet/eventlet/blob/v0.32.0/eventlet/green/threading.py#L128 # noqa + """ + + def __init__(self, *a, **kw): + eventlet_patched = eventlet.patcher.is_monkey_patched('thread') + mpatch = fixtures.MonkeyPatch( + 'threading.current_thread', eventlet.getcurrent) + with mpatch if eventlet_patched else contextlib.ExitStack(): + return super().__init__(*a, **kw) @@ -346,6 +346,7 @@ extension = N366 = checks:check_assert_has_calls N367 = checks:do_not_alias_mock_class N368 = checks:do_not_use_mock_class_as_new_mock_value + N369 = checks:check_lockutils_rwlocks paths = ./nova/hacking |