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" <b...@qqmail.nl>
Sent: 05/01/2014 09:51
To: "Branko Čibej" <br...@wandisco.com>; "Subversion Development"
<dev@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. br...@wandisco.com
