Patch #1731049: make threading.py use a proper "raise" when checking internal state, rather than assert statements (which get stripped out by -O).

3 files changed, 102 insertions, 39 deletions

diff --git a/Doc/lib/libthreading.tex b/Doc/lib/libthreading.tex
index 522ea2f..19c496e 100644
--- a/Doc/lib/libthreading.tex
+++ b/Doc/lib/libthreading.tex
@@ -174,11 +174,14 @@ until a call to \method{release()} in another thread changes it to
 unlocked, then the \method{acquire()} call resets it to locked and
 returns.  The \method{release()} method should only be called in the
 locked state; it changes the state to unlocked and returns
-immediately.  When more than one thread is blocked in
-\method{acquire()} waiting for the state to turn to unlocked, only one
-thread proceeds when a \method{release()} call resets the state to
-unlocked; which one of the waiting threads proceeds is not defined,
-and may vary across implementations.
+immediately. If an attempt is made to release an unlocked lock, a
+\exception{RuntimeError} will be raised.
+
+When more than one thread is blocked in \method{acquire()} waiting for
+the state to turn to unlocked, only one thread proceeds when a
+\method{release()} call resets the state to unlocked; which one of the
+waiting threads proceeds is not defined, and may vary across
+implementations.
 
 All methods are executed atomically.
 
@@ -257,8 +260,9 @@ become unlocked, allow exactly one of them to proceed.  If after the
 decrement the recursion level is still nonzero, the lock remains
 locked and owned by the calling thread.
 
-Only call this method when the calling thread owns the lock.
-Do not call this method when the lock is unlocked.
+Only call this method when the calling thread owns the lock. A
+\exception{RuntimeError} is raised if this method is called when the
+lock is unlocked.
 
 There is no return value.
 \end{methoddesc}
@@ -275,7 +279,8 @@ A condition variable has \method{acquire()} and \method{release()}
 methods that call the corresponding methods of the associated lock.
 It also has a \method{wait()} method, and \method{notify()} and
 \method{notifyAll()} methods.  These three must only be called when
-the calling thread has acquired the lock.
+the calling thread has acquired the lock, otherwise a
+\exception{RuntimeError} is raised.
 
 The \method{wait()} method releases the lock, and then blocks until it
 is awakened by a \method{notify()} or \method{notifyAll()} call for
@@ -343,9 +348,9 @@ lock; there is no return value.
 \end{methoddesc}
 
 \begin{methoddesc}{wait}{\optional{timeout}}
-Wait until notified or until a timeout occurs.
-This must only be called when the calling thread has acquired the
-lock.
+Wait until notified or until a timeout occurs. If the calling thread
+has not acquired the lock when this method is called, a
+\exception{RuntimeError} is raised.
 
 This method releases the underlying lock, and then blocks until it is
 awakened by a \method{notify()} or \method{notifyAll()} call for the
@@ -367,9 +372,10 @@ when the lock is reacquired.
 \end{methoddesc}
 
 \begin{methoddesc}{notify}{}
-Wake up a thread waiting on this condition, if any.
-This must only be called when the calling thread has acquired the
-lock.
+Wake up a thread waiting on this condition, if any. Wait until
+notified or until a timeout occurs. If the calling thread has not
+acquired the lock when this method is called, a
+\exception{RuntimeError} is raised.
 
 This method wakes up one of the threads waiting for the condition
 variable, if any are waiting; it is a no-op if no threads are waiting.
@@ -386,7 +392,9 @@ Note: the awakened thread does not actually return from its
 
 \begin{methoddesc}{notifyAll}{}
 Wake up all threads waiting on this condition.  This method acts like
-\method{notify()}, but wakes up all waiting threads instead of one.
+\method{notify()}, but wakes up all waiting threads instead of one. If
+the calling thread has not acquired the lock when this method is
+called, a \exception{RuntimeError} is raised.
 \end{methoddesc}
 
 
