Re: svn commit: r1211162 - /subversion/trunk/subversion/libsvn_wc/context.c

On Wed, Dec 7, 2011 at 7:23 AM, Bert Huijben <bert_at_qqmail.nl> wrote:
>
>
>> -----Original Message-----
>> From: Bert Huijben [mailto:bert_at_qqmail.nl]
>> Sent: woensdag 7 december 2011 11:42
>> To: dev_at_subversion.apache.org; hwright_at_apache.org
>> Subject: RE: svn commit: r1211162 -
>> /subversion/trunk/subversion/libsvn_wc/context.c
>>
>>
>>
>> > -----Original Message-----
>> > From: hwright_at_apache.org [mailto:hwright_at_apache.org]
>> > Sent: dinsdag 6 december 2011 22:22
>> > To: commits_at_subversion.apache.org
>> > Subject: svn commit: r1211162 -
>> > /subversion/trunk/subversion/libsvn_wc/context.c
>> >
>> > Author: hwright
>> > Date: Tue Dec  6 21:22:08 2011
>> > New Revision: 1211162
>> >
>> > URL: http://svn.apache.org/viewvc?rev=1211162&view=rev
>> > Log:
>> > Fix a potential memory cleanup ordering bug.
>>
>> This introduces too many other cleanup problems. This makes the context
>> and db unavailable from the normal cleanup handlers in the same pool, that
>> will run as normal cleanup.
>>
>> The wc_db is carefull to not make assumptions and allocates everything in
>> the same pool and *this* change breaks that by closing the db earlier than
>> those cleanups.
>>
>>  -0.5 (open for discussion).
>>
>>
>> If not extremely necessary we should never use a pre-cleanup handler in our
>> code as it just *starts* a race on who can cleanup first and thereby pushes
>> the segfaults in other code
>>
>> There should be no race: everything should be implemented as a proper last-
>> in, first out.
>
> One (of many) examples is how the the update editor uses pools.
> This editor installs (or installed) cleanup handlers to make sure it doesn't forget to run the workingqueue.
>
> This change (or similar changes in other places) will close the wc_db before the subpools had the option to access the database for their cleanup.
>
>
> Please revert this change and fix the real cause of the 'memory cleanup ordering bug' you describe. This change introduces too many other reordering bugs, which will be impossible to fix without introducing things like pre-pre-post-pre cleanup handlers.
>
> The only thing that works understandable for pool cleanup is a proper last-in-first-out. If somebody bends the rules by going out of order, other locations will have to do the same thing. And in the end everybody has to go this way :(

I happy to revert, though I don't know how to fix the problem I'm
seeing. The symptom is manifest when we go to close the context, we
iterate over all the wcroot's that the context holds. When running
against an APR compiled with pool lifetime debugging, this causes NULL
values to come out of the wcroot hash in the context, which is
something of an impossibility (setting a NULL value in an APR hash
erases the entry).

I figured this was due to the pool holding the above hash being
destroy *before* the context cleanup handler was run, and the change
in r1211162 seemed to bear that out. If possible, it'd be nice to get
some independent confirmation, to make sure I'm not imagining things.

If I do revert this change, it pretty much makes pool lifetime
debugging useless, as any operation would fail in this cleanup stage,
rather than where the real issue is. :(

In other words, there is a real bug here which is reproducible and
this change appears to fix. Are there real bugs which can be
demonstrated in response to your concerns above? I'm not discounting
your analysis or concerns, I'm just trying to justify reintroducing a
real issue in favor of fixing a theoretical one.

-Hyrum

-- 
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/