Allow workers to timeout. #149

tgxworld · 2017-04-18T04:46:46Z

No description provided.

mpalmer · 2017-04-18T05:48:51Z

README.markdown

@@ -271,6 +271,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 workers are timed out. *(defaults to `0`)*


What does the default of 0 mean? I assume it means "no timeout", but it would be good to clarify that, as a naive reading suggests that the default is basically "kill as soon as you start".

Good point 👍 I'll update the description for the option.

mpalmer · 2017-04-18T05:51:23Z

lib/image_optim/cmd.rb


 class ImageOptim
  # Helper for running commands
  module Cmd
+    class Timeout < StandardError; end


A more "obviously erroneous" name might be good here. The Timeout.timeout standard library method uses Timeout::Error by default, which isn't great but is at least obviously some sort of Bad Thing. Perhaps TimeoutExceeded?

mpalmer · 2017-04-18T05:56:13Z

lib/image_optim/cmd.rb

+        end
+      end
+
+      # Run commands using `Process.spawn`


This comment demonstrates why mentioning implementation details in comments is fraught with peril... as far as I can tell, you're not actually using Process.spawn in the method. Even if you were, anyone calling run_with_timeout shouldn't need to care how you're doing what you're doing, just what is supposed to happen. "Run the specified command, and kill it off if it runs longer than :timeout seconds" would be more descriptive.

mpalmer · 2017-04-18T05:58:12Z

lib/image_optim/cmd.rb

+            success = thread.value.exitstatus.zero?
+          end
+        ensure
+          stdin.close


If you're not going to use stdin, it's best to close it immediately after calling popen2, rather than in an ensure block after everything is done. It guarantees that the child process won't hang around unnecessarily waiting on input that will never come, and makes it clearer that the pipes definitely are getting closed. If you're very (very) sure that the subprocess won't produce output, you can close stdout too immediately, however if the subprocess does write anything, sadness will result (the process will get a SIGPIPE and almost certainly fall over in a heap). On the other hand, failing to read from stdout if it is left open can lead to the process stalling, because the pipe has a finite capacity, and if it writes too much to its stdout further writes will block on the pipe emptying, and the process won't make any progress, which is also sad.

Isn't multiprocessing fun?

Thank you for explaining @mpalmer 👍

I've gone ahead and just close stdout since the only method that uses this library redirects :out and :err to Path::NULL.

mpalmer · 2017-04-18T05:59:53Z

lib/image_optim/cmd.rb

+          now = Time.now
+          pid = thread[:pid]
+
+          sleep 0.001 while (Time.now - now) < timeout && thread.alive?


Willthread.join(timeout) work here instead?

👍 Now I know that I can set a limit on Thread#join

mpalmer · 2017-04-18T06:26:11Z

lib/image_optim/cmd.rb

+
+          while Time.now - now < 10
+            begin
+              Process.getpgid(pid)


This is more idiomatically expressed as Process.kill(0, pid) (and also raises ESRCH when the process goes away).

mpalmer · 2017-04-18T06:27:12Z

lib/image_optim/cmd.rb

+
+      def cleanup_process(pid)
+        Thread.new do
+          Process.kill('-TERM', pid)


From Process.kill:

If signal is negative (or starts with a minus sign), kills process groups instead of processes.

I'm pretty sure that isn't what you want to have happen here. A straight-up TERM might be more appropriate.

(Also applies to the -KILL on line 117)

Hmm I want to be killing the process group here since we won't know if the command being run will spawn child processes. My understanding is that sending a signal to the process group would also send the signal to all the processes in the process group.

Oooh, of course. Not sure why I didn't associate the new_pgroup code with the "kill the process group"... Carry on.

toy

Sorry for the wait. Checked a lot of stuff and created an issue in ruby bug tracker.

toy · 2017-04-23T14:16:11Z

lib/image_optim/worker/class_methods.rb

-            options[:allow_lossy] = image_optim.allow_lossy
+
+          [:allow_lossy, :timeout].each do |option_key|
+            if !options.key?(option_key) && klass.method_defined?(option_key)


I think a guard (or two) would be cleaner in this case

