Netflix / Hystrix

Hystrix is a latency and fault tolerance library designed to isolate points of access to remote systems, services and 3rd party libraries, stop cascading failure and enable resilience in complex distributed systems where failure is inevitable.
23.98k stars 4.7k forks source link

Thread pool never grows past core size when using a queue #1964

Open ehrmann opened 4 years ago

ehrmann commented 4 years ago

I ran into an issue where the thread pool never grows past its core size when using a queue. Here's an example tested with Hystrix 1.5.18:

HystrixCommand.Setter setter = HystrixCommand.Setter
    .withGroupKey(HystrixCommandGroupKey.Factory.asKey("test"))
    .andCommandPropertiesDefaults(HystrixCommandProperties.Setter()
        .withExecutionTimeoutEnabled(true)
        .withExecutionTimeoutInMilliseconds(1200))
    .andThreadPoolPropertiesDefaults(HystrixThreadPoolProperties.Setter()
        .withAllowMaximumSizeToDivergeFromCoreSize(true)
        .withMaxQueueSize(25)
        .withQueueSizeRejectionThreshold(25)
        .withMaximumSize(40)
        .withCoreSize(1));

for (int i = 0; i < 30; ++i) {
    // This should always work; it's larger than the queue size, but smaller than the max thread pool size.
    new HystrixCommand<Void>(setter) {
        @Override
        protected Void run() throws Exception {
            Thread.sleep(1000);
            return null;
        }
    }.queue();
    System.out.printf("Enqueued %d%n", i);
}

Instead, once the queue is full,

Enqueued 25
Exception in thread "main" com.netflix.hystrix.exception.HystrixRuntimeException: Main$1 could not be queued for execution and no fallback available.

If withMaxQueueSize(25) and withQueueSizeRejectionThreshold(25) are commented out, the thread pool spins up to handle additional load.

The bug is in how Hystrix checks to see if the queue is full and how this interacts with Java's ThreadPoolExecutor.

Hystrix checks the size on its own so that it can honor the queue size rejection threshold. The problem is that because Hystrix does its own check, ThreadPoolExecutor never gets an execute() call with a full queue, thus never triggering a new thread to spin up. From the javadoc:

When a new task is submitted in method execute(java.lang.Runnable), and fewer than corePoolSize threads are running, a new thread is created to handle the request, even if other worker threads are idle. If there are more than corePoolSize but less than maximumPoolSize threads running, a new thread will be created only if the queue is full.

This only applies to bounded queues, so HystrixThreadPoolProperties with the default synchronous queue still spins up threads.

mediocaballero commented 4 years ago

Did this ever get solved? I'm running into the same issue... I have to choose, either having a variable amount of threads in the pool or having a fixed size queue, but I can't have both... EDIT: Did some testing, so I'll leave my findings here should anyone else finds themselves in the same situation:

So it seems that the workaround is having the withQueueSizeRejectionThreshold above the withMaxQueueSize. This way, you can have up to the maxQueueSize enqueued tasks and after that, it will spin up additional executors so the right way to build the command would be:

HystrixCommand.Setter setter = HystrixCommand.Setter
    .withGroupKey(HystrixCommandGroupKey.Factory.asKey("test"))
    .andCommandPropertiesDefaults(HystrixCommandProperties.Setter()
        .withExecutionTimeoutEnabled(true)
        .withExecutionTimeoutInMilliseconds(1200))
    .andThreadPoolPropertiesDefaults(HystrixThreadPoolProperties.Setter()
        .withAllowMaximumSizeToDivergeFromCoreSize(true)
        .withMaxQueueSize(25)
        .withQueueSizeRejectionThreshold(26)
        .withMaximumSize(40)
        .withCoreSize(1));

This way you may have up to 25 queued tasks and up to 40 threads executing them. If you try to execute one more command after that point, you will have a RejectedExecutionException (mind you, from the java.util.concurrent.ThreadPoolExecutor, not from Hystrix!).