On Sep 5, 2009, at 3:27 PM, Bert Huijben wrote:
>> -----Original Message-----
>> From: Hyrum K. Wright [mailto:hyrum_at_hyrumwright.org]
>> Sent: zaterdag 5 september 2009 20:27
>> To: svn_at_subversion.tigris.org
>> Subject: svn commit: r39153 - in trunk/subversion: libsvn_wc
>> tests/cmdline
>>
>> Author: hwright
>> Date: Sat Sep 5 11:26:48 2009
>> New Revision: 39153
>>
>> Log:
>> When opening access batons, recursively lock all its subdirectories
>> to
>> an
>> infinite depth as well. In wc-ng, we don't have the notion of
>> recursively
>> opening a database; it's either open or it's not. By making this
>> change now,
>> we can remove the existing complexity of determining how many
>> levels to
>> lock
>> and where to lock them.
>
> Hyrum,
>
> [Third mail on the same subject]
>
> Can you please revert this commit?
>
> I don't think we can ever 'fix' access batons. The access baton
> model is
> fixed.
>
> We can fix the locking problems by deprecating access batons (what
> we did),
> but we can't handle this by breaking all our compatibility wrappers.
> We need
> a new locking api... and we have to model access batons over them.
>
> Just look at the binding tests. (Ignore the java errors; those are
> caused by
> r39056).
>
> Almost every binding that using the wc apis, fails now because this
> changes
> the definition of what an access baton is.
>
> We need a new lock implementation, which can be recursive below a
> directory
> or global per wc-db, or anything.
>
> But access batons will have to be allocated per directory like they
> always
> were.. And svn_wc_adm_open(..., adm_related, ...) has to fail when
> there is
> already an access baton for that directory in the shared set.. Or we
> can
> never close access batons (and never free the write lock hold
> inside...)
>
> If we could have disabled it this way, why did we spend all this time
> building a compatibility layer?
>
> We promise to stay compatible, which we were until this patch, via
> all the
> access baton handling Greg created in wc-db (and I debugged through
> and
> through to fight those access batons close crashes over the last
> month).
>
>
> I think a simple helper like
> svn_wc__lock_recursive(wc_ctx, local_abspath, scratch_pool)
>
> Which can be called instead of virtually all svn_wc__adm_open()
> calls would
> make things easier too.. (Who needs an access baton return
> argument? ;)
>
>
> But, I don't think all this is necessary, since as far as I can tell
> all
> access batons are already related to the context now. (Since a few
> hours
> before your commit). Maybe I missed a few places in libsvn_wc, but I
> expect
> that we can now assume a lock without your change.
Bert,
I trust your judgement; you've been digging in the access baton code
much more than me at this point. If you feel r39153 needs to be
reverted, feel free to do so. However, I'd like to make a few
comments (hence the reason I'm not doing the revert myself). Please
correct me if my understanding is wrong.
1.7 will have centralized metadata, and in such a scheme "locking" a
single subdirectory to a given depth is non-sensical. It doesn't
matter if a caller wants to operate on a single file or the entire
working copy: they will need to use the centralized database. As you
mentioned with appropriate sqlite transactions and API boundaries,
many of our locking problems go away. So long as each API leaves the
database in consistent state, other processes with handles to the same
database can also do operations on it concurrently.
What this means for pre-1.7 clients is that an access baton just
becomes an opaque handle to the wc_db, possibly carrying around the
path for which it was opened to help in constructing absolute paths in
the compatibility wrappers. This may break backward compat right now,
but my feeling (and Greg's when I ping him in IRC about it a couple of
weeks ago) was that we could make this change as part of the
transition step toward simplifying the access baton in our compat
code. Once the baton becomes a thin wrapper around the wc_db,
access_close() becomes a no-op, for the concurrency reasons you
mentioned above. The shared handle just gets closed on pool cleanup.
I don't think we should introduce new locking APIs for libsvn_wc.
This adds complexity and overhead. Our interactions with the database
should be done in atomic units.
Oh, and all the tests still passed with the patch before r39150, so it
may be more correct than it initially appears.
-Hyrum
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2391389
Received on 2009-09-06 02:43:09 CEST