Fixed a variety of race conditions in Thread.cpp. Made RWRecMutex smaller.

8 files changed, 274 insertions, 128 deletions

diff --git a/cpp/include/IceUtil/RWRecMutex.h b/cpp/include/IceUtil/RWRecMutex.h
index f64d675e368..9e47bdba894 100644
--- a/cpp/include/IceUtil/RWRecMutex.h
+++ b/cpp/include/IceUtil/RWRecMutex.h
@@ -234,9 +234,9 @@ private:
     mutable int _count;
 
     //
-    // If there is an active writer this is a control for thread.
+    // If there is an active writer this is the ID of the writer thread.
     //
-    mutable ThreadControl _writerControl;
+    mutable ThreadId _writerId;
 
     //
     // Number of waiting writers.
diff --git a/cpp/include/IceUtil/Thread.h b/cpp/include/IceUtil/Thread.h
index 5dbc94365a0..73d0b7e70d9 100644
--- a/cpp/include/IceUtil/Thread.h
+++ b/cpp/include/IceUtil/Thread.h
@@ -17,6 +17,7 @@
 
 #include <IceUtil/Shared.h>
 #include <IceUtil/Handle.h>
+#include <IceUtil/Mutex.h>
 
 namespace IceUtil
 {
@@ -47,6 +48,12 @@ struct HandleWrapper : public Shared
 typedef Handle<HandleWrapper> HandleWrapperPtr;
 #endif
 
+#ifdef _WIN32
+    typedef unsigned int ThreadId;
+#else
+    typedef pthread_t ThreadId;
+#endif
+
 class ICE_UTIL_API ThreadControl
 {
 public:
@@ -64,6 +71,11 @@ public:
     bool operator<(const ThreadControl&) const;
 
     //
+    // Return the ID of the thread underlying this ThreadControl.
+    //
+    ThreadId id() const;
+
+    //
     // Wait until the controlled thread terminates. The call has POSIX
     // semantics.
     //
@@ -97,10 +109,8 @@ private:
 
 #ifdef _WIN32
     HandleWrapperPtr _handle;
-    unsigned int _id;
-#else
-    pthread_t _id;
 #endif
+    ThreadId _id;
 
     bool _detached;
 };
@@ -108,11 +118,6 @@ private:
 class ICE_UTIL_API Thread : virtual public IceUtil::Shared
 {
 public:
-#ifdef _WIN32
-    typedef unsigned int ThreadId;
-#else
-    typedef pthread_t ThreadId;
-#endif
 
     Thread();
     virtual ~Thread();
@@ -131,13 +136,13 @@ public:
 
 private:
 
+    Mutex _stateMutex;
+
     bool _started;
 #ifdef _WIN32
-    unsigned int _id;
     HandleWrapperPtr _handle;
-#else
-    pthread_t _id;
 #endif
+    ThreadId _id;
 };
 
 typedef Handle<Thread> ThreadPtr;
diff --git a/cpp/include/IceUtil/ThreadException.h b/cpp/include/IceUtil/ThreadException.h
index aaadc257f7f..a96f0da16c2 100644
--- a/cpp/include/IceUtil/ThreadException.h
+++ b/cpp/include/IceUtil/ThreadException.h
@@ -54,6 +54,16 @@ public:
     virtual Exception* ice_clone() const;
     virtual void ice_throw() const;
 };
+
+class ICE_UTIL_API ThreadNotStartedException : public Exception
+{
+public:
+
+    ThreadNotStartedException(const char*, int);
+    virtual std::string ice_name() const;
+    virtual Exception* ice_clone() const;
+    virtual void ice_throw() const;
+};
     
 }
 
