Clarify behaviour and remove some redundant code - see ChangeLogs

1 files changed, 112 insertions, 51 deletions

diff --git a/pthread_join.c b/pthread_join.c
index 97cbd51..89ee1a5 100644
--- a/pthread_join.c
+++ b/pthread_join.c
@@ -83,22 +83,62 @@ pthread_join (pthread_t thread, void **value_ptr)
   int result;
   pthread_t self;
 
-  /* This is the proper way to test for a valid thread ID */
-  result = pthread_kill (thread, 0);
-  if (0 != result)
+  /*
+   * Possibilities for the target thread on entry to pthread_join():
+   *
+   * 1) the target thread is detached, in which case it could be destroyed and
+   * it's thread id (struct) reused before we get the reuse lock;
+   * 2) the target thread is joinable, in which case it will not be destroyed
+   * until it has been joined, and it's thread id cannot be reused before we
+   * get access to it.
+   *
+   * Only (1) is a potential race condition.
+   * While (1) is possibly an application programming error, pthread_join is
+   * required to handle it with an error.
+   * If an application attempts to join a detached thread that exits before we
+   * get the reuse lock, it's thread struct could be reused for a new joinable
+   * thread before we get the reuse_lock and we will then be joining the wrong
+   * thread.
+   *
+   * To fix this properly would require a reuse count as part of the thread id
+   * so that we could confirm that we are working with the same thread. This
+   * option will require a major change to the API, forcing recompilation of
+   * applications that use the library.
+   *
+   * Cheaper alternatives that do not change the API are:
+   * - separate reuse stacks could be used for detached and joinable
+   * threads. Threads which detach themselves will not present a race condition
+   * because they will be placed on a different stack to that which provides
+   * reusable structs to new joinable threads.
+   * The problem with this solution is that, an application that creates
+   * joinable threads which then detach themselves can cause the detach reuse
+   * stack to grow indefinitely.
+   *
+   * - maintain a sufficiently large reuse pool and manage it as a FIFO
+   * queue. This would prevent the immediate reuse of thread structs, but this
+   * only provides a statistical safeguard, not a deterministic one.
+   *
+   * - include an entry point array at the start of each thread struct so that
+   * each reuse (modulo N) returns a different thread id (pointer to the thread
+   * struct). Offset 0 from this pointer will contain a validity field and an
+   * offset value field (subtracted from the pointer to obtain the address of
+   * thread struct). This option is similar to including a reuse counter with
+   * the thread id but maintains the thread id as a simple pointer.
+   *
+   * As at 27/08/2004, none of the above have been implemented.
+   */
+
+  EnterCriticalSection (&ptw32_thread_reuse_lock);
+  /*
+   * The first test is the same as pthread_kill(thread, 0), duplicated here
+   * so that we can use the reuse_lock to ensure the thread isn't destroyed
+   * and reused before we've finished with the POSIX thread struct.
+   */
+  if (NULL == thread
+      || NULL == thread->threadH
+      || THREAD_PRIORITY_ERROR_RETURN == GetThreadPriority (thread->threadH))
     {
-      return result;
-    }
-
-  self = pthread_self ();
-  if (NULL == self)
-    {
-      return ENOENT;
-    }
-
-  if (0 != pthread_equal (self, thread))
-    {
-      result = EDEADLK;
+      result = ESRCH;
     }
   else if (PTHREAD_CREATE_DETACHED == thread->detachState)
     {
@@ -106,60 +146,81 @@ pthread_join (pthread_t thread, void **value_ptr)
     }
   else
     {
-      /*
-       * Pthread_join is a cancelation point.
-       * If we are canceled then our target thread must not be
-       * detached (destroyed). This is guarranteed because
-       * pthreadCancelableWait will not return if we
-       * are canceled.
+      /* 
+       * The target thread is joinable and can't be reused before we join it.
        */
-      result = pthreadCancelableWait (thread->threadH);
 
-      if (result == 0)
+      LeaveCriticalSection (&ptw32_thread_reuse_lock);
+
+      if (NULL == (self = pthread_self ()))
+	{
+	  result = ENOENT;
+	}
+      else if (0 != pthread_equal (self, thread))
+	{
+	  result = EDEADLK;
+	}
+      else
 	{
+	  /*
+	   * Pthread_join is a cancelation point.
+	   * If we are canceled then our target thread must not be
+	   * detached (destroyed). This is guarranteed because
+	   * pthreadCancelableWait will not return if we
+	   * are canceled.
+	   */
+	  result = pthreadCancelableWait (thread->threadH);
+
+	  if (0 == result)
+	    {
 
 #if ! defined (__MINGW32__) || defined (__MSVCRT__) || defined (__DMC__)
 
-	  if (value_ptr != NULL
-	      && !GetExitCodeThread (thread->threadH, (LPDWORD) value_ptr))
-	    {
-	      result = ESRCH;
-	    }
-	  else
-	    {
+	      if (value_ptr != NULL
+		  && !GetExitCodeThread (thread->threadH, (LPDWORD) value_ptr))
+		{
+		  result = ESRCH;
+		}
+	      else
+		{
+		  /*
+		   * The result of making multiple simultaneous calls to
+		   * pthread_join() specifying the same target is undefined.
+		   */
+		  ptw32_threadDestroy (thread);
+		}
+
+#else /* __MINGW32__ && ! __MSVCRT__ */
+
+	      /*
+	       * If using CRTDLL, the thread may have exited, and endthread
+	       * will have closed the handle.
+	       */
+	      if (value_ptr != NULL)
+		{
+		  *value_ptr = thread->exitStatus;
+		}
+
 	      /*
 	       * The result of making multiple simultaneous calls to
 	       * pthread_join() specifying the same target is undefined.
 	       */
 	      ptw32_threadDestroy (thread);
-	    }
 
-#else /* __MINGW32__ && ! __MSVCRT__ */
+#endif /* __MINGW32__ && ! __MSVCRT__ */
 
-	  /*
-	   * If using CRTDLL, the thread may have exited, and endthread
-	   * will have closed the handle.
-	   */
-	  if (value_ptr != NULL)
+	    }
+	  else
 	    {
-	      *value_ptr = thread->exitStatus;
+	      result = ESRCH;
 	    }
-
-	  /*
-	   * The result of making multiple simultaneous calls to
-	   * pthread_join() specifying the same target is undefined.
-	   */
-	  ptw32_threadDestroy (thread);
-
-#endif /* __MINGW32__ && ! __MSVCRT__ */
-
-	}
-      else
-	{
-	  result = ESRCH;
 	}
+
+      goto FAIL0;
     }
 
+  LeaveCriticalSection (&ptw32_thread_reuse_lock);
+FAIL0:
   return (result);
 
 }				/* pthread_join */