FEATURE: Add global and worker timeout option.

This PR addresses the feedback what was originally given on PR toy#162 by
adding a default worker specific timeout option in addition to a global
timeout.

The original commit

discourse@8bf3c0e

on PR toy#162 with just the global timeout has been in production since
2018-07-08 on Discourse instances using a forked version of this gem.

We would like to get this change merged in, so that we can get off of
the discourse specific fork and use the latest version of the
image_optim gem.

Co-authored-by: Blake Erickson <[email protected]>

.rubocop.yml

@@ -67,7 +67,7 @@ Metrics/BlockLength:
     - 'spec/**/*.rb'
 
 Metrics/ClassLength:
-  Max: 150
+  Max: 200
 
 Metrics/CyclomaticComplexity:
   Max: 11

CHANGELOG.markdown

@@ -2,6 +2,8 @@
 
 ## unreleased
 
+* Add `timeout` option to ImageOptim [#21](https://github.com/toy/image_optim/issues/21) [@tgxworld](https://github.com/tgxworld) [@oblakeerickson](https://github.com/oblakeerickson)
+
 ## v0.27.1 (2020-09-30)
 
 * Fixed atomic replacement for case when equal `File::Stat#dev` doesn't mean that file can be linked [#180](https://github.com/toy/image_optim/issues/180) [@toy](https://github.com/toy)

README.markdown

@@ -291,6 +291,7 @@ optipng:
 * `:allow_lossy` — Allow lossy workers and optimizations *(defaults to `false`)*
 * `:cache_dir` — Configure cache directory
 * `:cache_worker_digests` - Also cache worker digests along with original file digest and worker options: updating workers invalidates cache
+* `:timeout` - Number of seconds before ImageOptim is timed out.
 
 Worker can be disabled by passing `false` instead of options hash or by setting option `:disable` to `true`.
 

lib/image_optim.rb

@@ -22,6 +22,8 @@
 
 # Main interface
 class ImageOptim
+  class TimeoutExceeded < StandardError; end
+
   # Nice level
   attr_reader :nice
 
@@ -46,6 +48,9 @@ class ImageOptim
   # Cache worker digests
   attr_reader :cache_worker_digests
 
+  # Timeout
+  attr_reader :timeout
+
   # Initialize workers, specify options using worker underscored name:
   #
   # pass false to disable worker
@@ -78,6 +83,7 @@ def initialize(options = {})
       allow_lossy
       cache_dir
       cache_worker_digests
+      timeout
     ].each do |name|
       instance_variable_set(:"@#{name}", config.send(name))
       $stderr << "#{name}: #{send(name)}\n" if verbose
@@ -111,10 +117,28 @@ def optimize_image(original)
 
     optimized = @cache.fetch(original) do
       Handler.for(original) do |handler|
-        workers.each do |worker|
-          handler.process do |src, dst|
-            worker.optimize(src, dst)
+        current_worker = nil
+
+        begin
+          with_timeout(@timeout) do
+            workers.each do |worker|
+              current_worker = worker
+              if worker.timeout && worker.timeout >= 0
+                with_timeout(worker.timeout) do
+                  handler.process{ |src, dst| worker.optimize(src, dst) }
+                end
+              else
+                handler.process{ |src, dst| worker.optimize(src, dst) }
+              end
+            end
           end
+        rescue TimeoutExceeded => e
+          if current_worker && current_worker.pid
+            pid = current_worker.pid
+            cleanup_process(pid)
+          end
+
+          raise e
         end
       end
     end
@@ -231,6 +255,37 @@ def env_path
 
 private
 
+  def with_timeout(timeout)
+    if timeout && timeout >= 0
+      thread = Thread.new{ yield if block_given? }
+      if thread.respond_to?(:report_on_exception)
+        thread.report_on_exception = false
+      end
+      fail TimeoutExceeded if thread.join(timeout).nil?
+    elsif block_given?
+      yield
+    end
+  end
+
+  def cleanup_process(pid)
+    Process.detach(pid)
+    Process.kill('-TERM', pid)
+    now = Time.now
+
+    while Time.now - now < 10
+      begin
+        Process.getpgid(pid)
+        sleep 0.1
+      rescue Errno::ESRCH
+        break
+      end
+    end
+
+    Process.kill('-KILL', pid) if Process.getpgid(pid)
+  rescue Errno::ESRCH
+    pid
+  end
+
   def log_workers_by_format
     $stderr << "Workers by format:\n"
     @workers_by_format.each do |format, workers|

lib/image_optim/cmd.rb

@@ -10,11 +10,32 @@ class << self
       # Return success status
       # Will raise SignalException if process was interrupted
       def run(*args)
-        success = system(*args)
+        if Process.respond_to?(:spawn)
+          if args.last.is_a?(Hash)
+            args.last[Gem.win_platform? ? :new_pgroup : :pgroup] = true
+          end
 
-        check_status!
+          begin
+            pid = Process.spawn(*args)
+          ensure
+            yield pid if block_given?
+          end
+
+          begin
+            Process.waitpid(pid)
+          rescue Errno::ECHILD
+            return
+          end
+
+          check_status!
+          $CHILD_STATUS.success?
+        else
+          success = system(*args)
+
+          check_status!
 
-        success
+          success
+        end
       end
 
       # Run using backtick

lib/image_optim/config.rb

@@ -119,6 +119,11 @@ def threads
       end
     end
 
+    def timeout
+      timeout = get!(:timeout)
+      timeout ? timeout.to_f : nil
+    end
+
     # Verbose mode, converted to boolean
     def verbose
       !!get!(:verbose)

lib/image_optim/runner/option_parser.rb

@@ -182,6 +182,10 @@ def wrap_regex(width)
     options[:allow_lossy] = allow_lossy
   end
 
