[svk-devel] a few locking-related issues

[David Glasser]

I have a few comments/questions on SVK's locking code (and some
related issues). The code is kind of subtle, so I'd appreciate it if
source-divey folks took a look and see if they agree with what I've
found.

(a) I don't think the acquisition of the giant lock is actually
atomic.  I believe there's a race condition in SVK::XD->giant_lock
between the -e test and the file creation.

What do folks use in Perl these days for (cross-platform)
filesystem-level locking?

(b) giant_unlock doesn't actually check $self->{giantlocked} before
deleting the lock file, and it really does look like it's possible for
giant_unlock to be called multiple times (in fact, it looks like it
*always* gets called multiple times in run_command), which means it
might actually be "unlocking" some other process's lock.

I think I need to add "return unless $self->{giantlocked}" to it. Does
that analysis sound correct?

(c) SVK::XD->lock is pretty clearly written to be only called once; as
part of its call, it dumps .svk/config *and releases the giant lock
protecting it*.  However, a bunch of commands call it multiple times,
either directly or through lock_target or lock_coroot: at the very
least, mkdir move propdel propset resolved and update.  This means
that it's writing to .svk/config without holding the giant lock, which
I think defeats the point of the giant lock.

What's the best thing to do here? I'm thinking we should remove the
_store_config and giant_unlock from SVK::XD->lock, and put a call to
_store_config if $self->{xd}->{modified} in SVK::Command->run_command
in between lock() and run().

(d) Several commands lock a condensed (arg_condensed or
target_condensed) version of the arguments.  Let's say that you have
checkouts at /path/a and /path/b and you use one of these commands on
files inside both checkouts at the same time; locking the condensed
argument will place the lock on /path (which is not inside any
checkout!)

First, I'm not sure if this in and of itself should be considered a
problem. Even if it isn't, though, it means that the idiom used in
several places of

	my ($cinfo, $coroot) = $self->{checkout}->get ($copath);

is flawed: $coroot here can end up with a datapoint that just contains
a lock entry and not an actual checkout at all! I haven't quite
managed to make this give me an obvious bug, but in playing around
with commands which use condensed arguments I have had a bit of odd
behavior which I think is related; I'll try to get more details later.

--dave

-- 
David Glasser | glasser at mit.edu | http://www.davidglasser.net/