> From: Philip Martin
> Sent: 29 May 2002 23:30
> "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.
There is a difference between debug and production. Your patch
only addressed the debug version.
> 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.
This is all correct. I thought it was general knowledge that if you
go around clearing/destroying a pool in one thread and using it in the
other, you are asking for trouble. I think we need to document this,
but I don't see it as something we should fix.
[...]
> 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.
Indeed. This is why I didn't want to go that route.
Sander
---------------------------------------------------------------------
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:09 2002