ruby-concurrency/concurrent-ruby

ThreadPoolExecutor#shutdown? inconsistency in JRuby and C Ruby

bensheldon opened this issue · 4 comments

bensheldon commented

In the scenario in which a ThreadPoolExecutor has received #shutdown and there are active threads/tasks, there is an inconsistency between what JRuby and C Ruby return for #shutdown?:

  • JRuby will return #shutdown? => true while there are still active threads/tasks, at the same time as #shuttingdown? => true. Java's executor service narrowly defines #isShutdown as "no new tasks will be accepted" which is distinct from #isTerminated which is "all tasks have completed following shut down".
  • The Ruby implementation does more of what my expectations are: #shutdown? => false while there are still active threads/tasks, at the same time as #shuttingdown? => true.

It's fairly easy to work around this inconsistency using executor.shutdown? && !executor.shuttingdown? to achieve the same result in both rubies for determining when there are no more threads/tasks on a shutdown executor. It would be nice if the behavior was aligned.

bensheldon commented

Another discovery here is that in J Ruby, once ThreadPoolExecutor#kill is called, it's necessary to do a wait_for_termination to achieve a fully shutdown and terminated executor.

This is now the shutdown wrapper I'm using:

# @param timeout [Numeric, nil] Seconds to wait for active threads.
#   * +nil+, the scheduler will trigger a shutdown but not wait for it to complete.
#   * +-1+, the scheduler will wait until the shutdown is complete.
#   * +0+, the scheduler will immediately shutdown and stop any threads.
#   * A positive number will wait that many seconds before stopping any remaining active threads.
def shutdown(timeout: -1)
  return if @executor.nil? || (@executor.shutdown? && !@executor.shuttingdown?)

  @executor.shutdown if @executor.running?

  if @executor.shuttingdown? && timeout # rubocop:disable Style/GuardClause
    executor_wait = timeout.negative? ? nil : timeout
    unless @executor.wait_for_termination(executor_wait)
      @executor.kill
      @executor.wait_for_termination
    end
  end
end

This is similar to the usage example given in Java:

 void shutdownAndAwaitTermination(ExecutorService pool) {
   pool.shutdown(); // Disable new tasks from being submitted
   try {
     // Wait a while for existing tasks to terminate
     if (!pool.awaitTermination(60, TimeUnit.SECONDS)) {
       pool.shutdownNow(); // Cancel currently executing tasks
       // Wait a while for tasks to respond to being cancelled
       if (!pool.awaitTermination(60, TimeUnit.SECONDS))
           System.err.println("Pool did not terminate");
     }
   } catch (InterruptedException ie) {
     // (Re-)Cancel if current thread also interrupted
     pool.shutdownNow();
     // Preserve interrupt status
     Thread.currentThread().interrupt();
   }
 }
bensheldon commented

Thanks! This line does look like the culprit:

concurrent-ruby/lib/concurrent-ruby/concurrent/executor/java_executor_service.rb

Lines 67 to 69 in eae2851

def ns_shutdown?
@executor.isShutdown || @executor.isTerminated
end

I missed that last night because I got distracted trying to discover the need for the feature-check in #shuttingdown?:

concurrent-ruby/lib/concurrent-ruby/concurrent/executor/java_executor_service.rb

Lines 59 to 64 in eae2851

def ns_shuttingdown?
if @executor.respond_to? :isTerminating
@executor.isTerminating
else
false
end

That was introduced 9+ years ago (I couldn't find the exact place), but I'm not sure if isTerminating/isTerminated only exist for certain implementations or versions of Java. Any idea how to check that?

bensheldon commented

Aha, it looks like just isTerminating maybe didn't exist in Java 6, but isTerminated did. ok, I think I have enough for a PR 👍