Merge "Add wrapper for oslo.concurrency lockutils.ReaderWriterLock()"

This commit is contained in:
Zuul 2022-01-12 15:56:17 +00:00 committed by Gerrit Code Review
commit ea3945f71c
5 changed files with 86 additions and 20 deletions

View File

@ -136,6 +136,10 @@ mock_class_aliasing_re = re.compile(
# Regex for catching aliasing mock.Mock class in test # Regex for catching aliasing mock.Mock class in test
mock_class_as_new_value_in_patching_re = re.compile( mock_class_as_new_value_in_patching_re = re.compile(
r"mock\.patch(\.object)?.* new=mock\.(Magic|NonCallable)?Mock[^(]") 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): 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. " "leak out from the test and can cause interference. "
"Use new=mock.Mock() or new_callable=mock.Mock instead." "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')}
)

View File

@ -17,7 +17,6 @@
"""Fixtures for Nova tests.""" """Fixtures for Nova tests."""
import collections import collections
import contextlib
from contextlib import contextmanager from contextlib import contextmanager
import functools import functools
import logging as std_logging import logging as std_logging
@ -29,7 +28,6 @@ import fixtures
import futurist import futurist
import mock import mock
from openstack import service_description from openstack import service_description
from oslo_concurrency import lockutils
from oslo_config import cfg from oslo_config import cfg
from oslo_db import exception as db_exc from oslo_db import exception as db_exc
from oslo_db.sqlalchemy import enginefacade 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 # to point to a cell, we need to take an exclusive lock to
# prevent any other calls to get_context_manager() until we # prevent any other calls to get_context_manager() until we
# reset to the default. # reset to the default.
# NOTE(melwitt): As of fasteners >= 0.15, the workaround code to use self._cell_lock = utils.ReaderWriterLock()
# 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()
def _cache_schema(self, connection_str): def _cache_schema(self, connection_str):
# NOTE(melwitt): See the regular Database fixture for why # NOTE(melwitt): See the regular Database fixture for why

View File

@ -1000,3 +1000,23 @@ class HackingTestCase(test.NoDBTestCase):
self._assert_has_no_errors( self._assert_has_no_errors(
code, checks.do_not_use_mock_class_as_new_mock_value, code, checks.do_not_use_mock_class_as_new_mock_value,
filename="nova/tests/unit/test_context.py") 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)

View File

@ -29,6 +29,7 @@ import shutil
import tempfile import tempfile
import eventlet import eventlet
import fixtures
from keystoneauth1 import loading as ks_loading from keystoneauth1 import loading as ks_loading
import netaddr import netaddr
from openstack import connection from openstack import connection
@ -1143,3 +1144,37 @@ def run_once(message, logger, cleanup=None):
wrapper.reset = functools.partial(reset, wrapper) wrapper.reset = functools.partial(reset, wrapper)
return wrapper return wrapper
return outer_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)

View File

@ -346,6 +346,7 @@ extension =
N366 = checks:check_assert_has_calls N366 = checks:check_assert_has_calls
N367 = checks:do_not_alias_mock_class N367 = checks:do_not_alias_mock_class
N368 = checks:do_not_use_mock_class_as_new_mock_value N368 = checks:do_not_use_mock_class_as_new_mock_value
N369 = checks:check_lockutils_rwlocks
paths = paths =
./nova/hacking ./nova/hacking