"Sander Striker" <striker@apache.org> writes:
> Ok, I like this patch somewhat better. Could you test it please?
It works, but see later.
>
> Furthermore I think we have a problem in the production code aswell,
> unless noone clears the top level pool in subversion (which would
> cause the problem to show its ugly head). What is happening there
> is that the mutex is created in the top level pool. So, if you
> do:
>
> p = svn_pool_create(p);
>
> ... pool operations on p
>
> svn_pool_clear(p);
>
> ... pool operations on p
> BOOM
With my patch it doesn't go BOOM. Look at the main function for the
test harness, subversion/tests/svn_tests_main.c, it does
pool = svn_pool_create (NULL);
for (i = 1; i < array_size; i++)
{
do_test_num(... i ... );
svn_pool_clear (pool);
}
and it works fine. That's because the mutex gets allocated in
global_pool which is static in apr_pools.c. The application can never
clear global_pool, it can't even see it.
The drawback with my simple patch is of course the unbounded memory
use. The advantage is that it's quick-n-dirty and works :)
>
> Ofcourse the simple fix would be to implement svn_pool_clear as
> somewhat more than a thin wrapper. However, if we can avoid it
> (by not ever clearing the top level pool), that would have my
> preference.
>
>
> Sander
>
> Index: memory/unix/apr_pools.c
> ===================================================================
> RCS file: /home/cvs/apr/memory/unix/apr_pools.c,v
> retrieving revision 1.169
> diff -u -r1.169 apr_pools.c
> --- memory/unix/apr_pools.c 26 May 2002 09:00:08 -0000 1.169
> +++ memory/unix/apr_pools.c 29 May 2002 19:55:31 -0000
> @@ -1381,6 +1381,17 @@
> #endif /* (APR_POOL_DEBUG & APR_POOL_DEBUG_VERBOSE) */
>
> pool_clear_debug(pool, file_line);
> +
> +
> +#if APR_HAS_THREADS
> + /* If we had our own mutex, it will have been destroyed by
> + * the registered cleanups. Recreate the mutex.
> + */
> + if (pool->parent == NULL || pool->parent->mutex != pool->mutex) {
> + (void)apr_thread_mutex_create(&pool->mutex,
> + APR_THREAD_MUTEX_NESTED, pool);
> + }
> +#endif /* APR_HAS_THREADS */
This patch recreates the mutex. That works fine in Subversion but
then Subversion only has one thread! What is this mutex for? It's
used in apr_pool_walk_tree so that threads can safely access pools
used in other threads. Now consider what happens when a pool gets
cleared, the pool cleanup handlers destroy the mutex and the pool
free's the memory occupied by the mutex. Nowhere does the code
consider that another thread might have the mutex locked. Nowhere
does the code consider that another thread might be blocked waiting to
lock the mutex. I don't understand how this can work. Until the pool
has been removed from its parent's list of children the mutex cannot
be destroyed.
If you really want to go down the destroy/recreate route I think you
need something like
if pool has parent
lock parent's mutex
remove pool from parent's list
unlock parent's mutex
endif
clear pool
recreate pool's mutex
if pool has parent
lock parent's mutex
add pool to parent's list
unlock parent's mutex
endif
That, as far as I can see, is thread-safe and doesn't have the
potentially unbounded memory problem. The unlock while clearing the
pool is optional, you could hold the parent's lock for the whole
operation.
> }
>
> APR_DECLARE(void) apr_pool_destroy_debug(apr_pool_t *pool,
Another alternative (perhaps what you meant by "more than a thin
wrapper" above?) would be to avoid having the the mutex destroyed by
the cleanup handler, and make pool_clear_debug recognise the address
of the mutex and not free it. Then the mutex would persist for the
lifetime of the pool. I don't recommend this, it introduces nasty
coupling between the pool and mutex implementations.
--
Philip
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Jun 1 14:23:08 2002