Fix #750 by making patched threading.RLock interpret timeout the same as the stdlib.

This required a change to our test_threading_2.py: we weren't testing
patched RLocks at all! Now we run the full set on them.

Also add test_threading.py from Python 3.4 and 3.5 to the test suite to
identify any other differences. A few tests have to be commented out
because they rely on reprs or internal class details, but nothing too
severe or concerning.

Locally tox 3.3, 3.4 and 3.5 are all green.

changelog.rst

@@ -7,7 +7,12 @@
 1.1rc6 (unreleased)
 ===================
 
-- TBD
+- Python 3: A monkey-patched :class:`threading.RLock` now properly
+  blocks (or deadlocks) in ``acquire`` if the default value for
+  *timeout* of -1 is used (which differs from gevent's default of
+  None). The ``acquire`` method also raises the same :exc:`ValueError`
+  exceptions that the standard library does for invalid parameters.
+  Reported in :issue:`750` by Joy Zheng.
 
 1.1rc5 (Feb 24, 2016)
 =====================

doc/whatsnew_1_1.rst

@@ -282,6 +282,18 @@ reduce the cases of undocumented or non-standard behaviour.
   ) now throws an exception, just like the documented parameter to the
   same stdlib method in Python 3.
 
+- Under Python 3, several standard library methods added ``timeout``
+  parameters. These often default to -1 to mean "no timeout", whereas
+  gevent uses a default of ``None`` to mean the same thing,
+  potentially leading to great confusion and bugs in portable code. In
+  gevent, using a negative value has always been ill-defined and hard
+  to reason about. Because of those two things, as of this release,
+  negative ``timeout`` values should be considered deprecated (unless
+  otherwise documented). The current ill-defined behaviour is
+  maintained, but future releases may choose to treat it the same as
+  ``None`` or raise an error. No runtime warnings are issued for this
+  change for performance reasons.
+
 - The previously undocumented class
   ``gevent.fileobject.SocketAdapter`` has been removed, as have the
   internal ``gevent._util`` module and some internal implementation modules

gevent/_semaphore.py

@@ -160,9 +160,7 @@ class Semaphore(object):
         switch = getcurrent().switch
         self.rawlink(switch)
         try:
-            # As a tiny efficiency optimization, avoid allocating a timer
-            # if not needed.
-            timer = Timeout.start_new(timeout) if timeout is not None else None
+            timer = Timeout._start_new_or_dummy(timeout)
             try:
                 try:
                     result = get_hub().switch()
@@ -172,7 +170,6 @@ class Semaphore(object):
                         raise
                     return ex
             finally:
-                if timer is not None:
                 timer.cancel()
         finally:
             self.unlink(switch)

gevent/thread.py

@@ -30,7 +30,7 @@ else:
                     'interrupt_main',
                     'start_new']
 error = __thread__.error
-from gevent.hub import getcurrent, GreenletExit
+from gevent.hub import getcurrent, GreenletExit, PY3
 from gevent.greenlet import Greenlet
 from gevent.lock import BoundedSemaphore
 from gevent.local import local as _local
@@ -53,6 +53,26 @@ class LockType(BoundedSemaphore):
     # and any other API changes we need to make to match behaviour
     _OVER_RELEASE_ERROR = __thread__.error
 
+    if PY3:
+        _TIMEOUT_MAX = __thread__.TIMEOUT_MAX
+
+        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)
+
 allocate_lock = LockType
 
 

gevent/timeout.py

@@ -107,11 +107,19 @@ class Timeout(BaseException):
     If the *seconds* argument is not given or is ``None`` (e.g.,
     ``Timeout()``), then the timeout will never expire and never raise
     *exception*. This is convenient for creating functions which take
-    an optional timeout parameter of their own.
+    an optional timeout parameter of their own. (Note that this is not the same thing
+    as a *seconds* value of 0.)
+
+    .. caution::
+       A *seconds* value less than 0.0 (e.g., -1) is poorly defined. In the future,
+       support for negative values is likely to do the same thing as a value
+       if ``None``.
 
     .. versionchanged:: 1.1b2
        If *seconds* is not given or is ``None``, no longer allocate a libev
        timer that will never be started.
+    .. versionchanged:: 1.1
+       Add warning about negative *seconds* values.
     """
 
     def __init__(self, seconds=None, exception=None, ref=True, priority=-1):
@@ -167,7 +175,9 @@ 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.
+        # return an actual 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'
         # followed by 'if timer is not None: timer.cancel()'.
         # That idiom was used to avoid any object allocations.

greentest/lock_tests.py

@@ -44,7 +44,7 @@ class Bunch(object):
                 self.finished.append(tid)
                 while not self._can_exit:
                     _wait()
-        for i in range(n):
+        for _ in range(n):
             start_new_thread(task, ())
 
     def wait_for_started(self):
@@ -73,6 +73,9 @@ class BaseLockTests(BaseTestCase):
     Tests for both recursive and non-recursive locks.
     """
 
+    def locktype(self):
+        raise NotImplementedError()
+
     def test_constructor(self):
         lock = self.locktype()
         del lock
@@ -244,6 +247,9 @@ class EventTests(BaseTestCase):
     Tests for Event objects.
     """
 
+    def eventtype(self):
+        raise NotImplementedError()
+
     def test_is_set(self):
         evt = self.eventtype()
         self.assertFalse(evt.is_set())
@@ -316,6 +322,9 @@ class ConditionTests(BaseTestCase):
     Tests for condition variables.
     """
 
+    def condtype(self, *args):
+        raise NotImplementedError()
+
     def test_acquire(self):
         cond = self.condtype()
         # Be default we have an RLock: the condition can be acquired multiple
