apr_thread: destroy the thread's pool at _join() time, unless _detach()ed.

Destroying a joinable thread pool from apr_thread_exit() or when the thread
function returns, i.e. from inside the thread itself, is racy or deadlocky
with APR_POOL_DEBUG, with the parent pool being destroyed.

This commit adds a ->detached flag in each arch's apr_thread_t struct to track
whether a thread is detached (either at _create() or _detach() time). If
detached, the pool is destroyed when the thread exits, otherwise when the
thread is joined with apr_thread_join().


git-svn-id: https://svn.apache.org/repos/asf/apr/apr/trunk@1884077 13f79535-47bb-0310-9956-ffa450edef68

5 files changed, 97 insertions, 28 deletions

diff --git a/threadproc/beos/thread.c b/threadproc/beos/thread.c
index c372f135e..61d7fd0d7 100644
--- a/threadproc/beos/thread.c
+++ b/threadproc/beos/thread.c
@@ -69,7 +69,9 @@ static void *dummy_worker(void *opaque)
 
     apr_pool_owner_set(thd->pool, 0);
     ret = thd->func(thd, thd->data);
-    apr_pool_destroy(thd->pool);
+    if (thd->detached) {
+        apr_pool_destroy(thd->pool);
+    }
     return ret;
 }
 