@@ -404,8 +412,9 @@ finds that it is zero, it blocks, waiting until some other thread
 calls \method{release()}.
 
 \begin{classdesc}{Semaphore}{\optional{value}}
-The optional argument gives the initial value for the internal
-counter; it defaults to \code{1}.
+The optional argument gives the initial \var{value} for the internal
+counter; it defaults to \code{1}. If the \var{value} given is less
+than 0, \exception{ValueError} is raised.
 \end{classdesc}
 
 \begin{methoddesc}{acquire}{\optional{blocking}}
@@ -586,9 +595,12 @@ before doing anything else to the thread.
 \begin{methoddesc}{start}{}
 Start the thread's activity.
 
-This must be called at most once per thread object.  It
-arranges for the object's \method{run()} method to be invoked in a
-separate thread of control.
+It must be called at most once per thread object.  It arranges for the
+object's \method{run()} method to be invoked in a separate thread of
+control.
+
+This method will raise a \exception{RuntimeException} if called more
+than once on the same thread object.
 \end{methoddesc}
 
 \begin{methoddesc}{run}{}
@@ -618,11 +630,10 @@ operation will block until the thread terminates.
 
 A thread can be \method{join()}ed many times.
 
-A thread cannot join itself because this would cause a
-deadlock.
-
-It is an error to attempt to \method{join()} a thread before it has
-been started.
+\method{join()} may throw a \exception{RuntimeError}, if an attempt is
+made to join the current thread as that would cause a deadlock. It is
+also an error to \method{join()} a thread before it has been started
+and attempts to do so raises same exception.
 \end{methoddesc}
 
 \begin{methoddesc}{getName}{}
@@ -651,7 +662,8 @@ Return the thread's daemon flag.
 
 \begin{methoddesc}{setDaemon}{daemonic}
 Set the thread's daemon flag to the Boolean value \var{daemonic}.
-This must be called before \method{start()} is called.
+This must be called before \method{start()} is called, otherwise
+\exception{RuntimeError} is raised.
 
 The initial value is inherited from the creating thread.
 
diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py
index 8614ecb..dced9cb 100644
--- a/Lib/test/test_threading.py
+++ b/Lib/test/test_threading.py
@@ -3,6 +3,7 @@
 import test.test_support
 from test.test_support import verbose
 import random
+import sys
 import threading
 import thread
 import time
@@ -201,8 +202,47 @@ class ThreadTests(unittest.TestCase):
             t.join()
         # else the thread is still running, and we have no way to kill it
 
+class ThreadingExceptionTests(unittest.TestCase):
+    # A RuntimeError should be raised if Thread.start() is called
+    # multiple times.
+    def test_start_thread_again(self):
+        thread = threading.Thread()
+        thread.start()
+        self.assertRaises(RuntimeError, thread.start)
+
+    def test_releasing_unacquired_rlock(self):
+        rlock = threading.RLock()
+        self.assertRaises(RuntimeError, rlock.release)
+
+    def test_waiting_on_unacquired_condition(self):
+        cond = threading.Condition()
+        self.assertRaises(RuntimeError, cond.wait)
+
+    def test_notify_on_unacquired_condition(self):
+        cond = threading.Condition()
+        self.assertRaises(RuntimeError, cond.notify)
+
+    def test_semaphore_with_negative_value(self):
+        self.assertRaises(ValueError, threading.Semaphore, value = -1)
+        self.assertRaises(ValueError, threading.Semaphore, value = -sys.maxint)
+
+    def test_joining_current_thread(self):
+        currentThread = threading.currentThread()
+        self.assertRaises(RuntimeError, currentThread.join);
+
+    def test_joining_inactive_thread(self):
+        thread = threading.Thread()
+        self.assertRaises(RuntimeError, thread.join)
+
+    def test_daemonize_active_thread(self):
+        thread = threading.Thread()
+        thread.start()
+        self.assertRaises(RuntimeError, thread.setDaemon, True)
+
+
 def test_main():