diff --git a/cpp/src/IceUtil/.depend b/cpp/src/IceUtil/.depend
index c654fe69d1f..b29e659154e 100644
--- a/cpp/src/IceUtil/.depend
+++ b/cpp/src/IceUtil/.depend
@@ -4,7 +4,7 @@ UUID.o: UUID.cpp ../../include/IceUtil/UUID.h ../../include/IceUtil/Config.h ../
 RecMutex.o: RecMutex.cpp ../../include/IceUtil/RecMutex.h ../../include/IceUtil/Config.h ../../include/IceUtil/Lock.h ../../include/IceUtil/ThreadException.h ../../include/IceUtil/Exception.h
 RWRecMutex.o: RWRecMutex.cpp ../../include/IceUtil/RWRecMutex.h ../../include/IceUtil/Mutex.h ../../include/IceUtil/Config.h ../../include/IceUtil/Lock.h ../../include/IceUtil/ThreadException.h ../../include/IceUtil/Exception.h ../../include/IceUtil/Cond.h ../../include/IceUtil/Time.h ../../include/IceUtil/Thread.h ../../include/IceUtil/Shared.h ../../include/IceUtil/Handle.h
 Cond.o: Cond.cpp ../../include/IceUtil/Cond.h ../../include/IceUtil/Config.h ../../include/IceUtil/Time.h ../../include/IceUtil/ThreadException.h ../../include/IceUtil/Exception.h
-Thread.o: Thread.cpp ../../include/IceUtil/Thread.h ../../include/IceUtil/Shared.h ../../include/IceUtil/Config.h ../../include/IceUtil/Handle.h ../../include/IceUtil/Exception.h ../../include/IceUtil/Time.h ../../include/IceUtil/ThreadException.h
+Thread.o: Thread.cpp ../../include/IceUtil/Thread.h ../../include/IceUtil/Shared.h ../../include/IceUtil/Config.h ../../include/IceUtil/Handle.h ../../include/IceUtil/Exception.h ../../include/IceUtil/Mutex.h ../../include/IceUtil/Lock.h ../../include/IceUtil/ThreadException.h ../../include/IceUtil/Time.h
 ThreadException.o: ThreadException.cpp ../../include/IceUtil/ThreadException.h ../../include/IceUtil/Exception.h ../../include/IceUtil/Config.h
 Time.o: Time.cpp ../../include/IceUtil/Time.h ../../include/IceUtil/Config.h
 InputUtil.o: InputUtil.cpp ../../include/IceUtil/InputUtil.h ../../include/IceUtil/Config.h
diff --git a/cpp/src/IceUtil/RWRecMutex.cpp b/cpp/src/IceUtil/RWRecMutex.cpp
index c6be0120fd0..d5ff0a6b2db 100644
--- a/cpp/src/IceUtil/RWRecMutex.cpp
+++ b/cpp/src/IceUtil/RWRecMutex.cpp
@@ -20,6 +20,7 @@
 
 IceUtil::RWRecMutex::RWRecMutex() :
     _count(0),
+    _writerId(0),
     _waitingWriters(0)
 {
 }
@@ -95,7 +96,7 @@ IceUtil::RWRecMutex::writelock() const
     // If the mutex is already write locked by this writer then
     // decrement _count, and return.
     //
-    if(_count < 0 && _writerControl == ThreadControl())
+    if(_count < 0 && _writerId == ThreadControl().id())
     {
 	--_count;
 	return;
@@ -124,7 +125,7 @@ IceUtil::RWRecMutex::writelock() const
     // Got the lock, indicate it's held by a writer.
     //
     _count = -1;
-    _writerControl = ThreadControl();
+    _writerId = ThreadControl().id();
 }
 
 void
@@ -136,7 +137,7 @@ IceUtil::RWRecMutex::tryWritelock() const
     // If the mutex is already write locked by this writer then
     // decrement _count, and return.
     //
-    if(_count < 0 && _writerControl == ThreadControl())
+    if(_count < 0 && _writerId == ThreadControl().id())
     {
 	--_count;
 	return;
@@ -154,7 +155,7 @@ IceUtil::RWRecMutex::tryWritelock() const
     // Got the lock, indicate it's held by a writer.
     //
     _count = -1;
-    _writerControl = ThreadControl();
+    _writerId = ThreadControl().id();
 }
 
 void
