apr_thread: Follow up to r1884078: Unmanaged pools for attached threads too.

r1884078 fixed lifetime issues with detached threads by using unmanaged pool
destroyed by the thread itself on exit, with no binding to the parent pool.

This commit makes use of unmanaged pools for attached threads too, they needed
their own allocator anyway due to apr_thread_detach() being callable anytime
later. apr__pool_unmanage() was a hack to detach a subpool from its parent, but
if a subpool needs its own allocator for this to work correctly there is no
point in creating a subpool for threads (no memory reuse on destroy for short
living threads for instance).

Since an attached thread has its own lifetime now, apr_thread_join() must be
called to free its resources/pool, though it's no different than before when
destroying the parent pool was UB if the thread was still running (i.e.  not
joined yet).

Let's acknoledge that threads want no binding with the pool passed to them at
creation time, besides the abort_fn which they can steal :)


Merge r1897179 from trunk.
Submitted by: ylavic


git-svn-id: https://svn.apache.org/repos/asf/apr/apr/branches/1.8.x@1897220 13f79535-47bb-0310-9956-ffa450edef68

6 files changed, 158 insertions, 265 deletions

diff --git a/memory/unix/apr_pools.c b/memory/unix/apr_pools.c
index 27d0d528a..fbef277ad 100644
--- a/memory/unix/apr_pools.c
+++ b/memory/unix/apr_pools.c
@@ -2324,45 +2324,6 @@ APR_DECLARE(void) apr_pool_lock(apr_pool_t *pool, int flag)
 
 #endif /* !APR_POOL_DEBUG */
 