-    test.test_support.run_unittest(ThreadTests)
+    test.test_support.run_unittest(ThreadTests,
+                                   ThreadingExceptionTests)
 
 if __name__ == "__main__":
     test_main()
diff --git a/Lib/threading.py b/Lib/threading.py
index fecd3cc..a46090d 100644
--- a/Lib/threading.py
+++ b/Lib/threading.py
@@ -111,8 +111,8 @@ class _RLock(_Verbose):
     __enter__ = acquire
 
     def release(self):
-        me = currentThread()
-        assert self.__owner is me, "release() of un-acquire()d lock"
+        if self.__owner is not currentThread():
+            raise RuntimeError("cannot release un-aquired lock")
         self.__count = count = self.__count - 1
         if not count:
             self.__owner = None
@@ -204,7 +204,8 @@ class _Condition(_Verbose):
             return True
 
     def wait(self, timeout=None):
-        assert self._is_owned(), "wait() of un-acquire()d lock"
+        if not self._is_owned():
+            raise RuntimeError("cannot wait on un-aquired lock")
         waiter = _allocate_lock()
         waiter.acquire()
         self.__waiters.append(waiter)
@@ -245,7 +246,8 @@ class _Condition(_Verbose):
             self._acquire_restore(saved_state)
 
     def notify(self, n=1):
-        assert self._is_owned(), "notify() of un-acquire()d lock"
+        if not self._is_owned():
+            raise RuntimeError("cannot notify on un-aquired lock")
         __waiters = self.__waiters
         waiters = __waiters[:n]
         if not waiters:
@@ -273,7 +275,8 @@ class _Semaphore(_Verbose):
     # After Tim Peters' semaphore class, but not quite the same (no maximum)
 
     def __init__(self, value=1, verbose=None):
-        assert value >= 0, "Semaphore initial value must be >= 0"
+        if value < 0:
+            raise ValueError("semaphore initial value must be >= 0")
         _Verbose.__init__(self, verbose)
         self.__cond = Condition(Lock())
         self.__value = value
@@ -424,8 +427,10 @@ class Thread(_Verbose):
         return "<%s(%s, %s)>" % (self.__class__.__name__, self.__name, status)
 
     def start(self):
-        assert self.__initialized, "Thread.__init__() not called"
-        assert not self.__started, "thread already started"
+        if not self.__initialized:
+            raise RuntimeError("thread.__init__() not called")
+        if self.__started:
+            raise RuntimeError("thread already started")
         if __debug__:
             self._note("%s.start(): starting thread", self)
         _active_limbo_lock.acquire()
@@ -545,9 +550,13 @@ class Thread(_Verbose):
             _active_limbo_lock.release()
 
     def join(self, timeout=None):
-        assert self.__initialized, "Thread.__init__() not called"
-        assert self.__started, "cannot join thread before it is started"
-        assert self is not currentThread(), "cannot join current thread"
+        if not self.__initialized:
+            raise RuntimeError("Thread.__init__() not called")
+        if not self.__started:
+            raise RuntimeError("cannot join thread before it is started")
+        if self is currentThread():
+            raise RuntimeError("cannot join current thread")
+
         if __debug__:
             if not self.__stopped:
                 self._note("%s.join(): waiting until thread stops", self)
@@ -590,8 +599,10 @@ class Thread(_Verbose):
         return self.__daemonic
 
     def setDaemon(self, daemonic):
-        assert self.__initialized, "Thread.__init__() not called"
-        assert not self.__started, "cannot set daemon status of active thread"
+        if not self.__initialized:
+            raise RuntimeError("Thread.__init__() not called")
+        if self.__started:
+            raise RuntimeError("cannot set daemon status of active thread");
         self.__daemonic = daemonic
 
 # The timer class was contributed by Itamar Shtull-Trauring