Use RunLock while daemonizing which is based on flock to prevent race conditions.

3 files changed, 35 insertions, 200 deletions

diff --git a/lib/chef/daemon.rb b/lib/chef/daemon.rb
index e5479905af..1426234645 100644
--- a/lib/chef/daemon.rb
+++ b/lib/chef/daemon.rb
@@ -18,6 +18,7 @@
 # I love you Merb (lib/merb-core/server.rb)
 
 require 'chef/config'
+require 'chef/run_lock'
 require 'etc'
 
 class Chef
@@ -32,9 +33,9 @@ class Chef
       #
       def daemonize(name)
         @name = name
-        pid = pid_from_file
-        unless running?
-          remove_pid_file()
+        runlock = RunLock.new(pid_file)
+        if runlock.test
+          # We've acquired the daemon lock. Now daemonize.
           Chef::Log.info("Daemonizing..")
           begin
             exit if fork
@@ -45,50 +46,12 @@ class Chef
             $stdin.reopen("/dev/null")
             $stdout.reopen("/dev/null", "a")
             $stderr.reopen($stdout)
-            save_pid_file
-            at_exit { remove_pid_file }
+            runlock.save_pid
           rescue NotImplementedError => e
             Chef::Application.fatal!("There is no fork: #{e.message}")
           end
         else
-          Chef::Application.fatal!("Chef is already running pid #{pid}")
-        end
-      end
-
-      # Check if Chef is running based on the pid_file
-      # ==== Returns
-      # Boolean::
-      # True if Chef is running
-      # False if Chef is not running
-      #
-      def running?
-        if pid_from_file.nil?
-          false
-        else
-          Process.kill(0, pid_from_file)
-          true
-        end
-      rescue Errno::ESRCH, Errno::ENOENT
-        false
-      rescue Errno::EACCES => e
-        Chef::Application.fatal!("You don't have access to the PID file at #{pid_file}: #{e.message}")
-      end
-
-      # Check if this process if forked from a Chef daemon
-      # ==== Returns
-      # Boolean::
-      # True if this process is forked
-      # False if this process is not forked
-      #
-      def forked?
-        if running? and Process.ppid == pid_from_file.to_i
-          # chef daemon is running and this process is a child of it
-          true
-        elsif not running? and Process.ppid == 1
-          # an orphaned fork, its parent becomes init, launchd, etc. after chef daemon dies
-          true
-        else
-          false
+          Chef::Application.fatal!("Chef is already running pid #{pid_from_file}")
         end
       end
 
@@ -113,31 +76,6 @@ class Chef
         nil
       end
 
-      # Store the PID on the filesystem
-      # This uses the Chef::Config[:pid_file] option, or "/tmp/name.pid" otherwise
-      #
-      def save_pid_file
-        file = pid_file
-        begin
-          FileUtils.mkdir_p(File.dirname(file))
-        rescue Errno::EACCES => e
-          Chef::Application.fatal!("Failed store pid in #{File.dirname(file)}, permission denied: #{e.message}")
-        end
-
-        begin
-          File.open(file, "w") { |f| f.write(Process.pid.to_s) }
-        rescue Errno::EACCES => e
-          Chef::Application.fatal!("Couldn't write to pidfile #{file}, permission denied: #{e.message}")
-        end
-      end
-
-      # Delete the PID from the filesystem
-      def remove_pid_file
-        if not forked? then
-          FileUtils.rm(pid_file) if File.exists?(pid_file)
-        end
-      end
-
       # Change process user/group to those specified in Chef::Config
       #
       def change_privilege
diff --git a/lib/chef/run_lock.rb b/lib/chef/run_lock.rb
index 0e1f24a9c5..2ddf02973a 100644
--- a/lib/chef/run_lock.rb
+++ b/lib/chef/run_lock.rb
@@ -35,11 +35,8 @@ class Chef
     # Create a new instance of RunLock
     # === Arguments
     # * :lockfile::: the full path to the lockfile.
-    # * :wait::: should wait for the release of the lock if it can't
-    #            be acquired
-    def initialize(lockfile, wait = true)
+    def initialize(lockfile)
       @runlock_file = lockfile
-      @wait = wait
       @runlock = nil
     end
 
@@ -50,31 +47,49 @@ class Chef
     #
     # The implementation is based on File#flock (see also: flock(2)).
     def acquire
+      wait unless test
+    end
+
+    #
+    # Tests and if successful acquires the system-wide lock.
+    # Returns true if the lock is acquired, false otherwise.
+    #
+    def test
       # ensure the runlock_file path exists
       create_path(File.dirname(runlock_file))
-      @runlock = File.open(runlock_file,'w+')
+      @runlock = File.open(runlock_file,'a+')
       # if we support FD_CLOEXEC (linux, !windows), then use it.
       # NB: ruby-2.0.0-p195 sets FD_CLOEXEC by default, but not ruby-1.8.7/1.9.3
       if Fcntl.const_defined?('F_SETFD') && Fcntl.const_defined?('FD_CLOEXEC')
         runlock.fcntl(Fcntl::F_SETFD, runlock.fcntl(Fcntl::F_GETFD, 0) | Fcntl::FD_CLOEXEC)
       end
