From 796203c94846d44237d869e3b20a6cd8a3602e36 Mon Sep 17 00:00:00 2001
From: Balazs Gibizer <gibi@redhat.com>
Date: Sat, 3 Sep 2022 13:16:56 +0200
Subject: [PATCH] Prove that spawn_n with fair lock is broken

The fasteners lib in version 0.15.0 removed the
threading.current_thread workaround for eventlet[1] because eventlet
seemed to fixed the current_thread issues tracked in [2]. However the
fix for [2] only fixed half of the problem. The threading.current_thread
call works if it is called from thread created by eventlet.spawn.
However if the thread is created with eventlet.spawn_n then
threading.current_thread is still broken and returns the ID of the
python native thread.

The fasteners' ReaderWriterLock depends heavily on
threading.current_thread to decide which thread holds a lock and to
allow re-entry of that thread. This leads to the situation that
multiple threads created from spawn_n could take the same
ReaderWriterLock at the same time.

The fair internal lock in oslo.concurrency uses ReaderWriterLock and
as a result such lock is broken for threads created with spawn_n.

Note that this issue was raised with eventlet in [3] when the nova team
detected it via a direct usage of ReaderWriterLock in the nova test
code. As [3] did not lead to a solution in eventlet nova implemented a
nova local fix for the test code in [4].

However now we detected that oslo.concurrency is affected by this issue
as well.

This patch adds tests to show the scope of the problem.

Note that the coverage tox target is changed to explicitly enable native
threading otherwise it runs eventlet specific tests in a native
environment.

Also note that [5] was opened to reintroduce the workaround[1] in fasteners.

[1] https://github.com/harlowja/fasteners/commit/467ed75ee1e9465ebff8b5edf452770befb93913
[2] https://github.com/eventlet/eventlet/issues/172
[3] https://github.com/eventlet/eventlet/issues/731
[4] https://review.opendev.org/c/openstack/nova/+/813114
[5] https://github.com/harlowja/fasteners/issues/96

Related-Bug: #1988311
Change-Id: Ibc193c855b49b95b46ebd2aac82ea89e33f885f0
---
 .../tests/unit/test_lockutils_eventlet.py     | 63 +++++++++++++++++++
 tox.ini                                       |  3 +-
 2 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/oslo_concurrency/tests/unit/test_lockutils_eventlet.py b/oslo_concurrency/tests/unit/test_lockutils_eventlet.py
index cb09298..24fd7f6 100644
--- a/oslo_concurrency/tests/unit/test_lockutils_eventlet.py
+++ b/oslo_concurrency/tests/unit/test_lockutils_eventlet.py
@@ -51,3 +51,66 @@ class TestFileLocks(test_base.BaseTestCase):
         pool.waitall()
 
         self.assertTrue(self.completed)
+
+
+class TestInternalLock(test_base.BaseTestCase):
+    def _test_internal_lock_with_two_threads(self, fair, spawn):
+        self.other_started = eventlet.event.Event()
+        self.other_finished = eventlet.event.Event()
+
+        def other():
+            self.other_started.send('started')
+            with lockutils.lock("my-lock", fair=fair):
+                pass
+            self.other_finished.send('finished')
+
+        with lockutils.lock("my-lock", fair=fair):
+            # holding the lock and starting another thread that also wants to
+            # take it before finishes
+            spawn(other)
+            # let the other thread start
+            self.other_started.wait()
+            eventlet.sleep(0)
+            # the other thread should not have finished as it would need the
+            # lock we are holding
+            self.assertIsNone(
+                self.other_finished.wait(0.5),
+                "Two threads was able to take the same lock",
+            )
+
+        # we released the lock, let the other thread take it and run to
+        # completion
+        result = self.other_finished.wait()
+        self.assertEqual('finished', result)
+
+    def test_lock_with_spawn(self):
+        self._test_internal_lock_with_two_threads(
+            fair=False, spawn=eventlet.spawn
+        )
+
+    def test_lock_with_spawn_n(self):
+        self._test_internal_lock_with_two_threads(
+            fair=False, spawn=eventlet.spawn_n
+        )
+
+    def test_fair_lock_with_spawn(self):
+        self._test_internal_lock_with_two_threads(
+            fair=True, spawn=eventlet.spawn
+        )
+
+    def test_fair_lock_with_spawn_n(self):
+        # This is bug 1988311 where spawn_n does not work with fair locks
+        ex = self.assertRaises(
+            AssertionError,
+            self._test_internal_lock_with_two_threads,
+            fair=True,
+            spawn=eventlet.spawn_n,
+        )
+        self.assertEqual(
+            "'finished' is not None: "
+            "Two threads was able to take the same lock",
+            str(ex),
+        )
+        # self._test_internal_lock_with_two_threads(
+        #     fair=True, spawn=eventlet.spawn_n
+        # )
diff --git a/tox.ini b/tox.ini
index b7a468c..962f08b 100644
--- a/tox.ini
+++ b/tox.ini
@@ -37,7 +37,7 @@ commands =
 setenv =
   PYTHON=coverage run --source oslo_concurrency --parallel-mode
 commands =
-  stestr run {posargs}
+  env TEST_EVENTLET=0 lockutils-wrapper stestr run {posargs}
   coverage combine
   coverage html -d cover
   coverage xml -o cover/coverage.xml
@@ -55,4 +55,3 @@ import_exceptions =
 deps = {[testenv:docs]deps}
 commands =
   sphinx-build -a -E -W -d releasenotes/build/doctrees --keep-going -b html releasenotes/source releasenotes/build/html
-