Add wrapper for oslo.concurrency lockutils.ReaderWriterLock()

This is a follow up change to I168fffac8002f274a905cfd53ac4f6c9abe18803
which added a hackaround to enable our tests to pass with
fasteners>=0.15 which was upgraded recently as part of a
openstack/requirements update.

The ReaderWriterLock from fasteners (and thus lockutils) cannot work
correctly with eventlet patched code, so this adds a wrapper containing
the aforementioned hackaround along with a hacking check to do our best
to ensure that future use of ReaderWriterLock will be through the
wrapper.

Change-Id: Ia7bcb40a21a804c7bc6b74f501d95ce2a88b09b5

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')}
+        )

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

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)

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)

tox.ini

@@ -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