Re: Lock non-existent to allow reserving a path

Thanks Philip for sharing your insight into the lock mechanisms.

Sorry about the delay, wanted to find time to investigate.

On 24 feb 2014, at 19:56, Philip Martin <philip.martin_at_wandisco.com> wrote:

> Thomas Åkesson <thomas.akesson_at_simonsoft.se> writes:
>
>> Svn does not allow locking non-existent paths. It is blocked both in
>> libsvn_fs_base/libsvn_fs_fs as well as in mod_dav_svn. In the same
>> areas of the code in fs comments say:
>> "While our locking implementation easily supports the locking of
>> nonexistent paths, we deliberately choose not to allow such madness."
>>
>> Given that we now present valid use-cases, it is reassuring that the
>> authors of the locking code said it "easily supports the locking of
>> nonexistent paths".
>
> There is a way to create such locks at present: checkout, lock a file,
> delete the file or parent directory, commit with --no-unlock. We have a
> regression test for this: lock_tests.py:deleted_path_lock. (Possibly
> this behaviour could be considered a bug, perhaps 'commit --no-unlock'
> should remove the locks on deleted files, but implementing that would be
> hard.)

As discussed else-thread, should likely be considered a bug. It is not useful for these use cases since it requires the file to already exist.

>
>> Breakdown of changes:
>>
>> 1. Make mod_dav_svn and fs allow locking of non-existent paths. This
>> part is mandatory and I am attaching a rough patch that demonstrates
>> the feasibility. With this change, the use-case 3 can be supported.
>>
>> This part is the most important for us, to enable our software to
>> avoid race conditions btw users.
>>
>> It can be discussed whether it should be possible to configure the
>> server to disallow these locks. I don't think they do much harm and
>> configurability makes testing more complex.
>>
>> 2. There are already functional tools to manage these locks (svnadmin
>> lslocks/rmlocks/unlock and svn unlock URL). Are any improvements
>> required? E.g. making svn lslocks available (remotely).
>>
>> 3. Make the Working Copy support svn lock after svn add. This also
>> requires taking care of (or blocking) some combinations with move and
>> switch.
>
> The working copy is likely to be the hard bit.
>
> Probably we would want to block operations that undo the add: revert,
> move, delete, etc. when the lock is present. Attempting to move/remove
> the lock automatically would make these non-network operations into
> network operations. I'm not sure exactly what behaviour would be
> acceptable here. Do we want to prevent people reverting without network
> access?

Yes, I think we would want to block a good number of operations. If using locks on added files one might very well have to unlock before performing certain operations.

I just noticed that revert does not have a switch to release the locks. Actually, when reverting it is very easy to accidentally leave locks behind. It might make sense to never allow a revert of locked files without specifying either "--unlock" or "--no-unlock".

>
> Another tricky bit is how are update/status going to interact with these
> locks? I think we would want the behaviour to be similar to the current
> locks. So update will report if a lock gets broken/stolen and will
> remove the lock from the working copy. I think this requires
> update/status to start reporting added files to the server so that the
> server can report on the locks. That looks as if it will require
> protocol changes.

Interesting point. I agree we would want a consistent behaviour.

Perhaps it would be an acceptable performance degradation to get a separate lock report when having locked added files in WC.

>
> It gets even more tricky with directories:
>
> svn mkdir wc1/A
> touch wc1/A/f
> svn add wc1/A/f
> svn lock wc1/A/f
>
> svn mkdir wc2/A
>
> The lock wc1/A/f will prevent the commit of wc2/A, or at least I think
> it will.

Yes it does. Verified with the patch in my first post.

This code is in fs_make_dir(..) in tree.c:
  /* Check (recursively) to see if some lock is 'reserving' a path at
     that location, or even some child-path; if so, check that we can
     use it. */
  if (root->txn_flags & SVN_FS_TXN_CHECK_LOCKS)
    SVN_ERR(svn_fs_fs__allow_locked_operation(path, root->fs, TRUE, FALSE,
                                              pool));

I can't see any reason to make a recursive check for locks when doing mkdir (does not matter now, when locking non-existent is not allowed). Should be a simple change. An mkdir does not carry the same implications as a move or rmdir (they must of course do recursive check).

Copy is more complicated.

 svn mkdir wc1/A
 touch wc1/A/f
 svn add wc1/A/f
 svn lock wc1/A/f

 svn cp wc2/B wc2/A

Copy also checks locks recursively for to_path. Not sure if that matters now (when locking non-existent is not allowed).

If allowing locking non-existent I personally think it would be fine to fail that copy if there are locks below (no change to current code). Otherwise we could investigate if the locks actually conflict with nodes in the copy-source.

> Should status on wc2 show the lock? If wc2 doesn't have an
> added 'A' then the lock is of no interest, but when 'A' is added then
> the lock should be reported.

Yes, preferably. Is 'svn stat -u' doing a report similar to update? No separate lock report? More or less the same challenges as removing broken locks during update.

Do we see any other reasons to start reporting added files during an update? I have been thinking about the move improvements but not yet identified any situation.

I have identified a situation related to sparse Working Copies where reporting the added file would improve the user experience. When adding a file to a dir, which due to depth settings doesn't contain all files, a subsequent update does not trigger a conflict. The commit does. Reporting the added file could reveal the conflict earlier.

I would be interested in finishing the patch with the scope of allowing locking non-existent paths using URL and enhancing svn import such that the locks can be used for creating new files in the repository (add switch to send in lock tokens). I will also investigate the feasibility of reporting these locks in 'svn stat -u'. Support in the Working Copy can be a separate task awaiting more interest in those use cases.

Thoughts?

  
/Thomas Å.
Received on 2014-03-03 09:56:35 CET