You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Sander Striker <st...@apache.org> on 2002/11/10 22:25:59 UTC
Allocating from a pool in cleanup, WAS: RE: svn commit: rev 3709 - in trunk/subversion: libsvn_wc libsvn_client
> From: Philip Martin [mailto:pm@localhost.striker.xs4all.nl]On Behalf Of
> Philip Martin
> Sent: 10 November 2002 17:10
> Philip Martin <ph...@codematters.co.uk> writes:
>
>> The thing that worries me, something I have just recently thought of,
>> is that the parent pool could be the global pool. If that were the
>> case all the thread-safety problems would reappear.
>
> It doesn't have to be the global pool for this to be a threading
> problem. A thread cannot, in general, assume that it can safely
> access a parent pool, that only works if the thread has exclusive
> access to the parent pool.
Ouch! You're right.
> I'm not sure what to do here. We have gradually been moving away from
> functions that allocate their own pools, to functions that use the
> pools passed in to them. So svn_wc_adm_open creates an access baton
> in the pool it is given. When the cleanup handler for the access
> baton runs, it can choose to use either the pool it was given, or that
> pool's parent pool, and at present neither appears to be correct.
>
> Here's the best solution I can come up with: first we need an APR that
> allows us to allocate from the pool during pool cleanup.
We do have that, see below.
> Second we revert to using the pool, rather than the parent pool, in cleanup
> handlers.
Agreed, but currently with the limitation that no new cleanups should be
registered during cleanup.
> Finally, we change the Subversion access baton code, so
> that it registers its cleanup handler later, in particular it must
> register it *after* doing the first UTF8 conversion. This will ensure
> that even if the access baton is responsible for creating the
> apr_xlate_t object in the pool, the access baton cleanup handler will
> run *before* the APR handler cleans-up the apr_xlate_t object.
>
> I don't really understand the prohibition on allocating from a pool
> during a cleanup handler, Subversion has been doing it for some time
> and it doesn't appear to be a problem. I've had a quick look at the
> APR pool code and I see no obvious reason for it not to work (but I
> may well have missed something).
I can confirm that it is safe to do allocations from the pool being cleaned
up. The problem is registering cleanups in a pool in the process of being
cleaned up. Opening files, like a lot of other APR operations, registers
a cleanup with the pool being passed in. It probably wouldn't even be that
hard to allow registering cleanups during cleanup; I'll look into that.
> So working on the assumption that it is indeed OK, I made the other changes.
> FWIW, everything seems to work, the regression tests pass.
Great!
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Allocating from a pool in cleanup, WAS: RE: svn commit: rev 3709 - in trunk/subversion: libsvn_wc libsvn_client
Posted by Philip Martin <ph...@codematters.co.uk>.
"Sander Striker" <st...@apache.org> writes:
> > I don't really understand the prohibition on allocating from a pool
> > during a cleanup handler, Subversion has been doing it for some time
> > and it doesn't appear to be a problem. I've had a quick look at the
> > APR pool code and I see no obvious reason for it not to work (but I
> > may well have missed something).
>
> I can confirm that it is safe to do allocations from the pool being cleaned
> up.
Good.
> The problem is registering cleanups in a pool in the process of being
> cleaned up. Opening files, like a lot of other APR operations, registers
> a cleanup with the pool being passed in. It probably wouldn't even be that
> hard to allow registering cleanups during cleanup; I'll look into that.
Interesting. When would they run? I think they need to be inserted
into the stack, so that they run before all previously registered
cleanups.
If this turns out to be too difficult to support it would be much
better if an error was returned rather than simply making it undefined
behaviour. This is important when the cleanup makes library calls
(such as apr_xlate_open say :) where there is no documentation to say
that it is not safe to call them from a cleanup handler.
Unfortunately, apr_pool_cleanup_register doesn't have a return value,
so it may be harder to return an error than to support registering
cleanups during cleanup!
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Allocating from a pool in cleanup
Posted by Philip Martin <ph...@codematters.co.uk>.
Greg Hudson <gh...@MIT.EDU> writes:
> On Mon, 2002-11-11 at 06:28, Philip Martin wrote:
> > I don't think that runs the cleanups in the ideal order. Given two
> > cleanups cleanupA and cleanupB, with cleanupA run before cleanupB,
> > then if cleanupA registers cleanupC I would expect cleanupC to run
> > before cleanupB. Each cleanup that is registered goes to the head of
> > the queue.
>
> Nothing in apr_pools.h mentions cleanup order, and I cannot think of any
> legitimate design which relies on cleanup order within a particular
> pool.
1. The documentation in apr_pools.h is far from complete :-) Try
determining pool thread safety from the documentation, or any
restriction on calling apr_pool_cleanup_register during cleanup.
2. Sander has already stated that the order is first in, last out
in response to an earlier claim that the order was unspecified.
3. I cannot image a system that *doesn't* specify the order being
useful. We rely on the ordering, to ensure that the access baton
cleanup happens before the apr_xlate_t cleanup.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Allocating from a pool in cleanup
Posted by Greg Hudson <gh...@MIT.EDU>.
On Mon, 2002-11-11 at 06:28, Philip Martin wrote:
> I don't think that runs the cleanups in the ideal order. Given two
> cleanups cleanupA and cleanupB, with cleanupA run before cleanupB,
> then if cleanupA registers cleanupC I would expect cleanupC to run
> before cleanupB. Each cleanup that is registered goes to the head of
> the queue.
Nothing in apr_pools.h mentions cleanup order, and I cannot think of any
legitimate design which relies on cleanup order within a particular
pool.
(On the other hand, this is easy to implement:
while (p->cleanups)
{
cleanup = p->cleanups;
p->cleanups = cleanup->next;
cleanup->plain_cleanup_fn(cleanup->data);
}
)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Allocating from a pool in cleanup
Posted by Philip Martin <ph...@codematters.co.uk>.
Greg Stein <gs...@lyra.org> writes:
> > The problem is registering cleanups in a pool in the process of being
> > cleaned up. Opening files, like a lot of other APR operations, registers
> > a cleanup with the pool being passed in. It probably wouldn't even be that
> > hard to allow registering cleanups during cleanup; I'll look into that.
>
> Set pool->cleanups to NULL before running them. After you're done running
> them, run again if cleanups != NULL. Note the potential for an infinite
> loop... :-)
I don't think that runs the cleanups in the ideal order. Given two
cleanups cleanupA and cleanupB, with cleanupA run before cleanupB,
then if cleanupA registers cleanupC I would expect cleanupC to run
before cleanupB. Each cleanup that is registered goes to the head of
the queue.
What about allowing the creation of subpools during a cleanup handler
as well? Just clear any subpools created by running a handler before
running the next, possibly newly added, handler :)
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Allocating from a pool in cleanup
Posted by Greg Stein <gs...@lyra.org>.
On Sun, Nov 10, 2002 at 11:25:59PM +0100, Sander Striker wrote:
>...
> I can confirm that it is safe to do allocations from the pool being cleaned
> up.
Yup. I was looking at this, too, and it is definitely quite safe to do this,
and it really is the only thing that makes sense. A user should never assume
that it can allocate in a parent pool. You could accumulate a lot of cruft
if you do that.
Note that we've got a lot of code that allocates during cleanup. I did an
abort() when somebody tried that. Wasn't pretty :-)
Sometime over the week, I'll see if I can clean up my patch to allow (again)
the allocate-during-cleanup and post that; basically it would just stop
register-during-cleanup.
Hmm. Well, I've just attached my patch. It doesn't do the right thing, but
it could be simplified to scope it down.
> The problem is registering cleanups in a pool in the process of being
> cleaned up. Opening files, like a lot of other APR operations, registers
> a cleanup with the pool being passed in. It probably wouldn't even be that
> hard to allow registering cleanups during cleanup; I'll look into that.
Set pool->cleanups to NULL before running them. After you're done running
them, run again if cleanups != NULL. Note the potential for an infinite
loop... :-)
Cheers,
-g
--
Greg Stein, http://www.lyra.org/