-/* For APR internal use only (for now).
- * Detach the pool from its/any parent (i.e. un-manage).
- */
-apr_status_t apr__pool_unmanage(apr_pool_t *pool);
-apr_status_t apr__pool_unmanage(apr_pool_t *pool)
-{
-    apr_pool_t *parent = pool->parent;
-
-    if (!parent) {
-        return APR_NOTFOUND;
-    }
-
-#if APR_POOL_DEBUG
-    if (pool->allocator && pool->allocator == parent->allocator) {
-        return APR_EINVAL;
-    }
-    apr_thread_mutex_lock(parent->mutex);
-#else
-    if (pool->allocator == parent->allocator) {
-        return APR_EINVAL;
-    }
-    allocator_lock(parent->allocator);
-#endif
-
-    /* Remove the pool from the parent's children */
-    if ((*pool->ref = pool->sibling) != NULL) {
-        pool->sibling->ref = pool->ref;
-    }
-    pool->parent = NULL;
-
-#if APR_POOL_DEBUG
-    apr_thread_mutex_unlock(parent->mutex);
-#else
-    allocator_unlock(parent->allocator);
-#endif
-
-    return APR_SUCCESS;
-}
-
 #ifdef NETWARE
 void netware_pool_proc_cleanup ()
 {
diff --git a/threadproc/beos/thread.c b/threadproc/beos/thread.c
index 1d79c32bf..269ee9cb6 100644
--- a/threadproc/beos/thread.c
+++ b/threadproc/beos/thread.c
@@ -17,9 +17,6 @@
 #include "apr_arch_threadproc.h"
 #include "apr_portable.h"
 
-/* Internal (from apr_pools.c) */
-extern apr_status_t apr__pool_unmanage(apr_pool_t *pool);
-
 APR_DECLARE(apr_status_t) apr_threadattr_create(apr_threadattr_t **new, apr_pool_t *pool)
 {
     (*new) = (apr_threadattr_t *)apr_palloc(pool, 
@@ -83,63 +80,56 @@ APR_DECLARE(apr_status_t) apr_thread_create(apr_thread_t **new, apr_threadattr_t
 {
     int32 temp;
     apr_status_t stat;
+    apr_allocator_t *allocator;
     
     (*new) = (apr_thread_t *)apr_pcalloc(pool, sizeof(apr_thread_t));
     if ((*new) == NULL) {
         return APR_ENOMEM;
     }
 
-    (*new)->data = data;
-    (*new)->func = func;
-    (*new)->exitval = -1;
-
-    (*new)->detached = (attr && apr_threadattr_detach_get(attr) == APR_DETACH);
-    if ((*new)->detached) {
-        stat = apr_pool_create_unmanaged_ex(&(*new)->pool,
-                                            apr_pool_abort_get(pool),
-                                            NULL);
-    }
-    else {
-        /* The thread can be apr_thread_detach()ed later, so the pool needs
-         * its own allocator to not depend on the parent pool which could be
-         * destroyed before the thread exits.  The allocator needs no mutex
-         * obviously since the pool should not be used nor create children
-         * pools outside the thread.
-         */
-        apr_allocator_t *allocator;
-        if (apr_allocator_create(&allocator) != APR_SUCCESS) {
-            return APR_ENOMEM;
-        }
-        stat = apr_pool_create_ex(&(*new)->pool, pool, NULL, allocator);
-        if (stat == APR_SUCCESS) {
-            apr_allocator_owner_set(allocator, (*new)->pool);
-        }
-        else {
-            apr_allocator_destroy(allocator);
-        }
+    /* The thread can be detached anytime (from the creation or later with
+     * apr_thread_detach), so it needs its own pool and allocator to not
+     * depend on a parent pool which could be destroyed before the thread
+     * exits. The allocator needs no mutex obviously since the pool should 
+     * not be used nor create children pools outside the thread.
+     */
+    stat = apr_allocator_create(&allocator);
+    if (stat != APR_SUCCESS) {
+        return stat;
     }
+    stat = apr_pool_create_unmanaged_ex(&(*new)->pool,
+                                        apr_pool_abort_get(pool),
+                                        allocator);
     if (stat != APR_SUCCESS) {
+        apr_allocator_destroy(allocator);
         return stat;
     }
+    apr_allocator_owner_set(allocator, (*new)->pool);
 
-    /* First we create the new thread...*/
-	if (attr)
-	    temp = attr->attr;
-	else
-	    temp = B_NORMAL_PRIORITY;
+    (*new)->data = data;
+    (*new)->func = func;
+    (*new)->exitval = -1;
+    (*new)->detached = (attr && apr_threadattr_detach_get(attr) == APR_DETACH);
+
+    if (attr)
+        temp = attr->attr;
+    else
+        temp = B_NORMAL_PRIORITY;
 
+    /* First we create the new thread...*/
     (*new)->td = spawn_thread((thread_func)dummy_worker, 
                               "apr thread", 
                               temp, 
                               (*new));
 
     /* Now we try to run it...*/
-    if (resume_thread((*new)->td) == B_NO_ERROR) {
-        return APR_SUCCESS;
+    if (resume_thread((*new)->td) != B_NO_ERROR) {
+        stat = errno;
+        apr_pool_destroy((*new)->pool);
+        return stat;
     }
-    else {
-        return errno;
-    } 
+
+    return APR_SUCCESS;
 }
 
 APR_DECLARE(apr_os_thread_t) apr_os_thread_current(void)
@@ -196,8 +186,6 @@ APR_DECLARE(apr_status_t) apr_thread_detach(apr_thread_t *thd)
     }
 
     if (suspend_thread(thd->td) == B_NO_ERROR) {
-        /* Detach from the parent pool too */
-        apr__pool_unmanage(thd->pool);
         thd->detached = 1;
         return APR_SUCCESS;
     }
diff --git a/threadproc/netware/thread.c b/threadproc/netware/thread.c
index ac9331db9..2f126539b 100644
--- a/threadproc/netware/thread.c
+++ b/threadproc/netware/thread.c
@@ -21,9 +21,6 @@
 
 static int thread_count = 0;
 
-/* Internal (from apr_pools.c) */
-extern apr_status_t apr__pool_unmanage(apr_pool_t *pool);
-
 apr_status_t apr_threadattr_create(apr_threadattr_t **new,
                                                 apr_pool_t *pool)
 {
@@ -87,14 +84,48 @@ apr_status_t apr_thread_create(apr_thread_t **new,
 {
     apr_status_t stat;
     unsigned long flags = NX_THR_BIND_CONTEXT;
-    char threadName[NX_MAX_OBJECT_NAME_LEN+1];
     size_t stack_size = APR_DEFAULT_STACK_SIZE;
+    apr_allocator_t *allocator;
+    
+    (*new) = (apr_thread_t *)apr_pcalloc(pool, sizeof(apr_thread_t));
+    if ((*new) == NULL) {
+        return APR_ENOMEM;
+    }
+
+    /* The thread can be detached anytime (from the creation or later with
+     * apr_thread_detach), so it needs its own pool and allocator to not
+     * depend on a parent pool which could be destroyed before the thread
+     * exits. The allocator needs no mutex obviously since the pool should 
+     * not be used nor create children pools outside the thread.
+     */
+    stat = apr_allocator_create(&allocator);
+    if (stat != APR_SUCCESS) {
+        return stat;
+    }
+    stat = apr_pool_create_unmanaged_ex(&(*new)->pool,
+                                        apr_pool_abort_get(pool),
+                                        allocator);
+    if (stat != APR_SUCCESS) {
+        apr_allocator_destroy(allocator);
+        return stat;
+    }
+    apr_allocator_owner_set(allocator, (*new)->pool);
 
+    (*new)->data = data;
+    (*new)->func = func;
+    (*new)->exitval = -1;
+    (*new)->detached = (attr && apr_threadattr_detach_get(attr) == APR_DETACH);
     if (attr && attr->thread_name) {
-        strncpy (threadName, attr->thread_name, NX_MAX_OBJECT_NAME_LEN);
+        (*new)->thread_name = apr_pstrndup(pool, ttr->thread_name,
+                                           NX_MAX_OBJECT_NAME_LEN);
     }
     else {
-        sprintf(threadName, "APR_thread %04ld", ++thread_count);
+        (*new)->thread_name = apr_psprintf(pool, "APR_thread %04d",
+                                           ++thread_count);
+    }
+    if ((*new)->thread_name == NULL) {
+        apr_pool_destroy((*new)->pool);
+        return APR_ENOMEM;
     }
 
     /* An original stack size of 0 will allow NXCreateThread() to
@@ -106,45 +137,6 @@ apr_status_t apr_thread_create(apr_thread_t **new,
         stack_size = attr->stack_size;
     }
     
-    (*new) = (apr_thread_t *)apr_pcalloc(pool, sizeof(apr_thread_t));
-
-    if ((*new) == NULL) {
-        return APR_ENOMEM;
-    }
-    
-    (*new)->data = data;
-    (*new)->func = func;
-    (*new)->thread_name = (char*)apr_pstrdup(pool, threadName);
-
-    (*new)->detached = (attr && apr_threadattr_detach_get(attr) == APR_DETACH);
-    if ((*new)->detached) {
-        stat = apr_pool_create_unmanaged_ex(&(*new)->pool,
-                                            apr_pool_abort_get(pool),
-                                            NULL);
-    }
-    else {
-        /* The thread can be apr_thread_detach()ed later, so the pool needs
-         * its own allocator to not depend on the parent pool which could be
-         * destroyed before the thread exits.  The allocator needs no mutex
-         * obviously since the pool should not be used nor create children
-         * pools outside the thread.
-         */
-        apr_allocator_t *allocator;
-        if (apr_allocator_create(&allocator) != APR_SUCCESS) {
-            return APR_ENOMEM;
-        }
-        stat = apr_pool_create_ex(&(*new)->pool, pool, NULL, allocator);
-        if (stat == APR_SUCCESS) {
-            apr_allocator_owner_set(allocator, (*new)->pool);
-        }
-        else {
-            apr_allocator_destroy(allocator);
-        }
-    }
-    if (stat != APR_SUCCESS) {
-        return stat;
-    }
-    
     if (attr && attr->detach) {
         flags |= NX_THR_DETACHED;
     }
@@ -157,19 +149,21 @@ apr_status_t apr_thread_create(apr_thread_t **new,
         /* unsigned long flags */             NX_CTX_NORMAL,
         /* int *error */                      &stat);
 
-    stat = NXContextSetName(
+    (void) NXContextSetName(
         /* NXContext_t ctx */  (*new)->ctx,
-        /* const char *name */ threadName);
+        /* const char *name */ (*new)->thread_name);
 
     stat = NXThreadCreate(
         /* NXContext_t context */     (*new)->ctx,
         /* unsigned long flags */     flags,
         /* NXThreadId_t *thread_id */ &(*new)->td);
 
-    if (stat == 0)
-        return APR_SUCCESS;
+    if (stat) {
+        apr_pool_destroy((*new)->pool);
+        return stat;
+    }
         
-    return(stat); /* if error */    
+    return APR_SUCCESS;
 }
 
 apr_os_thread_t apr_os_thread_current()
@@ -224,8 +218,6 @@ apr_status_t apr_thread_detach(apr_thread_t *thd)
         return APR_EINVAL;
     }
 
-    /* Detach from the parent pool too */
-    apr__pool_unmanage(thd->pool);
     thd->detached = 1;
 
     return APR_SUCCESS;
diff --git a/threadproc/os2/thread.c b/threadproc/os2/thread.c
index 441de42bb..43bc4a1b0 100644
--- a/threadproc/os2/thread.c
+++ b/threadproc/os2/thread.c
@@ -24,8 +24,6 @@
 #include "apr_arch_file_io.h"
 #include <stdlib.h>
 
-/* Internal (from apr_pools.c) */
-extern apr_status_t apr__pool_unmanage(apr_pool_t *pool);
 
 
 APR_DECLARE(apr_status_t) apr_threadattr_create(apr_threadattr_t **new, apr_pool_t *pool)
@@ -70,9 +68,9 @@ APR_DECLARE(apr_status_t) apr_threadattr_guardsize_set(apr_threadattr_t *attr,
     return APR_ENOTIMPL;
 }
 
-static void apr_thread_begin(void *arg)
+static void dummy_worker(void *opaque)
 {
-  apr_thread_t *thread = (apr_thread_t *)arg;
+  apr_thread_t *thread = (apr_thread_t *)opaque;
   thread->exitval = thread->func(thread, thread->data);
   if (thd->attr->attr & APR_THREADATTR_DETACHED) {
       apr_pool_destroy(thread->pool);
@@ -87,70 +85,51 @@ 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_pcalloc(pool, sizeof(apr_thread_t));
-    *new = thread;
-
+    apr_allocator_t *allocator;
+    
+    *new = thread = (apr_thread_t *)apr_pcalloc(pool, sizeof(apr_thread_t));
     if (thread == NULL) {
         return APR_ENOMEM;
     }
 
-    thread->attr = attr;
-    thread->func = func;
-    thread->data = data;
-
-    if (attr && attr->attr & APR_THREADATTR_DETACHED) {
-        stat = apr_pool_create_unmanaged_ex(&thread->pool,
-                                            apr_pool_abort_get(pool),
-                                            NULL);
-    }
-    else {
-        /* The thread can be apr_thread_detach()ed later, so the pool needs
-         * its own allocator to not depend on the parent pool which could be
-         * destroyed before the thread exits.  The allocator needs no mutex
-         * obviously since the pool should not be used nor create children
-         * pools outside the thread.
-         */
-        apr_allocator_t *allocator;
-        if (apr_allocator_create(&allocator) != APR_SUCCESS) {
-            return APR_ENOMEM;
-        }
-        stat = apr_pool_create_ex(&thread->pool, pool, NULL, allocator);
-        if (stat == APR_SUCCESS) {
-            apr_thread_mutex_t *mutex;
-            stat = apr_thread_mutex_create(&mutex, APR_THREAD_MUTEX_DEFAULT,
-                                           thread->pool);
-            if (stat == APR_SUCCESS) {
-                apr_allocator_mutex_set(allocator, mutex);
-                apr_allocator_owner_set(allocator, thread->pool);
-            }
-            else {
-                apr_pool_destroy(thread->pool);
-            }
-        }
-        if (stat != APR_SUCCESS) {
-            apr_allocator_destroy(allocator);
-        }
+    /* The thread can be detached anytime (from the creation or later with
+     * apr_thread_detach), so it needs its own pool and allocator to not
+     * depend on a parent pool which could be destroyed before the thread
+     * exits. The allocator needs no mutex obviously since the pool should 
+     * not be used nor create children pools outside the thread.
+     */
+    stat = apr_allocator_create(&allocator);
+    if (stat != APR_SUCCESS) {
+        return stat;
     }
+    stat = apr_pool_create_unmanaged_ex(&thread->pool,
+                                        apr_pool_abort_get(pool),
+                                        allocator);
     if (stat != APR_SUCCESS) {
+        apr_allocator_destroy(allocator);
         return stat;
     }
+    apr_allocator_owner_set(allocator, thread->pool);
 
+    thread->func = func;
+    thread->data = data;
     if (attr == NULL) {
-        stat = apr_threadattr_create(&thread->attr, thread->pool);
-        
+        stat = apr_threadattr_create(&attr, thread->pool);
         if (stat != APR_SUCCESS) {
+            apr_pool_destroy(thread->pool);
             return stat;
         }
     }
+    thread->attr = attr;
 
-    thread->tid = _beginthread(apr_thread_begin, NULL, 
+    thread->tid = _beginthread(dummy_worker, NULL, 
                                thread->attr->stacksize > 0 ?
                                thread->attr->stacksize : APR_THREAD_STACKSIZE,
                                thread);
-        
     if (thread->tid < 0) {
-        return errno;
+        stat = errno;
+        apr_pool_destroy(thread->pool);
+        return stat;
     }
 
     return APR_SUCCESS;
@@ -208,8 +187,6 @@ APR_DECLARE(apr_status_t) apr_thread_detach(apr_thread_t *thd)
         return APR_EINVAL;
     }
 
-    /* Detach from the parent pool too */
-    apr__pool_unmanage(thd->pool);
     thd->attr->attr |= APR_THREADATTR_DETACHED;
 
     return APR_SUCCESS;
diff --git a/threadproc/unix/thread.c b/threadproc/unix/thread.c
index b16c6f9e2..e5919a4b8 100644
--- a/threadproc/unix/thread.c
+++ b/threadproc/unix/thread.c
@@ -22,9 +22,6 @@
 
 #if APR_HAVE_PTHREAD_H
 
-/* Internal (from apr_pools.c) */
-extern apr_status_t apr__pool_unmanage(apr_pool_t *pool);
-
 /* Destroy the threadattr object */
 static apr_status_t threadattr_cleanup(void *data)
 {
@@ -159,49 +156,39 @@ APR_DECLARE(apr_status_t) apr_thread_create(apr_thread_t **new,
 {
     apr_status_t stat;
     pthread_attr_t *temp;
-
+    apr_allocator_t *allocator;
+    
     (*new) = (apr_thread_t *)apr_pcalloc(pool, sizeof(apr_thread_t));
-
     if ((*new) == NULL) {
         return APR_ENOMEM;
     }
 
-    (*new)->td = (pthread_t *)apr_pcalloc(pool, sizeof(pthread_t));
-
-    if ((*new)->td == NULL) {
-        return APR_ENOMEM;
+    /* The thread can be detached anytime (from the creation or later with
+     * apr_thread_detach), so it needs its own pool and allocator to not
+     * depend on a parent pool which could be destroyed before the thread
+     * exits. The allocator needs no mutex obviously since the pool should 
+     * not be used nor create children pools outside the thread.
+     */
+    stat = apr_allocator_create(&allocator);
+    if (stat != APR_SUCCESS) {
+        return stat;
     }
+    stat = apr_pool_create_unmanaged_ex(&(*new)->pool,
+                                        apr_pool_abort_get(pool),
+                                        allocator);
+    if (stat != APR_SUCCESS) {
+        apr_allocator_destroy(allocator);
+        return stat;
+    }
+    apr_allocator_owner_set(allocator, (*new)->pool);
 
     (*new)->data = data;
     (*new)->func = func;
-
     (*new)->detached = (attr && apr_threadattr_detach_get(attr) == APR_DETACH);
-    if ((*new)->detached) {
-        stat = apr_pool_create_unmanaged_ex(&(*new)->pool,
-                                            apr_pool_abort_get(pool),
-                                            NULL);
-    }
-    else {
-        /* The thread can be apr_thread_detach()ed later, so the pool needs
-         * its own allocator to not depend on the parent pool which could be
-         * destroyed before the thread exits.  The allocator needs no mutex
-         * obviously since the pool should not be used nor create children
-         * pools outside the thread.
-         */
-        apr_allocator_t *allocator;
-        if (apr_allocator_create(&allocator) != APR_SUCCESS) {
-            return APR_ENOMEM;
-        }
-        stat = apr_pool_create_ex(&(*new)->pool, pool, NULL, allocator);
-        if (stat == APR_SUCCESS) {
-            apr_allocator_owner_set(allocator, (*new)->pool);
-        }
-        else {
-            apr_allocator_destroy(allocator);
-        }
-    }
-    if (stat != APR_SUCCESS) {
-        return stat;
+    (*new)->td = (pthread_t *)apr_pcalloc(pool, sizeof(pthread_t));
+    if ((*new)->td == NULL) {
+        apr_pool_destroy((*new)->pool);
+        return APR_ENOMEM;
     }
 
     if (attr)
@@ -209,16 +196,15 @@ APR_DECLARE(apr_status_t) apr_thread_create(apr_thread_t **new,
     else
         temp = NULL;
 
-    if ((stat = pthread_create((*new)->td, temp, dummy_worker, (*new))) == 0) {
-        return APR_SUCCESS;
-    }
-    else {
+    if ((stat = pthread_create((*new)->td, temp, dummy_worker, (*new)))) {
 #ifdef HAVE_ZOS_PTHREADS
         stat = errno;
 #endif
-
+        apr_pool_destroy((*new)->pool);
         return stat;
     }
+
+    return APR_SUCCESS;
 }
 
 APR_DECLARE(apr_os_thread_t) apr_os_thread_current(void)
@@ -280,8 +266,6 @@ APR_DECLARE(apr_status_t) apr_thread_detach(apr_thread_t *thd)
 #else
     if ((stat = pthread_detach(*thd->td)) == 0) {
 #endif
-        /* Detach from the parent pool too */
-        apr__pool_unmanage(thd->pool);
         thd->detached = 1;
 
         return APR_SUCCESS;
diff --git a/threadproc/win32/thread.c b/threadproc/win32/thread.c
index 7fa71784d..6b04abb98 100644
--- a/threadproc/win32/thread.c
+++ b/threadproc/win32/thread.c
@@ -28,9 +28,6 @@
 /* Chosen for us by apr_initialize */
 DWORD tls_apr_thread = 0;
 
-/* Internal (from apr_pools.c) */
-extern apr_status_t apr__pool_unmanage(apr_pool_t *pool);
-
 APR_DECLARE(apr_status_t) apr_threadattr_create(apr_threadattr_t **new,
                                                 apr_pool_t *pool)
 {
@@ -94,45 +91,36 @@ APR_DECLARE(apr_status_t) apr_thread_create(apr_thread_t **new,
                                             void *data, apr_pool_t *pool)
 {
     apr_status_t stat;
-	unsigned temp;
+    unsigned temp;
     HANDLE handle;
-
+    apr_allocator_t *allocator;
+    
     (*new) = (apr_thread_t *)apr_pcalloc(pool, sizeof(apr_thread_t));
-
     if ((*new) == NULL) {
         return APR_ENOMEM;
     }
 
-    (*new)->data = data;
-    (*new)->func = func;
-
-    if (attr && attr->detach) {
-        stat = apr_pool_create_unmanaged_ex(&(*new)->pool,
-                                            apr_pool_abort_get(pool),
-                                            NULL);
-    }
-    else {
-        /* The thread can be apr_thread_detach()ed later, so the pool needs
-         * its own allocator to not depend on the parent pool which could be
-         * destroyed before the thread exits.  The allocator needs no mutex
-         * obviously since the pool should not be used nor create children
-         * pools outside the thread.
-         */
-        apr_allocator_t *allocator;
-        if (apr_allocator_create(&allocator) != APR_SUCCESS) {
-            return APR_ENOMEM;
-        }
-        stat = apr_pool_create_ex(&(*new)->pool, pool, NULL, allocator);
-        if (stat == APR_SUCCESS) {
-            apr_allocator_owner_set(allocator, (*new)->pool);
-        }
-        else {
-            apr_allocator_destroy(allocator);
-        }
+    /* The thread can be detached anytime (from the creation or later with
+     * apr_thread_detach), so it needs its own pool and allocator to not
+     * depend on a parent pool which could be destroyed before the thread
+     * exits. The allocator needs no mutex obviously since the pool should 
+     * not be used nor create children pools outside the thread.
+     */
+    stat = apr_allocator_create(&allocator);
+    if (stat != APR_SUCCESS) {
+        return stat;
     }
+    stat = apr_pool_create_unmanaged_ex(&(*new)->pool,
+                                        apr_pool_abort_get(pool),
+                                        allocator);
     if (stat != APR_SUCCESS) {
+        apr_allocator_destroy(allocator);
         return stat;
     }
+    apr_allocator_owner_set(allocator, (*new)->pool);
+
+    (*new)->data = data;
+    (*new)->func = func;
 
     /* Use 0 for default Thread Stack Size, because that will
      * default the stack to the same size as the calling thread.
@@ -142,21 +130,26 @@ APR_DECLARE(apr_status_t) apr_thread_create(apr_thread_t **new,
                         (DWORD) (attr ? attr->stacksize : 0),
                         (unsigned int (APR_THREAD_FUNC *)(void *))dummy_worker,
                         (*new), 0, &temp)) == 0) {
-        return APR_FROM_OS_ERROR(_doserrno);
+        stat = APR_FROM_OS_ERROR(_doserrno);
+        apr_pool_destroy((*new)->pool);
+        return stat;
     }
 #else
    if ((handle = CreateThread(NULL,
                         attr && attr->stacksize > 0 ? attr->stacksize : 0,
                         (unsigned int (APR_THREAD_FUNC *)(void *))dummy_worker,
                         (*new), 0, &temp)) == 0) {
-        return apr_get_os_error();
+        stat = apr_get_os_error();
+        apr_pool_destroy((*new)->pool);
+        return stat;
     }
 #endif
     if (attr && attr->detach) {
         CloseHandle(handle);
     }
-    else
+    else {
         (*new)->td = handle;
+    }
 
     return APR_SUCCESS;
 }
@@ -215,8 +208,6 @@ APR_DECLARE(apr_status_t) apr_thread_detach(apr_thread_t *thd)
     }
 
     if (CloseHandle(thd->td)) {
-        /* Detach from the parent pool too */
-        apr__pool_unmanage(thd->pool);
         thd->td = NULL;
         return APR_SUCCESS;
     }