Sorry I don't quite what you mean here. What would the guard be used for?

next if … and next unless … instead of if with block

toy · 2017-04-23T14:19:30Z

lib/image_optim/worker/class_methods.rb

+          0,
+          'Number of seconds before worker is timed out. Must be greater than' \
+          '0 to enable timeout.'
+        ){ |v| v }


toy · 2017-04-23T17:46:25Z

lib/image_optim/worker/class_methods.rb

+        option(
+          :timeout,
+          0,
+          'Number of seconds before worker is timed out. Must be greater than' \


Space missing between than and 0

Ah good catch will fix

toy · 2017-04-23T17:52:39Z

lib/image_optim/worker/jhead.rb

@@ -7,6 +7,8 @@ class Worker
    #
    # Jhead internally uses jpegtran which should be on path
    class Jhead < Worker
+      TIMEOUT_OPTION = timeout_option


It is be better to not timeout jhead – it should not be a time consuming process, but it may time out for a big jpeg and if exif will be removed, the image will be "broken"

From what I understand, none of the image optimizing is done in place right? Even for optimize_image!, it does not replace the original image if optimization fails.

You are right. I understood only later that you've implemented the timeout in the way that if any worker timeouts, than optimisation will not continue. But if workers that timeout are just skipped, it is better to now allow jhead to timeout.

I still think that jhead should not timeout

toy · 2017-04-23T18:05:55Z

lib/image_optim/worker.rb

@@ -151,7 +153,18 @@ def run_command(cmd_args)
          {:out => Path::NULL, :err => Path::NULL},
        ].flatten
      end
-      Cmd.run(*args)
+
+      seconds_to_timeout = timeout || @image_optim.timeout


timeout will already contain specific or global value, it is passed in Worker.init_all

toy · 2017-04-23T23:17:29Z

lib/image_optim/cmd.rb

@@ -15,6 +18,44 @@ def run(*args)
        success
      end

+      def support_timeout?


I think supports_timeout? or timeout_supported? would be better

Sure I'll go with supports_timeout?

toy · 2017-04-23T23:21:26Z

lib/image_optim/cmd.rb

+        init_options!(args)
+
+        begin
+          stdin, stdout, thread = Open3.popen2(*args)


As pipe is not really used, why not directly use spawn and Process.detach?

Also it looks like some workers don't like closed stdin or stdout, so maybe better to use :in, :out with Path::NULL options of spawn

toy · 2017-04-23T23:30:37Z

lib/image_optim/cmd.rb

+            thread.kill
+            fail TimeoutExceeded
+          else
+            success = thread.value.exitstatus.zero?


Running ruby -Ilib bin/image_optim --timeout 1 -r spec/images dies with undefined method zero?' for nil:NilClass`

toy · 2017-04-23T23:51:19Z

lib/image_optim/worker.rb

+        begin
+          Cmd.run_with_timeout(seconds_to_timeout, *args)
+        rescue Cmd::TimeoutExceeded
+          raise ImageOptim::Worker::TimeoutExceeded


This exception is not handled, cli interface will fail for all images and I would expect timeout to stop only one worker and try next one.

toy · 2017-04-23T23:58:23Z

lib/image_optim/cmd.rb

+          pid = thread[:pid]
+
+          if thread.join(timeout).nil?
+            cleanup_process(pid)


cleanup_process creates a thread which is never joined, then the watching thread is killed, I think in this case it is better to wait for cleanup to finish

toy · 2017-05-10T22:00:41Z

ping @tgxworld

tgxworld · 2017-05-15T10:24:27Z

@toy Noted. Give me awhile as I've been alittle busy at work, will fix those changes soon.

tgxworld · 2017-05-19T08:38:51Z

lib/image_optim/cmd.rb

+          thread = Process.detach(pid)
+
+          if thread.join(timeout).nil?
+            cleanup_process(pid)


@toy The reason I'm not calling join here is because we don't want the cleanup process to be blocking the main process.

Check the comment for Thread.new of cleanup_process

tgxworld · 2017-05-19T08:40:01Z

lib/image_optim/cmd.rb