@@ -165,7 +166,7 @@ IceUtil::RWRecMutex::timedTryWritelock(const Time& timeout) const
     //
     // If the mutex is already write locked by this writer then
     // decrement _count, and return.
-    if(_count < 0 && _writerControl == ThreadControl())
+    if(_count < 0 && _writerId == ThreadControl().id())
     {
 	--_count;
 	return;
@@ -203,7 +204,7 @@ IceUtil::RWRecMutex::timedTryWritelock(const Time& timeout) const
     // Got the lock, indicate it's held by a writer.
     //
     _count = -1;
-    _writerControl = ThreadControl();
+    _writerId = ThreadControl().id();
 }
 
 void
@@ -267,6 +268,7 @@ IceUtil::RWRecMutex::unlock() const
     }
     else if(wr)
     {
+	_writerId = 0;
 	//
 	// Wake readers
 	//
@@ -309,7 +311,7 @@ IceUtil::RWRecMutex::upgrade() const
     // Got the lock, indicate it's held by a writer.
     //
     _count = -1;
-    _writerControl = ThreadControl();
+    _writerId = ThreadControl().id();
 }
 
 void
@@ -359,5 +361,5 @@ IceUtil::RWRecMutex::timedUpgrade(const Time& timeout) const
     // Got the lock, indicate it's held by a writer.
     //
     _count = -1;
-    _writerControl = ThreadControl();
+    _writerId = ThreadControl().id();
 }
diff --git a/cpp/src/IceUtil/Thread.cpp b/cpp/src/IceUtil/Thread.cpp
index 4994079002a..3ad29cecda3 100644
--- a/cpp/src/IceUtil/Thread.cpp
+++ b/cpp/src/IceUtil/Thread.cpp
@@ -59,52 +59,47 @@ IceUtil::ThreadControl::operator<(const ThreadControl& rhs) const
     return _id != rhs._id;
 }
 
+IceUtil::ThreadId
+IceUtil::ThreadControl::id() const
+{
+    return _id;
+}
+
 void
 IceUtil::ThreadControl::join()
 {
-    if(_handle->handle)
+    if(_detached)
     {
-	if (_detached)
-	{
-	    throw ThreadSyscallException(__FILE__, __LINE__);
-	}
-	_detached = true;
-	int rc = WaitForSingleObject(_handle->handle, INFINITE);
-	if(rc != WAIT_OBJECT_0)
-	{
-	    throw ThreadSyscallException(__FILE__, __LINE__);
-	}
+	throw ThreadSyscallException(__FILE__, __LINE__);
+    }
+    int rc = WaitForSingleObject(_handle->handle, INFINITE);
+    if(rc != WAIT_OBJECT_0)
+    {
+	throw ThreadSyscallException(__FILE__, __LINE__);
     }
+    _detached = true;
 }
 
 void
 IceUtil::ThreadControl::detach()
 {
-    if(_handle->handle)
+    if(_detached)
     {
-	if (_detached)
-	{
-	    throw ThreadSyscallException(__FILE__, __LINE__);
-	}
-	_detached = true;
-    }
-}
-

-bool

-IceUtil::ThreadControl::isAlive() const

-{

-    if(!_handle->handle)

-    {

-	return false;

-    }

-

-    DWORD rc;

-    if(GetExitCodeThread(_handle->handle, &rc) == 0)

-    {

-	return false;

-    }

-    return rc == STILL_ACTIVE;

-}

+	throw ThreadSyscallException(__FILE__, __LINE__);
+    }
+    _detached = true;
+}
+
+bool
+IceUtil::ThreadControl::isAlive() const
+{
+    DWORD rc;
+    if(GetExitCodeThread(_handle->handle, &rc) == 0)
+    {
+	return false;
+    }
+    return rc == STILL_ACTIVE;
+}
 
 void
 IceUtil::ThreadControl::sleep(const Time& timeout)
@@ -139,6 +134,11 @@ IceUtil::Thread::~Thread()
 IceUtil::Thread::ThreadId
 IceUtil::Thread::id() const
 {
+    IceUtil::Mutex::Lock lock(_stateMutex);
+    if(!_started)
+    {
+	throw ThreadNotStartedException(__FILE__, __LINE__);
+    }
     return _id;
 }
 