-      unless runlock.flock(File::LOCK_EX|File::LOCK_NB)
-        # Another chef client running...
-        if @wait
-          runpid = runlock.read.strip.chomp
-          Chef::Log.warn("Chef client #{runpid} is running, will wait for it to finish and then run.")
-          runlock.flock(File::LOCK_EX)
-        end
+      # Flock will return 0 if it can acquire the lock otherwise it
+      # will return false
+      if runlock.flock(File::LOCK_NB|File::LOCK_EX) == 0
+        true
+      else
+        false
       end
     end
 
+    #
+    # Waits until acquiring the system-wide lock.
+    #
+    def wait
+      runpid = runlock.read.strip.chomp
+      Chef::Log.warn("Chef client #{runpid} is running, will wait for it to finish and then run.")
+      runlock.flock(File::LOCK_EX)
+    end
+
     def save_pid
       runlock.truncate(0)
       runlock.rewind # truncate doesn't reset position to 0.
       runlock.write(Process.pid.to_s)
-      runlock.flush # flush the file
+      # flush the file fsync flushes the system buffers
+      # in addition to ruby buffers
+      runlock.fsync
     end
-    
+
     # Release the system-wide lock.
     def release
       if runlock
diff --git a/spec/unit/daemon_spec.rb b/spec/unit/daemon_spec.rb
index 5cc32d0e40..8d66e08511 100644
--- a/spec/unit/daemon_spec.rb
+++ b/spec/unit/daemon_spec.rb
@@ -32,38 +32,6 @@ describe Chef::Daemon do
     end
   end
 
-  describe ".running?" do
-
-    before do
-      Chef::Daemon.name = "spec"
-    end
-
-    describe "when a pid file exists" do
-
-      before do
-        Chef::Daemon.stub!(:pid_from_file).and_return(1337)
-      end
-
-      it "should check that there is a process matching the pidfile" do
-        Process.should_receive(:kill).with(0, 1337)
-        Chef::Daemon.running?
-      end
-
-    end
-
-    describe "when the pid file is nonexistent" do
-
-      before do
-        Chef::Daemon.stub!(:pid_from_file).and_return(nil)
-      end
-
-      it "should return false" do
-        Chef::Daemon.running?.should be_false
-      end
-
-    end
-  end
-
   describe ".pid_file" do
 
     describe "when the pid_file option has been set" do
@@ -102,92 +70,6 @@ describe Chef::Daemon do
     end
   end
 
-  describe ".save_pid_file" do
-
-    before do
-      Process.stub!(:pid).and_return(1337)
-      Chef::Config[:pid_file] = "/var/run/chef/chef-client.pid"
-      Chef::Application.stub!(:fatal!).and_return(true)
-      @f_mock = mock(File, { :print => true, :close => true, :write => true })
-      File.stub!(:open).with("/var/run/chef/chef-client.pid", "w").and_yield(@f_mock)
-    end
-
-    it "should try and create the parent directory" do
-      FileUtils.should_receive(:mkdir_p).with("/var/run/chef")
-      Chef::Daemon.save_pid_file
-    end
-
-    it "should open the pid file for writing" do
-      File.should_receive(:open).with("/var/run/chef/chef-client.pid", "w")
-      Chef::Daemon.save_pid_file
-    end
-
-    it "should write the pid, converted to string, to the pid file" do
-      @f_mock.should_receive(:write).with("1337").once.and_return(true)
-      Chef::Daemon.save_pid_file
-    end
-
-  end
-
-  describe ".remove_pid_file" do
-    before do
-      Chef::Config[:pid_file] = "/var/run/chef/chef-client.pid"
-    end
-
-    describe "when the pid file exists" do
-
-      before do
-        File.stub!(:exists?).with("/var/run/chef/chef-client.pid").and_return(true)
-      end
-
-      it "should remove the file" do
-        FileUtils.should_receive(:rm).with("/var/run/chef/chef-client.pid")
-        Chef::Daemon.remove_pid_file
-      end
-
-
-    end
-
-    describe "when the pid file exists and the process is forked" do
-
-      before do
-        File.stub!(:exists?).with("/var/run/chef/chef-client.pid").and_return(true)
-        Chef::Daemon.stub!(:forked?) { true }
-      end
-
-      it "should not remove the file" do
-        FileUtils.should_not_receive(:rm)
-        Chef::Daemon.remove_pid_file
-      end
-
-    end
-
-    describe "when the pid file exists and the process is not forked" do
-      before do
-        File.stub!(:exists?).with("/var/run/chef/chef-client.pid").and_return(true)
-        Chef::Daemon.stub!(:forked?) { false }
-      end
-
-      it "should remove the file" do
-        FileUtils.should_receive(:rm)
-        Chef::Daemon.remove_pid_file
-      end
-    end
-
-    describe "when the pid file does not exist" do
-
-      before do
-        File.stub!(:exists?).with("/var/run/chef/chef-client.pid").and_return(false)
-      end
-
-      it "should not remove the file" do
-        FileUtils.should_not_receive(:rm)
-        Chef::Daemon.remove_pid_file
-      end
-
-    end
-  end
-
   describe ".change_privilege" do
 
     before do