+            Process.detach(pid)
+            now = Time.now
+
+            while Time.now - now < 10


@toy 10 is sort of a magic number here. Any ideas about how we can handle this better?

tgxworld · 2017-05-19T08:41:22Z

@toy Updated per your review comments. Rubocop is broken on master so Travis is having abit of sad here as well.

toy · 2017-05-20T12:57:04Z

@tgxworld I've reverted the last commit, so rubocop doesn't complain. I'll try to review the changes in the nearest time

toy

Lots of small comments, but main points are:

simplifying run_with_timeout
cleanup in background can create problems with reusing of temporary files
it is better not to timeout jhead

toy · 2017-05-20T12:53:32Z

lib/image_optim.rb

+          begin
+            handler.process{ |src, dst| worker.optimize(src, dst) }
+          rescue Worker::TimeoutExceeded
+            next


next is not needed, but a message to stderr if verbose is on can be helpful

Sure I can add that

toy · 2017-05-22T21:53:12Z

lib/image_optim/cmd.rb

@@ -15,6 +17,40 @@ def run(*args)
        success
      end

+      def supports_timeout?
+        if defined?(JRUBY_VERSION)
+          JRUBY_VERSION >= '9.0.0.0'


There are a lot of versions to go till 10.0 and all of them >= 1.9, but next major jruby version will not be.
Does it really work well with jruby at all and only starting with version 9? I remember having lots of problems trying to just make new system (spawn) syntax work with jruby (though maybe pre 9).

toy · 2017-05-22T21:54:33Z

lib/image_optim/cmd.rb

+        if defined?(JRUBY_VERSION)
+          JRUBY_VERSION >= '9.0.0.0'
+        else
+          RUBY_VERSION >= '1.9'


Maybe Kernel.respond_to?(:spawn)

toy · 2017-05-22T21:56:04Z

lib/image_optim/worker.rb

+      if timeout
+        begin
+          Cmd.run_with_timeout(timeout, *args)
+        rescue Cmd::TimeoutExceeded


Is it works having two new exceptions just to reraise?

toy · 2017-05-22T22:20:09Z

lib/image_optim/cmd.rb

+        return run(*args) unless timeout > 0 && supports_timeout?
+
+        success = false
+        init_options!(args)


Method name is not descriptive, method is short and not used for anything else, is it worth extraction?

Rubocop was complaining about the method being too long 😉

This method can be shortened, so maybe it will fit inside ;)

toy · 2017-05-22T22:33:55Z

lib/image_optim/cmd.rb

+              begin
+                Process.kill(0, pid)
+                sleep 0.001
+                next


toy · 2017-05-22T22:34:54Z

lib/image_optim/cmd.rb

+                sleep 0.001
+                next
+              rescue Errno::ESRCH
+                break


At this place you can already return or even combine it with other rescue as in both cases there should be no such process

toy · 2017-05-22T22:36:08Z

lib/image_optim/worker/jhead.rb

@@ -7,6 +7,8 @@ class Worker
    #
    # Jhead internally uses jpegtran which should be on path
    class Jhead < Worker
+      TIMEOUT_OPTION = timeout_option


I still think that jhead should not timeout

toy · 2017-05-23T19:30:16Z

lib/image_optim/worker/class_methods.rb

