Better fix for bug #824

6 files changed, 185 insertions, 133 deletions

diff --git a/cpp/include/IceUtil/ThreadException.h b/cpp/include/IceUtil/ThreadException.h
index 7faa1f54271..be425f12136 100644
--- a/cpp/include/IceUtil/ThreadException.h
+++ b/cpp/include/IceUtil/ThreadException.h
@@ -73,6 +73,20 @@ private:
 
     static const char* _name;
 };
+
+class ICE_UTIL_API BadThreadControlException : public Exception
+{
+public:
+
+    BadThreadControlException(const char*, int);
+    virtual const std::string ice_name() const;
+    virtual Exception* ice_clone() const;
+    virtual void ice_throw() const;
+
+private:
+
+    static const char* _name;
+};
     
 }
 
diff --git a/cpp/src/IceUtil/ThreadException.cpp b/cpp/src/IceUtil/ThreadException.cpp
index abaa79f26a6..97ba03c47cd 100644
--- a/cpp/src/IceUtil/ThreadException.cpp
+++ b/cpp/src/IceUtil/ThreadException.cpp
@@ -154,3 +154,29 @@ IceUtil::ThreadNotStartedException::ice_throw() const
 {
     throw *this;
 }
+
+
+IceUtil::BadThreadControlException::BadThreadControlException(const char* file, int line) :
+    Exception(file, line)
+{
+}
+
+const char* IceUtil::BadThreadControlException::_name = "IceUtil::BadThreadControlException";
+
+const string
+IceUtil::BadThreadControlException::ice_name() const
+{
+    return _name;
+}
+
+IceUtil::Exception*
+IceUtil::BadThreadControlException::ice_clone() const
+{
+    return new BadThreadControlException(*this);
+}
+
+void
+IceUtil::BadThreadControlException::ice_throw() const
+{
+    throw *this;
+}
diff --git a/cppe/include/IceE/Thread.h b/cppe/include/IceE/Thread.h
index 031010ef90e..f966bfac639 100644
--- a/cppe/include/IceE/Thread.h
+++ b/cppe/include/IceE/Thread.h
@@ -19,45 +19,30 @@ namespace IceUtil
 
 class Time;
 
-#ifdef _WIN32
-struct HandleWrapper : public Shared
-{
-    // Inline for performance reasons.
-    HandleWrapper(HANDLE h, bool r = true) :
-	handle(h), release(r)
-    {
-    }
-
-    // Inline for performance reasons.
-    virtual ~HandleWrapper()
-    {
-	if(handle && release)
-	{
-	    CloseHandle(handle);
-	}
-    }
-
-    HANDLE handle;
-    bool release;
-};
-typedef Handle<HandleWrapper> HandleWrapperPtr;
-#endif
-
 class ICE_API ThreadControl
 {
 public:
 
+    //
+    // Constructs a ThreadControl representing the current thread.
+    // join and detach cannot be called on such ThreadControl object.
+    //
     ThreadControl();
 
 #ifdef _WIN32
-    ThreadControl(const HandleWrapperPtr&, DWORD);
+    ThreadControl(HANDLE, DWORD);
 #else
     ThreadControl(pthread_t);
 #endif
 
-    ThreadControl(const 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;
 
@@ -100,10 +85,17 @@ public:
 private:
 
 #ifdef _WIN32
-    DWORD _id;
-    HandleWrapperPtr _handle;
+    HANDLE _handle;
+    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.
+    //
+    bool _detachable;
 #endif
 };
 
@@ -125,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;
 
@@ -136,19 +127,19 @@ public:
     //
     void _done();
 
-
 protected:
     Mutex _stateMutex;
     bool _started;
     bool _running;
 
 #ifdef _WIN32
-    HandleWrapperPtr _handle;
-    DWORD _id;
+    HANDLE _handle;
+    DWORD  _id;
 #else
     pthread_t _thread;
 #endif
 
+private:
     Thread(const Thread&);		// Copying is forbidden
     void operator=(const Thread&);	// Assignment is forbidden
 };
diff --git a/cppe/include/IceE/ThreadException.h b/cppe/include/IceE/ThreadException.h
index ba71918b422..e65a4522a4a 100644
--- a/cppe/include/IceE/ThreadException.h
+++ b/cppe/include/IceE/ThreadException.h
@@ -73,6 +73,20 @@ private:
 
     static const char* _name;
 };
+
+class ICE_API BadThreadControlException : public Exception
+{
+public:
+
+    BadThreadControlException(const char*, int);
+    virtual const std::string ice_name() const;
+    virtual Exception* ice_clone() const;
+    virtual void ice_throw() const;
+
+private:
+
+    static const char* _name;
+};
     
 }
 
diff --git a/cppe/src/IceE/Thread.cpp b/cppe/src/IceE/Thread.cpp
index af7cf031eda..35d3b411686 100644
--- a/cppe/src/IceE/Thread.cpp
+++ b/cppe/src/IceE/Thread.cpp
@@ -13,70 +13,20 @@
 
 #include <stdio.h>
 
