Executors/Threads: Enforce reasonable rules - #4675
Conversation
One ExecutorUtil.newMDCAwareCachedThreadPool method is misleading as-named, and should be newMDCAwareFixedThreadPool. Renamed.
There was a problem hiding this comment.
many changes in this PR are just for the rename of one newMDCAwareCachedThreadPool to newMDCAwareFixedThreadPool
| ExecutorService coreCloseExecutor = | ||
| ExecutorUtil.newMDCAwareFixedThreadPool( | ||
| Integer.MAX_VALUE, new SolrNamedThreadFactory("coreCloseExecutor")); | ||
| 64, new SolrNamedThreadFactory("coreCloseExecutor")); |
There was a problem hiding this comment.
new enforcement caught this problem
| } | ||
|
|
||
| private ExecutorService newThreadPerUpdatePool(List<?> updates) { | ||
| return ExecutorUtil.newMDCAwareFixedThreadPool(updates.size() + 1, 1, getTestName()); |
There was a problem hiding this comment.
1 queue size... we don't expect to use the queue at all in this test; this is a glorified thread-per-task generator
There was a problem hiding this comment.
Very much deserves to be a separate PR, see #4668
There was a problem hiding this comment.
Very much deserves a separate PR, see #4655 or could be combined with the Jetty one.
| * | ||
| * <p>This is good for fixed-size workloads, like for heavy/intensive work. | ||
| */ | ||
| @Deprecated(since = "10.1") // prefer the overloaded one, thus explicit about queue capacity |
There was a problem hiding this comment.
Wanting to see this deprecated because I think the queue capacity should be more visible at the call-site, surfacing a trade-off/choice. Also, this method doesn't do the 60 second core pool size reclamation, which seems to me a universally good thing. I'm looking for feedback here. If there are some cases that don't want idle reclaimation, they can easily toggle that aspect after pool creation.
| // the ThreadPoolExecutor ignores the configured max value and only considers core pool size. | ||
| // Since we allow core threads to die when idle for too long, this ends in having a pool with | ||
| // lazily-initialized and cached threads. | ||
| public static ExecutorService newMDCAwareFixedThreadPool( |
There was a problem hiding this comment.
renamed. And using a String pool name since every caller is using SolrNamedThreadFactory. Many years ago, there was a thread factory split that doesn't exist today.
| * capacity silently caps the pool at {@code corePoolSize} (or at one thread, which is always | ||
| * created to rescue a queued task when {@code corePoolSize} is 0). | ||
| */ | ||
| private static void checkPoolConfig( |
There was a problem hiding this comment.
see the rules being added
| // lazily-initialized and cached threads. | ||
| public static ExecutorService newMDCAwareFixedThreadPool( | ||
| int nThreads, int queueCapacity, String poolName) { | ||
| if (nThreads > 1024) { // likely wrong choice |
psalagnac
left a comment
psalagnac
left a comment
There was a problem hiding this comment.
This clarifies things.
| * simultaneous tasks starts a thread apiece. Only use this where something upstream already | ||
| * limits how many tasks can be in flight. | ||
| */ | ||
| public static ExecutorService newMDCAwareCachedThreadPool(ThreadFactory threadFactory) { |
There was a problem hiding this comment.
This method still takes a ThreadFactory.
Should it be deprecated in favor of the one taking a string?
| * Create a new pool of threads. Threads are created for new work if there is room to do so up to | ||
| * {@code maxThreads}. Beyond that, the queue is used up to {@code queueCapacity}. Beyond that, | ||
| * work is rejected with an exception. Unused threads will be closed after 60 seconds. | ||
| * Create a new pool of at most {@code nThreads} threads. Each submitted task starts a thread |
There was a problem hiding this comment.
Each submitted task starts a thread until nThreads exist — even when an idle thread could have taken it
Wow; I was really surprised by that so I wrote my own test to confirm this statement...
Maybe we should revisit this. This does not make sense to me to create more threads as long we have some available.
Why don't we always set the core pool size to 0 ?
| } | ||
|
|
||
| /** | ||
| * Rejects a pool that can never reach {@code maximumPoolSize}. {@link ThreadPoolExecutor} only |
There was a problem hiding this comment.
I don't think this statement is correct.
A queue with unlimited capacity caps the pool at corePoolSize, but even a queue with a fixed capacity, like 1000 tasks, will allow the pool to grow wil the queue is full.
That's a pattern that we may want to disallow, but that's not an invalid pool config.
2 participants
There are foot-guns in creating Executors. This PR adds checks in ExecutorUtil designed to catch them. Read the javadocs I'm adding there.
Additionally, one
ExecutorUtil.newMDCAwareCachedThreadPoolmethod is misleading as-named since it creates a pool consistent with a fixed pool -- min & max size equal and a real/sized queue (not SynchronousQueue). YesallowCoreThreadTimeOutis set but IMO that doesn't change the fundamental characterization of the pool. This even confused Claude Opus, as I worked on something and it called factory this method and then later realized it's name mislead it, as it should have chosen another one.