lib/zk/locker/locker_base.rb in zk-1.5.1 vs lib/zk/locker/locker_base.rb in zk-1.5.2

@@ -46,15 +46,18 @@
       #   locks will be generated, the default is Locker.default_root_lock_node
       #
       def initialize(client, name, root_lock_node=nil) 
         @zk = client
         @root_lock_node = root_lock_node || Locker.default_root_lock_node
-        @path = name
-        @locked = false
-        @waiting = false
-        @lock_path = nil
+
+        @path           = name
+        @locked         = false
+        @waiting        = false
+        @lock_path      = nil
+        @parent_stat    = nil
         @root_lock_path = "#{@root_lock_node}/#{@path.gsub("/", "__")}"
+
         @mutex  = Monitor.new
         @cond   = @mutex.new_cond
         @node_deletion_watcher = nil
       end
       
@@ -117,23 +120,25 @@
       end
       
       # @return [true] if we held the lock and this method has
       #   unlocked it successfully
       #
-      # @return [false] we did not own the lock
+      # @return [false] if we did not own the lock.
       #
+      # @note There is more than one way you might not "own the lock" 
+      #   see [issue #34](https://github.com/slyphon/zk/issues/34)
+      #
       def unlock
+        rval = false
         synchronize do
           if @locked
-            cleanup_lock_path!
+            rval = cleanup_lock_path!
             @locked = false
             @node_deletion_watcher = nil
-            true
-          else
-            false # i know, i know, but be explicit
           end
         end
+        rval
       end
 
       # (see #unlock)
       # @deprecated the use of unlock! is deprecated and may be removed or have
       #   its semantics changed in a future release
@@ -218,10 +223,11 @@
         synchronize do
           raise LockAssertionFailedError, "have not obtained the lock yet"            unless locked?
           raise LockAssertionFailedError, "not connected"                             unless zk.connected?
           raise LockAssertionFailedError, "lock_path was #{lock_path.inspect}"        unless lock_path
           raise LockAssertionFailedError, "the lock path #{lock_path} did not exist!" unless zk.exists?(lock_path)
+          raise LockAssertionFailedError, "the parent node was replaced!"             unless root_lock_path_same?
           raise LockAssertionFailedError, "we do not actually hold the lock"          unless got_lock?
         end
       end
 
       protected 
@@ -246,10 +252,12 @@
           lock_children(watch).tap do |ary|
             ary.sort! { |a,b| digit_from(a) <=> digit_from(b) }
           end
         end
 
+        # root_lock_path is /_zklocking/foobar
+        #
         def create_root_path!
           zk.mkdir_p(@root_lock_path)
         end
 
         # performs the checks that (according to the recipe) mean that we hold
@@ -260,27 +268,63 @@
         end
 
         # prefix is the string that will appear in front of the sequence num,
         # defaults to 'lock'
         #
+        # this method also saves the stat of root_lock_path at the time of creation
+        # to ensure we don't accidentally remove a lock we don't own. see 
+        # [rule #34](https://github.com/slyphon/zk/issues/34)...er, *issue* #34.
+        #
         def create_lock_path!(prefix='lock')
           synchronize do
-            @lock_path = @zk.create("#{root_lock_path}/#{prefix}", "", :mode => :ephemeral_sequential)
+            @lock_path = @zk.create("#{root_lock_path}/#{prefix}", :mode => :ephemeral_sequential)
+            @parent_stat = @zk.stat(root_lock_path)
           end
 
           logger.debug { "got lock path #{@lock_path}" }
           @lock_path
         rescue NoNode
           create_root_path!
           retry
         end
 
+        # if the root_lock_path has the same stat .ctime as the one
+        # we cached when we created our lock path, then we can be sure
+        # that we actually own the lock_path 
+        #
+        # see [issue #34](https://github.com/slyphon/zk/issues/34)
+        #
+        def root_lock_path_same?
+          synchronize do
+            return false unless @parent_stat
+
+            cur_stat = zk.stat(root_lock_path)  
+            cur_stat.exists? and (cur_stat.ctime == @parent_stat.ctime)
+          end
+        end
+
+        # we make a best-effort to clean up, this case is rife with race
+        # conditions if there is a lot of contention for the locks, so if we
+        # can't remove a path or if that path happens to not be empty we figure
+        # either we got pwned or that someone else will run this same method
+        # later and get to it
+        #
         def cleanup_lock_path!
-          logger.debug { "removing lock path #{@lock_path}" }
-          zk.delete(@lock_path)
+          rval = false
 
-          zk.delete(root_lock_path, :ignore => :not_empty) 
-          @lock_path = nil
+          synchronize do
+            if root_lock_path_same?
+              logger.debug { "removing lock path #{@lock_path}" }
+
+              zk.delete(@lock_path, :ignore => :no_node)
+              zk.delete(root_lock_path, :ignore => [:not_empty, :no_node])
+              rval = true
+            end
+
+            @lock_path = @parent_stat = nil
+          end
+
+          rval
         end
     end # LockerBase
   end # Locker
 end # ZK