Merge pull request #1486 from gevent/issue1464

Fix #1464 and fix #1465 by making LockType sleep on failure to non-blocking acquire the lock.

CHANGES.rst

@@ -25,10 +25,16 @@
   ``r``, for consistency with the other file objects and the standard
   ``open`` and ``io.open`` functions.
 
-- Fix ``FilObjectPosix`` improperly being used from multiple
+- Fix ``FileObjectPosix`` improperly being used from multiple
   greenlets. Previously this was hidden by forcing buffering, which
   raised ``RuntimeError``.
 
+- Fix using monkey-patched ``threading.Lock`` and ``threading.RLock``
+  objects as spin locks by making them call ``sleep(0)`` if they
+  failed to acquire the lock in a non-blocking call. This lets other
+  callbacks run to release the lock, simulating preemptive threading.
+  Using spin locks is not recommended, but may have been done in code
+  written for threads, especially on Python 3. See :issue:`1464`.
 
 1.5a2 (2019-10-21)
 ==================

src/gevent/monkey.py

@@ -498,6 +498,16 @@ def patch_time():
 def _patch_existing_locks(threading):
     if len(list(threading.enumerate())) != 1:
         return
+    # This is used to protect internal data structures for enumerate.
+    # It's acquired when threads are started and when they're stopped.
+    # Stopping a thread checks a Condition, which on Python 2 wants to test
+    # _is_owned of its (patched) Lock. Since our LockType doesn't have
+    # _is_owned, it tries to acquire the lock non-blocking; that triggers a
+    # switch. If the next thing in the callback list was a thread that needed
+    # to start or end, we wouldn't be able to acquire this native lock
+    # because it was being held already; we couldn't switch either, so we'd
+    # block permanently.
+    threading._active_limbo_lock = threading._allocate_lock()
     try:
         tid = threading.get_ident()
     except AttributeError:
