Better fix for bug #824

2 files changed, 51 insertions, 104 deletions

diff --git a/cpp/include/IceUtil/Thread.h b/cpp/include/IceUtil/Thread.h
index 76b4ca82313..5f2e6b837c4 100644
--- a/cpp/include/IceUtil/Thread.h
+++ b/cpp/include/IceUtil/Thread.h
@@ -23,6 +23,10 @@ class ICE_UTIL_API ThreadControl
 {
 public:
 
+    //
+    // Constructs a ThreadControl representing the current thread.
+    // join and detach cannot be called on such ThreadControl object.
+    //
     ThreadControl();
 
 #ifdef _WIN32
@@ -31,12 +35,14 @@ public:
     ThreadControl(pthread_t);
 #endif
 
-    ThreadControl(const ThreadControl&);
-
-    ~ThreadControl();
-
-    ThreadControl& operator=(const ThreadControl&);
+    //
+    // Default copy destructor, assignment operator and destructor OK
+    //
 
+    //
+    // == and != are meaningful only before the thread is joined/detached,
+    // or while the thread is still running.
+    //
     bool operator==(const ThreadControl&) const;
     bool operator!=(const ThreadControl&) const;
 
@@ -83,6 +89,13 @@ private:
     DWORD  _id;
 #else
     pthread_t _thread;
+
+    //
+    // Used to prevent joining/detaching a ThreadControl constructed
+    // with the default constructor. Only needed to enforce our
+    // portable join/detach behavior.
+    //
+    const bool _detachable;
 #endif
 };
 
@@ -104,8 +117,7 @@ public:
     bool operator<(const Thread&) const;
 
     //
-    // Check whether a thread is still alive. This is useful to implement
-    // a non-blocking join().
+    // Check whether a thread is still alive.
     //
     bool isAlive() const;
 
diff --git a/cpp/src/IceUtil/Thread.cpp b/cpp/src/IceUtil/Thread.cpp
index de33d92d928..50dc6cf5d47 100644
--- a/cpp/src/IceUtil/Thread.cpp
+++ b/cpp/src/IceUtil/Thread.cpp
@@ -19,75 +19,12 @@ IceUtil::ThreadControl::ThreadControl() :
     _handle(0),
     _id(GetCurrentThreadId())
 {
-    HANDLE currentThread = GetCurrentThread();
-    HANDLE currentProcess = GetCurrentProcess();   
-
-    if(DuplicateHandle(currentProcess,
-		       currentThread,
-		       currentProcess,
-		       &_handle,
-		       0,
-		       FALSE,
-		       DUPLICATE_SAME_ACCESS) == 0)
-    {
-	throw ThreadSyscallException(__FILE__, __LINE__, GetLastError());
-    }
 }
 
 IceUtil::ThreadControl::ThreadControl(HANDLE handle, IceUtil::ThreadControl::ID id) :
+    _handle(handle),
     _id(id)
 {
-    HANDLE currentProcess = GetCurrentProcess();    
-
-    if(DuplicateHandle(currentProcess,
-		       handle,
-		       currentProcess,
-		       &_handle,
-		       0,
-		       FALSE,
-		       DUPLICATE_SAME_ACCESS) == 0)
-    {
-	throw ThreadSyscallException(__FILE__, __LINE__, GetLastError());
-    }
-}
-  
-IceUtil::ThreadControl::ThreadControl(const ThreadControl& tc) :
-    _handle(0)
-{
-    operator=(tc);
-}
-
-IceUtil::ThreadControl::~ThreadControl()
-{
-    assert(_handle != 0);
-    bool ok = CloseHandle(_handle);
-    assert(ok);
-}
-
-IceUtil::ThreadControl&
-IceUtil::ThreadControl::operator=(const ThreadControl& rhs)
-{
-    if(&rhs != this)
-    {
-	if(_handle != 0 && CloseHandle(_handle) == 0)
-	{
-	    throw ThreadSyscallException(__FILE__, __LINE__, GetLastError());
-	}	
-	_id = rhs._id;
-
-	HANDLE currentProcess = GetCurrentProcess(); 
-	if(DuplicateHandle(currentProcess,
-			   rhs._handle,
-			   currentProcess,
-			   &_handle,
-			   0,
-			   FALSE,
-			   DUPLICATE_SAME_ACCESS) == 0)
-	{
-	    throw ThreadSyscallException(__FILE__, __LINE__, GetLastError());
-	}
-    }
-    return *this;
 }
 
 bool
