ruby-concurrency / concurrent-ruby

Modern concurrency tools including agents, futures, promises, thread pools, supervisors, and more. Inspired by Erlang, Clojure, Scala, Go, Java, JavaScript, and classic concurrency patterns.
https://ruby-concurrency.github.io/concurrent-ruby/
Other
5.68k stars 418 forks source link

Create method ThreadPoolExecutor#active_count to expose the number of threads that are actively executing tasks #1002

Closed bensheldon closed 10 months ago

bensheldon commented 1 year ago

Connects to #684.

I called it "available" because ready means the thread has already been created but is idle, which isn't inclusive of uncreated workers that could still be added to the pool.

This exposes the equivalent of the Java ThreadPoolExecutor's getActiveCount: "Returns the approximate number of threads that are actively executing tasks."

ioquatix commented 10 months ago

5.times do
  Thread.new do
    if pool.available_worker_count > 0 # query
      pool << proc {...} # use
    end
  end
end

This PR encourages time-of-check / time-of-use race conditions.

While I see the value of this interface/method, I don't think it solves the original problem.

Given that max_queue: 0 == unbounded queue is probably impossible to change, maybe we should implement the desired behaviour with max_queue: nil.

Alternatively, if we think that is a terrible idea (consistency max_queue: 1 vs max_queue: 0 is definitely confusing), then another option is to deprecate the current syntax, release that, rework the interface, and then release that some time later.

  1. Make max_queue: 0 and max_queue: nil the same but issue a deprecation warning for the former.
  2. Release minor.
  3. Make max_queue: 0 do the logical thing.
eregon commented 10 months ago

I'm OK to add this as a monitoring method, and of course yeah it's always potentially stale info, as for other monitoring methods.

It doesn't address #684 though, we'd probably need to fix the ThreadPoolExecutor to handle having a not-unlimited queue.

ioquatix commented 10 months ago

@eregon max_queue: 0 => unbounded. To me, that's pretty confusing. However, code search reveals at least some usage. My concern is, those people may have been expecting a queue of size 0.

What do you think of using max_queue: nil to mean no queue?

bensheldon commented 10 months ago

oh! I think I understand this queuing thing better. The synchronous: true option is actually synonymous with "no queue", and easier to understand in the JavaThreadPoolExecutor because a SynchronousQueue is no queue at all ("A synchronous queue does not have any internal capacity, not even a capacity of one."):

https://github.com/ruby-concurrency/concurrent-ruby/blob/9f40827be9a8a192a6993a8d157bd3ed0662ada0/lib/concurrent-ruby/concurrent/executor/java_thread_pool_executor.rb#L117-L125

...though confusingly it must be combined with max_queue: 0.

I don't object if we wanted to say max_queue: nil was just shorthand for max_queue: 0, synchronous: true