Recommended method to set up job execution timeout

[francois-ferrandis]

Hello there! 👋

First of all, thank you for this great gem, it solves many problems that we have and it is wonderfully documented. 😍

This issue is kind of a followup to #19.

My team and I were wondering whether it is dangerous to use the Timeout.timeout method as demonstrated in README.md. We have come across several articles explaining that using this method puts you at risk:

• https://jvns.ca/blog/2015/11/27/why-rubys-timeout-is-dangerous-and-thread-dot-raise-is-terrifying/
• http://blog.headius.com/2008/02/ruby-threadraise-threadkill-timeoutrb.html

So we got curious as to why there is no mention of the risks in the README even though you seem to know about them (#19)! Is there something that would make you consider the risks here are acceptable? We would love to have your input on this 👂

I just took a peek at other librairies that offer a timeout feature:

• Delayed Job uses Timeout.timeout
• Resque seems to save a timeout value to DB but I could not find how it is used
• Sneakers is event more mysterious about how it handles timeouts

Most librairies don't offer that feature, for good reasons I presume.

Thank you so much for your time! 🙏

[bensheldon]

@francois-ferrandis great question!

Yes, it is dangerous to use Timeout.timeout in GoodJob. That's because GoodJob, like Puma, is multithreaded and Thread.raise/kill can leave shared resources (like Active Record database connection pool) in uncertain states.

we got curious as to why there is no mention of the risks in the README

Simply overlooked on my part. I think I wrote that in the very early days of GoodJob and that section is inadequate today. I think my thinking there was that if your code is safe to be wrapped in a Timeout.timeout then it's safe for GoodJob too... which is true, but doesn't mention that it's very, very, very likely that your code isn't safe to do that.

I do think the Readme should mention the dangers.