@@ -102,22 +39,35 @@ IceUtil::ThreadControl::operator!=(const ThreadControl& rhs) const
     return _id != rhs._id;
 }
 
-
 void
 IceUtil::ThreadControl::join()
 {
+    if(_handle == 0)
+    {
+	throw BadThreadControlException(__FILE__, __LINE__);
+    }
+
     int rc = WaitForSingleObject(_handle, INFINITE);
     if(rc != WAIT_OBJECT_0)
     {
 	throw ThreadSyscallException(__FILE__, __LINE__, GetLastError());
     }
+    
+    detach();
 }
 
 void
 IceUtil::ThreadControl::detach()
 {
-    // On Windows, a thread is cleaned up when the last handle to this thread
-    // is closed.
+    if(_handle == 0)
+    {
+	throw BadThreadControlException(__FILE__, __LINE__);
+    }
+    
+    if(CloseHandle(_handle) == 0)
+    {
+	throw ThreadSyscallException(__FILE__, __LINE__, GetLastError());
+    }
 }
 
 IceUtil::ThreadControl::ID
@@ -126,7 +76,6 @@ IceUtil::ThreadControl::id() const
     return _id;
 }
 
-
 void
 IceUtil::ThreadControl::sleep(const Time& timeout)
 {
@@ -154,11 +103,6 @@ IceUtil::Thread::Thread() :
 
 IceUtil::Thread::~Thread()
 {
-    if(_handle != 0)
-    {
-	bool ok = CloseHandle(_handle);
-	assert(ok);
-    }
 }
 
 static unsigned int
@@ -293,38 +237,20 @@ IceUtil::Thread::_done()
     _running = false;
 }
 
-
 #else
 
-IceUtil::ThreadControl::ThreadControl(IceUtil::ThreadControl::ID thread) :
-    _thread(thread)
+IceUtil::ThreadControl::ThreadControl(pthread_t thread) :
+    _thread(thread),
+    _detachable(true)
 {
 }
 
 IceUtil::ThreadControl::ThreadControl() :
-    _thread(pthread_self())
+    _thread(pthread_self()),
+    _detachable(false)
 {
 }
 
-IceUtil::ThreadControl::ThreadControl(const ThreadControl& tc) :
-    _thread(tc._thread)
-{
-}
-
-IceUtil::ThreadControl::~ThreadControl()
-{
-}
-
-IceUtil::ThreadControl&
-IceUtil::ThreadControl::operator=(const ThreadControl& rhs)
-{
-    if(&rhs != this)
-    {
-	_thread = rhs._thread;
-    }
-    return *this;
-}
-
 bool
 IceUtil::ThreadControl::operator==(const ThreadControl& rhs) const
 {
@@ -340,6 +266,11 @@ IceUtil::ThreadControl::operator!=(const ThreadControl& rhs) const
 void
 IceUtil::ThreadControl::join()
 {
+    if(!_detachable)
+    {
+	throw BadThreadControlException(__FILE__, __LINE__);
+    }
+
     void* ignore = 0;
     int rc = pthread_join(_thread, &ignore);
     if(rc != 0)
@@ -351,6 +282,11 @@ IceUtil::ThreadControl::join()
 void
 IceUtil::ThreadControl::detach()
 {
+    if(!_detachable)
+    {
+	throw BadThreadControlException(__FILE__, __LINE__);
+    }
+
     int rc = pthread_detach(_thread);
     if(rc != 0)
     {
@@ -390,7 +326,6 @@ IceUtil::Thread::~Thread()
 {
 }
 
-
 extern "C" 
 {
 static void*