@@ -174,11 +174,12 @@ startHook(void* arg)
 IceUtil::ThreadControl
 IceUtil::Thread::start()
 {
+    IceUtil::Mutex::Lock lock(_stateMutex);
+
     if(_started)
     {
 	throw ThreadStartedException(__FILE__, __LINE__);
     }
-    _started = true;
 
     //
     // It's necessary to increment the reference count since
@@ -197,6 +198,8 @@ IceUtil::Thread::start()
 	__decRef();
 	throw ThreadSyscallException(__FILE__, __LINE__);
     }
+
+    _started = true;
 			
     return ThreadControl(_handle, _id);
 }
@@ -204,24 +207,71 @@ IceUtil::Thread::start()
 IceUtil::ThreadControl
 IceUtil::Thread::getThreadControl() const
 {
+    IceUtil::Mutex::Lock lock(_stateMutex);
+    if(!_started)
+    {
+	throw ThreadNotStartedException(__FILE__, __LINE__);
+    }
     return ThreadControl(_handle, _id);
 }
 
 bool
 IceUtil::Thread::operator==(const Thread& rhs) const
 {
+    {
+	IceUtil::Mutex::Lock lock(_stateMutex);
+	if(!_started)
+	{
+	    throw ThreadNotStartedException(__FILE__, __LINE__);
+	}
+    }
+    {
+	IceUtil::Mutex::Lock lock(rhs._stateMutex);
+	if(!rhs._started)
+	{
+	    throw ThreadNotStartedException(__FILE__, __LINE__);
+	}
+    }
     return _id == rhs._id;
 }
 
 bool
 IceUtil::Thread::operator!=(const Thread& rhs) const
 {
+    {
+	IceUtil::Mutex::Lock lock(_stateMutex);
+	if(!_started)
+	{
+	    throw ThreadNotStartedException(__FILE__, __LINE__);
+	}
+    }
+    {
+	IceUtil::Mutex::Lock lock(rhs._stateMutex);
+	if(!rhs._started)
+	{
+	    throw ThreadNotStartedException(__FILE__, __LINE__);
+	}
+    }
     return _id != rhs._id;
 }
 
 bool
 IceUtil::Thread::operator<(const Thread& rhs) const
 {
+    {
+	IceUtil::Mutex::Lock lock(_stateMutex);
+	if(!_started)
+	{
+	    throw ThreadNotStartedException(__FILE__, __LINE__);
+	}
+    }
+    {
+	IceUtil::Mutex::Lock lock(rhs._stateMutex);
+	if(!rhs._started)
+	{
+	    throw ThreadNotStartedException(__FILE__, __LINE__);
+	}
+    }
     return _id < rhs._id;
 }
 
@@ -258,41 +308,41 @@ IceUtil::ThreadControl::operator<(const ThreadControl& rhs) const
     return _id < rhs._id;
 }
 
+IceUtil::ThreadId
+IceUtil::ThreadControl::id() const
+{
+    return _id;
+}
+
 void
 IceUtil::ThreadControl::join()
 {
-    if(_id)
+    if(_detached)
     {
-	if (_detached)
-	{
-	    throw ThreadSyscallException(__FILE__, __LINE__);
-	}
-	_detached = true;
-	void* ignore = 0;
-	int rc = pthread_join(_id, &ignore);
-	if(rc != 0)
-	{
-	    throw ThreadSyscallException(__FILE__, __LINE__);
-	}
+	throw ThreadSyscallException(__FILE__, __LINE__);
     }
+    void* ignore = 0;
+    int rc = pthread_join(_id, &ignore);
+    if(rc != 0)
+    {
+	throw ThreadSyscallException(__FILE__, __LINE__);
+    }
+    _detached = true;
 }
 
 void
 IceUtil::ThreadControl::detach()
 {
-    if(_id)
+    if(_detached)
     {
-	if (_detached)
-	{
-	    throw ThreadSyscallException(__FILE__, __LINE__);
-	}
-	_detached = true;
-	int rc = pthread_detach(_id);
-	if(rc != 0)
-	{
-	    throw ThreadSyscallException(__FILE__, __LINE__);
-	}
+	throw ThreadSyscallException(__FILE__, __LINE__);
+    }
+    int rc = pthread_detach(_id);
+    if(rc != 0)
+    {
+	throw ThreadSyscallException(__FILE__, __LINE__);
     }
+    _detached = true;
 }
 
 bool
