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
Received on 2014-01-05 04:51:33 CET