ThreadPoolExecutor#shutdown? inconsistency in JRuby and C Ruby

[bensheldon]

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]

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();
   }
 }

[eregon]

That seems worth fixing in https://github.com/ruby-concurrency/concurrent-ruby/blob/master/lib/concurrent-ruby/concurrent/executor/java_executor_service.rb

[bensheldon]

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]

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 👍