Hi Bert,
All your arguments would be valid and I would fully agree
with them, if the FS layer was the root cause of the pool
issue. However, IMO, that is not the case and r1555297
merely accidentally caused free memory to be reused
earlier than before.
My understanding of mucc_commit is the following sequence:
ra = open_session(mtcc->pool)
editor = open_editor(ra, scratch_pool)
destroy(mtcc->pool)
... // editor cannot be cleaned up safely anymore
destroy(scratch_pool)
This is clearly invalid (unless I'm completely misreading the code).
I see three reasonable ways to fix this:
* don't destroy mtcc->pool but let it be cleaned up with mtcc,
i.e. the user has full control over its lifetime - as usual
* stick with the destructive commit() code and use a sub-pool
of scratch_pool create the editor and the clean it up *before*
the ra session becomes invalid
* allocate the editor in the same pool as the ra layer (current
code) which would not mean a extra memory consumption
in the "o.k." case.
-- Stefan^2.
On Sun, Jan 5, 2014 at 12:07 AM, Bert Huijben <bert_at_qqmail.nl> wrote:
> In the case of svnmucc the passed scratch pool is a subpool of the pool
> that the Ra session id created it (where the mtcc is created in). This is a
> perfectly valid case of using pools, just like the common iterpool pattern.
>
> If this shows a bug in the caching of the fs layer we should fix that bug
> instead of requiring the rest of our code to just use a single pool for
> everything. So your fs changes +- requires users to stop using iterpool and
> scratch pool everywhere.
>
> The fs layer should manage its own memory and not require API users to
> manage it for this library.
>
> The cache pattern of allocating pointers in an unrelated pool, pointing to
> data sorted centrally with different lifetime is broken. To make this work
> you need cleanup handlers both ways… or a separate allocation strategy
> (refcounts, global allocator?)
>
> (I'm not able to look at the svnmucc code in detail now, so there might be
> another bug that you fixed… but by reading your log message and change this
> looks like covering up for a design problem)
>
> We can't fix all code using our APIs until 2.0. Until then our existing
> API surface should cover for requirements changes.
>
> Bert
>
> Sent from Windows Mail
>
> *From:* Stefan Fuhrmann <stefan.fuhrmann_at_wandisco.com>
> *Sent:* Saturday, January 4, 2014 10:26 PM
> *To:* Bert Huijben <bert_at_qqmail.nl>
> *Cc:* Subversion Development <dev_at_subversion.apache.org>,
> commits_at_subversion.apache.org
>
> On Sat, Jan 4, 2014 at 9:47 PM, Bert Huijben <bert_at_qqmail.nl> wrote:
>
>> You only know this for the success path. The fs api should hide these
>> details..
>>
>
> With all due respect, the fault is svn_client_mtcc_commit
> destroying mtcc->pool before destroying mtcc itself (via
> its owning pool). Hence, the caller has no control over pool
> lifetimes and any allocation in sibling pools, e.g. scratch_pool,
> is potentially invalid.
>
> There is no way we can document these guarantees for public APIs and
>> really assume that all existing and future users follow all these new
>> requirements for a new Fs layer cache.
>>
>> We had similar caches in the past, but hid the ugly details by installing
>> cleanup handlers...
>>
>> We should really look at this well before 1.9, as adding such
>> requirements is a breaking change. And the commit API already existed since
>> well before 1.0 without these pool requirements. Strict dual pools are only
>> common since 1.7, but especially generated bindings (like swig) have
>> strange pool handling much longer.
>>
>
> I leave it to the author of the mucc code to simply go back
> to standard pool handling by destroying the mtcc with its
> parent pool instead of partly destroying it in some API function.
>
> -- Stefan^2.
>
> ------------------------------
>> From: stefan2_at_apache.org
>> Sent: 4-1-2014 15:05
>> To: commits_at_subversion.apache.org
>> Subject: svn commit: r1555350
>> -/subversion/trunk/subversion/libsvn_client/mtcc.c
>>
>> Author: stefan2
>> Date: Sat Jan 4 14:04:36 2014
>> New Revision: 1555350
>>
>> URL: http://svn.apache.org/r1555350
>> Log:
>> Fix segfault in svnmucc.
>>
>> * subversion/libsvn_client/mtcc.c
>> (svn_client_mtcc_commit): We explicitly destroy the MTCC pool and
>> have no knowledge about its relation to
>> SCRATCH_POOL. Thus, we can't use the
>> for anything non-trivial.
>>
>> Modified:
>> subversion/trunk/subversion/libsvn_client/mtcc.c
>>
>> Modified: subversion/trunk/subversion/libsvn_client/mtcc.c
>> URL:
>> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/mtcc.c?rev=1555350&r1=1555349&r2=1555350&view=diff
>>
>> ==============================================================================
>> --- subversion/trunk/subversion/libsvn_client/mtcc.c (original)
>> +++ subversion/trunk/subversion/libsvn_client/mtcc.c Sat Jan 4 14:04:36
>> 2014
>> @@ -1337,12 +1337,15 @@ svn_client_mtcc_commit(apr_hash_t *revpr
>> "is not a directory"),
>> session_url);
>>
>> + /* Beware that the editor object must not live longer than the MTCC.
>> + Otherwise, txn objects etc. in EDITOR may live longer than their
>> + respective FS objects. So, we can't use SCRATCH_POOL here. */
>> SVN_ERR(svn_ra_get_commit_editor3(mtcc->ra_session, &editor,
>> &edit_baton,
>> commit_revprops,
>> commit_callback, commit_baton,
>> NULL /* lock_tokens */,
>> FALSE /* keep_locks */,
>> - scratch_pool));
>> + mtcc->pool));
>>
>> err = editor->open_root(edit_baton, mtcc->base_revision, scratch_pool,
>> &root_baton);
>>
>>
>>
>>
>
Received on 2014-01-05 01:53:35 CET