> From: Philip Martin [mailto:pm@localhost.striker.xs4all.nl]On Behalf Of
> Philip Martin
> "Sander Striker" <striker@apache.org> writes:
>
> > You should never use a pool after calling apr_pool_destroy or apr_pool_clear.
> > Cleanups should not use the pool they are cleaning up in for new allocations
> > or other operations.
>
> Subversion is doing so in several places. What does "other
> operations" mean? Can I call apr_pool_parent_get on a pointer to the
> pool being destroyed?
apr_pool_parent_get should still work. The pool is removed from the hierarchy
_after_ running the cleanups.
>>> Cleanups can't assume that their pools are still valid and have anything in
>>> them
>>
>> correct.
>
> Yikes! "can't assume [they] have anything in them" You mean memory
> allocated from the pool may already be invalid? That makes the system
> very hard to use :-(
Memory allocated from the pool is still valid. It doesn't reuse the
memory until after the cleanups are run. But, new allocations and registering
new cleanups is a no go.
>>> (there is purposely no order to how cleanups in a pool are invoked -
>>
>> Last in, first out.
>
> One sane thing.
>
>>> this is to minimize what people do in a cleanup). So, the iconv_close()
>>> has already been called by its pool cleanup function (registered by
>>> apr_xlate). (Sander would know even more about the pool logic than I and
>>> could confirm my hunch.)
>>>
>>> The reason reverting the patch works is that the iconv buffer isn't coming
>>> from that pool, but instead from the global pool (which is never cleaned
>>> up). I bet that on Solaris and Darwin, the iconv() buffers are stateless
>>> (i.e. a close does nothing), or the pool_cleanup is called before the
>>> iconv_close cleanup.
>>>
>>> You can't have any dependencies between pool cleanups. Either that pool
>>> passed to svn_wc__adm_is_cleanup_required must be that pool's parent
>>> (perhaps easiest), or pool_cleanup must go away.
>>
>> That are the options that came to my mind aswell.
>
> Well, passing the parent doesn't help if the memory is invalid (see
> above) as we won't be able to look at the access baton's stored path.
> The path would need to be allocated from the parents pool. You don't
> really mean that to cleanup an allocated structure I have to allocate
> it from the parent pool?
>
> I accept that attempting to add new cleanup handlers should fail, but
> not being able to allocate new memory is unfortunate, and not being
> able to read already allocated memory is disasterous!
I'll recheck the code to see if allocating new memory isn't going to
cause us trouble. This is mostly an issue with locking IIRC.
Adding new cleanups is indeed a no-go. This could be problematic
when, for example, trying to open a file through apr (since it registers
a cleanup).
>>> Regardless, r3701 isn't at fault. It just indicated the faulty logic here.
>>> =) -- justin
>>
>> I agree.
>
> Well, in mitigation, I would point to the lack of documentation
> describing these restrictions. This is not the first callback/cleanup
> system I've used, but it appears to be the least powerful.
I'll verify how things work now and will work to improve things.
I agree that it currently might be too restrictive.
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Nov 9 11:01:37 2002