RE: svn commit:r1555350-/subversion/trunk/subversion/libsvn_client/mtcc.c

Note that I don't disagree with the mtcc change/fix.

But I think we must fix the reason why this pattern causes problems in fsfs, fsx, etc.

We only test extensively Subversion with short lived client tools and in some ways svnmucc is an exception there... But so are TortoiseSVN, AnkhSvn, Subclipse, etc. Etc.

That all our command line tools had to be patched shows a pattern to a bigger problem of new pool lifetime issues that we should really fix before 1.9 or we will have many segfaults to fix for the 1.9 patch releases.

Bert

-----Original Message-----
From: "Bert Huijben" <bert_at_qqmail.nl>
Sent: ‎05/‎01/‎2014 09:51
To: "Branko Čibej" <brane_at_wandisco.com>; "Subversion Development" <dev_at_subversion.apache.org>
Subject: RE: svn commit:r1555350-/subversion/trunk/subversion/libsvn_client/mtcc.c

While I agree that the mtcc code needs review and tweaks, many other arguments here would equally be valid to veto every wc-ng API added during 1.7 and 1.8 where we explicitly moved the context to the start of the argument list.

Making svnmucc a library is one of the most common feature requests on the SharpSvn list and I expect other users to benefit from this, for the sane reason as we added apis to svnversion and other tools in the past.

Releasing editor v2 in its current unproven state in javahl looks like a bigger problem but lets not tie every problem to each other and try to solve them separately instead of vetoing everything in one batch.

The pool requirements for fs* need work for 1.9.

The decision whether we allow external api usage of the WIP editor v2 for 1.9 needs work.

And opening up svnmucc as an api needs work. (If I had just implemented this as part of SharpSvn we would just have found the fsfs symptoms months from now when releasing 1.9)

Bert

From: Branko Čibej
Sent: ‎05/‎01/‎2014 04:51
To: Subversion Development
Subject: Re: svn commit: r1555350-/subversion/trunk/subversion/libsvn_client/mtcc.c

On 05.01.2014 01:53, Stefan Fuhrmann wrote:

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 agree that it looks clearly invalid; however, the API documentation is unclear. It only says that the RA session may not be used during the lifetime of the editor.

I would expect that "editor->close_edit()" (which you omit from that sequence and which happens before the "destroy(mtcc->pool)" call) to in fact kills the editor, and that destroying its containing pool is just a minor detail. It's obviously a bad idea for the editor implementation to assume anything about the RA session's pool after close_edit (or abort_edit).

On the other hand, the mtcc code is decidedly weird when it comes to pool lifetime handling, which becomes clear when the sequence above is seen in the context of the surrounding API calls and pool creations:

    execute(..., pool):
        scratch_pool = create(pool)
        mtcc = mtcc_create(pool, scratch_pool):
            mtcc.pool = create(pool)
            mtcc.ra = open_session(pool, scratch_pool)
        mtcc_commit(mtcc, scratch_pool):
            editor = open_editor(mtcc.ra, scratch_pool)
            editor.close_edit()
            destroy(mtcc.pool) ## Oops ...
        destroy(scratch_pool)

This makes the lifetime of the editor object (as opposed to the edit drive) overlap with the lifetime of the session. As I said, there's nothing in the API docs saying explicitly that this is wrong, but at the very least it's extremely unusual (and, FWIW, it has nothing to do with the single- vs. dual-pool pattern).

While it may be true that we use this pattern elsewhere, there's absolutely no call for repeating it here and perpetuating the broken pool usage. I'll note that this kind of crud (and in new code, no less) will make switching to Ev2 even more of a nightmare.

As an aside to all of the above: Why in blazes is svn_client_mtcc_* a public API? It is nothing but a wrapper for the RA commit editor API. It doesn't even hide the nasty details of getting file contents streams and checksums from the API consumer. As such, it's nothing but a maintenance burden. Yes, it hides the nasty details of Ev1 from the poor user, but that goal would be much better served by working on making Ev2 public (as I've already done in JavaHL). This svn_client_mtcc thing adds absolutely no benefit compared to Ev2, as far as I can see.

(It also breaks our established API pattern by putting the API context (the svn_client_mtcc_t) last in the parameter list, instead of first, but given that the whole idea of making this a public API is silly, that's just a minor nit.)

In other words, -1 to the whole svn_client_mtcc idea for the "technical" reason that it's a total waste of time.

-- Brane

-- 
Branko Čibej | Director of Subversion 
WANdisco // Non-Stop Data 
e. brane_at_wandisco.com