Make gevent.thread.allocate_lock closer to stdlib. Fixes #308.

changelog.rst

@@ -27,6 +27,12 @@ Unreleased
   monkey-patch. Implemented in :pr:`604` by Eddi Linder.
 - Allow passing of events to the io callback under PyPy. Reported in
   :issue:`531` by M. Nunberg and implemented in :pr:`604`.
+- ``gevent.thread.allocate_lock`` (and so a monkey-patched standard
+  library ``allocate_lock``) more closely matches the behaviour of the
+  builtin: an unlocked lock cannot be released, and attempting to do
+  so throws the correct exception (``thread.error`` on Python 2,
+  ``RuntimeError`` on Python 3). Previously, over-releasing a lock was
+  silently ignored. Reported in :issue:`308` by Jędrzej Nowak.
 
 1.1a2 (Jul 8, 2015)
 ===================

gevent/_semaphore.pxd

@@ -12,3 +12,8 @@ cdef class Semaphore:
     cpdef acquire(self, int blocking=*, object timeout=*)
     cpdef __enter__(self)
     cpdef __exit__(self, object t, object v, object tb)
+
+cdef class BoundedSemaphore(Semaphore):
+    cdef readonly int _initial_value
+
+    cpdef release(self)

gevent/_semaphore.pyx

@@ -3,18 +3,20 @@ from gevent.hub import get_hub, getcurrent
 from gevent.timeout import Timeout
 
 
-__all__ = ['Semaphore']
+__all__ = ['Semaphore', 'BoundedSemaphore']
 
 
 
 class Semaphore(object):
-    """A semaphore manages a counter representing the number of release() calls minus the number of acquire() calls,
-    plus an initial value. The acquire() method blocks if necessary until it can return without making the counter
-    negative.
+    """
+    A semaphore manages a counter representing the number of release()
+    calls minus the number of acquire() calls, plus an initial value.
+    The acquire() method blocks if necessary until it can return
+    without making the counter negative.
 
-    If not given, value defaults to 1.
+    If not given, ``value`` defaults to 1.
 
-    This Semaphore's __exit__ method does not call the trace function.
+    This Semaphore's ``__exit__`` method does not call the trace function.
     """
 
     def __init__(self, value=1):
@@ -133,3 +135,25 @@ class Semaphore(object):
 
     def __exit__(self, t, v, tb):
         self.release()
+
+class BoundedSemaphore(Semaphore):
+    """
+    A bounded semaphore checks to make sure its current value doesn't
+    exceed its initial value. If it does, :class:`ValueError` is
+    raised. In most situations semaphores are used to guard resources
+    with limited capacity. If the semaphore is released too many times
+    it's a sign of a bug.
+
+    If not given, *value* defaults to 1.
+    """
+
+    _OVER_RELEASE_ERROR = ValueError
+
+    def __init__(self, value=1):
+        Semaphore.__init__(self, value)
+        self._initial_value = value
+
+    def release(self):
+        if self.counter >= self._initial_value:
+            raise self._OVER_RELEASE_ERROR("Semaphore released too many times")
+        return Semaphore.release(self)

gevent/lock.py

@@ -2,7 +2,7 @@
 """Locking primitives"""
 
 from gevent.hub import getcurrent
-from gevent._semaphore import Semaphore
+from gevent._semaphore import Semaphore, BoundedSemaphore
 
 
 __all__ = ['Semaphore', 'DummySemaphore', 'BoundedSemaphore', 'RLock']
@@ -11,7 +11,7 @@ __all__ = ['Semaphore', 'DummySemaphore', 'BoundedSemaphore', 'RLock']
 class DummySemaphore(object):
     """
     A Semaphore initialized with "infinite" initial value. None of its
-methods ever block.
+    methods ever block.
 
     This can be used to parameterize on whether or not to actually
     guard access to a potentially limited resource. If the resource is
@@ -67,23 +67,6 @@ methods ever block.
         pass
 
 
-class BoundedSemaphore(Semaphore):
-    """A bounded semaphore checks to make sure its current value doesn't exceed its initial value.
-    If it does, ``ValueError`` is raised. In most situations semaphores are used to guard resources
-    with limited capacity. If the semaphore is released too many times it's a sign of a bug.
-
-    If not given, *value* defaults to 1."""
-
-    def __init__(self, value=1):
-        Semaphore.__init__(self, value)
-        self._initial_value = value
-
-    def release(self):
-        if self.counter >= self._initial_value:
-            raise ValueError("Semaphore released too many times")
-        return Semaphore.release(self)
-
-
 class RLock(object):
 
     def __init__(self):

gevent/thread.py

@@ -32,7 +32,7 @@ else:
 error = __thread__.error
 from gevent.hub import getcurrent, GreenletExit
 from gevent.greenlet import Greenlet
-from gevent.lock import Semaphore as LockType
+from gevent.lock import BoundedSemaphore
 from gevent.local import local as _local
 
 
@@ -48,6 +48,11 @@ def start_new_thread(function, args=(), kwargs={}):
     return get_ident(greenlet)
 
 
+class LockType(BoundedSemaphore):
+    # Change the ValueError into the appropriate thread error
+    # and any other API changes we need to make to match behaviour
+    _OVER_RELEASE_ERROR = __thread__.error
+
 allocate_lock = LockType
 
 

greentest/test__select.py

@@ -23,17 +23,19 @@ if sys.platform != 'win32':
                 os.close(r)
                 os.close(w)
 
-    class TestPollRead(greentest.GenericWaitTestCase):
-        def wait(self, timeout):
-            r, w = os.pipe()
-            try:
-                poll = select.poll()
-                poll.register(r)
-                poll.poll(timeout * 1000)
-                poll.unregister(r)
-            finally:
-                os.close(r)
-                os.close(w)
+    if hasattr(select, 'poll') and sys.platform != 'darwin':
+
+        class TestPollRead(greentest.GenericWaitTestCase):
+            def wait(self, timeout):
+                r, w = os.pipe()
+                try:
+                    poll = select.poll()
+                    poll.register(r)
+                    poll.poll(timeout * 1000)
+                    poll.unregister(r)
+                finally:
+                    os.close(r)
+                    os.close(w)
 
 
 class TestSelectTypes(greentest.TestCase):

greentest/test__semaphore.py

 import greentest
 import gevent
 from gevent.lock import Semaphore
+from gevent.thread import allocate_lock
+try:
+    from _thread import allocate_lock as std_allocate_lock
+except ImportError: # Py2
+    from thread import allocate_lock as std_allocate_lock
 
 
 class TestTimeoutAcquire(greentest.TestCase):
@@ -22,5 +27,24 @@ class TestTimeoutAcquire(greentest.TestCase):
         self.assertEqual(result, ['a', 'b'])
 
 
+class TestLock(greentest.TestCase):
+
+    def test_release_unheld_lock(self):
+        std_lock = std_allocate_lock()
+        g_lock = allocate_lock()
+        try:
+            std_lock.release()
+            self.fail("Should have thrown an exception")
+        except Exception as e:
+            std_exc = e
+
+        try:
+            g_lock.release()
+            self.fail("Should have thrown an exception")
+        except Exception as e:
+            g_exc = e
+        self.assertTrue(isinstance(g_exc, type(std_exc)), (g_exc, std_exc))
+
+
 if __name__ == '__main__':
     greentest.main()