Merge pull request resque#672 from kjwierenga/feature/conditional_fork_hooks

Don't invoke :before_fork and :after_fork hooks when not forking - fixes fd leak when used with NewRelic

Author: steveklabnik

lib/resque/worker.rb

@@ -136,10 +136,9 @@ def work(interval = 5.0, &block)
         if not paused? and job = reserve
           log "got: #{job.inspect}"
           job.worker = self
-          run_hook :before_fork, job
           working_on job
 
-          if @child = fork
+          if @child = fork(job)
             srand # Reseeding
             procline "Forked #{@child} at #{Time.now.to_i}"
             begin
@@ -148,11 +147,11 @@ def work(interval = 5.0, &block)
               nil
             end
           else
-            unregister_signal_handlers if !@cant_fork && term_child
+            unregister_signal_handlers if will_fork? && term_child
             procline "Processing #{job.queue} since #{Time.now.to_i}"
             redis.client.reconnect # Don't share connection with parent
             perform(job, &block)
-            exit! unless @cant_fork
+            exit! if will_fork?
           end
 
           done_working
@@ -184,7 +183,7 @@ def process(job = nil, &block)
     # Processes a given job in the child.
     def perform(job)
       begin
-        run_hook :after_fork, job
+        run_hook :after_fork, job unless @cant_fork
         job.perform
       rescue Object => e
         log "#{job.inspect} failed: #{e.inspect}"
@@ -228,15 +227,17 @@ def queues
 
     # Not every platform supports fork. Here we do our magic to
     # determine if yours does.
-    def fork
-      @cant_fork = true if $TESTING
-
+    def fork(job)
       return if @cant_fork
+      
+      # Only run before_fork hooks if we're actually going to fork
+      # (after checking @cant_fork)
+      run_hook :before_fork, job
 
       begin
         # IronRuby doesn't support `Kernel.fork` yet
         if Kernel.respond_to?(:fork)
-          Kernel.fork
+          Kernel.fork if will_fork?
         else
           raise NotImplementedError
         end
@@ -507,6 +508,10 @@ def working?
     def idle?
       state == :idle
     end
+    
+    def will_fork?
+      !(@cant_fork || $TESTING)
+    end
 
     # Returns a symbol representing the current worker state,
     # which can be either :working or :idle

test/worker_test.rb

@@ -376,6 +376,19 @@ def self.perform
     workerA.work(0)
     assert $BEFORE_FORK_CALLED
   end
+  
+  test "Will not call a before_fork hook when the worker can't fork" do
+    Resque.redis.flushall
+    $BEFORE_FORK_CALLED = false
+    Resque.before_fork = Proc.new { $BEFORE_FORK_CALLED = true }
+    workerA = Resque::Worker.new(:jobs)
+    workerA.cant_fork = true
+
+    assert !$BEFORE_FORK_CALLED, "before_fork should not have been called before job runs"
+    Resque::Job.create(:jobs, SomeJob, 20, '/tmp')
+    workerA.work(0)
+    assert !$BEFORE_FORK_CALLED, "before_fork should not have been called after job runs"
+  end
 
   test "very verbose works in the afternoon" do
     begin
@@ -408,6 +421,19 @@ def self.perform
     assert $AFTER_FORK_CALLED
   end
 
+  test "Will not call an after_fork hook when the worker can't fork" do
+    Resque.redis.flushall
+    $AFTER_FORK_CALLED = false
+    Resque.after_fork = Proc.new { $AFTER_FORK_CALLED = true }
+    workerA = Resque::Worker.new(:jobs)
+    workerA.cant_fork = true
+
+    assert !$AFTER_FORK_CALLED
+    Resque::Job.create(:jobs, SomeJob, 20, '/tmp')
+    workerA.work(0)
+    assert !$AFTER_FORK_CALLED
+  end
+
   test "returns PID of running process" do
     assert_equal @worker.to_s.split(":")[1].to_i, @worker.pid
   end