@@ -80,7 +82,7 @@ APR_DECLARE(apr_status_t) apr_thread_create(apr_thread_t **new, apr_threadattr_t
     int32 temp;
     apr_status_t stat;
     
-    (*new) = (apr_thread_t *)apr_palloc(pool, sizeof(apr_thread_t));
+    (*new) = (apr_thread_t *)apr_pcalloc(pool, sizeof(apr_thread_t));
     if ((*new) == NULL) {
         return APR_ENOMEM;
     }
@@ -88,6 +90,7 @@ APR_DECLARE(apr_status_t) apr_thread_create(apr_thread_t **new, apr_threadattr_t
     (*new)->data = data;
     (*new)->func = func;
     (*new)->exitval = -1;
+    (*new)->detached = (attr && apr_threadattr_detach_get(attr) == APR_DETACH);
 
     /* First we create the new thread...*/
 	if (attr)
@@ -126,14 +129,21 @@ int apr_os_thread_equal(apr_os_thread_t tid1, apr_os_thread_t tid2)
 
 APR_DECLARE(void) apr_thread_exit(apr_thread_t *thd, apr_status_t retval)
 {
-    apr_pool_destroy(thd->pool);
     thd->exitval = retval;
+    if (thd->detached) {
+        apr_pool_destroy(thd->pool);
+    }
     exit_thread ((status_t)(retval));
 }
 
 APR_DECLARE(apr_status_t) apr_thread_join(apr_status_t *retval, apr_thread_t *thd)
 {
     status_t rv = 0, ret;
+
+    if (thd->detached) {
+        return APR_EINVAL;
+    }
+
     ret = wait_for_thread(thd->td, &rv);
     if (ret == B_NO_ERROR) {
         *retval = rv;
@@ -145,6 +155,7 @@ APR_DECLARE(apr_status_t) apr_thread_join(apr_status_t *retval, apr_thread_t *th
          */
         if (thd->exitval != -1) {
             *retval = thd->exitval;
+            apr_pool_destroy(thd->pool);
             return APR_SUCCESS;
         } else 
             return ret;
@@ -153,7 +164,12 @@ APR_DECLARE(apr_status_t) apr_thread_join(apr_status_t *retval, apr_thread_t *th
 
 APR_DECLARE(apr_status_t) apr_thread_detach(apr_thread_t *thd)
 {
-	if (suspend_thread(thd->td) == B_NO_ERROR){
+    if (thd->detached) {
+        return APR_EINVAL;
+    }
+
+    if (suspend_thread(thd->td) == B_NO_ERROR) {
+        thd->detached = 1;
         return APR_SUCCESS;
     }
     else {
diff --git a/threadproc/netware/thread.c b/threadproc/netware/thread.c
index bf0396395..4864f5e57 100644
--- a/threadproc/netware/thread.c
+++ b/threadproc/netware/thread.c
@@ -71,7 +71,9 @@ static void *dummy_worker(void *opaque)
 
     apr_pool_owner_set(thd->pool, 0);
     ret = thd->func(thd, thd->data);
-    apr_pool_destroy(thd->pool);
+    if (thd->detached) {
+        apr_pool_destroy(thd->pool);
+    }
     return ret;
 }
 
@@ -102,7 +104,7 @@ apr_status_t apr_thread_create(apr_thread_t **new,
         stack_size = attr->stack_size;
     }
     
-    (*new) = (apr_thread_t *)apr_palloc(pool, sizeof(apr_thread_t));
+    (*new) = (apr_thread_t *)apr_pcalloc(pool, sizeof(apr_thread_t));
 
     if ((*new) == NULL) {
         return APR_ENOMEM;
@@ -111,6 +113,7 @@ apr_status_t apr_thread_create(apr_thread_t **new,
     (*new)->data = data;
     (*new)->func = func;
     (*new)->thread_name = (char*)apr_pstrdup(pool, threadName);
+    (*new)->detached = (attr && apr_threadattr_detach_get(attr) == APR_DETACH);
     
     stat = apr_pool_create(&(*new)->pool, pool);
     if (stat != APR_SUCCESS) {
@@ -162,7 +165,9 @@ void apr_thread_yield()
 void apr_thread_exit(apr_thread_t *thd, apr_status_t retval)
 {
     thd->exitval = retval;
-    apr_pool_destroy(thd->pool);
+    if (thd->detached) {
+        apr_pool_destroy(thd->pool);
+    }
     NXThreadExit(NULL);
 }
 
@@ -172,8 +177,13 @@ apr_status_t apr_thread_join(apr_status_t *retval,
     apr_status_t  stat;    
     NXThreadId_t dthr;
 
+    if (thd->detached) {
+        return APR_EINVAL;
+    }
+
     if ((stat = NXThreadJoin(thd->td, &dthr, NULL)) == 0) {
         *retval = thd->exitval;
+        apr_pool_destroy(thd->pool);
         return APR_SUCCESS;
     }
     else {
@@ -183,6 +193,11 @@ apr_status_t apr_thread_join(apr_status_t *retval,
 
 apr_status_t apr_thread_detach(apr_thread_t *thd)
 {
+    if (thd->detached) {
+        return APR_EINVAL;
+    }
+
+    thd->detached = 1;
     return APR_SUCCESS;
 }
 
diff --git a/threadproc/os2/thread.c b/threadproc/os2/thread.c
index 8781f932a..d7da4e128 100644
--- a/threadproc/os2/thread.c
+++ b/threadproc/os2/thread.c
@@ -71,7 +71,9 @@ static void apr_thread_begin(void *arg)
   apr_thread_t *thread = (apr_thread_t *)arg;
   apr_pool_owner_set(thread->pool, 0);
   thread->exitval = thread->func(thread, thread->data);
-  apr_pool_destroy(thread->pool);
+  if (thd->attr->attr & APR_THREADATTR_DETACHED) {
+      apr_pool_destroy(thread->pool);
+  }
 }
 
 
@@ -83,7 +85,7 @@ APR_DECLARE(apr_status_t) apr_thread_create(apr_thread_t **new, apr_threadattr_t
     apr_status_t stat;
     apr_thread_t *thread;
  
-    thread = (apr_thread_t *)apr_palloc(pool, sizeof(apr_thread_t));
+    thread = (apr_thread_t *)apr_pcalloc(pool, sizeof(apr_thread_t));
     *new = thread;
 
     if (thread == NULL) {
@@ -134,7 +136,9 @@ APR_DECLARE(apr_os_thread_t) apr_os_thread_current()
 APR_DECLARE(void) apr_thread_exit(apr_thread_t *thd, apr_status_t retval)
 {
     thd->exitval = retval;
-    apr_pool_destroy(thd->pool);
+    if (thd->attr->attr & APR_THREADATTR_DETACHED) {
+        apr_pool_destroy(thd->pool);
+    }
     _endthread();
 }
 
@@ -154,6 +158,9 @@ APR_DECLARE(apr_status_t) apr_thread_join(apr_status_t *retval, apr_thread_t *th
         rc = 0; /* Thread had already terminated */
 
     *retval = thd->exitval;
+    if (rc == 0) {
+        apr_pool_destroy(thd->pool);
+    }
     return APR_FROM_OS_ERROR(rc);
 }
 
@@ -161,6 +168,10 @@ APR_DECLARE(apr_status_t) apr_thread_join(apr_status_t *retval, apr_thread_t *th
 
 APR_DECLARE(apr_status_t) apr_thread_detach(apr_thread_t *thd)
 {
+    if (thd->attr->attr & APR_THREADATTR_DETACHED) {
+        return APR_EINVAL;
+    }
+
     thd->attr->attr |= APR_THREADATTR_DETACHED;
     return APR_SUCCESS;
 }
diff --git a/threadproc/unix/thread.c b/threadproc/unix/thread.c
index f76e6ce44..976a254fa 100644
--- a/threadproc/unix/thread.c
+++ b/threadproc/unix/thread.c
@@ -143,7 +143,9 @@ static void *dummy_worker(void *opaque)
 
     apr_pool_owner_set(thread->pool, 0);
     ret = thread->func(thread, thread->data);
-    apr_pool_destroy(thread->pool);
+    if (thread->detached) {
+        apr_pool_destroy(thread->pool);
+    }
     return ret;
 }
 
@@ -170,17 +172,17 @@ APR_DECLARE(apr_status_t) apr_thread_create(apr_thread_t **new,
 
     (*new)->data = data;
     (*new)->func = func;
+    (*new)->detached = (attr && apr_threadattr_detach_get(attr) == APR_DETACH);
+    stat = apr_pool_create(&(*new)->pool, pool);
+    if (stat != APR_SUCCESS) {
+        return stat;
+    }
 
     if (attr)
         temp = &attr->attr;
     else
         temp = NULL;
 
-    stat = apr_pool_create(&(*new)->pool, pool);
-    if (stat != APR_SUCCESS) {
-        return stat;
-    }
-
     if ((stat = pthread_create((*new)->td, temp, dummy_worker, (*new))) == 0) {
         return APR_SUCCESS;
     }
@@ -208,7 +210,9 @@ APR_DECLARE(void) apr_thread_exit(apr_thread_t *thd,
                                   apr_status_t retval)
 {
     thd->exitval = retval;
-    apr_pool_destroy(thd->pool);
+    if (thd->detached) {
+        apr_pool_destroy(thd->pool);
+    }
     pthread_exit(NULL);
 }
 
@@ -218,8 +222,13 @@ APR_DECLARE(apr_status_t) apr_thread_join(apr_status_t *retval,
     apr_status_t stat;
     apr_status_t *thread_stat;
 
+    if (thd->detached) {
+        return APR_EINVAL;
+    }
+
     if ((stat = pthread_join(*thd->td,(void *)&thread_stat)) == 0) {
         *retval = thd->exitval;
+        apr_pool_destroy(thd->pool);
         return APR_SUCCESS;
     }
     else {
@@ -235,11 +244,16 @@ APR_DECLARE(apr_status_t) apr_thread_detach(apr_thread_t *thd)
 {
     apr_status_t stat;
 
+    if (thd->detached) {
+        return APR_EINVAL;
+    }
+
 #ifdef HAVE_ZOS_PTHREADS
     if ((stat = pthread_detach(thd->td)) == 0) {
 #else
     if ((stat = pthread_detach(*thd->td)) == 0) {
 #endif
+        thd->detached = 1;
 
         return APR_SUCCESS;
     }
diff --git a/threadproc/win32/thread.c b/threadproc/win32/thread.c
index 429ac2aac..757a9f0d5 100644
--- a/threadproc/win32/thread.c
+++ b/threadproc/win32/thread.c
@@ -80,7 +80,9 @@ static void *dummy_worker(void *opaque)
     TlsSetValue(tls_apr_thread, thd->td);
     apr_pool_owner_set(thd->pool, 0);
     ret = thd->func(thd, thd->data);
-    apr_pool_destroy(thd->pool);
+    if (!thd->td) { /* detached? */
+        apr_pool_destroy(thd->pool);
+    }
     return ret;
 }
 
@@ -93,7 +95,7 @@ APR_DECLARE(apr_status_t) apr_thread_create(apr_thread_t **new,
 	unsigned temp;
     HANDLE handle;
 
-    (*new) = (apr_thread_t *)apr_palloc(pool, sizeof(apr_thread_t));
+    (*new) = (apr_thread_t *)apr_pcalloc(pool, sizeof(apr_thread_t));
 
     if ((*new) == NULL) {
         return APR_ENOMEM;
@@ -101,7 +103,6 @@ APR_DECLARE(apr_status_t) apr_thread_create(apr_thread_t **new,
 
     (*new)->data = data;
     (*new)->func = func;
-    (*new)->td   = NULL;
     stat = apr_pool_create(&(*new)->pool, pool);
     if (stat != APR_SUCCESS) {
         return stat;
@@ -136,9 +137,11 @@ APR_DECLARE(apr_status_t) apr_thread_create(apr_thread_t **new,
 
 APR_DECLARE(void) apr_thread_exit(apr_thread_t *thd, apr_status_t retval)
 {
+    thd->exited = 1;
     thd->exitval = retval;
-    apr_pool_destroy(thd->pool);
-    thd->pool = NULL;
+    if (!thd->td) { /* detached? */
+        apr_pool_destroy(thd->pool);
+    }
 #ifndef _WIN32_WCE
     _endthreadex(0);
 #else
@@ -150,30 +153,40 @@ APR_DECLARE(apr_status_t) apr_thread_join(apr_status_t *retval,
                                           apr_thread_t *thd)
 {
     apr_status_t rv = APR_SUCCESS;
+    DWORD ret;
     
     if (!thd->td) {
         /* Can not join on detached threads */
         return APR_DETACH;
     }
-    rv = WaitForSingleObject(thd->td, INFINITE);
-    if ( rv == WAIT_OBJECT_0 || rv == WAIT_ABANDONED) {
+
+    ret = WaitForSingleObject(thd->td, INFINITE);
+    if (ret == WAIT_OBJECT_0 || ret == WAIT_ABANDONED) {
         /* If the thread_exit has been called */
-        if (!thd->pool)
+        if (thd->exited)
             *retval = thd->exitval;
         else
             rv = APR_INCOMPLETE;
     }
     else
         rv = apr_get_os_error();
-    CloseHandle(thd->td);
-    thd->td = NULL;
+
+    if (rv == APR_SUCCESS) {
+        CloseHandle(thd->td);
+        apr_pool_destroy(thd->pool);
+        thd->td = NULL;
+    }
 
     return rv;
 }
 
 APR_DECLARE(apr_status_t) apr_thread_detach(apr_thread_t *thd)
 {
-    if (thd->td && CloseHandle(thd->td)) {
+    if (!thd->td) {
+        return APR_EINVAL;
+    }
+
+    if (CloseHandle(thd->td)) {
         thd->td = NULL;
         return APR_SUCCESS;
     }