@@ -329,9 +379,14 @@ IceUtil::Thread::~Thread()
 {
 }
 
-IceUtil::Thread::ThreadId
+IceUtil::ThreadId
 IceUtil::Thread::id() const
 {
+    IceUtil::Mutex::Lock lock(_stateMutex);
+    if(!_started)
+    {
+	throw ThreadNotStartedException(__FILE__, __LINE__);
+    }
     return _id;
 }
 
@@ -367,11 +422,12 @@ startHook(void* arg)
 IceUtil::ThreadControl
 IceUtil::Thread::start()
 {
+    IceUtil::Mutex::Lock lock(_stateMutex);
+
     if(_started)
     {
 	throw ThreadStartedException(__FILE__, __LINE__);
     }
-    _started = true;
 
     //
     // It's necessary to increment the reference count since
@@ -389,33 +445,82 @@ IceUtil::Thread::start()
 	__decRef();
 	throw ThreadSyscallException(__FILE__, __LINE__);
     }
+
+    _started = true;
+
     return ThreadControl(_id);
 }
 
 IceUtil::ThreadControl
 IceUtil::Thread::getThreadControl() const
 {
+    IceUtil::Mutex::Lock lock(_stateMutex);
+    if(!_started)
+    {
+	throw ThreadNotStartedException(__FILE__, __LINE__);
+    }
     return ThreadControl(_id);
 }
 
 bool
 IceUtil::Thread::operator==(const Thread& rhs) const
 {
+    {
+	IceUtil::Mutex::Lock lock(_stateMutex);
+	if(!_started)
+	{
+	    throw ThreadNotStartedException(__FILE__, __LINE__);
+	}
+    }
+    {
+	IceUtil::Mutex::Lock lock(rhs._stateMutex);
+	if(!rhs._started)
+	{
+	    throw ThreadNotStartedException(__FILE__, __LINE__);
+	}
+    }
     return pthread_equal(_id, rhs._id);
 }
 
 bool
 IceUtil::Thread::operator!=(const Thread& rhs) const
 {
+    {
+	IceUtil::Mutex::Lock lock(_stateMutex);
+	if(!_started)
+	{
+	    throw ThreadNotStartedException(__FILE__, __LINE__);
+	}
+    }
+    {
+	IceUtil::Mutex::Lock lock(rhs._stateMutex);
+	if(!rhs._started)
+	{
+	    throw ThreadNotStartedException(__FILE__, __LINE__);
+	}
+    }
     return !pthread_equal(_id, rhs._id);
 }
 
 bool
 IceUtil::Thread::operator<(const Thread& rhs) const
 {
+    {
+	IceUtil::Mutex::Lock lock(_stateMutex);
+	if(!_started)
+	{
+	    throw ThreadNotStartedException(__FILE__, __LINE__);
+	}
+    }
+    {
+	IceUtil::Mutex::Lock lock(rhs._stateMutex);
+	if(!rhs._started)
+	{
+	    throw ThreadNotStartedException(__FILE__, __LINE__);
+	}
+    }
     // NOTE: Linux specific
     return _id < rhs._id;
 }
 
 #endif
-
diff --git a/cpp/src/IceUtil/ThreadException.cpp b/cpp/src/IceUtil/ThreadException.cpp
index 1139764a8ce..86fd1e20852 100644
--- a/cpp/src/IceUtil/ThreadException.cpp
+++ b/cpp/src/IceUtil/ThreadException.cpp
@@ -125,3 +125,26 @@ IceUtil::ThreadStartedException::ice_throw() const
 {
     throw *this;
 }
