sneakers: Alternative timeout mechanism worker operations (the work method)
Hi,
Could you please remove the use of Timeout::timeout from line 55 of worker.rb? This function is inherently unsafe and will potentially ‘randomly’ kill workers.
At the very least, would it be possible to default :timeout_job_after to 0 so that timeouts are switched off by default? Then people can decide for themselves whether they want to run the risks of Timeout::timeout in their code.
I’ve been hit by this a few times now and finally understood what’s going on. Given that the timeout exceptions spring up in random places in my worker code (in fact, deep in some random Rails library) it took a long time to understand that it was actually Sneakers breaking stuff and not my own code. The ability to time out is fairly well hidden in the documentation, so I only found it when I specifically searched for timeout in connection to Sneakers.
Many thanks,
Steffen
About this issue
- Original URL
- State: closed
- Created 6 years ago
- Reactions: 7
- Comments: 15 (3 by maintainers)
Commits related to this issue
- Bump default worker operation timeout to 600 seconds See #343 for background. 5 seconds is an objectively low default. An alternative timeout mechanism is still under consideration. — committed to jondot/sneakers by michaelklishin 6 years ago
- Follow-up test expectation update to ccbd5a5e31f53a7ca6f5712ade4371c3efa78f0b References #343. — committed to jondot/sneakers by michaelklishin 6 years ago
- BREAKING CHANGE: remove enforced Worker#work timeout See #343 for the background. Worker implementations now must handle timeouts in a way they see fit. This is unfortunate but there is no way to enf... — committed to jondot/sneakers by michaelklishin 6 years ago
https://jvns.ca/blog/2015/11/27/why-rubys-timeout-is-dangerous-and-thread-dot-raise-is-terrifying/
There is also cool repo about timeouts in the major Ruby gems https://github.com/ankane/the-ultimate-guide-to-ruby-timeouts
@michaelklishin I’m OK with removal, I even have a separate local branch with removed timeout, but stuck with tests, because Sneakers uses
minitestandrr, which I’m not much familiar with (I’mrspecandrspec-mockfanboy). Unfortunately, I had to switch to Rails app development and stay aside of our async messaging system right now, so I’m not sure I will find the time to fix the subject and adjust docs in the nearest future. I may push my changes and maybe someone could pick up the work.Anyway I’m happy we have consolidated decision, I guess @szschaler is about the same. Thank you 🙇