You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by sf...@apache.org on 2011/05/22 22:10:42 UTC
svn commit: r1126207 - in /apr/apr/trunk: CHANGES util-misc/apr_thread_pool.c
Author: sf
Date: Sun May 22 20:10:42 2011
New Revision: 1126207
URL: http://svn.apache.org/viewvc?rev=1126207&view=rev
Log:
Fix thread unsafe pool usage. This is a potential culprit for the occasional
testreslist failures.
Modified:
apr/apr/trunk/CHANGES
apr/apr/trunk/util-misc/apr_thread_pool.c
Modified: apr/apr/trunk/CHANGES
URL: http://svn.apache.org/viewvc/apr/apr/trunk/CHANGES?rev=1126207&r1=1126206&r2=1126207&view=diff
==============================================================================
--- apr/apr/trunk/CHANGES [utf-8] (original)
+++ apr/apr/trunk/CHANGES [utf-8] Sun May 22 20:10:42 2011
@@ -1,6 +1,8 @@
-*- coding: utf-8 -*-
Changes for APR 2.0.0
+ *) apr_thread_pool: Fix thread unsafe pool usage. [Stefan Fritsch]
+
*) apr_brigades: add a check to prevent infinite while loop in case
of a corrupted brigade. Problem evidenced in PR 51062. Analysis by
Krzysztof Kostałkowicz <KKostalkowicz ivmx.pl>, patch [Nick Kew].
Modified: apr/apr/trunk/util-misc/apr_thread_pool.c
URL: http://svn.apache.org/viewvc/apr/apr/trunk/util-misc/apr_thread_pool.c?rev=1126207&r1=1126206&r2=1126207&view=diff
==============================================================================
--- apr/apr/trunk/util-misc/apr_thread_pool.c (original)
+++ apr/apr/trunk/util-misc/apr_thread_pool.c Sun May 22 20:10:42 2011
@@ -353,13 +353,18 @@ APR_DECLARE(apr_status_t) apr_thread_poo
*me = NULL;
tp = apr_pcalloc(pool, sizeof(apr_thread_pool_t));
- tp->pool = pool;
-
+ /*
+ * This pool will be used by different threads. As we cannot ensure that
+ * our caller won't use the pool without acquiring the mutex, we must
+ * create a new sub pool.
+ */
+ rv = apr_pool_create(&tp->pool, pool);
+ if (APR_SUCCESS != rv)
+ return rv;
rv = thread_pool_construct(tp, init_threads, max_threads);
- if (APR_SUCCESS != rv) {
+ if (APR_SUCCESS != rv)
return rv;
- }
- apr_pool_cleanup_register(pool, tp, thread_pool_cleanup,
+ apr_pool_cleanup_register(tp->pool, tp, thread_pool_cleanup,
apr_pool_cleanup_null);
while (init_threads) {
Re: testreslist failures / thread-safety issues
Posted by Jeff Trawick <tr...@gmail.com>.
On Tue, May 24, 2011 at 4:01 PM, Stefan Fritsch <sf...@sfritsch.de> wrote:
> 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?
tested with apr-util 1.3.x and gcc
no change
I don't remember if this test has ever worked for me in this environment
>
>
>> > 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.
>
--
Born in Roswell... married an alien...
Re: testreslist failures / thread-safety issues
Posted by Stefan Fritsch <sf...@sfritsch.de>.
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.
Re: testreslist failures / thread-safety issues
Posted by Stefan Fritsch <sf...@sfritsch.de>.
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%
:-(
Thanks for testing, though.
Re: testreslist failures / thread-safety issues
Posted by Jeff Trawick <tr...@gmail.com>.
On Sun, May 22, 2011 at 4:42 PM, Stefan Fritsch <sf...@sfritsch.de> wrote:
> On Sunday 22 May 2011, sf@apache.org wrote:
>> Author: sf
>> Date: Sun May 22 20:10:42 2011
>> New Revision: 1126207
>>
>> URL: http://svn.apache.org/viewvc?rev=1126207&view=rev
>> Log:
>> Fix thread unsafe pool usage. This is a potential culprit for the
>> occasional testreslist failures.
>
> 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%
>
> 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)
>
>
> Cheers,
> Stefan
>
--
Born in Roswell... married an alien...
testreslist failures / thread-safety issues
Posted by Stefan Fritsch <sf...@sfritsch.de>.
On Sunday 22 May 2011, sf@apache.org wrote:
> Author: sf
> Date: Sun May 22 20:10:42 2011
> New Revision: 1126207
>
> URL: http://svn.apache.org/viewvc?rev=1126207&view=rev
> Log:
> Fix thread unsafe pool usage. This is a potential culprit for the
> occasional testreslist failures.
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.
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)
Cheers,
Stefan