On Sunday 22 May 2011, Jeff Trawick wrote:
> > Can those people who frequently hit the testreslist failures
> > please check if this commit helps? From a powerpc Debian build
> > host, it looks promising, but I don't have direct access to that
> > machine and can't do repeated tests there.
>
> no luck on Windows 7
>
> apr-util 1.3.x, gcc, still fails
> apr trunk, Visual C++ 2008 Express, still fails
>
> Seven:~/svn/apr-util-1.3.x-xx/test$ ./testall -v testreslist
> testreslist : /Line 258: expected <10>, but saw <20>
> FAILED 1 of 1
> Failed Tests Total Fail Failed %
> ===================================================
> testreslist 1 1 100.00%
Can you try the attached patch in addition? Is this failure sporadic
or always?
> > Unfortunately, there are also other thread-safety issues in
> > apr/apr- util. Using hellgrind on testreslist found these
> > issues:
> >
> > - unsafe read of pool->child in apr_pool_destroy. This one
> > worries me. If a subpool is destroyed in one thread while the
> > pool itself is destroyed in another thread, it can happen that
> > apr_pool_destroy is called again for the subpool. This would
> > likely lead to a crash. Any ideas on how to fix this without
> > sacrificing too much performance?
> >
> > - unsafe read of allocator->max_index (PR 48535, likely not
> > critical)
> >
> > - unsafe read of _myself->thd_cnt in thread_pool_cleanup()
> > (likely not critical; but there seem to be other thread-safety
> > issues in that file, at least the access of thd_high is also
> > unsafe)
The last one is fixed by the attached patch. I no longer think that it
is no problem.
diff --git a/util-misc/apr_thread_pool.c b/util-misc/apr_thread_pool.c
index 006370b..b6a5297 100644
--- a/util-misc/apr_thread_pool.c
+++ b/util-misc/apr_thread_pool.c
@@ -333,9 +333,13 @@ static apr_status_t thread_pool_cleanup(void *me)
_myself->terminated = 1;
apr_thread_pool_idle_max_set(_myself, 0);
+ apr_thread_mutex_lock(_myself->lock);
while (_myself->thd_cnt) {
+ apr_thread_mutex_unlock(_myself->lock);
apr_sleep(20 * 1000); /* spin lock with 20 ms */
+ apr_thread_mutex_lock(_myself->lock);
}
+ apr_thread_mutex_unlock(_myself->lock);
apr_thread_mutex_destroy(_myself->lock);
apr_thread_cond_destroy(_myself->cond);
return APR_SUCCESS;
@@ -367,14 +371,13 @@ APR_DECLARE(apr_status_t) apr_thread_pool_create(apr_thread_pool_t ** me,
apr_pool_cleanup_register(tp->pool, tp, thread_pool_cleanup,
apr_pool_cleanup_null);
+ apr_thread_mutex_lock(tp->lock);
while (init_threads) {
/* Grab the mutex as apr_thread_create() and thread_pool_func() will
* allocate from (*me)->pool. This is dangerous if there are multiple
* initial threads to create.
*/
- apr_thread_mutex_lock(tp->lock);
rv = apr_thread_create(&t, NULL, thread_pool_func, tp, tp->pool);
- apr_thread_mutex_unlock(tp->lock);
if (APR_SUCCESS != rv) {
break;
}
@@ -384,6 +387,7 @@ APR_DECLARE(apr_status_t) apr_thread_pool_create(apr_thread_pool_t ** me,
}
--init_threads;
}
+ apr_thread_mutex_unlock(tp->lock);
if (rv == APR_SUCCESS) {
*me = tp;