+
+IceUtil::ThreadNotStartedException::ThreadNotStartedException(const char* file, int line) :
+    Exception(file, line)
+{
+}
+
+string
+IceUtil::ThreadNotStartedException::ice_name() const
+{
+    return "IceUtil::ThreadNotStartedException";
+}
+
+IceUtil::Exception*
+IceUtil::ThreadNotStartedException::ice_clone() const
+{
+    return new ThreadNotStartedException(*this);
+}
+
+void
+IceUtil::ThreadNotStartedException::ice_throw() const
+{
+    throw *this;
+}
diff --git a/cpp/test/IceUtil/thread/AliveTest.cpp b/cpp/test/IceUtil/thread/AliveTest.cpp
index 63c4db28100..351a07943de 100644
--- a/cpp/test/IceUtil/thread/AliveTest.cpp
+++ b/cpp/test/IceUtil/thread/AliveTest.cpp
@@ -22,48 +22,49 @@
 using namespace std;
 using namespace IceUtil;
 
-static const string createTestName("thread alive");

-

-class Synchronizer : public IceUtil::Monitor<IceUtil::Mutex>

-{

-public:

-    Synchronizer() : _done(false)

-    {

-    }

-

-    void p()

-    {

-	IceUtil::Monitor<IceUtil::Mutex>::Lock lock(*this);

-	while (!_done)

-	{

-	    wait();

-	}

-    }

-

-    void v()

-    {

-	IceUtil::Monitor<IceUtil::Mutex>::Lock lock(*this);

-	_done = true;

-	notify();

-    }

-

-private:

-    bool _done;

+static const string createTestName("thread alive");
+
+class CondVar : public IceUtil::Monitor<IceUtil::Mutex>
+{
+public:
+    CondVar() : _done(false)
+    {
+    }
+
+    void wait()
+    {
+	IceUtil::Monitor<IceUtil::Mutex>::Lock lock(*this);
+	while (!_done)
+	{
+	    this->IceUtil::Monitor<IceUtil::Mutex>::wait();
+	}
+    }
+
+    void signal()
+    {
+	IceUtil::Monitor<IceUtil::Mutex>::Lock lock(*this);
+	_done = true;
+	notify();
+    }
+
+private:
+    bool _done;
 };
 
 class AliveTestThread : public Thread
 {
-public:

-    AliveTestThread(Synchronizer& child, Synchronizer& parent) : _child(child), _parent(parent)

-    {

-    }

+public:
+    AliveTestThread(CondVar& childCreated, CondVar& parentReady) :
+	_childCreated(childCreated), _parentReady(parentReady)
+    {
+    }
 
     virtual void run()
     {
 	try
-	{

-	   _child.v();

-	   _parent.p();
+	{
+	   _childCreated.signal();
+	   _parentReady.wait();
 	}
 	catch(IceUtil::ThreadLockedException &)
 	{
@@ -71,8 +72,8 @@ public:
     }
 
 private:
-    Synchronizer& _child;

-    Synchronizer& _parent;
+    CondVar& _childCreated;
+    CondVar& _parentReady;
 };
 
 typedef Handle<AliveTestThread> AliveTestThreadPtr;
@@ -88,14 +89,14 @@ AliveTest::run()
     //
     // Check that calling isAlive() returns the correct result for alive and
     // and dead threads.
-    //

-    Synchronizer parentReady;

-    Synchronizer childReady;
-    AliveTestThreadPtr t = new AliveTestThread(childReady, parentReady);
-    IceUtil::ThreadControl c = t->start();

-    childReady.p();

-    test(c.isAlive());

-    parentReady.v();
+    //
+    CondVar childCreated;
+    CondVar parentReady;
+    AliveTestThreadPtr t = new AliveTestThread(childCreated, parentReady);
+    IceUtil::ThreadControl c = t->start();
+    childCreated.wait();
+    test(c.isAlive());
+    parentReady.signal();
     c.join();
     test(!c.isAlive());
 }