@@ -421,6 +430,9 @@ class BaseSemaphoreTests(BaseTestCase):
     Common tests for {bounded, unbounded} semaphore objects.
     """
 
+    def semtype(self, *args):
+        raise NotImplementedError()
+
     def test_constructor(self):
         self.assertRaises(ValueError, self.semtype, value = -1)
         # Py3 doesn't have sys.maxint
@@ -459,13 +471,13 @@ class BaseSemaphoreTests(BaseTestCase):
             _wait()
         self.assertEqual(results1 + results2, [0] * 6)
         phase_num = 1
-        for i in range(7):
+        for _ in range(7):
             sem.release()
         while len(results1) + len(results2) < 13:
             _wait()
         self.assertEqual(sorted(results1 + results2), [0] * 6 + [1] * 7)
         phase_num = 2
-        for i in range(6):
+        for _ in range(6):
             sem.release()
         while len(results1) + len(results2) < 19:
             _wait()
@@ -554,3 +566,6 @@ class BoundedSemaphoreTests(BaseSemaphoreTests):
         sem.acquire()
         sem.release()
         self.assertRaises(ValueError, sem.release)
+
+if __name__ == '__main__':
+    print("This module contains no tests; it is used by other test cases like test_threading_2")

greentest/patched_tests_setup.py

@@ -222,6 +222,33 @@ if hasattr(sys, 'pypy_version_info'):
             # https://bitbucket.org/cffi/cffi/issue/152/handling-errors-from-signal-handlers-in
         ]
 
+# Generic Python 3
+
+if sys.version_info[0] == 3:
+
+    disabled_tests += [
+        # Triggers the crash reporter
+        'test_threading.SubinterpThreadingTests.test_daemon_threads_fatal_error',
+
+        # Relies on an implementation detail, Thread._tstate_lock
+        'test_threading.ThreadTests.test_tstate_lock',
+        # Relies on an implementation detail (reprs); we have our own version
+        'test_threading.ThreadTests.test_various_ops',
+        'test_threading.ThreadTests.test_various_ops_large_stack',
+        'test_threading.ThreadTests.test_various_ops_small_stack',
+
+        # Relies on Event having a _cond and an _reset_internal_locks()
+        # XXX: These are commented out in the source code of test_threading because
+        # this doesn't work.
+        # 'lock_tests.EventTests.test_reset_internal_locks',
+
+        # Python bug 13502. We may or may not suffer from this as its
+        # basically a timing race condition.
+        # XXX Same as above
+        # 'lock_tests.EventTests.test_set_and_clear',
+
+    ]
+
 if sys.version_info[:2] == (3, 4) and sys.version_info[:3] < (3, 4, 4):
     # Older versions have some issues with the SSL tests. Seen on Appveyor
     disabled_tests += [
@@ -310,6 +337,14 @@ if sys.version_info[:2] >= (3, 5):
         'test_socket.GeneralModuleTests.test_repr',
         'test_socket.GeneralModuleTests.test_str_for_enums',
         'test_socket.GeneralModuleTests.testGetaddrinfo',
+
+        # Relies on the regex of the repr having the locked state (TODO: it'd be nice if
+        # we did that).
+        # XXX: These are commented out in the source code of test_threading because
+        # this doesn't work.
+        # 'lock_tests.LockTests.lest_locked_repr',
+        # 'lock_tests.LockTests.lest_repr',
+
     ]
 
     if os.environ.get('GEVENT_RESOLVER') == 'ares':

greentest/test__threading_vs_settrace.py

@@ -84,6 +84,7 @@ class TestTrace(unittest.TestCase):
         else:
             old = None
         PYPY = hasattr(sys, 'pypy_version_info')
+        PY3 = sys.version_info[0] > 2
         lst = []
         # we should be able to use unrelated locks from within the trace function
         l = allocate_lock()
@@ -104,7 +105,8 @@ class TestTrace(unittest.TestCase):
         finally:
             sys.settrace(old)
 
-        if not PYPY:
+        if not PYPY and 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
@@ -117,6 +119,7 @@ class TestTrace(unittest.TestCase):
         else:
             old = None
         PYPY = hasattr(sys, 'pypy_version_info')
+        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,7 +140,8 @@ class TestTrace(unittest.TestCase):
         finally:
             sys.settrace(old)
 
-        if not PYPY:
+        if not PYPY and 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

greentest/test_threading_2.py

@@ -9,6 +9,11 @@ from gevent.thread import allocate_lock as Lock
 import threading
 threading.Event = Event
 threading.Lock = Lock
+# XXX: We're completely patching around the allocate_lock
+# patch we try to do with RLock; our monkey patch doesn't
+# behave this way, why do we do it in tests? Save it so we can
+# at least access it sometimes.
+threading.NativeRLock = threading.RLock
 threading.RLock = RLock
 threading.Semaphore = Semaphore
 threading.BoundedSemaphore = BoundedSemaphore
@@ -522,6 +527,10 @@ class LockTests(lock_tests.LockTests):
 class RLockTests(lock_tests.RLockTests):
     locktype = staticmethod(threading.RLock)
 
+class NativeRLockTests(lock_tests.RLockTests):
+    # XXX: See comments at the top of the file for the difference
+    # between this and RLockTests, and why its weird.
+    locktype = staticmethod(threading.NativeRLock)
 
 class EventTests(lock_tests.EventTests):
     eventtype = staticmethod(threading.Event)
@@ -551,6 +560,7 @@ def main():
                          ThreadTests,
                          ThreadJoinOnShutdown,
                          ThreadingExceptionTests,
+                         NativeRLockTests,
                          )
 
 if __name__ == "__main__":