+      def timeout_option
+        option(
+          :timeout,
+          nil,


Insert Integer as next parameter so you don't need to add NilClass in option_parser.rb

toy · 2017-05-23T19:34:42Z

lib/image_optim/cmd.rb

+          thread = Process.detach(pid)
+
+          if thread.join(timeout).nil?
+            cleanup_process(pid)


Check the comment for Thread.new of cleanup_process

tgxworld · 2017-06-05T05:52:25Z

@toy I've been thinking about my approach here and feel like it might be wrong. The use case that we had at Discourse was to timeout calls to optimize_images! so that we wouldn't hit Unicorn's per request timeout.. What I initially wanted to achieve is to implement a global timeout for all the workers instead of timeouts for individual workers. Are you open to that? At least for us, timeouts for individual workers isn't going to be useful.

toy · 2017-06-08T22:00:57Z

@tgxworld Sure, there can be timeout per worker and timeout for the whole optimisation (two global options). I'm fine with a PR for one of them or both. Probably if you decide to only implement one for whole optimisation, better open a separate PR, so this one can also be finished later

smileart · 2017-09-06T15:18:20Z

Guys, thanks for the lib and all the work done! I'm really waiting for this one to be merged since when I use external Timeout.timeout around the compression method it (obviously) leaves zombie processes which eventually use too many threads! For now I came up with a dirty hack of killing parent (puma) workers which produce those zombies but it'd be much better to have this functionality in the gem itself. Or at least it'd be nice to expose worker's PID to give a chance to detach it from the outside if we ran out of execution time. Anyway, thanks one more time. Cheers! 👍

toy · 2017-09-06T20:54:36Z

@smileart Thanks for reminding that the issue will be useful

tgxworld · 2018-07-05T05:14:42Z

Closing in favor of #162

The original commit discourse/image_optim@8bf3c0e, see discussion in resolved PRs. Resolves toy#21, resolves toy#148, resolves toy#149, resolves toy#162, resolves toy#184, resolves toy#189. Co-authored-by: Blake Erickson <o.blakeerickson@gmail.com>

The original commit discourse/image_optim@8bf3c0e, see discussion in resolved PRs. Resolves toy#21, resolves toy#148, resolves toy#149, resolves toy#162, resolves toy#184, resolves toy#189. Co-authored-by: Blake Erickson <o.blakeerickson@gmail.com> Co-authored-by: Ivan Kuchin <tadump+git@gmail.com>

The original commit discourse/image_optim@8bf3c0e, see discussion in resolved PRs. Resolves #21, resolves #148, resolves #149, resolves #162, resolves #184, resolves #189. Co-authored-by: Blake Erickson <o.blakeerickson@gmail.com> Co-authored-by: Ivan Kuchin <tadump+git@gmail.com>

tgxworld force-pushed the implement_timeout branch 4 times, most recently from 3459cd0 to ed50964 Compare April 18, 2017 06:16

mpalmer reviewed Apr 18, 2017

View reviewed changes

tgxworld force-pushed the implement_timeout branch 6 times, most recently from 1101d9c to 5b4fd97 Compare April 20, 2017 07:11

toy requested changes Apr 24, 2017

View reviewed changes

tgxworld force-pushed the implement_timeout branch 8 times, most recently from c0bef26 to e49089b Compare May 19, 2017 08:37

tgxworld commented May 19, 2017

View reviewed changes

tgxworld force-pushed the implement_timeout branch from e49089b to 9670fb7 Compare May 22, 2017 00:10

toy requested changes May 23, 2017

View reviewed changes

Allow workers to timeout.

48fc172

tgxworld force-pushed the implement_timeout branch from 9670fb7 to 13e5d81 Compare June 5, 2017 04:47

tgxworld force-pushed the implement_timeout branch 2 times, most recently from eb6e374 to 4feeb03 Compare June 5, 2017 05:03

tgxworld added 2 commits June 5, 2017 14:08

Try Process spawn

72e7823

Remove timeout option for JHead.

82bd995

tgxworld force-pushed the implement_timeout branch 3 times, most recently from 5e130be to 8416f03 Compare June 5, 2017 05:28

Fix PR comments.

0a576eb

tgxworld force-pushed the implement_timeout branch from 8416f03 to 0a576eb Compare June 5, 2017 05:49

tgxworld closed this Jul 5, 2018

toy mentioned this pull request Jul 22, 2018

FEATURE: Timeout option for ImageOptim. #162

Closed

Allow workers to timeout. #149

Allow workers to timeout. #149

Conversation

tgxworld commented Apr 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgxworld Apr 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toy commented May 10, 2017

tgxworld commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgxworld commented May 19, 2017

toy commented May 20, 2017

toy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgxworld commented Jun 5, 2017

toy commented Jun 8, 2017

smileart commented Sep 6, 2017 • edited

toy commented Sep 6, 2017

tgxworld commented Jul 5, 2018

tgxworld Apr 18, 2017 •

edited

smileart commented Sep 6, 2017 •

edited