[svk-devel] Re: a few locking-related issues

[David Glasser]

On 7/27/06, David Glasser <glasser at mit.edu> wrote:
> (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().

I have a patch that attempts to deal with the two above points. Key changes:

* XD->_store_config dies if the giant lock is not held.  I'd recommend
that folks apply just this addition and then try things like making
two separate checkouts and running "svk mkdir checkout1/foo
checkout2/bar", noticing that this blows up, and getting scared.

* XD->lock no longer stores config and un-giant-locks.

* XD->giant_lock doesn't unlock (deleting the file) unless it thinks
it holds the lock.

* $cmd->run_command explicitly calls store instead of giant_unlock
after calling $cmd->lock.

I would LOVE some eyes before I apply it!

Note that I'm not yet dealing with the following two issues, on which
advice would still be great:

> (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?
>


> (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.


-- 
David Glasser | glasser at mit.edu | http://www.davidglasser.net/
-------------- next part --------------
=== t/19cleanup.t
==================================================================
--- t/19cleanup.t	(revision 51857)
+++ t/19cleanup.t	(patch locking-fixes level 1)
@@ -53,8 +53,8 @@
 # concurrency
 $xd->load;
 $xd->lock(__("$corpath/A"));
+$xd->store;
 $xd->{checkout}->store(__("$corpath/A"), { revision => 1});
-#$xd->store;
 
 my $xd2 = Storable::dclone($xd);
 my $svk2 = SVK->new(xd=> $xd2, output => \$output2);
=== lib/SVK/XD.pm
==================================================================
--- lib/SVK/XD.pm	(revision 51857)
+++ lib/SVK/XD.pm	(patch locking-fixes level 1)
@@ -180,6 +180,10 @@
 
 sub _store_config {
     my ($self, $hash) = @_;
+
+    $self->{giantlocked} or
+        die "Internal error: trying to save config without a lock!\n";
+
     local $SIG{INT} = sub { warn loc("Please hold on a moment. SVK is writing out a critical configuration file.\n")};
 
     my $file = $self->{statefile};
@@ -250,8 +254,6 @@
     }
     $self->{checkout}->store ($path, {lock => $$});
     $self->{modified} = 1;
-    $self->_store_config($self) if $self->{statefile};
-    $self->giant_unlock ();
 }
 
 =item unlock
@@ -305,7 +307,8 @@
 
 sub giant_unlock {
     my ($self) = @_;
-    return unless $self->{giantlock};
+    return unless $self->{giantlock} and $self->{giantlocked};
+
     unlink ($self->{giantlock});
     delete $self->{giantlocked};
 }
=== lib/SVK/Command.pm
==================================================================
--- lib/SVK/Command.pm	(revision 51857)
+++ lib/SVK/Command.pm	(patch locking-fixes level 1)
@@ -172,7 +172,7 @@
 	else {
 	    $self->msg_handler ($SVN::Error::FS_NO_SUCH_REVISION);
 	    eval { $self->lock (@args);
-		   $self->{xd}->giant_unlock if $self->{xd} && !$self->{hold_giant};
+		   $self->{xd}->store if $self->{xd} && !$self->{hold_giant};
 		   $ret = $self->run (@args) };
 	    $self->{xd}->unlock if $self->{xd};
 	    die $@ if $@;