+  op.on('--timeout N', Integer, 'Sets a timeout for optimization') do |timeout|
+    options[:timeout] = timeout
+  end
+
   op.separator nil
 
   ImageOptim::Worker.klasses.each_with_index do |klass, i|

lib/image_optim/worker.rb

@@ -19,13 +19,16 @@ class << self
       alias_method :init, :new
     end
 
+    attr_accessor :pid
+
     # Configure (raises on extra options)
     def initialize(image_optim, options = {})
       unless image_optim.is_a?(ImageOptim)
         fail ArgumentError, 'first parameter should be an ImageOptim instance'
       end
 
       @image_optim = image_optim
+      default_options
       parse_options(options)
       assert_no_unknown_options!(options)
     end
@@ -91,6 +94,17 @@ def inspect
 
   private
 
+    # Specify any options that should exist on every worker
+    def default_options
+      self.class.option('timeout', false, 'Specify a worker specific timeout') do |v|
+        if v && v.to_f > 0
+          v.to_f
+        else
+          false
+        end
+      end
+    end
+
     def parse_options(options)
       self.class.option_definitions.each do |option_definition|
         value = option_definition.value(self, options)
@@ -157,7 +171,8 @@ def run_command(cmd_args)
           {:out => Path::NULL, :err => Path::NULL},
         ].flatten
       end
-      Cmd.run(*args)
+
+      Cmd.run(*args){ |pid| self.pid = pid }
     end
   end
 end

spec/image_optim/cmd_spec.rb

@@ -17,13 +17,6 @@ def expect_int_exception(&block)
   end
 
   describe '.run' do
-    it 'calls system and returns result' do
-      status = double
-      expect(Cmd).to receive(:system).with('cmd', 'arg').and_return(status)
-      allow(Cmd).to receive(:check_status!)
-      expect(Cmd.run('cmd', 'arg')).to eq(status)
-    end
-
     it 'returns process success status' do
       expect(Cmd.run('sh -c "exit 0"')).to eq(true)
       expect($CHILD_STATUS.exitstatus).to eq(0)
@@ -35,6 +28,14 @@ def expect_int_exception(&block)
       expect($CHILD_STATUS.exitstatus).to eq(66)
     end
 
+    it 'accepts a block that yields the pid' do
+      expect(
+        Cmd.run('sh -c "exit 66"') do |pid|
+          expect(pid.is_a?(Integer)).to eq(true)
+        end
+      ).to eq(false)
+    end
+
     it 'raises SignalException if process terminates after signal' do
       skip 'signals are not supported' unless signals_supported?
       expect_int_exception do

spec/image_optim/config_spec.rb

@@ -237,4 +237,20 @@ def self.bin_sym
       expect(IOConfig.read_options(path)).to eq({})
     end
   end
+
+  describe '#timeout' do
+    before do
+      allow(IOConfig).to receive(:read_options).and_return({})
+    end
+
+    it 'is nil by default' do
+      config = IOConfig.new({})
+      expect(config.timeout).to eq(nil)
+    end
+
+    it 'converts value to a float' do
+      config = IOConfig.new(:timeout => '15.1')
+      expect(config.timeout).to eq(15.1)
+    end
+  end
 end

spec/image_optim/worker_spec.rb

@@ -31,7 +31,7 @@
 
       worker = worker_class.new(ImageOptim.new, :three => '...')
 
-      expect(worker.options).to eq(:one => 1, :two => 2, :three => '...')
+      expect(worker.options).to eq(:one => 1, :two => 2, :three => '...', :timeout => false)
     end
   end
 
@@ -74,7 +74,7 @@
 
       worker = DefOptim.new(ImageOptim.new, :three => '...')
 
-      expect(worker.inspect).to eq('#<DefOptim @one=1, @two=2, @three="...">')
+      expect(worker.inspect).to eq('#<DefOptim @one=1, @two=2, @three="...", @timeout=false>')
     end
   end
 
@@ -283,4 +283,35 @@ def worker_class_doubles(workers)
       expect(definition.default).to eq(1)
     end
   end
+
+  describe 'worker timeout option' do
+
+    it 'exists on every worker by default' do
+      worker_class = Class.new(Worker)
+      worker = worker_class.new(ImageOptim.new)
+
+      expect(worker.timeout).to eq(false)
+    end
+
+    it 'only accepts numbers' do
+      worker_class = Class.new(Worker)
+      worker = worker_class.new(ImageOptim.new, :timeout => 'asdf')
+
+      expect(worker.timeout).to eq(false)
+    end
+
+    it 'converts to floats' do
+      worker_class = Class.new(Worker)
+      worker = worker_class.new(ImageOptim.new, :timeout => 9)
+
+      expect(worker.timeout).to eq(9.0)
+    end
+
+    it 'only accepts positive numbers' do
+      worker_class = Class.new(Worker)
+      worker = worker_class.new(ImageOptim.new, :timeout => -1)
+
+      expect(worker.timeout).to eq(false)
+    end
+  end
 end

spec/image_optim_spec.rb

@@ -105,6 +105,21 @@ def temp_copy(image)
         expect(ImageOptim.optimize_image(path)).to be_nil
       end
     end
+
+    it 'allows timeout to be configured' do
+      base_options = {:timeout => 0.001}
+      image_optim = ImageOptim.new(base_options)
+
+      original_threads = Thread.list
+
+      expect do
+        image_optim.optimize_image(test_images.first)
+      end.to raise_error(ImageOptim::TimeoutExceeded)
+
+      # Ensure we don't leak any threads
+      (Thread.list - original_threads).each(&:join)
+      expect(Thread.list.count).to eq(original_threads.count)
+    end
   end
 
   describe '#optimize_image!' do