Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

bensheldon
Copy link
Contributor

@bensheldon bensheldon commented May 29, 2023

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."

@bensheldon bensheldon force-pushed the thread_pool_executor_available_worker_count branch from 3919c53 to 9a4fbcb Compare May 29, 2023 16:04
@ioquatix ioquatix force-pushed the thread_pool_executor_available_worker_count branch from 9a4fbcb to d9bf21d Compare November 14, 2023 23:30
@@ -73,6 +73,11 @@ def completed_task_count
@executor.getCompletedTaskCount
end

# @!macro thread_pool_executor_attr_reader_available_worker_count
def available_worker_count
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, should this just be available_count or is the worker_count taxonomy established here/elsewhere?

@ioquatix
Copy link
Contributor

ioquatix commented Nov 14, 2023

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
Copy link
Collaborator

eregon commented Nov 17, 2023

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
Copy link
Contributor

@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
Copy link
Contributor Author

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."):

if @max_queue == 0
if @synchronous
queue = java.util.concurrent.SynchronousQueue.new
else
queue = java.util.concurrent.LinkedBlockingQueue.new
end
else
queue = java.util.concurrent.LinkedBlockingQueue.new(@max_queue)
end

...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

@bensheldon bensheldon force-pushed the thread_pool_executor_available_worker_count branch from d9bf21d to 06b5bfb Compare November 18, 2023 01:36
@bensheldon bensheldon changed the title Create method ThreadPoolExecutor#available_worker_count to expose number of idle/uncreated worker threads Create method ThreadPoolExecutor#active_count to expose the number of threads that are actively executing tasks Nov 18, 2023
@bensheldon bensheldon force-pushed the thread_pool_executor_available_worker_count branch from 06b5bfb to 5d286b6 Compare November 18, 2023 18:31
@ioquatix ioquatix merged commit 904c94d into ruby-concurrency:master Nov 19, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants