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

4 files changed, 201 insertions, 71 deletions

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;
+}