Stefan Sperling wrote on Wed, Sep 22, 2010 at 14:31:58 +0200:
> On Wed, Sep 22, 2010 at 01:05:24PM +0200, Daniel Shahaf wrote:
> > Daniel Shahaf wrote on Tue, Sep 21, 2010 at 18:50:56 +0200:
> > > How about combining (1) and (3) --- that is, using the old locking mode
> > > (with its known race condition) but print a warning that the mirror
> > > server should be upgraded? That seems better than a fatal error for
> > > a condition that needs to be fixed on the server end.
> > >
> >
> > No replies, so I've implemented this in r999868. Let me know what you
> > think :).
> >
> > The error message says "external locking"; does the svnbook define this term?
>
> Maybe use "disabled built-in locking", like the svnsync help sync says:
>
> --disable-locking : Disable built-in locking. Use of this option can
> corrupt the mirror unless you ensure that no other
> instance of svnsync is running concurrently.
>
The intention of the phrasing "external locking" is to remind users that
they shouldn't pass --disable-locking unless they have some other,
out-of-band mutex in use. (Which may be in their own head in the case of
a single-user file://localhost mirror.) I'm afraid that just saying
"disable built-in locking" will result in people passing --disable-locking
without a second thought, which is much more likely to result in corruption.
> > (so the error message could link to the workaround docs)
>
> There are no workaround docs, apart from some scattered mailing list
> posts :(
>
> I'd prefer requiring people to pass either --disable-locking or
> --use-pre-1.7-style-locking to write to mirrors using pre-1.7 servers.
I see how an explicit --use-pre-1.7-style-locking would be justified if
the race condition was easy to trigger and often caused corruptions, but
I don't recall hearing much reports of the corruption issue on users@.
Does the corruption issue (considering its severity and commonness)
really warrant an explicit flag?
Either way, this is a SMOP to implement, since get_lock()'s caller has
a subcommand_baton_t available.
> This may be uncomfortable but will raise awareness of the issue.
> The error message should explain that the old servers will expose a
> race condition in svnsync's locking mechanism which can cause svnsync
> meta data to be corrupted on the mirror, and that for this reason, either
> the mirror server should be upgraded, or locking must be disabled entirely
> or the racy algorithm must be requested by the user. Users should also be
> made aware that they need to take care of preventing svnsync instances from
> writing to the mirror concurrently if the mirror doesn't run a 1.7 or
> later server.
>
> > > While here, there is a similar issue in svn_client_revprop_set2(). On
> > > the branch, it tries to be atomic if possible; but on trunk, it has
> > > always used a racy implementation. What do you think should be done
> > > in that case?
> >
> > The docstring on trunk has always promised only a racy implementation.
> > I'll just leave that as-is.
>
> But the Book hasn't. That is the problem.
> Our users don't read the docstrings, and shouldn't have to.
>
Our users don't call svn_client_revprop_set2(); our API consumers do.
And they should have warned *their* users about the raciness of that API
since long before the branch.
One of those consumers is 'svn propedit --revprop'; are you suggesting we
change the way 'svn propedit --revprop' works with pre-1.7 servers?
On a more general issue, svn_client_revprop_set2()'s docstring promises
that the change will be atomic if the server has the
SVN_RA_CAPABILITY_ATOMIC_REVPROPS capability, but I'm not sure whether
that condition is testable by svn_client_revprop_set2()'s caller?
> Stefan
Received on 2010-09-22 15:34:26 CEST