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 us...@. 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
