search

brandonhilkert / sucker_punch

Sucker Punch is a Ruby asynchronous processing library using concurrent-ruby, heavily influenced by Sidekiq and girl_friday.
MIT License
2.64k stars 114 forks source link

Don't stop thread pool on at_exit #144

Closed kt0 closed 8 years ago

kt0 commented 8 years ago

Concurrent-ruby's ThreadPoolExecutor is automatically terminating pool on app exit [doc], to prevent this default changed auto_terminate to false. Also set min_threads to 0 to create new threads just when need (handled by concurrent) Fixes https://github.com/brandonhilkert/sucker_punch/issues/143

jdantonio commented 8 years ago

I recommend not merging this PR without further discussion and analysis.

The reason for the at_exit handler is to address a specific issue with JRuby. Turning off auto-termination will cause significant problems for JRuby users. Additionally, it may have unintended consequences on MRI and Rbx.

The at_exit handler explicitly shuts down the thread pool then waits for the shutdown to complete. Turning this handler off means that the runtime will unceremoniously kill the thread pool--regardless of the state of any in-process jobs. No guarantees can be made regarding in-use resources in this situation.

On JRuby there will be a bigger problem, however. When concurrent-ruby is run on JRuby, Concurrent::ThreadPoolExecutor becomes a think wrapper around java.util.concurrent.ThreadPoolExecutor. This is very fast and efficient. Unfortunately, it has a strange side-effect. The JVM will not shut down if there are any running thread pools. This is well-known and well-documented behavior that affects all JVM languages (like Clojure, for example). If you don't explicitly stop the thread pool using either the #shutdown or #kill method the JVM will hang forever and refuse to exit--even if there are no jobs being processed. There isno way around this--it's a feature of the JVM. It's intended to solve the inconsistency problem I mention above. So with this PR, users who run Sucker Punch will find that the JVM will never exit, even if no jobs have been post.

If the default shutdown behavior as implemented in the at_exit handler is insufficient for Sucker Punch's needs, there are other ways to handle shutdown. One option is to use the AtExit registry in concurrent-ruby to register a custom handler for Sucker Punch's thread pool. Another is to call at_exit directly from within Sucker Punch's initialization routine. You could also hook into Rails' shutdown process, or provide an explicit shutdown method on Sucker Punch.

I'm happy to help figure out and implement the best solution.

kt0 commented 8 years ago

I'm going to close this PR, we could continue discussion in this issue https://github.com/brandonhilkert/sucker_punch/issues/143

kt0 commented 8 years ago

Could changing auto_terminate method to shutdown instead of kill solve the problem? Or maybe making more!