-using namespace std;
-
 #ifdef _WIN32
 
-#ifndef _WIN32_WCE
-#   include <process.h>
-#endif
-
-#ifdef _WIN32_WCE
-//
-// Under WCE it is not possible to call DuplicateHandle on a thread
-// handle. Instead we use the handle wrapper. This constructor uses
-// GetCurrentThreadId() to get the current thread object. This object
-// can also be used as the thread handle.
-//
 IceUtil::ThreadControl::ThreadControl() :
-    _id(GetCurrentThreadId()),
-    _handle(new HandleWrapper(reinterpret_cast<HANDLE>(_id), false))
+    _handle(0),
+    _id(GetCurrentThreadId())
 {
 }
-#else
-IceUtil::ThreadControl::ThreadControl() :
-    _id(GetCurrentThreadId()),
-    _handle(new HandleWrapper(0))
-{
-    HANDLE currentThread = GetCurrentThread();
-    HANDLE currentProcess = GetCurrentProcess();   
-
-    if(DuplicateHandle(currentProcess,
-		       currentThread,
-		       currentProcess,
-		       &_handle->handle,
-		       0,
-		       FALSE,
-		       DUPLICATE_SAME_ACCESS) == 0)
-    {
-	throw ThreadSyscallException(__FILE__, __LINE__, GetLastError());
-    }
-}
-#endif
 
-IceUtil::ThreadControl::ThreadControl(const HandleWrapperPtr& handle, IceUtil::ThreadControl::ID id) :
-    _id(id),
-    _handle(handle)
+IceUtil::ThreadControl::ThreadControl(HANDLE handle, IceUtil::ThreadControl::ID id) :
+    _handle(handle),
+    _id(id)
 {
 }
 