@@ -566,6 +576,7 @@ def patch_thread(threading=True, _threading_local=True, Event=True, logging=True
         instances that are currently locked can be properly unlocked. **Important**: This is a
         best-effort attempt and, on certain implementations, may not detect all
         locks. It is important to monkey-patch extremely early in the startup process.
+        Setting this to False is not recommended, especially on Python 2.
 
     .. caution::
         Monkey-patching :mod:`thread` and using

src/gevent/tests/test__threading.py

+"""
+Tests specifically for the monkey-patched threading module.
+"""
 from gevent import monkey; monkey.patch_all()
 import gevent.hub
 
@@ -14,7 +17,7 @@ def helper():
     gevent.sleep(0.2)
 
 
-class Test(greentest.TestCase):
+class TestCleanup(greentest.TestCase):
 
     def _do_test(self, spawn):
         before = len(threading._active)
@@ -46,5 +49,44 @@ class Test(greentest.TestCase):
     def test_cleanup_raw(self):
         self._do_test(gevent.spawn_raw)
 
+
+class TestLockThread(greentest.TestCase):
+
+    def _spawn(self, func):
+        t = threading.Thread(target=func)
+        t.start()
+        return t
+
+    def test_spin_lock_switches(self):
+        # https://github.com/gevent/gevent/issues/1464
+        lock = threading.Lock()
+        lock.acquire()
+        spawned = []
+
+        def background():
+            spawned.append(True)
+            while 1:
+                # blocking= in Py3, wait (no default, no name) in Py2
+                if lock.acquire(False):
+                    break
+
+        thread = threading.Thread(target=background)
+        # If lock.acquire(False) doesn't yield when it fails,
+        # then this never returns.
+        thread.start()
+        # Verify it tried to run
+        self.assertEqual(spawned, [True])
+        # We can attempt to join it, which won't work.
+        thread.join(0)
+        # We can release the lock and then it will acquire.
+        lock.release()
+        thread.join()
+
+
+class TestLockGreenlet(TestLockThread):
+
+    def _spawn(self, func):
+        return gevent.spawn(func)
+
 if __name__ == '__main__':
     greentest.main()

src/gevent/tests/test__threading_vs_settrace.py

@@ -82,7 +82,6 @@ class TestTrace(unittest.TestCase):
         else:
             old = None
 
-        PY3 = sys.version_info[0] > 2
         lst = []
         # we should be able to use unrelated locks from within the trace function
         l = allocate_lock()
@@ -90,7 +89,7 @@ class TestTrace(unittest.TestCase):
             def trace(frame, ev, _arg):
                 with l:
                     lst.append((frame.f_code.co_filename, frame.f_lineno, ev))
-                print("TRACE: %s:%s %s" % lst[-1])
+                # print("TRACE: %s:%s %s" % lst[-1])
                 return trace
 
             l2 = allocate_lock()
@@ -102,12 +101,8 @@ class TestTrace(unittest.TestCase):
         finally:
             sys.settrace(old)
 
-        if not PY3:
-            # Py3 overrides acquire in Python to do argument checking
-            self.assertEqual(lst, [], "trace not empty")
-        else:
-            # Have an assert so that we know if we miscompile
-            self.assertTrue(lst, "should not compile on pypy")
+        # Have an assert so that we know if we miscompile
+        self.assertTrue(lst, "should not compile on pypy")
 
     @greentest.skipOnPurePython("Locks can be traced in Pure Python")
     def test_untraceable_lock_uses_same_lock(self):
@@ -116,7 +111,7 @@ class TestTrace(unittest.TestCase):
             old = sys.gettrace()
         else:
             old = None
-        PY3 = sys.version_info[0] > 2
+
         lst = []
         e = None
         # we should not be able to use the same lock from within the trace function
@@ -137,13 +132,9 @@ class TestTrace(unittest.TestCase):
         finally:
             sys.settrace(old)
 
-        if not PY3:
-            # Py3 overrides acquire in Python to do argument checking
-            self.assertEqual(lst, [], "trace not empty")
-        else:
-            # Have an assert so that we know if we miscompile
-            self.assertTrue(lst, "should not compile on pypy")
-            self.assertTrue(isinstance(e, LoopExit))
+        # Have an assert so that we know if we miscompile
+        self.assertTrue(lst, "should not compile on pypy")
+        self.assertTrue(isinstance(e, LoopExit))
 
     def run_script(self, more_args=()):
         args = [sys.executable, "-c", script]

src/gevent/thread.py

@@ -53,7 +53,10 @@ error = __thread__.error
 
 from gevent._compat import PYPY
 from gevent._util import copy_globals
-from gevent.hub import getcurrent, GreenletExit
+from gevent.hub import getcurrent
+from gevent.hub import GreenletExit
+from gevent.hub import sleep
+from gevent._hub_local import get_hub_if_exists
 from gevent.greenlet import Greenlet
 from gevent.lock import BoundedSemaphore
 from gevent.local import local as _local
@@ -89,23 +92,45 @@ class LockType(BoundedSemaphore):
 
     if PY3:
         _TIMEOUT_MAX = __thread__.TIMEOUT_MAX # python 2: pylint:disable=no-member
-
-        def acquire(self, blocking=True, timeout=-1):
-            # Transform the default -1 argument into the None that our
-            # semaphore implementation expects, and raise the same error
-            # the stdlib implementation does.
-            if timeout == -1:
-                timeout = None
-            if not blocking and timeout is not None:
-                raise ValueError("can't specify a timeout for a non-blocking call")
-            if timeout is not None:
-                if timeout < 0:
-                    # in C: if(timeout < 0 && timeout != -1)
-                    raise ValueError("timeout value must be strictly positive")
-                if timeout > self._TIMEOUT_MAX:
-                    raise OverflowError('timeout value is too large')
-
-            return BoundedSemaphore.acquire(self, blocking, timeout)
+    else:
+        _TIMEOUT_MAX = 9223372036.0
+
+    def acquire(self, blocking=True, timeout=-1):
+        # This is the Python 3 signature.
+        # On Python 2, Lock.acquire has the signature `Lock.acquire([wait])`
+        # where `wait` is a boolean that cannot be passed by name, only position.
+        # so we're fine to use the Python 3 signature.
+
+        # Transform the default -1 argument into the None that our
+        # semaphore implementation expects, and raise the same error
+        # the stdlib implementation does.
+        if timeout == -1:
+            timeout = None
+        if not blocking and timeout is not None:
+            raise ValueError("can't specify a timeout for a non-blocking call")
+        if timeout is not None:
+            if timeout < 0:
+                # in C: if(timeout < 0 && timeout != -1)
+                raise ValueError("timeout value must be strictly positive")
+            if timeout > self._TIMEOUT_MAX:
+                raise OverflowError('timeout value is too large')
+
+        acquired = BoundedSemaphore.acquire(self, blocking, timeout)
+        if not acquired and not blocking and getcurrent() is not get_hub_if_exists():
+            # Run other callbacks. This makes spin locks works.
+            # We can't do this if we're in the hub, which we could easily be:
+            # printing the repr of a thread checks its tstate_lock, and sometimes we
+            # print reprs in the hub.
+            # See https://github.com/gevent/gevent/issues/1464
+
+            # By using sleep() instead of self.wait(0), we don't force a trip
+            # around the event loop *unless* we've been running callbacks for
+            # longer than our switch interval.
+            sleep()
+        return acquired
+
+    # Should we implement _is_owned, at least for Python 2? See notes in
+    # monkey.py's patch_existing_locks.
 
 allocate_lock = LockType
 

src/gevent/timeout.py

@@ -266,12 +266,16 @@ class Timeout(BaseException):
         # Internal use only in 1.1
         # Return an object with a 'cancel' method; if timeout is None,
         # this will be a shared instance object that does nothing. Otherwise,
-        # return an actual Timeout. Because negative values are hard to reason about,
+        # return an actual Timeout. A 0 value is allowed and creates a real Timeout.
+
+        # Because negative values are hard to reason about,
         # and are often used as sentinels in Python APIs, in the future it's likely
         # that a negative timeout will also return the shared instance.
-        # This saves the previously common idiom of 'timer = Timeout.start_new(t) if t is not None else None'
+        # This saves the previously common idiom of
+        # 'timer = Timeout.start_new(t) if t is not None else None'
         # followed by 'if timer is not None: timer.cancel()'.
         # That idiom was used to avoid any object allocations.
+
         # A staticmethod is slightly faster under CPython, compared to a classmethod;
         # under PyPy in synthetic benchmarks it makes no difference.
         if timeout is None: