FEATURE: Add global and worker timeout option.

[oblakeerickson]

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

The original commit

discourse@8bf3c0e

on PR #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 o.blakeerickson@gmail.com

[toy]

Thanks a lot for reopening the PR and sorry for the wait.

I'm still procrastinating what would be the good interface for handling timeouts when optimising single image (optimize_image), multiple images (optimize_images) and using CLI interface. Also difference in timeout of whole optimisation and of single worker. And whether it should be configured by more options :)

Currently my train of thoughts lead to following:
I think that if single worker times out then whole optimisation should not abort and it should continue as if the worker was not able to optimise the image.
Also optimisation of multiple images and CLI interface should continue with other images even if it times out for some image.
But I'm not sure if timeout of whole optimisation should mean failure or whatever result was already achieved should be returned. And in case of failure if it should be expressed as it is already done by optimisation failure or using an exception.

What are your thoughts on this?

[oblakeerickson]

I think that if single worker times out then whole optimisation should not abort and it should continue as if the worker was not able to optimise the image.

Yes, I agree with this. I just played around with my PR and this currently isn't the case, so I'll work on a fix for it.

Also optimization of multiple images and CLI interface should continue with other images even if it times out for some image.

This should be fine. Discourse doesn't use optimize_images, but instead only does one at a time using multiple optimize_image calls that run in separate sidekiq jobs.

But I'm not sure if timeout of whole optimisation should mean failure or whatever result was already achieved should be returned. And in case of failure if it should be expressed as it is already done by optimisation failure or using an exception.

Oh okay, yes, I think if there is a global timeout and we have already achieved a result we should return the result instead of an exception. The exception should only be for when we have zero results and have reached the global timeout.

Looks like there are a couple of changes to make to this pr then. I'll start working on those change to match these ideas and also clean up any rubocop issues now that it has been updated.

[oblakeerickson]

Okay, I pushed a couple more commits addressing your feedback:

When optimizing multiple images with optimize_images a single image timeout doesn't break execution. The global timeout will still raise an exception though if it is reached.

When optimizing a single image do not raise an exception if a single worker has succeeded but has reached the timeout period while running through the remaining workers.

When optimizing a single image if a single worker times out it will continue on to the next worker until the global timeout is reached.

Looks like there are still a couple small rubocop issues. Most of them are just about a method and block being too long and there are a couple that aren't related to this pr, so I haven't touch them.

Overall, let me know what you think.

[oblakeerickson]

Looks like travis is showing an error when rspec is passing locally for me.

  1) ImageOptim#optimize_image optimizing images will timeout if specified
     Failure/Error:
       expect do
         image_optim.optimize_images(copies)
       end.to raise_error(ImageOptim::TimeoutExceeded)

       expected ImageOptim::TimeoutExceeded, got #<fatal: No live threads left. Deadlock?

Looking at the verbose info when running on travis it looks like it is related to setting the number of threads to 2.

$ bundle exec image_optim --info
image_optim v0.27.1
config:
  verbose: true
nice: 10
threads: 2

If I do the same locally I can reproduce the error when running rspec. Now that I can do that I'll see if I can come up with a fix.