Improve behaviour when posting to a shutdown thread pool

[rkday]

If an executor has a handle_overflow method defined, use that in the case where we post to an executor which is not running. If no handle_overflow method is defined (e.g. for the single-thread executor), preserve the current behaviour of returning false.

Fixes Github issue #192.

I've tested on MRI 2.0, but not on anything else yet (I'm hoping TravisCI will take care of some of that for me?).

I'm happy to apply any feedback (including any stylistic conventions I haven't spotted and followed), but had two specific questions:

• handle_overflow is now doing something a bit more general (i.e. handling any case where we can't execute a task), and I'd like to rename it - but I'm not sure to what. fallback_handler, maybe?
• I've used the following pattern for some of the tests, to try and remain agnostic to whether post returns false or throws an exception - is that OK or is there a nicer approach?

begin
  subject.post{ expected.increment }
  rescue Concurrent::RejectedExecutionError
end

[coveralls]

Coverage decreased (-0.08%) when pulling 7051d2f on rkday:post_to_shutdown into df20680 on ruby-concurrency:master.

[jdantonio]

Thank you for the PR! I definitely like this behavior better. Unfortunately, I realized after I merged the PR that the behavior for RubyExecutor#post and JavaExecutor#post now diverge. Would you be interested in taking a look at JavaExecutor#post in executor.rb line 223 and trying to update it to match? I can look at it if you would prefer.

Honestly, I've never been overly fond of the term "overflow" in this context even though I'm the one who gave it that name. But I'm far less fond of Java's "rejected execution" terminology. We aren't really rejecting the task in most cases, just handling it differently. I'm comfortable renaming it to "fallback" (fallback_policy, can_fallback?, handle_fallback) or anything else that better represents the new behavior.

[jdantonio]

Just a thought: we could move overflow/fallback handling in JavaThreadPoolExecutor to pure-Ruby code by moving/copying handle_overflow from RubyThreadPoolExecutor into a location accessible to JavaThreadPoolExecutor. The Ruby code was originally written to mimic the behavior of java.util.concurrent.ThreadPoolExecutor, but nothing says we have to remain 100% true to Java's implementation, so long as our Ruby and JRuby abstractions behave the same.

[rkday]

Yep, no problem - I'll take a look tomorrow. I was about to ask how you wanted me to handle the fact that Java*Executor classes don't have a handle_overflow method at the moment, but copying the code across sounds like a good approach. (I think copying the code across is best - having an OverflowHandling mixin just for this feels a bit like overkill - but let me know if you disagree).

[rkday]

Actually, on further reflection - now that it's a "fallback policy" rather than an "overflow policy", it's a bit more generally applicable. RubySingleThreadExecutor, for example, can't overflow, but you could still post to it after shutdown and require fallback behaviour.

So I might:

• rename overflow_policy -> fallback_policy, and handle_overflow -> handle_fallback, but leave can_overflow? - because everything can fall back (e.g. on post-after-shutdown) but not everything can overflow (only things with bounded work queues).
• move fallback_policy and handle_fallback into Executor to reflect that more general nature
• make fallback_policy default to :discard everywhere except RubyThreadPoolExecutor and JavaThreadPoolExecutor, to preserve existing behaviour

[jdantonio]

👍

For backward compatibility we should probably still honor the :overflow_policy constructor option (for now), but perhaps display a warning indicating that it's been deprecated in lieu of :fallback_policy.

Thank you!

[jrochkind]

thank you for honoring the overflow_policy option for backwards compat! I am considering using concurrent-ruby in production code, and not sure if that's a safe move with concurrent-ruby's pre-1.0 status, but decisions like that will make more confident.

[jdantonio]

@jrochkind I'm glad to hear you're considering using our work in production. I appreciate your concerns with the pre-1.0 status. We've been very careful with the various APIs and not making breaking changes. Most of our current plans are extensions to the existing APIs and internal improvements for performance (such as #197) or cleanser code (such as #176). I don't anticipate any breaking API changes in the foreseeable future. And as you can see, we try to be very transparent with such decisions so that everyone has a chance to weigh in.