FEATURE: Timeout option for ImageOptim.

Re-opening the original PR toy#162 and replace PR toy#184 (sorry for multiple
PRs).

The original commit

discourse/image_optim@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.

Add add some more details in the PR discussion.

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.28.0 (2020-12-18)
 
 * Fix and update list of markers in jpegoptim worker: allow to pass `com` instead of incorrect `comments` and add missing `xmp` and `none` [#188](https://github.com/toy/image_optim/issues/188) [@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,22 @@ 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
+              handler.process{ |src, dst| worker.optimize(src, dst) }
+            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 +249,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,6 +19,8 @@ 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)
@@ -157,7 +159,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_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