-IceUtil::ThreadControl::ThreadControl(const ThreadControl& tc) :
-    _id(tc._id),
-    _handle(tc._handle)
-{
-}
-
-IceUtil::ThreadControl&
-IceUtil::ThreadControl::operator=(const ThreadControl& rhs)
-{
-    if(&rhs != this)
-    {
-	_id = rhs._id;
-	_handle = rhs._handle;
-    }
-    return *this;
-}
-
 bool
 IceUtil::ThreadControl::operator==(const ThreadControl& rhs) const
 {
@@ -92,17 +42,32 @@ IceUtil::ThreadControl::operator!=(const ThreadControl& rhs) const
 void
 IceUtil::ThreadControl::join()
 {
-    int rc = WaitForSingleObject(_handle->handle, INFINITE);
+    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()
 {
-    // No-op: Windows doesn't have the concept of detaching a thread.
+    if(_handle == 0)
+    {
+	throw BadThreadControlException(__FILE__, __LINE__);
+    }
+    
+    if(CloseHandle(_handle) == 0)
+    {
+	throw ThreadSyscallException(__FILE__, __LINE__, GetLastError());
+    }
 }
 
 IceUtil::ThreadControl::ID
@@ -131,7 +96,7 @@ IceUtil::ThreadControl::yield()
 IceUtil::Thread::Thread() :
     _started(false),
     _running(false),
-    _handle(new HandleWrapper(0)),
+    _handle(0),
     _id(0)
 {
 }
@@ -140,14 +105,14 @@ IceUtil::Thread::~Thread()
 {
 }
 
-static void*
-startHook(void* arg)
+static unsigned int
+WINAPI startHook(void* arg)
 {
-    //

-    // Ensure that the thread doesn't go away until run() has

-    // completed.

-    //

-    IceUtil::ThreadPtr thread;

+    // Ensure that the thread doesn't go away until run() has
+    // completed.
+    //
+    IceUtil::ThreadPtr thread;
+
     try
     {
 	IceUtil::Thread* rawThread = static_cast<IceUtil::Thread*>(arg);
@@ -158,6 +123,10 @@ startHook(void* arg)
         unsigned int seed = static_cast<unsigned int>(IceUtil::Time::now().toMicroSeconds());
         srand(seed);            
 
+	//
+	// Ensure that the thread doesn't go away until run() has
+	// completed.
+	//
 	thread = rawThread;
 
 	//
@@ -169,12 +138,20 @@ startHook(void* arg)
     catch(const IceUtil::Exception& e)
     {
 	fprintf(stderr, "IceUtil::Thread::run(): uncaught exception: %s\n", e.toString());
+    } 
+    catch(...)
+    {
+	fprintf(stderr, "IceUtil::Thread::run(): uncaught exception\n");
     }
     thread->_done();
-
+   
     return 0;
 }
 
+#ifndef _WIN32_WCE
+#   include <process.h>
+#endif
+
 IceUtil::ThreadControl
 IceUtil::Thread::start(size_t stackSize)
 {
@@ -200,17 +177,21 @@ IceUtil::Thread::start(size_t stackSize)
     // __decRef().
     //
     __incRef();
-
-#ifndef _WIN32_WCE
-    unsigned int id;

-    _handle->handle = (HANDLE)_beginthreadex(
-	0, stackSize, (unsigned int (__stdcall*)(void*))startHook, (LPVOID)this, 0, &id);
-    _id = id;

-#else

-    _handle->handle = CreateThread(
-	0, stackSize, (unsigned long (__stdcall*)(void*))startHook, (LPVOID)this, 0, &_id);

+    
+#ifdef _WIN32_WCE
+    _handle = CreateThread(0, stackSize,
+			   startHook, this, 0, &_id);
+#else
+    unsigned int id;
+    _handle = 
+	reinterpret_cast<HANDLE>(
+	    _beginthreadex(0, 
+			   static_cast<unsigned int>(stackSize), 
+			   startHook, this, 0, &id));
+    _id = id;
 #endif
-    if(_handle->handle == 0)
+   
+    if(_handle == 0)
     {
 	__decRef();
 	throw ThreadSyscallException(__FILE__, __LINE__, GetLastError());
@@ -242,7 +223,7 @@ IceUtil::Thread::operator==(const Thread& rhs) const
 bool
 IceUtil::Thread::operator!=(const Thread& rhs) const
 {
-    return !operator==(rhs);
+    return this != &rhs;
 }
 
 bool
@@ -265,33 +246,21 @@ IceUtil::Thread::_done()
     _running = false;
 }
 
+
 #else
 
 IceUtil::ThreadControl::ThreadControl(pthread_t thread) :
-    _thread(thread)
+    _thread(thread),
+    _detachable(true)
 {
 }
 
 IceUtil::ThreadControl::ThreadControl() :
-    _thread(pthread_self())
-{
-}
-
-IceUtil::ThreadControl::ThreadControl(const ThreadControl& tc) :
-    _thread(tc._thread)
+    _thread(pthread_self()),
+    _detachable(false)
 {
 }
 
-IceUtil::ThreadControl&
-IceUtil::ThreadControl::operator=(const ThreadControl& rhs)
-{
-    if(&rhs != this)
-    {
-	_thread = rhs._thread;
-    }
-    return *this;
-}
-
 bool
 IceUtil::ThreadControl::operator==(const ThreadControl& rhs) const
 {
@@ -307,6 +276,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)
@@ -318,6 +292,11 @@ IceUtil::ThreadControl::join()
 void
 IceUtil::ThreadControl::detach()
 {
+    if(!_detachable)
+    {
+	throw BadThreadControlException(__FILE__, __LINE__);
+    }
+
     int rc = pthread_detach(_thread);
     if(rc != 0)
     {
@@ -328,7 +307,7 @@ IceUtil::ThreadControl::detach()
 IceUtil::ThreadControl::ID
 IceUtil::ThreadControl::id() const
 {
-    return _thread;
+    return _thread;;
 }
 
 void
@@ -357,7 +336,8 @@ IceUtil::Thread::~Thread()
 {
 }
 
-extern "C" {
+extern "C" 
+{
 static void*
 startHook(void* arg)
 {
@@ -366,6 +346,7 @@ startHook(void* arg)
     // completed.
     //
     IceUtil::ThreadPtr thread;
+
     try
     {
 	IceUtil::Thread* rawThread = static_cast<IceUtil::Thread*>(arg);
@@ -380,14 +361,14 @@ startHook(void* arg)
     }
     catch(const IceUtil::Exception& e)
     {
-	//fprintf(stderr, "IceUtil::Thread::run(): uncaught exception: %s\n", e.toString());
+	fprintf(stderr, "IceUtil::Thread::run(): uncaught exception: %s\n", e.toString());
     }
     catch(...)
     {
 	fprintf(stderr, "IceUtil::Thread::run(): uncaught exception\n");
     }
     thread->_done();
-
+    
     return 0;
 }
 }
@@ -476,7 +457,7 @@ IceUtil::Thread::operator==(const Thread& rhs) const
 bool
 IceUtil::Thread::operator!=(const Thread& rhs) const
 {
-    return !operator==(rhs);
+    return this != &rhs;
 }
 
 bool
diff --git a/cppe/src/IceE/ThreadException.cpp b/cppe/src/IceE/ThreadException.cpp
index fb096379644..26f9ef6c1d8 100644
--- a/cppe/src/IceE/ThreadException.cpp
+++ b/cppe/src/IceE/ThreadException.cpp
@@ -159,3 +159,29 @@ IceUtil::ThreadNotStartedException::ice_throw() const
 {
     throw *this;
 }
+
+IceUtil::BadThreadControlException::BadThreadControlException(const char* file, int line) :
+    Exception(file, line)
+{
+}
+
+const char* IceUtil::BadThreadControlException::_name = "IceUtil::BadThreadControlException";
+
+const string
+IceUtil::BadThreadControlException::ice_name() const
+{
+    return _name;
+}
+
+IceUtil::Exception*
+IceUtil::BadThreadControlException::ice_clone() const
+{
+    return new BadThreadControlException(*this);
+}
+
+void
+IceUtil::BadThreadControlException::ice_throw() const
+{
+    throw *this;
+}
+